git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jeff King <peff@peff.net>
To: Xavier Morel <xavier.morel@masklinn.net>
Cc: git@vger.kernel.org
Subject: [PATCH 3/3] fsck: downgrade tree badFilemode to "info"
Date: Wed, 10 Aug 2022 17:04:07 -0400	[thread overview]
Message-ID: <YvQdR3sDqDMCIjIE@coredump.intra.peff.net> (raw)
In-Reply-To: <YvQcNpizy9uOZiAz@coredump.intra.peff.net>

The previous commit un-broke the "badFileMode" check; before then it was
literally testing nothing. And as far as I can tell, it has been so
since the very initial version of fsck.

The current severity of "badFileMode" is just "warning". But in the
--strict mode used by transfer.fsckObjects, that is elevated to an
error. This will potentially cause hassle for users, because historical
objects with bad modes will suddenly start causing pushes to many server
operators to be rejected.

At the same time, these bogus modes aren't actually a big risk. Because
we canonicalize them everywhere besides fsck, they can't cause too much
mischief in the real world. The worst thing you can do is end up with
two almost-identical trees that have different hashes but are
interpreted the same. That will generally cause things to be inefficient
rather than wrong, and is a bug somebody working on a Git implementation
would want to fix, but probably not worth inconveniencing users by
refusing to push or fetch.

So let's downgrade this to "info" by default, which is our setting for
"mention this when fscking, but don't ever reject, even under strict
mode". If somebody really wants to be paranoid, they can still adjust
the level using config.

Suggested-by: Xavier Morel <xavier.morel@masklinn.net>
Signed-off-by: Jeff King <peff@peff.net>
---
 fsck.h                          |  2 +-
 t/t5504-fetch-receive-strict.sh | 17 +++++++++++++++++
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/fsck.h b/fsck.h
index d07f7a2459..6f801e53b1 100644
--- a/fsck.h
+++ b/fsck.h
@@ -56,7 +56,6 @@ enum fsck_msg_type {
 	FUNC(GITMODULES_PATH, ERROR) \
 	FUNC(GITMODULES_UPDATE, ERROR) \
 	/* warnings */ \
-	FUNC(BAD_FILEMODE, WARN) \
 	FUNC(EMPTY_NAME, WARN) \
 	FUNC(FULL_PATHNAME, WARN) \
 	FUNC(HAS_DOT, WARN) \
@@ -66,6 +65,7 @@ enum fsck_msg_type {
 	FUNC(ZERO_PADDED_FILEMODE, WARN) \
 	FUNC(NUL_IN_COMMIT, WARN) \
 	/* infos (reported as warnings, but ignored by default) */ \
+	FUNC(BAD_FILEMODE, INFO) \
 	FUNC(GITMODULES_PARSE, INFO) \
 	FUNC(GITIGNORE_SYMLINK, INFO) \
 	FUNC(GITATTRIBUTES_SYMLINK, INFO) \
diff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh
index b0b795aca9..ac4099ca89 100755
--- a/t/t5504-fetch-receive-strict.sh
+++ b/t/t5504-fetch-receive-strict.sh
@@ -352,4 +352,21 @@ test_expect_success \
 	grep "Cannot demote unterminatedheader" act
 '
 
+test_expect_success 'badFilemode is not a strict error' '
+	git init --bare badmode.git &&
+	tree=$(
+		cd badmode.git &&
+		blob=$(echo blob | git hash-object -w --stdin | hex2oct) &&
+		printf "123456 foo\0${blob}" |
+		git hash-object -t tree --stdin -w --literally
+	) &&
+
+	rm -rf dst.git &&
+	git init --bare dst.git &&
+	git -C dst.git config transfer.fsckObjects true &&
+
+	git -C badmode.git push ../dst.git $tree:refs/tags/tree 2>err &&
+	grep "$tree: badFilemode" err
+'
+
 test_done
-- 
2.37.1.917.gf0c714241f

  parent reply	other threads:[~2022-08-10 21:04 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-08-06 13:43 fsck: BAD_FILEMODE diagnostic broken / never triggers Xavier Morel
2022-08-09 22:48 ` Jeff King
2022-08-09 23:28   ` Jeff King
2022-08-10 15:01     ` Xavier Morel
2022-08-10 20:04       ` Jeff King
2022-08-10 20:59         ` [PATCH 0/3] actually detect bad file modes in fsck Jeff King
2022-08-10 21:01           ` [PATCH 1/3] tree-walk: add a mechanism for getting non-canonicalized modes Jeff King
2022-08-10 21:02           ` [PATCH 2/3] fsck: actually detect bad file modes in trees Jeff King
2022-08-10 21:35             ` Junio C Hamano
2022-08-10 21:46               ` Jeff King
2022-08-10 21:54                 ` Junio C Hamano
2022-08-10 21:04           ` Jeff King [this message]
2022-08-10 22:08             ` [PATCH 3/3] fsck: downgrade tree badFilemode to "info" Junio C Hamano
2022-08-11  8:33               ` Jeff King
2022-08-11 16:24                 ` Junio C Hamano
2022-08-11  9:39           ` [PATCH 0/3] actually detect bad file modes in fsck Ævar Arnfjörð Bjarmason
2022-08-14  7:03             ` Jeff King

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YvQdR3sDqDMCIjIE@coredump.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    --cc=xavier.morel@masklinn.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).