{"thread":{"id":"55604","subject":"[PATCH 0/9] leftover bits from symlinked gitattributes, etc topics","startedAt":"2021-05-01T15:40:55Z","lastAt":"2021-05-04T12:26:50Z","messageCount":35,"participants":["Jeff King","Eric Sunshine","Junio C Hamano","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":9},"messages":[{"id":"423407","messageId":"YI12hK4X/gfl3u29@coredump.intra.peff.net","threadId":"55604","inReplyTo":null,"subject":"[PATCH 0/9] leftover bits from symlinked gitattributes, etc topics","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-01T15:40:52Z","receivedAt":"2021-05-01T15:40:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"A while back, I had a topic[1] that treated symlinked .gitattributes\n(and .gitignore and .mailmap) the same as .gitmodules: forbidding them\nin the index, complaining about them via fsck, etc.\n\nIn the end, we decided not to do that[2], and instead just open the\nfiles with O_NOFOLLOW instead. As I said in that thread, we could\nsalvage some of the cleanups, fsck checks, and docs from the original\ntopic. So here that is. (The new topic is in master but not yet\nreleased; so while this is not strictly a bug-fix for an existing topic,\nit would be good to get especially the doc improvements into the same\nrelease).\n\n[1] https://lore.kernel.org/git/20201005071751.GA2290770@coredump.intra.peff.net/\n[2] https://lore.kernel.org/git/YDiWs6yyv3U9YvC2@coredump.intra.peff.net/\n\nThe patches are:\n\n [1/9]: t7415: remove out-dated comment about translation\n [2/9]: fsck_tree(): fix shadowed variable\n [3/9]: fsck_tree(): wrap some long lines\n\n These three are really independent fixes and cleanups, that could be\n taken separately.\n\n [4/9]: t7415: rename to expand scope\n [5/9]: t7450: test verify_path() handling of gitmodules\n [6/9]: t7450: test .gitmodules symlink matching against obscured names\n\n These three are just cleaning up and improving the tests for the\n existing handling of the .gitmodules symlinks.\n\n [7/9]: t0060: test ntfs/hfs-obscured dotfiles\n [8/9]: fsck: warn about symlinked dotfiles we'll open with O_NOFOLLOW\n\n These two extend fsck checks (warnings, not errors) to the new\n files. I think this is an improvement, but I could be persuaded\n otherwise (and these two could be dropped independent of the rest).\n\n [9/9]: docs: document symlink restrictions for dot-files\n\n And this covers documentation for all of the files (including\n .gitmodules).\n\n Documentation/gitattributes.txt | 7 ++\n Documentation/gitignore.txt | 5 +\n Documentation/gitmailmap.txt | 7 ++\n Documentation/gitmodules.txt | 8 ++\n cache.h | 1 +\n fsck.c | 84 ++++++++++---\n fsck.h | 3 +\n path.c | 5 +\n t/helper/test-path-utils.c | 46 +++++--\n t/t0060-path-utils.sh | 30 +++++\n ...ule-names.sh => t7450-bad-git-dotfiles.sh} | 116 +++++++++++++-----\n utf8.c | 5 +\n utf8.h | 1 +\n 13 files changed, 257 insertions(+), 61 deletions(-)\n rename t/{t7415-submodule-names.sh => t7450-bad-git-dotfiles.sh} (70%)\n\n-Peff\n"},{"id":"423408","messageId":"YI12n+1rbedpwyfW@coredump.intra.peff.net","threadId":"55604","inReplyTo":"YI12hK4X/gfl3u29@coredump.intra.peff.net","subject":"[PATCH 1/9] t7415: remove out-dated comment about translation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-01T15:41:19Z","receivedAt":"2021-05-01T15:41:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Since GETTEXT_POISON does not exist anymore, there is no point warning\npeople about whether we should use test_i18ngrep. This is doubly\nconfusing because the comment was describing why it was OK to use grep,\nbut it got caught up in the mass conversion of 674ba34038 (fsck: mark\nstrings for translation, 2018-11-10).\n\nNote there are other uses of test_i18ngrep in this script which are now\nobsolete; I'll save those for a mass-cleanup. My goal here was just to\nfix the confusing comment in code I'm about to refactor.\n\nSigned-off-by: Jeff King \n---\n t/t7415-submodule-names.sh | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t7415-submodule-names.sh b/t/t7415-submodule-names.sh\nindex f70368bc2e..fef6561d80 100755\n--- a/t/t7415-submodule-names.sh\n+++ b/t/t7415-submodule-names.sh\n@@ -151,10 +151,9 @@ test_expect_success 'fsck detects symlinked .gitmodules file' '\n \t\t} | git mktree &&\n \n \t\t# Check not only that we fail, but that it is due to the\n-\t\t# symlink detector; this grep string comes from the config\n-\t\t# variable name and will not be translated.\n+\t\t# symlink detector\n \t\ttest_must_fail git fsck 2>output &&\n-\t\ttest_i18ngrep gitmodulesSymlink output\n+\t\tgrep gitmodulesSymlink output\n \t)\n '\n \n-- \n2.31.1.875.g5dccece0aa\n\n"},{"id":"423409","messageId":"YI12stO48egyiHjv@coredump.intra.peff.net","threadId":"55604","inReplyTo":"YI12hK4X/gfl3u29@coredump.intra.peff.net","subject":"[PATCH 2/9] fsck_tree(): fix shadowed variable","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-01T15:41:38Z","receivedAt":"2021-05-01T15:41:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Commit b2f2039c2b (fsck: accept an oid instead of a \"struct tree\" for\nfsck_tree(), 2019-10-18) introduced a new \"oid\" parameter to\nfsck_tree(), and we pass it to the report() function when we find\nproblems. However, that is shadowed within the tree-walking loop by the\nexisting \"oid\" variable which we use to store the oid of each tree\nentry. As a result, we may report the wrong oid for some problems we\ndetect within the loop (the entry oid, instead of the tree oid).\n\nOur tests didn't catch this because they checked only that we found the\nexpected fsck problem, not that it was attached to the correct object.\n\nLet's rename both variables in the function to avoid confusion. This\nmakes the diff a little noisy (e.g., all of the report() calls outside\nthe loop were already correct but need to be touched), but makes sure we\ncatch all cases and will avoid similar confusion in the future.\n\nAnd we can update the test to be a bit more specific and catch this\nproblem.\n\nSigned-off-by: Jeff King \n---\n fsck.c | 42 ++++++++++++++++++++------------------\n t/t7415-submodule-names.sh | 5 +++--\n 2 files changed, 25 insertions(+), 22 deletions(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex f5ed6a2635..dd31ed1413 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -558,7 +558,7 @@ static int verify_ordered(unsigned mode1, const char *name1,\n \treturn c1 < c2 ? 0 : TREE_UNORDERED;\n }\n \n-static int fsck_tree(const struct object_id *oid,\n+static int fsck_tree(const struct object_id *tree_oid,\n \t\t const char *buffer, unsigned long size,\n \t\t struct fsck_options *options)\n {\n@@ -579,7 +579,7 @@ static int fsck_tree(const struct object_id *oid,\n \tstruct name_stack df_dup_candidates = { NULL };\n \n \tif (init_tree_desc_gently(&desc, buffer, size)) {\n-\t\tretval += report(options, oid, OBJ_TREE, FSCK_MSG_BAD_TREE, \"cannot be parsed as a tree\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_BAD_TREE, \"cannot be parsed as a tree\");\n \t\treturn retval;\n \t}\n \n@@ -589,11 +589,11 @@ static int fsck_tree(const struct object_id *oid,\n \twhile (desc.size) {\n \t\tunsigned short mode;\n \t\tconst char *name, *backslash;\n-\t\tconst struct object_id *oid;\n+\t\tconst struct object_id *entry_oid;\n \n-\t\toid = tree_entry_extract(&desc, &name, &mode);\n+\t\tentry_oid = tree_entry_extract(&desc, &name, &mode);\n \n-\t\thas_null_sha1 |= is_null_oid(oid);\n+\t\thas_null_sha1 |= is_null_oid(entry_oid);\n \t\thas_full_path |= !!strchr(name, '/');\n \t\thas_empty_name |= !*name;\n \t\thas_dot |= !strcmp(name, \".\");\n@@ -603,10 +603,11 @@ static int fsck_tree(const struct object_id *oid,\n \n \t\tif (is_hfs_dotgitmodules(name) || is_ntfs_dotgitmodules(name)) {\n \t\t\tif (!S_ISLNK(mode))\n-\t\t\t\toidset_insert(&options->gitmodules_found, oid);\n+\t\t\t\toidset_insert(&options->gitmodules_found,\n+\t\t\t\t\t entry_oid);\n \t\t\telse\n \t\t\t\tretval += report(options,\n-\t\t\t\t\t\t oid, OBJ_TREE,\n+\t\t\t\t\t\t tree_oid, OBJ_TREE,\n \t\t\t\t\t\t FSCK_MSG_GITMODULES_SYMLINK,\n \t\t\t\t\t\t \".gitmodules is a symbolic link\");\n \t\t}\n@@ -617,9 +618,10 @@ static int fsck_tree(const struct object_id *oid,\n \t\t\t\thas_dotgit |= is_ntfs_dotgit(backslash);\n \t\t\t\tif (is_ntfs_dotgitmodules(backslash)) {\n \t\t\t\t\tif (!S_ISLNK(mode))\n-\t\t\t\t\t\toidset_insert(&options->gitmodules_found, oid);\n+\t\t\t\t\t\toidset_insert(&options->gitmodules_found,\n+\t\t\t\t\t\t\t entry_oid);\n \t\t\t\t\telse\n-\t\t\t\t\t\tretval += report(options, oid, OBJ_TREE,\n+\t\t\t\t\t\tretval += report(options, tree_oid, OBJ_TREE,\n \t\t\t\t\t\t\t\t FSCK_MSG_GITMODULES_SYMLINK,\n \t\t\t\t\t\t\t\t \".gitmodules is a symbolic link\");\n \t\t\t\t}\n@@ -628,7 +630,7 @@ static int fsck_tree(const struct object_id *oid,\n \t\t}\n \n \t\tif (update_tree_entry_gently(&desc)) {\n-\t\t\tretval += report(options, oid, OBJ_TREE, FSCK_MSG_BAD_TREE, \"cannot be parsed as a tree\");\n+\t\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_BAD_TREE, \"cannot be parsed as a tree\");\n \t\t\tbreak;\n \t\t}\n \n@@ -676,25 +678,25 @@ static int fsck_tree(const struct object_id *oid,\n \tname_stack_clear(&df_dup_candidates);\n \n \tif (has_null_sha1)\n-\t\tretval += report(options, oid, OBJ_TREE, FSCK_MSG_NULL_SHA1, \"contains entries pointing to null sha1\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_NULL_SHA1, \"contains entries pointing to null sha1\");\n \tif (has_full_path)\n-\t\tretval += report(options, oid, OBJ_TREE, FSCK_MSG_FULL_PATHNAME, \"contains full pathnames\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_FULL_PATHNAME, \"contains full pathnames\");\n \tif (has_empty_name)\n-\t\tretval += report(options, oid, OBJ_TREE, FSCK_MSG_EMPTY_NAME, \"contains empty pathname\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_EMPTY_NAME, \"contains empty pathname\");\n \tif (has_dot)\n-\t\tretval += report(options, oid, OBJ_TREE, FSCK_MSG_HAS_DOT, \"contains '.'\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_HAS_DOT, \"contains '.'\");\n \tif (has_dotdot)\n-\t\tretval += report(options, oid, OBJ_TREE, FSCK_MSG_HAS_DOTDOT, \"contains '..'\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_HAS_DOTDOT, \"contains '..'\");\n \tif (has_dotgit)\n-\t\tretval += report(options, oid, OBJ_TREE, FSCK_MSG_HAS_DOTGIT, \"contains '.git'\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_HAS_DOTGIT, \"contains '.git'\");\n \tif (has_zero_pad)\n-\t\tretval += report(options, oid, OBJ_TREE, FSCK_MSG_ZERO_PADDED_FILEMODE, \"contains zero-padded file modes\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_ZERO_PADDED_FILEMODE, \"contains zero-padded file modes\");\n \tif (has_bad_modes)\n-\t\tretval += report(options, oid, OBJ_TREE, FSCK_MSG_BAD_FILEMODE, \"contains bad file modes\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_BAD_FILEMODE, \"contains bad file modes\");\n \tif (has_dup_entries)\n-\t\tretval += report(options, oid, OBJ_TREE, FSCK_MSG_DUPLICATE_ENTRIES, \"contains duplicate file entries\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_DUPLICATE_ENTRIES, \"contains duplicate file entries\");\n \tif (not_properly_sorted)\n-\t\tretval += report(options, oid, OBJ_TREE, FSCK_MSG_TREE_NOT_SORTED, \"not properly sorted\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_TREE_NOT_SORTED, \"not properly sorted\");\n \treturn retval;\n }\n \ndiff --git a/t/t7415-submodule-names.sh b/t/t7415-submodule-names.sh\nindex fef6561d80..6a8cf3f47b 100755\n--- a/t/t7415-submodule-names.sh\n+++ b/t/t7415-submodule-names.sh\n@@ -148,12 +148,13 @@ test_expect_success 'fsck detects symlinked .gitmodules file' '\n \t\t{\n \t\t\tprintf \"100644 blob $content\\t$tricky\\n\" &&\n \t\t\tprintf \"120000 blob $target\\t.gitmodules\\n\"\n-\t\t} | git mktree &&\n+\t\t} >bad-tree &&\n+\t\ttree=$(git mktree output &&\n-\t\tgrep gitmodulesSymlink output\n+\t\tgrep \"tree $tree: gitmodulesSymlink\" output\n \t)\n '\n \n-- \n2.31.1.875.g5dccece0aa\n\n"},{"id":"423410","messageId":"YI12t5gzfzIxvZs3@coredump.intra.peff.net","threadId":"55604","inReplyTo":"YI12hK4X/gfl3u29@coredump.intra.peff.net","subject":"[PATCH 3/9] fsck_tree(): wrap some long lines","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-01T15:41:43Z","receivedAt":"2021-05-01T15:41:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Many calls to report() in fsck_tree() are kept on a single line and are\nquite long. Most were pretty big to begin with, but have gotten even\nlonger over the years as we've added more parameters. Let's accept the\nchurn of wrapping them in order to conform to our usual line limits.\n\nSigned-off-by: Jeff King \n---\n fsck.c | 48 ++++++++++++++++++++++++++++++++++++------------\n 1 file changed, 36 insertions(+), 12 deletions(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex dd31ed1413..db94817898 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -579,7 +579,9 @@ static int fsck_tree(const struct object_id *tree_oid,\n \tstruct name_stack df_dup_candidates = { NULL };\n \n \tif (init_tree_desc_gently(&desc, buffer, size)) {\n-\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_BAD_TREE, \"cannot be parsed as a tree\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE,\n+\t\t\t\t FSCK_MSG_BAD_TREE,\n+\t\t\t\t \"cannot be parsed as a tree\");\n \t\treturn retval;\n \t}\n \n@@ -630,7 +632,9 @@ static int fsck_tree(const struct object_id *tree_oid,\n \t\t}\n \n \t\tif (update_tree_entry_gently(&desc)) {\n-\t\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_BAD_TREE, \"cannot be parsed as a tree\");\n+\t\t\tretval += report(options, tree_oid, OBJ_TREE,\n+\t\t\t\t\t FSCK_MSG_BAD_TREE,\n+\t\t\t\t\t \"cannot be parsed as a tree\");\n \t\t\tbreak;\n \t\t}\n \n@@ -678,25 +682,45 @@ static int fsck_tree(const struct object_id *tree_oid,\n \tname_stack_clear(&df_dup_candidates);\n \n \tif (has_null_sha1)\n-\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_NULL_SHA1, \"contains entries pointing to null sha1\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE,\n+\t\t\t\t FSCK_MSG_NULL_SHA1,\n+\t\t\t\t \"contains entries pointing to null sha1\");\n \tif (has_full_path)\n-\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_FULL_PATHNAME, \"contains full pathnames\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE,\n+\t\t\t\t FSCK_MSG_FULL_PATHNAME,\n+\t\t\t\t \"contains full pathnames\");\n \tif (has_empty_name)\n-\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_EMPTY_NAME, \"contains empty pathname\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE,\n+\t\t\t\t FSCK_MSG_EMPTY_NAME,\n+\t\t\t\t \"contains empty pathname\");\n \tif (has_dot)\n-\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_HAS_DOT, \"contains '.'\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE,\n+\t\t\t\t FSCK_MSG_HAS_DOT,\n+\t\t\t\t \"contains '.'\");\n \tif (has_dotdot)\n-\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_HAS_DOTDOT, \"contains '..'\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE,\n+\t\t\t\t FSCK_MSG_HAS_DOTDOT,\n+\t\t\t\t \"contains '..'\");\n \tif (has_dotgit)\n-\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_HAS_DOTGIT, \"contains '.git'\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE,\n+\t\t\t\t FSCK_MSG_HAS_DOTGIT,\n+\t\t\t\t \"contains '.git'\");\n \tif (has_zero_pad)\n-\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_ZERO_PADDED_FILEMODE, \"contains zero-padded file modes\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE,\n+\t\t\t\t FSCK_MSG_ZERO_PADDED_FILEMODE,\n+\t\t\t\t \"contains zero-padded file modes\");\n \tif (has_bad_modes)\n-\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_BAD_FILEMODE, \"contains bad file modes\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE,\n+\t\t\t\t FSCK_MSG_BAD_FILEMODE,\n+\t\t\t\t \"contains bad file modes\");\n \tif (has_dup_entries)\n-\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_DUPLICATE_ENTRIES, \"contains duplicate file entries\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE,\n+\t\t\t\t FSCK_MSG_DUPLICATE_ENTRIES,\n+\t\t\t\t \"contains duplicate file entries\");\n \tif (not_properly_sorted)\n-\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_TREE_NOT_SORTED, \"not properly sorted\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE,\n+\t\t\t\t FSCK_MSG_TREE_NOT_SORTED,\n+\t\t\t\t \"not properly sorted\");\n \treturn retval;\n }\n \n-- \n2.31.1.875.g5dccece0aa\n\n"},{"id":"423411","messageId":"YI120ELAX6EZ4NgI@coredump.intra.peff.net","threadId":"55604","inReplyTo":"YI12hK4X/gfl3u29@coredump.intra.peff.net","subject":"[PATCH 4/9] t7415: rename to expand scope","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-01T15:42:08Z","receivedAt":"2021-05-01T15:42:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This script has already expanded beyond its original intent of \".. in\nsubmodule names\" to include other malicious submodule bits. Let's update\nthe name and description to reflect that, as well as the fact that we'll\nsoon be adding similar tests for other dotfiles (.gitattributes, etc).\nWe'll also renumber it to move it out of the group of submodule-specific\ntests.\n\nSigned-off-by: Jeff King \n---\n ...submodule-names.sh => t7450-bad-git-dotfiles.sh} | 13 ++++++++++---\n 1 file changed, 10 insertions(+), 3 deletions(-)\n rename t/{t7415-submodule-names.sh => t7450-bad-git-dotfiles.sh} (95%)\n\ndiff --git a/t/t7415-submodule-names.sh b/t/t7450-bad-git-dotfiles.sh\nsimilarity index 95%\nrename from t/t7415-submodule-names.sh\nrename to t/t7450-bad-git-dotfiles.sh\nindex 6a8cf3f47b..34d4dc6def 100755\n--- a/t/t7415-submodule-names.sh\n+++ b/t/t7450-bad-git-dotfiles.sh\n@@ -1,9 +1,16 @@\n #!/bin/sh\n \n-test_description='check handling of .. in submodule names\n+test_description='check broken or malicious patterns in .git* files\n \n-Exercise the name-checking function on a variety of names, and then give a\n-real-world setup that confirms we catch this in practice.\n+Such as:\n+\n+ - presence of .. in submodule names;\n+ Exercise the name-checking function on a variety of names, and then give a\n+ real-world setup that confirms we catch this in practice.\n+\n+ - nested submodule names\n+\n+ - symlinked .gitmodules, etc\n '\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-pack.sh\n-- \n2.31.1.875.g5dccece0aa\n\n"},{"id":"423412","messageId":"YI124aZ1dbY5O67J@coredump.intra.peff.net","threadId":"55604","inReplyTo":"YI12hK4X/gfl3u29@coredump.intra.peff.net","subject":"[PATCH 5/9] t7450: test verify_path() handling of gitmodules","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-01T15:42:25Z","receivedAt":"2021-05-01T15:42:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Commit 10ecfa7649 (verify_path: disallow symlinks in .gitmodules,\n2018-05-04) made it impossible to load a symlink .gitmodules file into\nthe index. However, there are no tests of this behavior. Let's make sure\nthis case is covered. We can easily reuse the test setup created by\nthe matching b7b1fca175 (fsck: complain when .gitmodules is a symlink,\n2018-05-04).\n\nSigned-off-by: Jeff King \n---\n t/t7450-bad-git-dotfiles.sh | 19 ++++++++++++++++---\n 1 file changed, 16 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t7450-bad-git-dotfiles.sh b/t/t7450-bad-git-dotfiles.sh\nindex 34d4dc6def..c021d4e752 100755\n--- a/t/t7450-bad-git-dotfiles.sh\n+++ b/t/t7450-bad-git-dotfiles.sh\n@@ -139,7 +139,7 @@ test_expect_success 'index-pack --strict works for non-repo pack' '\n \tgrep gitmodulesName output\n '\n \n-test_expect_success 'fsck detects symlinked .gitmodules file' '\n+test_expect_success 'set up repo with symlinked .gitmodules file' '\n \tgit init symlink &&\n \t(\n \t\tcd symlink &&\n@@ -155,8 +155,14 @@ test_expect_success 'fsck detects symlinked .gitmodules file' '\n \t\t{\n \t\t\tprintf \"100644 blob $content\\t$tricky\\n\" &&\n \t\t\tprintf \"120000 blob $target\\t.gitmodules\\n\"\n-\t\t} >bad-tree &&\n-\t\ttree=$(git mktree bad-tree\n+\t) &&\n+\ttree=$(git -C symlink mktree err &&\n+\ttest_i18ngrep \"invalid path.*gitmodules\" err &&\n+\tgit -C symlink ls-files >out &&\n+\ttest_must_be_empty out\n+'\n+\n test_expect_success 'fsck detects non-blob .gitmodules' '\n \tgit init non-blob &&\n \t(\n-- \n2.31.1.875.g5dccece0aa\n\n"},{"id":"423413","messageId":"YI128Km8orE7OmtP@coredump.intra.peff.net","threadId":"55604","inReplyTo":"YI12hK4X/gfl3u29@coredump.intra.peff.net","subject":"[PATCH 6/9] t7450: test .gitmodules symlink matching against obscured names","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-01T15:42:40Z","receivedAt":"2021-05-01T15:42:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In t7450 we check that both verify_path() and fsck catch malformed\n.gitmodules entries in trees. However, we don't check that we catch\nfilesystem-equivalent forms of these (e.g., \".GITMOD~1\" on Windows).\nOur name-matching functions are exercised well in t0060, but there's\nnothing to test that we correctly call the matching functions from the\nactual fsck and verify_path() code.\n\nSo instead of testing just .gitmodules, let's repeat our tests for a few\nbasic cases. We don't need to be exhaustive here (t0060 handles that),\nbut just make sure we hit one name of each type.\n\nBesides pushing the tests into a function that takes the path as a\nparameter, we'll need to do a few things:\n\n - adjust the directory name to accommodate the tests running multiple\n times\n\n - set core.protecthfs for index checks. Fsck always protects all types\n by default, but we want to be able to exercise the HFS routines on\n every system. Note that core.protectntfs is already the default\n these days, but it doesn't hurt to explicitly label our need for it.\n\n - we'll also take the filename (\"gitmodules\") as a parameter. All\n calls use the same name for now, but a future patch will extend this\n to handle other .gitfoo files. Note that our fake-content symlink\n destination is somewhat .gitmodules specific. But it isn't necessary\n for other files (which don't do a content check). And it happens to\n be a valid attribute and ignore file anyway.\n\nSigned-off-by: Jeff King \n---\n t/t7450-bad-git-dotfiles.sh | 91 +++++++++++++++++++++----------------\n 1 file changed, 53 insertions(+), 38 deletions(-)\n\ndiff --git a/t/t7450-bad-git-dotfiles.sh b/t/t7450-bad-git-dotfiles.sh\nindex c021d4e752..4e142c798f 100755\n--- a/t/t7450-bad-git-dotfiles.sh\n+++ b/t/t7450-bad-git-dotfiles.sh\n@@ -139,44 +139,59 @@ test_expect_success 'index-pack --strict works for non-repo pack' '\n \tgrep gitmodulesName output\n '\n \n-test_expect_success 'set up repo with symlinked .gitmodules file' '\n-\tgit init symlink &&\n-\t(\n-\t\tcd symlink &&\n-\n-\t\t# Make the tree directly to avoid index restrictions.\n-\t\t#\n-\t\t# Because symlinks store the target as a blob, choose\n-\t\t# a pathname that could be parsed as a .gitmodules file\n-\t\t# to trick naive non-symlink-aware checking.\n-\t\ttricky=\"[foo]bar=true\" &&\n-\t\tcontent=$(git hash-object -w ../.gitmodules) &&\n-\t\ttarget=$(printf \"$tricky\" | git hash-object -w --stdin) &&\n-\t\t{\n-\t\t\tprintf \"100644 blob $content\\t$tricky\\n\" &&\n-\t\t\tprintf \"120000 blob $target\\t.gitmodules\\n\"\n-\t\t} >bad-tree\n-\t) &&\n-\ttree=$(git -C symlink mktree output &&\n-\t\tgrep \"tree $tree: gitmodulesSymlink\" output\n-\t)\n-'\n-\n-test_expect_success 'refuse to load symlinked .gitmodules into index' '\n-\ttest_must_fail git -C symlink read-tree $tree 2>err &&\n-\ttest_i18ngrep \"invalid path.*gitmodules\" err &&\n-\tgit -C symlink ls-files >out &&\n-\ttest_must_be_empty out\n-'\n+check_dotx_symlink () {\n+\tname=$1\n+\ttype=$2\n+\tpath=$3\n+\tdir=symlink-$name-$type\n+\n+\ttest_expect_success \"set up repo with symlinked $name ($type)\" '\n+\t\tgit init $dir &&\n+\t\t(\n+\t\t\tcd $dir &&\n+\n+\t\t\t# Make the tree directly to avoid index restrictions.\n+\t\t\t#\n+\t\t\t# Because symlinks store the target as a blob, choose\n+\t\t\t# a pathname that could be parsed as a .gitmodules file\n+\t\t\t# to trick naive non-symlink-aware checking.\n+\t\t\ttricky=\"[foo]bar=true\" &&\n+\t\t\tcontent=$(git hash-object -w ../.gitmodules) &&\n+\t\t\ttarget=$(printf \"$tricky\" | git hash-object -w --stdin) &&\n+\t\t\t{\n+\t\t\t\tprintf \"100644 blob $content\\t$tricky\\n\" &&\n+\t\t\t\tprintf \"120000 blob $target\\t$path\\n\"\n+\t\t\t} >bad-tree\n+\t\t) &&\n+\t\ttree=$(git -C $dir mktree <$dir/bad-tree)\n+\t'\n+\n+\ttest_expect_success \"fsck detects symlinked $name ($type)\" '\n+\t\t(\n+\t\t\tcd $dir &&\n+\n+\t\t\t# Check not only that we fail, but that it is due to the\n+\t\t\t# symlink detector\n+\t\t\ttest_must_fail git fsck 2>output &&\n+\t\t\tgrep \"tree $tree: ${name}Symlink\" output\n+\t\t)\n+\t'\n+\n+\ttest_expect_success \"refuse to load symlinked $name into index ($type)\" '\n+\t\ttest_must_fail \\\n+\t\t\tgit -C $dir \\\n+\t\t\t -c core.protectntfs \\\n+\t\t\t -c core.protecthfs \\\n+\t\t\t read-tree $tree 2>err &&\n+\t\ttest_i18ngrep \"invalid path.*$name\" err &&\n+\t\tgit -C $dir ls-files -s >out &&\n+\t\ttest_must_be_empty out\n+\t'\n+}\n+\n+check_dotx_symlink gitmodules vanilla .gitmodules\n+check_dotx_symlink gitmodules ntfs \".gitmodules .\"\n+check_dotx_symlink gitmodules hfs \".${u200c}gitmodules\"\n \n test_expect_success 'fsck detects non-blob .gitmodules' '\n \tgit init non-blob &&\n-- \n2.31.1.875.g5dccece0aa\n\n"},{"id":"423414","messageId":"YI12/oqmfIoSuxN2@coredump.intra.peff.net","threadId":"55604","inReplyTo":"YI12hK4X/gfl3u29@coredump.intra.peff.net","subject":"[PATCH 7/9] t0060: test ntfs/hfs-obscured dotfiles","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-01T15:42:54Z","receivedAt":"2021-05-01T15:42:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We have tests that cover various filesystem-specific spellings of\n\".gitmodules\", because we need to reliably identify that path for some\nsecurity checks. These are from dc2d9ba318 (is_{hfs,ntfs}_dotgitmodules:\nadd tests, 2018-05-12), with the actual code coming from e7cb0b4455\n(is_ntfs_dotgit: match other .git files, 2018-05-11) and 0fc333ba20\n(is_hfs_dotgit: match other .git files, 2018-05-02).\n\nThose latter two commits also added similar matching functions for\n.gitattributes and .gitignore. These ended up not being used in the\nfinal series, and are currently dead code. But in preparation for them\nbeing used in some fsck checks, let's make sure they actually work by\nthrowing a few basic tests at them. Likewise, let's cover .mailmap\n(which does need matching code added).\n\nI didn't bother with the whole battery of tests that we cover for\n.gitmodules. These functions are all based on the same generic matcher,\nso it's sufficient to test most of the corner cases just once.\n\nNote that the ntfs magic prefix names in the tests come from the\nalgorithm described in e7cb0b4455 (and are different for each file).\n\nSigned-off-by: Jeff King \n---\n cache.h | 1 +\n path.c | 5 +++++\n t/helper/test-path-utils.c | 46 +++++++++++++++++++++++++++-----------\n t/t0060-path-utils.sh | 30 +++++++++++++++++++++++++\n utf8.c | 5 +++++\n utf8.h | 1 +\n 6 files changed, 75 insertions(+), 13 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex b785ffb383..e6dda88fb0 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1271,6 +1271,7 @@ int is_ntfs_dotgit(const char *name);\n int is_ntfs_dotgitmodules(const char *name);\n int is_ntfs_dotgitignore(const char *name);\n int is_ntfs_dotgitattributes(const char *name);\n+int is_ntfs_dotmailmap(const char *name);\n \n /*\n * Returns true iff \"str\" could be confused as a command-line option when\ndiff --git a/path.c b/path.c\nindex 9e883eb524..7bccd830e9 100644\n--- a/path.c\n+++ b/path.c\n@@ -1493,6 +1493,11 @@ int is_ntfs_dotgitattributes(const char *name)\n \treturn is_ntfs_dot_str(name, \"gitattributes\", \"gi7d29\");\n }\n \n+int is_ntfs_dotmailmap(const char *name)\n+{\n+\treturn is_ntfs_dot_str(name, \"mailmap\", \"maba30\");\n+}\n+\n int looks_like_command_line_option(const char *str)\n {\n \treturn str && str[0] == '-';\ndiff --git a/t/helper/test-path-utils.c b/t/helper/test-path-utils.c\nindex 313a153209..229ed416b0 100644\n--- a/t/helper/test-path-utils.c\n+++ b/t/helper/test-path-utils.c\n@@ -172,9 +172,22 @@ static struct test_data dirname_data[] = {\n \t{ NULL, NULL }\n };\n \n-static int is_dotgitmodules(const char *path)\n+static int check_dotfile(const char *x, const char **argv,\n+\t\t\t int (*is_hfs)(const char *),\n+\t\t\t int (*is_ntfs)(const char *))\n {\n-\treturn is_hfs_dotgitmodules(path) || is_ntfs_dotgitmodules(path);\n+\tint res = 0, expect = 1;\n+\tfor (; *argv; argv++) {\n+\t\tif (!strcmp(\"--not\", *argv))\n+\t\t\texpect = !expect;\n+\t\telse if (expect != (is_hfs(*argv) || is_ntfs(*argv)))\n+\t\t\tres = error(\"'%s' is %s.git%s\", *argv,\n+\t\t\t\t expect ? \"not \" : \"\", x);\n+\t\telse\n+\t\t\tfprintf(stderr, \"ok: '%s' is %s.git%s\\n\",\n+\t\t\t\t*argv, expect ? \"\" : \"not \", x);\n+\t}\n+\treturn !!res;\n }\n \n static int cmp_by_st_size(const void *a, const void *b)\n@@ -382,17 +395,24 @@ int cmd__path_utils(int argc, const char **argv)\n \t\treturn test_function(dirname_data, posix_dirname, argv[1]);\n \n \tif (argc > 2 && !strcmp(argv[1], \"is_dotgitmodules\")) {\n-\t\tint res = 0, expect = 1, i;\n-\t\tfor (i = 2; i < argc; i++)\n-\t\t\tif (!strcmp(\"--not\", argv[i]))\n-\t\t\t\texpect = !expect;\n-\t\t\telse if (expect != is_dotgitmodules(argv[i]))\n-\t\t\t\tres = error(\"'%s' is %s.gitmodules\", argv[i],\n-\t\t\t\t\t expect ? \"not \" : \"\");\n-\t\t\telse\n-\t\t\t\tfprintf(stderr, \"ok: '%s' is %s.gitmodules\\n\",\n-\t\t\t\t\targv[i], expect ? \"\" : \"not \");\n-\t\treturn !!res;\n+\t\treturn check_dotfile(\"modules\", argv + 2,\n+\t\t\t\t is_hfs_dotgitmodules,\n+\t\t\t\t is_ntfs_dotgitmodules);\n+\t}\n+\tif (argc > 2 && !strcmp(argv[1], \"is_dotgitignore\")) {\n+\t\treturn check_dotfile(\"ignore\", argv + 2,\n+\t\t\t\t is_hfs_dotgitignore,\n+\t\t\t\t is_ntfs_dotgitignore);\n+\t}\n+\tif (argc > 2 && !strcmp(argv[1], \"is_dotgitattributes\")) {\n+\t\treturn check_dotfile(\"attributes\", argv + 2,\n+\t\t\t\t is_hfs_dotgitattributes,\n+\t\t\t\t is_ntfs_dotgitattributes);\n+\t}\n+\tif (argc > 2 && !strcmp(argv[1], \"is_dotmailmap\")) {\n+\t\treturn check_dotfile(\"mailmap\", argv + 2,\n+\t\t\t\t is_hfs_dotmailmap,\n+\t\t\t\t is_ntfs_dotmailmap);\n \t}\n \n \tif (argc > 2 && !strcmp(argv[1], \"file-size\")) {\ndiff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh\nindex 0ff06b5d1b..de4960783f 100755\n--- a/t/t0060-path-utils.sh\n+++ b/t/t0060-path-utils.sh\n@@ -468,6 +468,36 @@ test_expect_success 'match .gitmodules' '\n \t\t.gitmodules,:\\$DATA\n '\n \n+test_expect_success 'match .gitattributes' '\n+\ttest-tool path-utils is_dotgitattributes \\\n+\t\t.gitattributes \\\n+\t\t.git${u200c}attributes \\\n+\t\t.Gitattributes \\\n+\t\t.gitattributeS \\\n+\t\tGITATT~1 \\\n+\t\tGI7D29~1\n+'\n+\n+test_expect_success 'match .gitignore' '\n+\ttest-tool path-utils is_dotgitignore \\\n+\t\t.gitignore \\\n+\t\t.git${u200c}ignore \\\n+\t\t.Gitignore \\\n+\t\t.gitignorE \\\n+\t\tGITIGN~1 \\\n+\t\tGI250A~1\n+'\n+\n+test_expect_success 'match .mailmap' '\n+\ttest-tool path-utils is_dotmailmap \\\n+\t\t.mailmap \\\n+\t\t.mail${u200c}map \\\n+\t\t.Mailmap \\\n+\t\t.mailmaP \\\n+\t\tMAILMA~1 \\\n+\t\tMABA30~1\n+'\n+\n test_expect_success MINGW 'is_valid_path() on Windows' '\n \ttest-tool path-utils is_valid_path \\\n \t\twin32 \\\ndiff --git a/utf8.c b/utf8.c\nindex 5b39361ada..de4ce5c0e6 100644\n--- a/utf8.c\n+++ b/utf8.c\n@@ -777,6 +777,11 @@ int is_hfs_dotgitattributes(const char *path)\n \treturn is_hfs_dot_str(path, \"gitattributes\");\n }\n \n+int is_hfs_dotmailmap(const char *path)\n+{\n+\treturn is_hfs_dot_str(path, \"mailmap\");\n+}\n+\n const char utf8_bom[] = \"\\357\\273\\277\";\n \n int skip_utf8_bom(char **text, size_t len)\ndiff --git a/utf8.h b/utf8.h\nindex fcd5167baf..9a16c8679d 100644\n--- a/utf8.h\n+++ b/utf8.h\n@@ -61,6 +61,7 @@ int is_hfs_dotgit(const char *path);\n int is_hfs_dotgitmodules(const char *path);\n int is_hfs_dotgitignore(const char *path);\n int is_hfs_dotgitattributes(const char *path);\n+int is_hfs_dotmailmap(const char *path);\n \n typedef enum {\n \tALIGN_LEFT,\n-- \n2.31.1.875.g5dccece0aa\n\n"},{"id":"423415","messageId":"YI13CDSp59PKkM9p@coredump.intra.peff.net","threadId":"55604","inReplyTo":"YI12hK4X/gfl3u29@coredump.intra.peff.net","subject":"[PATCH 8/9] fsck: warn about symlinked dotfiles we'll open with O_NOFOLLOW","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-01T15:43:04Z","receivedAt":"2021-05-01T15:43:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In the commits merged in via 204333b015 (Merge branch\n'jk/open-dotgitx-with-nofollow', 2021-03-22), we stopped following\nsymbolic links for .gitattributes, .gitignore, and .mailmap files.\n\nLet's teach fsck to warn that these symlinks are not going to do\nanything. Note that this is just a warning, and won't block the objects\nvia transfer.fsckObjects, since there are reported to be cases of this\nin the wild (and even once fixed, they will continue to exist in the\ncommit history of those projects, but are not particularly dangerous).\n\nNote that we won't add these to the existing gitmodules block in the\nfsck code. The logic for gitmodules is a bit more complicated, as we\nalso check the content of non-symlink instances we find. But for these\nnew files, there is no content check; we're just looking at the name and\nmode of the tree entry (and we can avoid even the complicated name\nchecks in the common case that the mode doesn't indicate a symlink).\n\nWe can reuse the test helper function we defined for .gitmodules, though\n(it needs some slight adjustments for the fsck error code, and because\nwe don't block these symlinks via verify_path()).\n\nNote that I didn't explicitly test the transfer.fsckObjects case here\n(nor does the existing .gitmodules test that it blocks a push). The\ntranslation of fsck severities to outcomes is covered in general in\nt5504.\n\nSigned-off-by: Jeff King \n---\n fsck.c | 18 ++++++++++++++++++\n fsck.h | 3 +++\n t/t7450-bad-git-dotfiles.sh | 29 +++++++++++++++++++++++++++--\n 3 files changed, 48 insertions(+), 2 deletions(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex db94817898..3ec500d707 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -614,6 +614,24 @@ static int fsck_tree(const struct object_id *tree_oid,\n \t\t\t\t\t\t \".gitmodules is a symbolic link\");\n \t\t}\n \n+\t\tif (S_ISLNK(mode)) {\n+\t\t\tif (is_hfs_dotgitignore(name) ||\n+\t\t\t is_ntfs_dotgitignore(name))\n+\t\t\t\tretval += report(options, tree_oid, OBJ_TREE,\n+\t\t\t\t\t\t FSCK_MSG_GITIGNORE_SYMLINK,\n+\t\t\t\t\t\t \".gitignore is a symlink\");\n+\t\t\tif (is_hfs_dotgitattributes(name) ||\n+\t\t\t is_ntfs_dotgitattributes(name))\n+\t\t\t\tretval += report(options, tree_oid, OBJ_TREE,\n+\t\t\t\t\t\t FSCK_MSG_GITATTRIBUTES_SYMLINK,\n+\t\t\t\t\t\t \".gitattributes is a symlink\");\n+\t\t\tif (is_hfs_dotmailmap(name) ||\n+\t\t\t is_ntfs_dotmailmap(name))\n+\t\t\t\tretval += report(options, tree_oid, OBJ_TREE,\n+\t\t\t\t\t\t FSCK_MSG_MAILMAP_SYMLINK,\n+\t\t\t\t\t\t \".mailmap is a symlink\");\n+\t\t}\n+\n \t\tif ((backslash = strchr(name, '\\\\'))) {\n \t\t\twhile (backslash) {\n \t\t\t\tbackslash++;\ndiff --git a/fsck.h b/fsck.h\nindex 7202c3c87e..d07f7a2459 100644\n--- a/fsck.h\n+++ b/fsck.h\n@@ -67,6 +67,9 @@ enum fsck_msg_type {\n \tFUNC(NUL_IN_COMMIT, WARN) \\\n \t/* infos (reported as warnings, but ignored by default) */ \\\n \tFUNC(GITMODULES_PARSE, INFO) \\\n+\tFUNC(GITIGNORE_SYMLINK, INFO) \\\n+\tFUNC(GITATTRIBUTES_SYMLINK, INFO) \\\n+\tFUNC(MAILMAP_SYMLINK, INFO) \\\n \tFUNC(BAD_TAG_NAME, INFO) \\\n \tFUNC(MISSING_TAGGER_ENTRY, INFO) \\\n \t/* ignored (elevated when requested) */ \\\ndiff --git a/t/t7450-bad-git-dotfiles.sh b/t/t7450-bad-git-dotfiles.sh\nindex 4e142c798f..164282ab8f 100755\n--- a/t/t7450-bad-git-dotfiles.sh\n+++ b/t/t7450-bad-git-dotfiles.sh\n@@ -140,6 +140,18 @@ test_expect_success 'index-pack --strict works for non-repo pack' '\n '\n \n check_dotx_symlink () {\n+\tfsck_must_fail=test_must_fail\n+\tfsck_prefix=error\n+\trefuse_index=t\n+\tcase \"$1\" in\n+\t--warning)\n+\t\tfsck_must_fail=\n+\t\tfsck_prefix=warning\n+\t\trefuse_index=\n+\t\tshift\n+\t\t;;\n+\tesac\n+\n \tname=$1\n \ttype=$2\n \tpath=$3\n@@ -172,11 +184,12 @@ check_dotx_symlink () {\n \n \t\t\t# Check not only that we fail, but that it is due to the\n \t\t\t# symlink detector\n-\t\t\ttest_must_fail git fsck 2>output &&\n-\t\t\tgrep \"tree $tree: ${name}Symlink\" output\n+\t\t\t$fsck_must_fail git fsck 2>output &&\n+\t\t\tgrep \"$fsck_prefix.*tree $tree: ${name}Symlink\" output\n \t\t)\n \t'\n \n+\ttest -n \"$refuse_index\" &&\n \ttest_expect_success \"refuse to load symlinked $name into index ($type)\" '\n \t\ttest_must_fail \\\n \t\t\tgit -C $dir \\\n@@ -193,6 +206,18 @@ check_dotx_symlink gitmodules vanilla .gitmodules\n check_dotx_symlink gitmodules ntfs \".gitmodules .\"\n check_dotx_symlink gitmodules hfs \".${u200c}gitmodules\"\n \n+check_dotx_symlink --warning gitattributes vanilla .gitattributes\n+check_dotx_symlink --warning gitattributes ntfs \".gitattributes .\"\n+check_dotx_symlink --warning gitattributes hfs \".${u200c}gitattributes\"\n+\n+check_dotx_symlink --warning gitignore vanilla .gitignore\n+check_dotx_symlink --warning gitignore ntfs \".gitignore .\"\n+check_dotx_symlink --warning gitignore hfs \".${u200c}gitignore\"\n+\n+check_dotx_symlink --warning mailmap vanilla .mailmap\n+check_dotx_symlink --warning mailmap ntfs \".mailmap .\"\n+check_dotx_symlink --warning mailmap hfs \".${u200c}mailmap\"\n+\n test_expect_success 'fsck detects non-blob .gitmodules' '\n \tgit init non-blob &&\n \t(\n-- \n2.31.1.875.g5dccece0aa\n\n"},{"id":"423416","messageId":"YI13DTkOpDRP4YGa@coredump.intra.peff.net","threadId":"55604","inReplyTo":"YI12hK4X/gfl3u29@coredump.intra.peff.net","subject":"[PATCH 9/9] docs: document symlink restrictions for dot-files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-01T15:43:09Z","receivedAt":"2021-05-01T15:43:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We stopped allowing symlinks for .gitmodules files in 10ecfa7649\n(verify_path: disallow symlinks in .gitmodules, 2018-05-04), and we\nstopped following symlinks for .gitattributes, .gitignore, and .mailmap\nin the commits from 204333b015 (Merge branch 'jk/open-dotgitx-with-nofollow',\n2021-03-22). The reasons are discussed in detail there, but we never\nadjusted the documentation to let users know.\n\nThis hasn't been a big deal since the point is that such setups were\nmildly broken and thought to be unusual anyway. But it certainly doesn't\nhurt to be clear and explicit about it.\n\nSuggested-by: Philip Oakley \nSigned-off-by: Jeff King \n---\n Documentation/gitattributes.txt | 7 +++++++\n Documentation/gitignore.txt | 5 +++++\n Documentation/gitmailmap.txt | 7 +++++++\n Documentation/gitmodules.txt | 8 ++++++++\n 4 files changed, 27 insertions(+)\n\ndiff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt\nindex cfcfa800c2..dfda94d996 100644\n--- a/Documentation/gitattributes.txt\n+++ b/Documentation/gitattributes.txt\n@@ -1247,6 +1247,13 @@ to:\n [attr]binary -diff -merge -text\n ------------\n \n+NOTES\n+-----\n+\n+Note that Git does not follow symbolic links when accessing a\n+`.gitattributes` file in the working tree. This keeps behavior\n+consistent when the file is accessed from the index or a tree versus\n+from the filesystem.\n \n EXAMPLES\n --------\ndiff --git a/Documentation/gitignore.txt b/Documentation/gitignore.txt\nindex 5751603b13..4b6fd8d2cd 100644\n--- a/Documentation/gitignore.txt\n+++ b/Documentation/gitignore.txt\n@@ -149,6 +149,11 @@ not tracked by Git remain untracked.\n To stop tracking a file that is currently tracked, use\n 'git rm --cached'.\n \n+Note that Git does not follow symbolic links when accessing a\n+`.gitignore` file in the working tree. This keeps behavior consistent\n+when the file is accessed from the index or a tree versus from the\n+filesystem.\n+\n EXAMPLES\n --------\n \ndiff --git a/Documentation/gitmailmap.txt b/Documentation/gitmailmap.txt\nindex 3fb39f801f..eb65eeb37f 100644\n--- a/Documentation/gitmailmap.txt\n+++ b/Documentation/gitmailmap.txt\n@@ -55,6 +55,13 @@ this would also match the 'Commit Name ' above:\n \tProper Name CoMmIt NaMe \n --\n \n+NOTES\n+-----\n+\n+Note that Git does not follow symbolic links when accessing a `.mailmap`\n+file in the working tree. This keeps behavior consistent when the file\n+is accessed from the index or a tree versus from the filesystem.\n+\n EXAMPLES\n --------\n \ndiff --git a/Documentation/gitmodules.txt b/Documentation/gitmodules.txt\nindex 8e333dde1b..ca1c42b405 100644\n--- a/Documentation/gitmodules.txt\n+++ b/Documentation/gitmodules.txt\n@@ -98,6 +98,14 @@ submodule..shallow::\n \tshallow clone (with a history depth of 1) unless the user explicitly\n \tasks for a non-shallow clone.\n \n+NOTES\n+-----\n+\n+Note that Git does not allow the `.gitmodules` file within a working\n+tree to be a symbolic link, and will refuse to check out such a tree\n+entry. This keeps behavior consistent when the file is accessed from the\n+index or a tree versus from the filesystem, and helps Git reliably\n+enforce security checks of the file contents.\n \n EXAMPLES\n --------\n-- \n2.31.1.875.g5dccece0aa\n"},{"id":"423421","messageId":"CAPig+cSxB89mJcjaoLMLEgqdqi9SSP5+R9qjbVydvA-kVq-2+g@mail.gmail.com","threadId":"55604","inReplyTo":"YI124aZ1dbY5O67J@coredump.intra.peff.net","subject":"Re: [PATCH 5/9] t7450: test verify_path() handling of gitmodules","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-05-01T18:55:11Z","receivedAt":"2021-05-01T18:55:24Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, May 1, 2021 at 11:42 AM Jeff King wrote:\n> Commit 10ecfa7649 (verify_path: disallow symlinks in .gitmodules,\n> 2018-05-04) made it impossible to load a symlink .gitmodules file into\n> the index. However, there are no tests of this behavior. Let's make sure\n> this case is covered. We can easily reuse the test setup created by\n> the matching b7b1fca175 (fsck: complain when .gitmodules is a symlink,\n> 2018-05-04).\n>\n> Signed-off-by: Jeff King \n> ---\n> diff --git a/t/t7450-bad-git-dotfiles.sh b/t/t7450-bad-git-dotfiles.sh\n> @@ -139,7 +139,7 @@ test_expect_success 'index-pack --strict works for non-repo pack' '\n> -test_expect_success 'fsck detects symlinked .gitmodules file' '\n> +test_expect_success 'set up repo with symlinked .gitmodules file' '\n> git init symlink &&\n> (\n> cd symlink &&\n> @@ -155,8 +155,14 @@ test_expect_success 'fsck detects symlinked .gitmodules file' '\n> {\n> printf \"100644 blob $content\\t$tricky\\n\" &&\n> printf \"120000 blob $target\\t.gitmodules\\n\"\n> - } >bad-tree &&\n> - tree=$(git mktree + } >bad-tree\n> + ) &&\n> + tree=$(git -C symlink mktree +'\n\n`tree` is an unusually generic name for this now-global variable. One\ncan easily imagine it being re-used by some unrelated test arbitrarily\ninserted into this script, thus potentially breaking the following\ntests which depend upon it. I wonder if a name such as `BAD_TREE`\nwould be more appropriate.\n"},{"id":"423422","messageId":"CAPig+cQFUUncb17MFAvJAh2KFxK_OCq_6AoOwPViYyQJUDVJNg@mail.gmail.com","threadId":"55604","inReplyTo":"CAPig+cSxB89mJcjaoLMLEgqdqi9SSP5+R9qjbVydvA-kVq-2+g@mail.gmail.com","subject":"Re: [PATCH 5/9] t7450: test verify_path() handling of gitmodules","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-05-01T19:03:56Z","receivedAt":"2021-05-01T19:04:10Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, May 1, 2021 at 2:55 PM Eric Sunshine wrote:\n> On Sat, May 1, 2021 at 11:42 AM Jeff King wrote:\n> > + tree=$(git -C symlink mktree \n> `tree` is an unusually generic name for this now-global variable. One\n> can easily imagine it being re-used by some unrelated test arbitrarily\n> inserted into this script, thus potentially breaking the following\n> tests which depend upon it. I wonder if a name such as `BAD_TREE`\n> would be more appropriate.\n\nI see that all `$tree` references get encapsulated into a shell\nfunction by the next patch, so perhaps the generic name `tree` isn't a\nbig deal after all.\n"},{"id":"423423","messageId":"CAPig+cTmL3R=xB_zjFLZkZ3X4pOABj-OvLsbDsW91WKCv-bjHA@mail.gmail.com","threadId":"55604","inReplyTo":"YI13DTkOpDRP4YGa@coredump.intra.peff.net","subject":"Re: [PATCH 9/9] docs: document symlink restrictions for dot-files","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-05-01T19:16:09Z","receivedAt":"2021-05-01T19:16:24Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, May 1, 2021 at 11:43 AM Jeff King wrote:\n> We stopped allowing symlinks for .gitmodules files in 10ecfa7649\n> (verify_path: disallow symlinks in .gitmodules, 2018-05-04), and we\n> stopped following symlinks for .gitattributes, .gitignore, and .mailmap\n> in the commits from 204333b015 (Merge branch 'jk/open-dotgitx-with-nofollow',\n> 2021-03-22). The reasons are discussed in detail there, but we never\n> adjusted the documentation to let users know.\n>\n> This hasn't been a big deal since the point is that such setups were\n> mildly broken and thought to be unusual anyway. But it certainly doesn't\n> hurt to be clear and explicit about it.\n\nJust a really microscopic nit... feel free to ignore...\n\n> diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt\n> @@ -1247,6 +1247,13 @@ to:\n> +NOTES\n> +-----\n> +\n> +Note that Git does not follow symbolic links when accessing a\n> +`.gitattributes` file in the working tree. This keeps behavior\n> +consistent when the file is accessed from the index or a tree versus\n> +from the filesystem.\n\nWe're in the \"NOTES\" section, so it feels redundant to begin the\nsentence with \"Note that\".\n\n> diff --git a/Documentation/gitmodules.txt b/Documentation/gitmodules.txt\n> @@ -98,6 +98,14 @@ submodule..shallow::\n> +NOTES\n> +-----\n> +\n> +Note that Git does not allow the `.gitmodules` file within a working\n> +tree to be a symbolic link, and will refuse to check out such a tree\n> +entry. This keeps behavior consistent when the file is accessed from the\n> +index or a tree versus from the filesystem, and helps Git reliably\n> +enforce security checks of the file contents.\n\nDitto.\n"},{"id":"423467","messageId":"xmqqfsz48jll.fsf@gitster.g","threadId":"55604","inReplyTo":"YI12hK4X/gfl3u29@coredump.intra.peff.net","subject":"Re: [PATCH 0/9] leftover bits from symlinked gitattributes, etc topics","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-05-03T05:36:06Z","receivedAt":"2021-05-03T05:36:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> A while back, I had a topic[1] that treated symlinked .gitattributes\n> (and .gitignore and .mailmap) the same as .gitmodules: forbidding them\n> in the index, complaining about them via fsck, etc.\n>\n> In the end, we decided not to do that[2], and instead just open the\n> files with O_NOFOLLOW instead. As I said in that thread, we could\n> salvage some of the cleanups, fsck checks, and docs from the original\n> topic. So here that is. (The new topic is in master but not yet\n> released; so while this is not strictly a bug-fix for an existing topic,\n> it would be good to get especially the doc improvements into the same\n> release).\n\nThanks.\n\n\n> [1] https://lore.kernel.org/git/20201005071751.GA2290770@coredump.intra.peff.net/\n> [2] https://lore.kernel.org/git/YDiWs6yyv3U9YvC2@coredump.intra.peff.net/\n>\n> The patches are:\n>\n> [1/9]: t7415: remove out-dated comment about translation\n> [2/9]: fsck_tree(): fix shadowed variable\n> [3/9]: fsck_tree(): wrap some long lines\n>\n> These three are really independent fixes and cleanups, that could be\n> taken separately.\n>\n> [4/9]: t7415: rename to expand scope\n> [5/9]: t7450: test verify_path() handling of gitmodules\n> [6/9]: t7450: test .gitmodules symlink matching against obscured names\n>\n> These three are just cleaning up and improving the tests for the\n> existing handling of the .gitmodules symlinks.\n>\n> [7/9]: t0060: test ntfs/hfs-obscured dotfiles\n> [8/9]: fsck: warn about symlinked dotfiles we'll open with O_NOFOLLOW\n>\n> These two extend fsck checks (warnings, not errors) to the new\n> files. I think this is an improvement, but I could be persuaded\n> otherwise (and these two could be dropped independent of the rest).\n>\n> [9/9]: docs: document symlink restrictions for dot-files\n>\n> And this covers documentation for all of the files (including\n> .gitmodules).\n>\n> Documentation/gitattributes.txt | 7 ++\n> Documentation/gitignore.txt | 5 +\n> Documentation/gitmailmap.txt | 7 ++\n> Documentation/gitmodules.txt | 8 ++\n> cache.h | 1 +\n> fsck.c | 84 ++++++++++---\n> fsck.h | 3 +\n> path.c | 5 +\n> t/helper/test-path-utils.c | 46 +++++--\n> t/t0060-path-utils.sh | 30 +++++\n> ...ule-names.sh => t7450-bad-git-dotfiles.sh} | 116 +++++++++++++-----\n> utf8.c | 5 +\n> utf8.h | 1 +\n> 13 files changed, 257 insertions(+), 61 deletions(-)\n> rename t/{t7415-submodule-names.sh => t7450-bad-git-dotfiles.sh} (70%)\n>\n> -Peff\n"},{"id":"423475","messageId":"874kfkxhxe.fsf@evledraar.gmail.com","threadId":"55604","inReplyTo":"YI12n+1rbedpwyfW@coredump.intra.peff.net","subject":"Re: [PATCH 1/9] t7415: remove out-dated comment about translation","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-05-03T09:46:52Z","receivedAt":"2021-05-03T09:53:07Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sat, May 01 2021, Jeff King wrote:\n\n> Since GETTEXT_POISON does not exist anymore, there is no point warning\n> people about whether we should use test_i18ngrep. This is doubly\n> confusing because the comment was describing why it was OK to use grep,\n> but it got caught up in the mass conversion of 674ba34038 (fsck: mark\n> strings for translation, 2018-11-10).\n>\n> Note there are other uses of test_i18ngrep in this script which are now\n> obsolete; I'll save those for a mass-cleanup. My goal here was just to\n> fix the confusing comment in code I'm about to refactor.\n\nFor what it's worth between [1] and [2] I'm not sure what to do about\nthe test_i18ngrep cleanup. I think your patch below is fine, but the\n\"test_i18ngrep\" has mutated into a \"grep with debugging\", not just\nsomething needed for GETTEXT_POISON.\n\nSo that part of your patch right now is making it less friendly for\ndebugging. I don't care, and think if we want that we'd be better of\nscraping the trace ouput for such common cases and/or use \"verbose grep\n[...]\" and teach the \"verbose\" wrapper about these common cases, but\nknowing of that objection + having other outstanding things has been why\nI haven't sent s/test_i18ngrep/grep/g patches.\n\n1. https://lore.kernel.org/git/20210120152132.GC8396@szeder.dev/\n2. https://lore.kernel.org/git/20210120182759.31102-1-avarab@gmail.com/\n\n> Signed-off-by: Jeff King \n> ---\n> t/t7415-submodule-names.sh | 5 ++---\n> 1 file changed, 2 insertions(+), 3 deletions(-)\n>\n> diff --git a/t/t7415-submodule-names.sh b/t/t7415-submodule-names.sh\n> index f70368bc2e..fef6561d80 100755\n> --- a/t/t7415-submodule-names.sh\n> +++ b/t/t7415-submodule-names.sh\n> @@ -151,10 +151,9 @@ test_expect_success 'fsck detects symlinked .gitmodules file' '\n> \t\t} | git mktree &&\n> \n> \t\t# Check not only that we fail, but that it is due to the\n> -\t\t# symlink detector; this grep string comes from the config\n> -\t\t# variable name and will not be translated.\n> +\t\t# symlink detector\n> \t\ttest_must_fail git fsck 2>output &&\n> -\t\ttest_i18ngrep gitmodulesSymlink output\n> +\t\tgrep gitmodulesSymlink output\n> \t)\n> '\n\n"},{"id":"423476","messageId":"871raoxh09.fsf@evledraar.gmail.com","threadId":"55604","inReplyTo":"YI124aZ1dbY5O67J@coredump.intra.peff.net","subject":"Re: [PATCH 5/9] t7450: test verify_path() handling of gitmodules","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-05-03T10:12:32Z","receivedAt":"2021-05-03T10:12:58Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sat, May 01 2021, Jeff King wrote:\n\n> +test_expect_success 'refuse to load symlinked .gitmodules into index' '\n> +\ttest_must_fail git -C symlink read-tree $tree 2>err &&\n> +\ttest_i18ngrep \"invalid path.*gitmodules\" err &&\n> +\tgit -C symlink ls-files >out &&\n> +\ttest_must_be_empty out\n> +'\n> +\n\nIn 1/9 a test_i18ngrep is removed, but here's a new one.\n"},{"id":"423478","messageId":"87y2cwvz9e.fsf@evledraar.gmail.com","threadId":"55604","inReplyTo":"YI12stO48egyiHjv@coredump.intra.peff.net","subject":"Re: [PATCH 2/9] fsck_tree(): fix shadowed variable","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-05-03T11:15:03Z","receivedAt":"2021-05-03T11:21:37Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sat, May 01 2021, Jeff King wrote:\n\n> Commit b2f2039c2b (fsck: accept an oid instead of a \"struct tree\" for\n> fsck_tree(), 2019-10-18) introduced a new \"oid\" parameter to\n> fsck_tree(), and we pass it to the report() function when we find\n> problems. However, that is shadowed within the tree-walking loop by the\n> existing \"oid\" variable which we use to store the oid of each tree\n> entry. As a result, we may report the wrong oid for some problems we\n> detect within the loop (the entry oid, instead of the tree oid).\n>\n> Our tests didn't catch this because they checked only that we found the\n> expected fsck problem, not that it was attached to the correct object.\n>\n> Let's rename both variables in the function to avoid confusion. This\n> makes the diff a little noisy (e.g., all of the report() calls outside\n> the loop were already correct but need to be touched), but makes sure we\n> catch all cases and will avoid similar confusion in the future.\n>\n> And we can update the test to be a bit more specific and catch this\n> problem.\n\nHave you considered just passing down the \"obj\" instead of the \"oid\"?\nIt's a bit of a bigger change, but seems to be worth it in this case as\nthe below diff (on top of master) shows. We spend quite a bit of effort\npeeling an \"obj\" just to pass oid/type further down the stack.\n\nIf you're interested in picking that up consider it to have by SOB, but\nI don't think me submitting it while you have this in-flight is worth\nthe conflict.\n\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex 87a99b0108e..5ce8d4792d6 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -51,18 +51,18 @@ static int name_objects;\n #define ERROR_COMMIT_GRAPH 020\n #define ERROR_MULTI_PACK_INDEX 040\n \n-static const char *describe_object(const struct object_id *oid)\n+static const char *describe_object(const struct object *obj)\n {\n-\treturn fsck_describe_object(&fsck_walk_options, oid);\n+\treturn fsck_describe_object(&fsck_walk_options, obj);\n }\n \n-static const char *printable_type(const struct object_id *oid,\n-\t\t\t\t enum object_type type)\n+static const char *printable_type(const struct object *obj)\n {\n \tconst char *ret;\n+\tenum object_type type = obj->type;\n \n \tif (type == OBJ_NONE)\n-\t\ttype = oid_object_info(the_repository, oid, NULL);\n+\t\ttype = oid_object_info(the_repository, &obj->oid, NULL);\n \n \tret = type_name(type);\n \tif (!ret)\n@@ -76,14 +76,13 @@ static int objerror(struct object *obj, const char *err)\n \terrors_found |= ERROR_OBJECT;\n \t/* TRANSLATORS: e.g. error in tree 01bfda: */\n \tfprintf_ln(stderr, _(\"error in %s %s: %s\"),\n-\t\t printable_type(&obj->oid, obj->type),\n-\t\t describe_object(&obj->oid), err);\n+\t\t printable_type(obj),\n+\t\t describe_object(obj), err);\n \treturn -1;\n }\n \n static int fsck_error_func(struct fsck_options *o,\n-\t\t\t const struct object_id *oid,\n-\t\t\t enum object_type object_type,\n+\t\t\t const struct object *obj,\n \t\t\t enum fsck_msg_type msg_type,\n \t\t\t enum fsck_msg_id msg_id,\n \t\t\t const char *message)\n@@ -92,14 +91,14 @@ static int fsck_error_func(struct fsck_options *o,\n \tcase FSCK_WARN:\n \t\t/* TRANSLATORS: e.g. warning in tree 01bfda: */\n \t\tfprintf_ln(stderr, _(\"warning in %s %s: %s\"),\n-\t\t\t printable_type(oid, object_type),\n-\t\t\t describe_object(oid), message);\n+\t\t\t printable_type(obj),\n+\t\t\t describe_object(obj), message);\n \t\treturn 0;\n \tcase FSCK_ERROR:\n \t\t/* TRANSLATORS: e.g. error in tree 01bfda: */\n \t\tfprintf_ln(stderr, _(\"error in %s %s: %s\"),\n-\t\t\t printable_type(oid, object_type),\n-\t\t\t describe_object(oid), message);\n+\t\t\t printable_type(obj),\n+\t\t\t describe_object(obj), message);\n \t\treturn 1;\n \tdefault:\n \t\tBUG(\"%d (FSCK_IGNORE?) should never trigger this callback\",\n@@ -121,8 +120,8 @@ static int mark_object(struct object *obj, int type, void *data, struct fsck_opt\n \tif (!obj) {\n \t\t/* ... these references to parent->fld are safe here */\n \t\tprintf_ln(_(\"broken link from %7s %s\"),\n-\t\t\t printable_type(&parent->oid, parent->type),\n-\t\t\t describe_object(&parent->oid));\n+\t\t\t printable_type(parent),\n+\t\t\t describe_object(parent));\n \t\tprintf_ln(_(\"broken link from %7s %s\"),\n \t\t\t (type == OBJ_ANY ? _(\"unknown\") : type_name(type)),\n \t\t\t _(\"unknown\"));\n@@ -150,10 +149,10 @@ static int mark_object(struct object *obj, int type, void *data, struct fsck_opt\n \t\tif (parent && !has_object(the_repository, &obj->oid, 1)) {\n \t\t\tprintf_ln(_(\"broken link from %7s %s\\n\"\n \t\t\t\t \" to %7s %s\"),\n-\t\t\t\t printable_type(&parent->oid, parent->type),\n-\t\t\t\t describe_object(&parent->oid),\n-\t\t\t\t printable_type(&obj->oid, obj->type),\n-\t\t\t\t describe_object(&obj->oid));\n+\t\t\t\t printable_type(parent),\n+\t\t\t\t describe_object(parent),\n+\t\t\t\t printable_type(obj),\n+\t\t\t\t describe_object(obj));\n \t\t\terrors_found |= ERROR_REACHABLE;\n \t\t}\n \t\treturn 1;\n@@ -261,8 +260,8 @@ static void check_reachable_object(struct object *obj)\n \t\tif (has_object_pack(&obj->oid))\n \t\t\treturn; /* it is in pack - forget about it */\n \t\tprintf_ln(_(\"missing %s %s\"),\n-\t\t\t printable_type(&obj->oid, obj->type),\n-\t\t\t describe_object(&obj->oid));\n+\t\t\t printable_type(obj),\n+\t\t\t describe_object(obj));\n \t\terrors_found |= ERROR_REACHABLE;\n \t\treturn;\n \t}\n@@ -288,8 +287,8 @@ static void check_unreachable_object(struct object *obj)\n \t */\n \tif (show_unreachable) {\n \t\tprintf_ln(_(\"unreachable %s %s\"),\n-\t\t\t printable_type(&obj->oid, obj->type),\n-\t\t\t describe_object(&obj->oid));\n+\t\t\t printable_type(obj),\n+\t\t\t describe_object(obj));\n \t\treturn;\n \t}\n \n@@ -308,12 +307,12 @@ static void check_unreachable_object(struct object *obj)\n \tif (!(obj->flags & USED)) {\n \t\tif (show_dangling)\n \t\t\tprintf_ln(_(\"dangling %s %s\"),\n-\t\t\t\t printable_type(&obj->oid, obj->type),\n-\t\t\t\t describe_object(&obj->oid));\n+\t\t\t\t printable_type(obj),\n+\t\t\t\t describe_object(obj));\n \t\tif (write_lost_and_found) {\n \t\t\tchar *filename = git_pathdup(\"lost-found/%s/%s\",\n \t\t\t\tobj->type == OBJ_COMMIT ? \"commit\" : \"other\",\n-\t\t\t\tdescribe_object(&obj->oid));\n+\t\t\t\tdescribe_object(obj));\n \t\t\tFILE *f;\n \n \t\t\tif (safe_create_leading_directories_const(filename)) {\n@@ -326,7 +325,7 @@ static void check_unreachable_object(struct object *obj)\n \t\t\t\tif (stream_blob_to_fd(fileno(f), &obj->oid, NULL, 1))\n \t\t\t\t\tdie_errno(_(\"could not write '%s'\"), filename);\n \t\t\t} else\n-\t\t\t\tfprintf(f, \"%s\\n\", describe_object(&obj->oid));\n+\t\t\t\tfprintf(f, \"%s\\n\", describe_object(obj));\n \t\t\tif (fclose(f))\n \t\t\t\tdie_errno(_(\"could not finish '%s'\"),\n \t\t\t\t\t filename);\n@@ -345,7 +344,7 @@ static void check_unreachable_object(struct object *obj)\n static void check_object(struct object *obj)\n {\n \tif (verbose)\n-\t\tfprintf_ln(stderr, _(\"Checking %s\"), describe_object(&obj->oid));\n+\t\tfprintf_ln(stderr, _(\"Checking %s\"), describe_object(obj));\n \n \tif (obj->flags & REACHABLE)\n \t\tcheck_reachable_object(obj);\n@@ -403,8 +402,8 @@ static int fsck_obj(struct object *obj, void *buffer, unsigned long size)\n \n \tif (verbose)\n \t\tfprintf_ln(stderr, _(\"Checking %s %s\"),\n-\t\t\t printable_type(&obj->oid, obj->type),\n-\t\t\t describe_object(&obj->oid));\n+\t\t\t printable_type(obj),\n+\t\t\t describe_object(obj));\n \n \tif (fsck_walk(obj, NULL, &fsck_obj_options))\n \t\tobjerror(obj, _(\"broken links\"));\n@@ -417,7 +416,7 @@ static int fsck_obj(struct object *obj, void *buffer, unsigned long size)\n \n \t\tif (!commit->parents && show_root)\n \t\t\tprintf_ln(_(\"root %s\"),\n-\t\t\t\t describe_object(&commit->object.oid));\n+\t\t\t\t describe_object(&commit->object));\n \t}\n \n \tif (obj->type == OBJ_TAG) {\n@@ -425,10 +424,10 @@ static int fsck_obj(struct object *obj, void *buffer, unsigned long size)\n \n \t\tif (show_tags && tag->tagged) {\n \t\t\tprintf_ln(_(\"tagged %s %s (%s) in %s\"),\n-\t\t\t\t printable_type(&tag->tagged->oid, tag->tagged->type),\n-\t\t\t\t describe_object(&tag->tagged->oid),\n+\t\t\t\t printable_type(tag->tagged),\n+\t\t\t\t describe_object(tag->tagged),\n \t\t\t\t tag->tag,\n-\t\t\t\t describe_object(&tag->object.oid));\n+\t\t\t\t describe_object(&tag->object));\n \t\t}\n \t}\n \ndiff --git a/builtin/mktag.c b/builtin/mktag.c\nindex dddcccdd368..4ea768d6cc1 100644\n--- a/builtin/mktag.c\n+++ b/builtin/mktag.c\n@@ -15,8 +15,7 @@ static int option_strict = 1;\n static struct fsck_options fsck_options = FSCK_OPTIONS_STRICT;\n \n static int mktag_fsck_error_func(struct fsck_options *o,\n-\t\t\t\t const struct object_id *oid,\n-\t\t\t\t enum object_type object_type,\n+\t\t\t\t const struct object *obj,\n \t\t\t\t enum fsck_msg_type msg_type,\n \t\t\t\t enum fsck_msg_id msg_id,\n \t\t\t\t const char *message)\ndiff --git a/fsck.c b/fsck.c\nindex f5ed6a26358..4ded78b5af9 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -199,14 +199,14 @@ void fsck_set_msg_types(struct fsck_options *options, const char *values)\n }\n \n static int object_on_skiplist(struct fsck_options *opts,\n-\t\t\t const struct object_id *oid)\n+\t\t\t const struct object *obj)\n {\n-\treturn opts && oid && oidset_contains(&opts->skiplist, oid);\n+\treturn opts && obj && oidset_contains(&opts->skiplist, &obj->oid);\n }\n \n-__attribute__((format (printf, 5, 6)))\n+__attribute__((format (printf, 4, 5)))\n static int report(struct fsck_options *options,\n-\t\t const struct object_id *oid, enum object_type object_type,\n+\t\t const struct object *obj,\n \t\t enum fsck_msg_id msg_id, const char *fmt, ...)\n {\n \tva_list ap;\n@@ -217,7 +217,7 @@ static int report(struct fsck_options *options,\n \tif (msg_type == FSCK_IGNORE)\n \t\treturn 0;\n \n-\tif (object_on_skiplist(options, oid))\n+\tif (object_on_skiplist(options, obj))\n \t\treturn 0;\n \n \tif (msg_type == FSCK_FATAL)\n@@ -230,7 +230,7 @@ static int report(struct fsck_options *options,\n \n \tva_start(ap, fmt);\n \tstrbuf_vaddf(&sb, fmt, ap);\n-\tresult = options->error_func(options, oid, object_type,\n+\tresult = options->error_func(options, obj,\n \t\t\t\t msg_type, msg_id, sb.buf);\n \tstrbuf_release(&sb);\n \tva_end(ap);\n@@ -278,8 +278,9 @@ void fsck_put_object_name(struct fsck_options *options,\n }\n \n const char *fsck_describe_object(struct fsck_options *options,\n-\t\t\t\t const struct object_id *oid)\n+\t\t\t\t const struct object *obj)\n {\n+\tconst struct object_id *oid = &obj->oid;\n \tstatic struct strbuf bufs[] = {\n \t\tSTRBUF_INIT, STRBUF_INIT, STRBUF_INIT, STRBUF_INIT\n \t};\n@@ -333,7 +334,7 @@ static int fsck_walk_tree(struct tree *tree, void *data, struct fsck_options *op\n \t\t}\n \t\telse {\n \t\t\tresult = error(\"in tree %s: entry %s has bad mode %.6o\",\n-\t\t\t\t fsck_describe_object(options, &tree->object.oid),\n+\t\t\t\t fsck_describe_object(options, &tree->object),\n \t\t\t\t entry.path, entry.mode);\n \t\t}\n \t\tif (result < 0)\n@@ -443,7 +444,7 @@ int fsck_walk(struct object *obj, void *data, struct fsck_options *options)\n \t\treturn fsck_walk_tag((struct tag *)obj, data, options);\n \tdefault:\n \t\terror(\"Unknown object type for %s\",\n-\t\t fsck_describe_object(options, &obj->oid));\n+\t\t fsck_describe_object(options, obj));\n \t\treturn -1;\n \t}\n }\n@@ -558,7 +559,7 @@ static int verify_ordered(unsigned mode1, const char *name1,\n \treturn c1 < c2 ? 0 : TREE_UNORDERED;\n }\n \n-static int fsck_tree(const struct object_id *oid,\n+static int fsck_tree(const struct object *obj,\n \t\t const char *buffer, unsigned long size,\n \t\t struct fsck_options *options)\n {\n@@ -579,7 +580,7 @@ static int fsck_tree(const struct object_id *oid,\n \tstruct name_stack df_dup_candidates = { NULL };\n \n \tif (init_tree_desc_gently(&desc, buffer, size)) {\n-\t\tretval += report(options, oid, OBJ_TREE, FSCK_MSG_BAD_TREE, \"cannot be parsed as a tree\");\n+\t\tretval += report(options, obj, FSCK_MSG_BAD_TREE, \"cannot be parsed as a tree\");\n \t\treturn retval;\n \t}\n \n@@ -606,7 +607,7 @@ static int fsck_tree(const struct object_id *oid,\n \t\t\t\toidset_insert(&options->gitmodules_found, oid);\n \t\t\telse\n \t\t\t\tretval += report(options,\n-\t\t\t\t\t\t oid, OBJ_TREE,\n+\t\t\t\t\t\t obj,\n \t\t\t\t\t\t FSCK_MSG_GITMODULES_SYMLINK,\n \t\t\t\t\t\t \".gitmodules is a symbolic link\");\n \t\t}\n@@ -619,7 +620,7 @@ static int fsck_tree(const struct object_id *oid,\n \t\t\t\t\tif (!S_ISLNK(mode))\n \t\t\t\t\t\toidset_insert(&options->gitmodules_found, oid);\n \t\t\t\t\telse\n-\t\t\t\t\t\tretval += report(options, oid, OBJ_TREE,\n+\t\t\t\t\t\tretval += report(options, obj,\n \t\t\t\t\t\t\t\t FSCK_MSG_GITMODULES_SYMLINK,\n \t\t\t\t\t\t\t\t \".gitmodules is a symbolic link\");\n \t\t\t\t}\n@@ -628,7 +629,7 @@ static int fsck_tree(const struct object_id *oid,\n \t\t}\n \n \t\tif (update_tree_entry_gently(&desc)) {\n-\t\t\tretval += report(options, oid, OBJ_TREE, FSCK_MSG_BAD_TREE, \"cannot be parsed as a tree\");\n+\t\t\tretval += report(options, obj, FSCK_MSG_BAD_TREE, \"cannot be parsed as a tree\");\n \t\t\tbreak;\n \t\t}\n \n@@ -676,30 +677,30 @@ static int fsck_tree(const struct object_id *oid,\n \tname_stack_clear(&df_dup_candidates);\n \n \tif (has_null_sha1)\n-\t\tretval += report(options, oid, OBJ_TREE, FSCK_MSG_NULL_SHA1, \"contains entries pointing to null sha1\");\n+\t\tretval += report(options, obj, FSCK_MSG_NULL_SHA1, \"contains entries pointing to null sha1\");\n \tif (has_full_path)\n-\t\tretval += report(options, oid, OBJ_TREE, FSCK_MSG_FULL_PATHNAME, \"contains full pathnames\");\n+\t\tretval += report(options, obj, FSCK_MSG_FULL_PATHNAME, \"contains full pathnames\");\n \tif (has_empty_name)\n-\t\tretval += report(options, oid, OBJ_TREE, FSCK_MSG_EMPTY_NAME, \"contains empty pathname\");\n+\t\tretval += report(options, obj, FSCK_MSG_EMPTY_NAME, \"contains empty pathname\");\n \tif (has_dot)\n-\t\tretval += report(options, oid, OBJ_TREE, FSCK_MSG_HAS_DOT, \"contains '.'\");\n+\t\tretval += report(options, obj, FSCK_MSG_HAS_DOT, \"contains '.'\");\n \tif (has_dotdot)\n-\t\tretval += report(options, oid, OBJ_TREE, FSCK_MSG_HAS_DOTDOT, \"contains '..'\");\n+\t\tretval += report(options, obj, FSCK_MSG_HAS_DOTDOT, \"contains '..'\");\n \tif (has_dotgit)\n-\t\tretval += report(options, oid, OBJ_TREE, FSCK_MSG_HAS_DOTGIT, \"contains '.git'\");\n+\t\tretval += report(options, obj, FSCK_MSG_HAS_DOTGIT, \"contains '.git'\");\n \tif (has_zero_pad)\n-\t\tretval += report(options, oid, OBJ_TREE, FSCK_MSG_ZERO_PADDED_FILEMODE, \"contains zero-padded file modes\");\n+\t\tretval += report(options, obj, FSCK_MSG_ZERO_PADDED_FILEMODE, \"contains zero-padded file modes\");\n \tif (has_bad_modes)\n-\t\tretval += report(options, oid, OBJ_TREE, FSCK_MSG_BAD_FILEMODE, \"contains bad file modes\");\n+\t\tretval += report(options, obj, FSCK_MSG_BAD_FILEMODE, \"contains bad file modes\");\n \tif (has_dup_entries)\n-\t\tretval += report(options, oid, OBJ_TREE, FSCK_MSG_DUPLICATE_ENTRIES, \"contains duplicate file entries\");\n+\t\tretval += report(options, obj, FSCK_MSG_DUPLICATE_ENTRIES, \"contains duplicate file entries\");\n \tif (not_properly_sorted)\n-\t\tretval += report(options, oid, OBJ_TREE, FSCK_MSG_TREE_NOT_SORTED, \"not properly sorted\");\n+\t\tretval += report(options, obj, FSCK_MSG_TREE_NOT_SORTED, \"not properly sorted\");\n \treturn retval;\n }\n \n static int verify_headers(const void *data, unsigned long size,\n-\t\t\t const struct object_id *oid, enum object_type type,\n+\t\t\t const struct object *obj,\n \t\t\t struct fsck_options *options)\n {\n \tconst char *buffer = (const char *)data;\n@@ -708,7 +709,7 @@ static int verify_headers(const void *data, unsigned long size,\n \tfor (i = 0; i < size; i++) {\n \t\tswitch (buffer[i]) {\n \t\tcase '\\0':\n-\t\t\treturn report(options, oid, type,\n+\t\t\treturn report(options, obj,\n \t\t\t\tFSCK_MSG_NUL_IN_HEADER,\n \t\t\t\t\"unterminated header: NUL at offset %ld\", i);\n \t\tcase '\\n':\n@@ -726,12 +727,12 @@ static int verify_headers(const void *data, unsigned long size,\n \tif (size && buffer[size - 1] == '\\n')\n \t\treturn 0;\n \n-\treturn report(options, oid, type,\n-\t\tFSCK_MSG_UNTERMINATED_HEADER, \"unterminated header\");\n+\treturn report(options, obj,\n+\t\t FSCK_MSG_UNTERMINATED_HEADER, \"unterminated header\");\n }\n \n static int fsck_ident(const char **ident,\n-\t\t const struct object_id *oid, enum object_type type,\n+\t\t const struct object *obj,\n \t\t struct fsck_options *options)\n {\n \tconst char *p = *ident;\n@@ -742,28 +743,28 @@ static int fsck_ident(const char **ident,\n \t\t(*ident)++;\n \n \tif (*p == '<')\n-\t\treturn report(options, oid, type, FSCK_MSG_MISSING_NAME_BEFORE_EMAIL, \"invalid author/committer line - missing space before email\");\n+\t\treturn report(options, obj, FSCK_MSG_MISSING_NAME_BEFORE_EMAIL, \"invalid author/committer line - missing space before email\");\n \tp += strcspn(p, \"<>\\n\");\n \tif (*p == '>')\n-\t\treturn report(options, oid, type, FSCK_MSG_BAD_NAME, \"invalid author/committer line - bad name\");\n+\t\treturn report(options, obj, FSCK_MSG_BAD_NAME, \"invalid author/committer line - bad name\");\n \tif (*p != '<')\n-\t\treturn report(options, oid, type, FSCK_MSG_MISSING_EMAIL, \"invalid author/committer line - missing email\");\n+\t\treturn report(options, obj, FSCK_MSG_MISSING_EMAIL, \"invalid author/committer line - missing email\");\n \tif (p[-1] != ' ')\n-\t\treturn report(options, oid, type, FSCK_MSG_MISSING_SPACE_BEFORE_EMAIL, \"invalid author/committer line - missing space before email\");\n+\t\treturn report(options, obj, FSCK_MSG_MISSING_SPACE_BEFORE_EMAIL, \"invalid author/committer line - missing space before email\");\n \tp++;\n \tp += strcspn(p, \"<>\\n\");\n \tif (*p != '>')\n-\t\treturn report(options, oid, type, FSCK_MSG_BAD_EMAIL, \"invalid author/committer line - bad email\");\n+\t\treturn report(options, obj, FSCK_MSG_BAD_EMAIL, \"invalid author/committer line - bad email\");\n \tp++;\n \tif (*p != ' ')\n-\t\treturn report(options, oid, type, FSCK_MSG_MISSING_SPACE_BEFORE_DATE, \"invalid author/committer line - missing space before date\");\n+\t\treturn report(options, obj, FSCK_MSG_MISSING_SPACE_BEFORE_DATE, \"invalid author/committer line - missing space before date\");\n \tp++;\n \tif (*p == '0' && p[1] != ' ')\n-\t\treturn report(options, oid, type, FSCK_MSG_ZERO_PADDED_DATE, \"invalid author/committer line - zero-padded date\");\n+\t\treturn report(options, obj, FSCK_MSG_ZERO_PADDED_DATE, \"invalid author/committer line - zero-padded date\");\n \tif (date_overflows(parse_timestamp(p, &end, 10)))\n-\t\treturn report(options, oid, type, FSCK_MSG_BAD_DATE_OVERFLOW, \"invalid author/committer line - date causes integer overflow\");\n+\t\treturn report(options, obj, FSCK_MSG_BAD_DATE_OVERFLOW, \"invalid author/committer line - date causes integer overflow\");\n \tif ((end == p || *end != ' '))\n-\t\treturn report(options, oid, type, FSCK_MSG_BAD_DATE, \"invalid author/committer line - bad date\");\n+\t\treturn report(options, obj, FSCK_MSG_BAD_DATE, \"invalid author/committer line - bad date\");\n \tp = end + 1;\n \tif ((*p != '+' && *p != '-') ||\n \t !isdigit(p[1]) ||\n@@ -771,12 +772,12 @@ static int fsck_ident(const char **ident,\n \t !isdigit(p[3]) ||\n \t !isdigit(p[4]) ||\n \t (p[5] != '\\n'))\n-\t\treturn report(options, oid, type, FSCK_MSG_BAD_TIMEZONE, \"invalid author/committer line - bad time zone\");\n+\t\treturn report(options, obj, FSCK_MSG_BAD_TIMEZONE, \"invalid author/committer line - bad time zone\");\n \tp += 6;\n \treturn 0;\n }\n \n-static int fsck_commit(const struct object_id *oid,\n+static int fsck_commit(const struct object *obj,\n \t\t const char *buffer, unsigned long size,\n \t\t struct fsck_options *options)\n {\n@@ -786,20 +787,20 @@ static int fsck_commit(const struct object_id *oid,\n \tconst char *buffer_begin = buffer;\n \tconst char *p;\n \n-\tif (verify_headers(buffer, size, oid, OBJ_COMMIT, options))\n+\tif (verify_headers(buffer, size, obj, options))\n \t\treturn -1;\n \n \tif (!skip_prefix(buffer, \"tree \", &buffer))\n-\t\treturn report(options, oid, OBJ_COMMIT, FSCK_MSG_MISSING_TREE, \"invalid format - expected 'tree' line\");\n+\t\treturn report(options, obj, FSCK_MSG_MISSING_TREE, \"invalid format - expected 'tree' line\");\n \tif (parse_oid_hex(buffer, &tree_oid, &p) || *p != '\\n') {\n-\t\terr = report(options, oid, OBJ_COMMIT, FSCK_MSG_BAD_TREE_SHA1, \"invalid 'tree' line format - bad sha1\");\n+\t\terr = report(options, obj, FSCK_MSG_BAD_TREE_SHA1, \"invalid 'tree' line format - bad sha1\");\n \t\tif (err)\n \t\t\treturn err;\n \t}\n \tbuffer = p + 1;\n \twhile (skip_prefix(buffer, \"parent \", &buffer)) {\n \t\tif (parse_oid_hex(buffer, &parent_oid, &p) || *p != '\\n') {\n-\t\t\terr = report(options, oid, OBJ_COMMIT, FSCK_MSG_BAD_PARENT_SHA1, \"invalid 'parent' line format - bad sha1\");\n+\t\t\terr = report(options, obj, FSCK_MSG_BAD_PARENT_SHA1, \"invalid 'parent' line format - bad sha1\");\n \t\t\tif (err)\n \t\t\t\treturn err;\n \t\t}\n@@ -808,23 +809,23 @@ static int fsck_commit(const struct object_id *oid,\n \tauthor_count = 0;\n \twhile (skip_prefix(buffer, \"author \", &buffer)) {\n \t\tauthor_count++;\n-\t\terr = fsck_ident(&buffer, oid, OBJ_COMMIT, options);\n+\t\terr = fsck_ident(&buffer, obj, options);\n \t\tif (err)\n \t\t\treturn err;\n \t}\n \tif (author_count < 1)\n-\t\terr = report(options, oid, OBJ_COMMIT, FSCK_MSG_MISSING_AUTHOR, \"invalid format - expected 'author' line\");\n+\t\terr = report(options, obj, FSCK_MSG_MISSING_AUTHOR, \"invalid format - expected 'author' line\");\n \telse if (author_count > 1)\n-\t\terr = report(options, oid, OBJ_COMMIT, FSCK_MSG_MULTIPLE_AUTHORS, \"invalid format - multiple 'author' lines\");\n+\t\terr = report(options, obj, FSCK_MSG_MULTIPLE_AUTHORS, \"invalid format - multiple 'author' lines\");\n \tif (err)\n \t\treturn err;\n \tif (!skip_prefix(buffer, \"committer \", &buffer))\n-\t\treturn report(options, oid, OBJ_COMMIT, FSCK_MSG_MISSING_COMMITTER, \"invalid format - expected 'committer' line\");\n-\terr = fsck_ident(&buffer, oid, OBJ_COMMIT, options);\n+\t\treturn report(options, obj, FSCK_MSG_MISSING_COMMITTER, \"invalid format - expected 'committer' line\");\n+\terr = fsck_ident(&buffer, obj, options);\n \tif (err)\n \t\treturn err;\n \tif (memchr(buffer_begin, '\\0', size)) {\n-\t\terr = report(options, oid, OBJ_COMMIT, FSCK_MSG_NUL_IN_COMMIT,\n+\t\terr = report(options, obj, FSCK_MSG_NUL_IN_COMMIT,\n \t\t\t \"NUL byte in the commit object body\");\n \t\tif (err)\n \t\t\treturn err;\n@@ -832,16 +833,16 @@ static int fsck_commit(const struct object_id *oid,\n \treturn 0;\n }\n \n-static int fsck_tag(const struct object_id *oid, const char *buffer,\n+static int fsck_tag(const struct object *obj, const char *buffer,\n \t\t unsigned long size, struct fsck_options *options)\n {\n \tstruct object_id tagged_oid;\n \tint tagged_type;\n-\treturn fsck_tag_standalone(oid, buffer, size, options, &tagged_oid,\n+\treturn fsck_tag_standalone(obj, buffer, size, options, &tagged_oid,\n \t\t\t\t &tagged_type);\n }\n \n-int fsck_tag_standalone(const struct object_id *oid, const char *buffer,\n+int fsck_tag_standalone(const struct object *obj, const char *buffer,\n \t\t\tunsigned long size, struct fsck_options *options,\n \t\t\tstruct object_id *tagged_oid,\n \t\t\tint *tagged_type)\n@@ -851,49 +852,49 @@ int fsck_tag_standalone(const struct object_id *oid, const char *buffer,\n \tstruct strbuf sb = STRBUF_INIT;\n \tconst char *p;\n \n-\tret = verify_headers(buffer, size, oid, OBJ_TAG, options);\n+\tret = verify_headers(buffer, size, obj, options);\n \tif (ret)\n \t\tgoto done;\n \n \tif (!skip_prefix(buffer, \"object \", &buffer)) {\n-\t\tret = report(options, oid, OBJ_TAG, FSCK_MSG_MISSING_OBJECT, \"invalid format - expected 'object' line\");\n+\t\tret = report(options, obj, FSCK_MSG_MISSING_OBJECT, \"invalid format - expected 'object' line\");\n \t\tgoto done;\n \t}\n \tif (parse_oid_hex(buffer, tagged_oid, &p) || *p != '\\n') {\n-\t\tret = report(options, oid, OBJ_TAG, FSCK_MSG_BAD_OBJECT_SHA1, \"invalid 'object' line format - bad sha1\");\n+\t\tret = report(options, obj, FSCK_MSG_BAD_OBJECT_SHA1, \"invalid 'object' line format - bad sha1\");\n \t\tif (ret)\n \t\t\tgoto done;\n \t}\n \tbuffer = p + 1;\n \n \tif (!skip_prefix(buffer, \"type \", &buffer)) {\n-\t\tret = report(options, oid, OBJ_TAG, FSCK_MSG_MISSING_TYPE_ENTRY, \"invalid format - expected 'type' line\");\n+\t\tret = report(options, obj, FSCK_MSG_MISSING_TYPE_ENTRY, \"invalid format - expected 'type' line\");\n \t\tgoto done;\n \t}\n \teol = strchr(buffer, '\\n');\n \tif (!eol) {\n-\t\tret = report(options, oid, OBJ_TAG, FSCK_MSG_MISSING_TYPE, \"invalid format - unexpected end after 'type' line\");\n+\t\tret = report(options, obj, FSCK_MSG_MISSING_TYPE, \"invalid format - unexpected end after 'type' line\");\n \t\tgoto done;\n \t}\n \t*tagged_type = type_from_string_gently(buffer, eol - buffer, 1);\n \tif (*tagged_type < 0)\n-\t\tret = report(options, oid, OBJ_TAG, FSCK_MSG_BAD_TYPE, \"invalid 'type' value\");\n+\t\tret = report(options, obj, FSCK_MSG_BAD_TYPE, \"invalid 'type' value\");\n \tif (ret)\n \t\tgoto done;\n \tbuffer = eol + 1;\n \n \tif (!skip_prefix(buffer, \"tag \", &buffer)) {\n-\t\tret = report(options, oid, OBJ_TAG, FSCK_MSG_MISSING_TAG_ENTRY, \"invalid format - expected 'tag' line\");\n+\t\tret = report(options, obj, FSCK_MSG_MISSING_TAG_ENTRY, \"invalid format - expected 'tag' line\");\n \t\tgoto done;\n \t}\n \teol = strchr(buffer, '\\n');\n \tif (!eol) {\n-\t\tret = report(options, oid, OBJ_TAG, FSCK_MSG_MISSING_TAG, \"invalid format - unexpected end after 'type' line\");\n+\t\tret = report(options, obj, FSCK_MSG_MISSING_TAG, \"invalid format - unexpected end after 'type' line\");\n \t\tgoto done;\n \t}\n \tstrbuf_addf(&sb, \"refs/tags/%.*s\", (int)(eol - buffer), buffer);\n \tif (check_refname_format(sb.buf, 0)) {\n-\t\tret = report(options, oid, OBJ_TAG,\n+\t\tret = report(options, obj,\n \t\t\t FSCK_MSG_BAD_TAG_NAME,\n \t\t\t \"invalid 'tag' name: %.*s\",\n \t\t\t (int)(eol - buffer), buffer);\n@@ -904,12 +905,12 @@ int fsck_tag_standalone(const struct object_id *oid, const char *buffer,\n \n \tif (!skip_prefix(buffer, \"tagger \", &buffer)) {\n \t\t/* early tags do not contain 'tagger' lines; warn only */\n-\t\tret = report(options, oid, OBJ_TAG, FSCK_MSG_MISSING_TAGGER_ENTRY, \"invalid format - expected 'tagger' line\");\n+\t\tret = report(options, obj, FSCK_MSG_MISSING_TAGGER_ENTRY, \"invalid format - expected 'tagger' line\");\n \t\tif (ret)\n \t\t\tgoto done;\n \t}\n \telse\n-\t\tret = fsck_ident(&buffer, oid, OBJ_TAG, options);\n+\t\tret = fsck_ident(&buffer, obj, options);\n \tif (!*buffer)\n \t\tgoto done;\n \n@@ -921,7 +922,7 @@ int fsck_tag_standalone(const struct object_id *oid, const char *buffer,\n \t\t * garbage\" could be a custom header. E.g. \"mktag\"\n \t\t * doesn't want any unknown headers.\n \t\t */\n-\t\tret = report(options, oid, OBJ_TAG, FSCK_MSG_EXTRA_HEADER_ENTRY, \"invalid format - extra header(s) after 'tagger'\");\n+\t\tret = report(options, obj, FSCK_MSG_EXTRA_HEADER_ENTRY, \"invalid format - extra header(s) after 'tagger'\");\n \t\tif (ret)\n \t\t\tgoto done;\n \t}\n@@ -1075,7 +1076,7 @@ static int check_submodule_url(const char *url)\n }\n \n struct fsck_gitmodules_data {\n-\tconst struct object_id *oid;\n+\tconst struct object *obj;\n \tstruct fsck_options *options;\n \tint ret;\n };\n@@ -1094,27 +1095,27 @@ static int fsck_gitmodules_fn(const char *var, const char *value, void *vdata)\n \tname = xmemdupz(subsection, subsection_len);\n \tif (check_submodule_name(name) < 0)\n \t\tdata->ret |= report(data->options,\n-\t\t\t\t data->oid, OBJ_BLOB,\n+\t\t\t\t data->obj,\n \t\t\t\t FSCK_MSG_GITMODULES_NAME,\n \t\t\t\t \"disallowed submodule name: %s\",\n \t\t\t\t name);\n \tif (!strcmp(key, \"url\") && value &&\n \t check_submodule_url(value) < 0)\n \t\tdata->ret |= report(data->options,\n-\t\t\t\t data->oid, OBJ_BLOB,\n+\t\t\t\t data->obj,\n \t\t\t\t FSCK_MSG_GITMODULES_URL,\n \t\t\t\t \"disallowed submodule url: %s\",\n \t\t\t\t value);\n \tif (!strcmp(key, \"path\") && value &&\n \t looks_like_command_line_option(value))\n \t\tdata->ret |= report(data->options,\n-\t\t\t\t data->oid, OBJ_BLOB,\n+\t\t\t\t data->obj,\n \t\t\t\t FSCK_MSG_GITMODULES_PATH,\n \t\t\t\t \"disallowed submodule path: %s\",\n \t\t\t\t value);\n \tif (!strcmp(key, \"update\") && value &&\n \t parse_submodule_update_type(value) == SM_UPDATE_COMMAND)\n-\t\tdata->ret |= report(data->options, data->oid, OBJ_BLOB,\n+\t\tdata->ret |= report(data->options, data->obj,\n \t\t\t\t FSCK_MSG_GITMODULES_UPDATE,\n \t\t\t\t \"disallowed submodule update setting: %s\",\n \t\t\t\t value);\n@@ -1123,9 +1124,10 @@ static int fsck_gitmodules_fn(const char *var, const char *value, void *vdata)\n \treturn 0;\n }\n \n-static int fsck_blob(const struct object_id *oid, const char *buf,\n+static int fsck_blob(const struct object *obj, const char *buf,\n \t\t unsigned long size, struct fsck_options *options)\n {\n+\tconst struct object_id *oid = &obj->oid;\n \tstruct fsck_gitmodules_data data;\n \tstruct config_options config_opts = { 0 };\n \n@@ -1133,7 +1135,7 @@ static int fsck_blob(const struct object_id *oid, const char *buf,\n \t\treturn 0;\n \toidset_insert(&options->gitmodules_done, oid);\n \n-\tif (object_on_skiplist(options, oid))\n+\tif (object_on_skiplist(options, obj))\n \t\treturn 0;\n \n \tif (!buf) {\n@@ -1142,18 +1144,18 @@ static int fsck_blob(const struct object_id *oid, const char *buf,\n \t\t * blob too gigantic to load into memory. Let's just consider\n \t\t * that an error.\n \t\t */\n-\t\treturn report(options, oid, OBJ_BLOB,\n+\t\treturn report(options, obj,\n \t\t\t FSCK_MSG_GITMODULES_LARGE,\n \t\t\t \".gitmodules too large to parse\");\n \t}\n \n-\tdata.oid = oid;\n+\tdata.obj = obj;\n \tdata.options = options;\n \tdata.ret = 0;\n \tconfig_opts.error_action = CONFIG_ERROR_SILENT;\n \tif (git_config_from_mem(fsck_gitmodules_fn, CONFIG_ORIGIN_BLOB,\n \t\t\t\t\".gitmodules\", buf, size, &data, &config_opts))\n-\t\tdata.ret |= report(options, oid, OBJ_BLOB,\n+\t\tdata.ret |= report(options, obj,\n \t\t\t\t FSCK_MSG_GITMODULES_PARSE,\n \t\t\t\t \"could not parse gitmodules blob\");\n \n@@ -1164,35 +1166,34 @@ int fsck_object(struct object *obj, void *data, unsigned long size,\n \tstruct fsck_options *options)\n {\n \tif (!obj)\n-\t\treturn report(options, NULL, OBJ_NONE, FSCK_MSG_BAD_OBJECT_SHA1, \"no valid object to fsck\");\n+\t\treturn report(options, obj, FSCK_MSG_BAD_OBJECT_SHA1, \"no valid object to fsck\");\n \n \tif (obj->type == OBJ_BLOB)\n-\t\treturn fsck_blob(&obj->oid, data, size, options);\n+\t\treturn fsck_blob(obj, data, size, options);\n \tif (obj->type == OBJ_TREE)\n-\t\treturn fsck_tree(&obj->oid, data, size, options);\n+\t\treturn fsck_tree(obj, data, size, options);\n \tif (obj->type == OBJ_COMMIT)\n-\t\treturn fsck_commit(&obj->oid, data, size, options);\n+\t\treturn fsck_commit(obj, data, size, options);\n \tif (obj->type == OBJ_TAG)\n-\t\treturn fsck_tag(&obj->oid, data, size, options);\n+\t\treturn fsck_tag(obj, data, size, options);\n \n-\treturn report(options, &obj->oid, obj->type,\n+\treturn report(options, obj,\n \t\t FSCK_MSG_UNKNOWN_TYPE,\n \t\t \"unknown type '%d' (internal fsck error)\",\n \t\t obj->type);\n }\n \n int fsck_error_function(struct fsck_options *o,\n-\t\t\tconst struct object_id *oid,\n-\t\t\tenum object_type object_type,\n+\t\t\tconst struct object *obj,\n \t\t\tenum fsck_msg_type msg_type,\n \t\t\tenum fsck_msg_id msg_id,\n \t\t\tconst char *message)\n {\n \tif (msg_type == FSCK_WARN) {\n-\t\twarning(\"object %s: %s\", fsck_describe_object(o, oid), message);\n+\t\twarning(\"object %s: %s\", fsck_describe_object(o, obj), message);\n \t\treturn 0;\n \t}\n-\terror(\"object %s: %s\", fsck_describe_object(o, oid), message);\n+\terror(\"object %s: %s\", fsck_describe_object(o, obj), message);\n \treturn 1;\n }\n \n@@ -1204,6 +1205,7 @@ int fsck_finish(struct fsck_options *options)\n \n \toidset_iter_init(&options->gitmodules_found, &iter);\n \twhile ((oid = oidset_iter_next(&iter))) {\n+\t\tstruct object *obj;\n \t\tenum object_type type;\n \t\tunsigned long size;\n \t\tchar *buf;\n@@ -1215,20 +1217,24 @@ int fsck_finish(struct fsck_options *options)\n \t\tif (!buf) {\n \t\t\tif (is_promisor_object(oid))\n \t\t\t\tcontinue;\n+\t\t\tobj = (struct object *)lookup_blob(the_repository, oid);\n \t\t\tret |= report(options,\n-\t\t\t\t oid, OBJ_BLOB,\n+\t\t\t\t obj,\n \t\t\t\t FSCK_MSG_GITMODULES_MISSING,\n \t\t\t\t \"unable to read .gitmodules blob\");\n \t\t\tcontinue;\n \t\t}\n \n-\t\tif (type == OBJ_BLOB)\n-\t\t\tret |= fsck_blob(oid, buf, size, options);\n-\t\telse\n+\t\tif (type == OBJ_BLOB) {\n+\t\t\tobj = (struct object *)lookup_blob(the_repository, oid);\n+\t\t\tret |= fsck_blob(obj, buf, size, options);\n+\t\t} else {\n+\t\t\tobj = lookup_object(the_repository, oid);\n \t\t\tret |= report(options,\n-\t\t\t\t oid, type,\n+\t\t\t\t obj,\n \t\t\t\t FSCK_MSG_GITMODULES_BLOB,\n \t\t\t\t \"non-blob found at .gitmodules\");\n+\t\t}\n \t\tfree(buf);\n \t}\n \n@@ -1267,15 +1273,14 @@ int git_fsck_config(const char *var, const char *value, void *cb)\n */\n \n int fsck_error_cb_print_missing_gitmodules(struct fsck_options *o,\n-\t\t\t\t\t const struct object_id *oid,\n-\t\t\t\t\t enum object_type object_type,\n+\t\t\t\t\t const struct object *obj,\n \t\t\t\t\t enum fsck_msg_type msg_type,\n \t\t\t\t\t enum fsck_msg_id msg_id,\n \t\t\t\t\t const char *message)\n {\n \tif (msg_id == FSCK_MSG_GITMODULES_MISSING) {\n-\t\tputs(oid_to_hex(oid));\n+\t\tputs(oid_to_hex(&obj->oid));\n \t\treturn 0;\n \t}\n-\treturn fsck_error_function(o, oid, object_type, msg_type, msg_id, message);\n+\treturn fsck_error_function(o, obj, msg_type, msg_id, message);\n }\ndiff --git a/fsck.h b/fsck.h\nindex 7202c3c87e8..864032709c4 100644\n--- a/fsck.h\n+++ b/fsck.h\n@@ -103,17 +103,16 @@ typedef int (*fsck_walk_func)(struct object *obj, enum object_type object_type,\n \n /* callback for fsck_object, type is FSCK_ERROR or FSCK_WARN */\n typedef int (*fsck_error)(struct fsck_options *o,\n-\t\t\t const struct object_id *oid, enum object_type object_type,\n+\t\t\t const struct object *obj,\n \t\t\t enum fsck_msg_type msg_type, enum fsck_msg_id msg_id,\n \t\t\t const char *message);\n \n int fsck_error_function(struct fsck_options *o,\n-\t\t\tconst struct object_id *oid, enum object_type object_type,\n+\t\t\tconst struct object *obj,\n \t\t\tenum fsck_msg_type msg_type, enum fsck_msg_id msg_id,\n \t\t\tconst char *message);\n int fsck_error_cb_print_missing_gitmodules(struct fsck_options *o,\n-\t\t\t\t\t const struct object_id *oid,\n-\t\t\t\t\t enum object_type object_type,\n+\t\t\t\t\t const struct object *obj,\n \t\t\t\t\t enum fsck_msg_type msg_type,\n \t\t\t\t\t enum fsck_msg_id msg_id,\n \t\t\t\t\t const char *message);\n@@ -168,7 +167,7 @@ int fsck_object(struct object *obj, void *data, unsigned long size,\n * fsck a tag, and pass info about it back to the caller. This is\n * exposed fsck_object() internals for git-mktag(1).\n */\n-int fsck_tag_standalone(const struct object_id *oid, const char *buffer,\n+int fsck_tag_standalone(const struct object *obj, const char *buffer,\n \t\t\tunsigned long size, struct fsck_options *options,\n \t\t\tstruct object_id *tagged_oid,\n \t\t\tint *tag_type);\n@@ -203,7 +202,7 @@ void fsck_put_object_name(struct fsck_options *options,\n \t\t\t const struct object_id *oid,\n \t\t\t const char *fmt, ...);\n const char *fsck_describe_object(struct fsck_options *options,\n-\t\t\t\t const struct object_id *oid);\n+\t\t\t\t const struct object *obj);\n \n /*\n * git_config() callback for use by fsck-y tools that want to support\n"},{"id":"423479","messageId":"87v980vz2f.fsf@evledraar.gmail.com","threadId":"55604","inReplyTo":"YI12t5gzfzIxvZs3@coredump.intra.peff.net","subject":"Re: [PATCH 3/9] fsck_tree(): wrap some long lines","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-05-03T11:22:13Z","receivedAt":"2021-05-03T11:25:48Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sat, May 01 2021, Jeff King wrote:\n\n> Many calls to report() in fsck_tree() are kept on a single line and are\n> quite long. Most were pretty big to begin with, but have gotten even\n> longer over the years as we've added more parameters. Let's accept the\n> churn of wrapping them in order to conform to our usual line limits.\n\nIf we're going to have the churn I'd say just wrap the rest of the file\nas well, now it's mostly-consistent in having these long lines.\n\nFWIW I think having the long lines makes things more readable in this\ncase, but the inconsistency is worse.\n\nIf you do take me up on refactoring it more generally, there's also the\none mis-alignment of an argument list to report() in verify_headers().\n\nI wonder if this whole thing wouldn't be better by declaring the format\nin the msg_id_info struct. I.e. add this to fsck.h, but that's an even\nbigger change...\n"},{"id":"423516","messageId":"YJBRVQ7+CUkYf5Us@coredump.intra.peff.net","threadId":"55604","inReplyTo":"CAPig+cQFUUncb17MFAvJAh2KFxK_OCq_6AoOwPViYyQJUDVJNg@mail.gmail.com","subject":"Re: [PATCH 5/9] t7450: test verify_path() handling of gitmodules","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-03T19:39:01Z","receivedAt":"2021-05-03T19:39:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, May 01, 2021 at 03:03:56PM -0400, Eric Sunshine wrote:\n\n> On Sat, May 1, 2021 at 2:55 PM Eric Sunshine wrote:\n> > On Sat, May 1, 2021 at 11:42 AM Jeff King wrote:\n> > > + tree=$(git -C symlink mktree >\n> > `tree` is an unusually generic name for this now-global variable. One\n> > can easily imagine it being re-used by some unrelated test arbitrarily\n> > inserted into this script, thus potentially breaking the following\n> > tests which depend upon it. I wonder if a name such as `BAD_TREE`\n> > would be more appropriate.\n> \n> I see that all `$tree` references get encapsulated into a shell\n> function by the next patch, so perhaps the generic name `tree` isn't a\n> big deal after all.\n\nYeah. I'd like to think it is not that big a deal between even just\nadjacent tests, because anybody adding tests in the middle of a script\nwould take care not to split related ones. But that may be too\noptimistic. ;)\n\nAt any rate, it seems OK to assume the function will all be used\ntogether.\n\n-Peff\n"},{"id":"423518","messageId":"YJBZYt8ivlTDHpRM@coredump.intra.peff.net","threadId":"55604","inReplyTo":"87y2cwvz9e.fsf@evledraar.gmail.com","subject":"Re: [PATCH 2/9] fsck_tree(): fix shadowed variable","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-03T20:13:22Z","receivedAt":"2021-05-03T20:13:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 03, 2021 at 01:15:03PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> > Commit b2f2039c2b (fsck: accept an oid instead of a \"struct tree\" for\n> > fsck_tree(), 2019-10-18) introduced a new \"oid\" parameter to\n> > fsck_tree(), and we pass it to the report() function when we find\n> [...]\n> \n> Have you considered just passing down the \"obj\" instead of the \"oid\"?\n> It's a bit of a bigger change, but seems to be worth it in this case as\n> the below diff (on top of master) shows. We spend quite a bit of effort\n> peeling an \"obj\" just to pass oid/type further down the stack.\n\nThat would be undoing the point of the referenced commit above. More\ndiscussion in this thread:\n\n https://lore.kernel.org/git/20191018044103.GA17625@sigill.intra.peff.net/\n\nbut the gist is:\n\n - there were bugs caused by looking at the object structs; not having\n it make sure we can't introduce similar bugs\n\n - using oids and buffers makes it possible for code to avoid having\n object structs at all. index-pack could take advantage of this (to\n reduce memory usage), but nobody has yet written that patch.\n\n-Peff\n"},{"id":"423519","messageId":"YJBb2k4yUipefmzM@coredump.intra.peff.net","threadId":"55604","inReplyTo":"87v980vz2f.fsf@evledraar.gmail.com","subject":"Re: [PATCH 3/9] fsck_tree(): wrap some long lines","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-03T20:23:54Z","receivedAt":"2021-05-03T20:23:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 03, 2021 at 01:22:13PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> On Sat, May 01 2021, Jeff King wrote:\n> \n> > Many calls to report() in fsck_tree() are kept on a single line and are\n> > quite long. Most were pretty big to begin with, but have gotten even\n> > longer over the years as we've added more parameters. Let's accept the\n> > churn of wrapping them in order to conform to our usual line limits.\n> \n> If we're going to have the churn I'd say just wrap the rest of the file\n> as well, now it's mostly-consistent in having these long lines.\n> \n> FWIW I think having the long lines makes things more readable in this\n> case, but the inconsistency is worse.\n\nI'm not sure I agree. It depends on how big a chunk you consider for\nconsistency: a function, a file, or the whole project.\n\nfsck_tree() was already inconsistent, so this is making that function\ntotally consistent. Since that was the function I was working in, that\nseemed like the limit of \"while I'm here\", and I'd prefer to keep it\nthere for the series.\n\nI certainly don't mind extra clean up on top, though.\n\nAs far as preferring the long lines, I don't mind lines a _little_ long,\nbut some of these are 120+ characters. They wrap awkwardly even on my\nextra-wide terminals. ;) I guess we can have a discussion on whether\nlong lines are OK, but it should probably center on what we put into\nCodingGuidelines, and not these particular lines.\n\n> I wonder if this whole thing wouldn't be better by declaring the format\n> in the msg_id_info struct. I.e. add this to fsck.h, but that's an even\n> bigger change...\n\nI think it gets tricky, as not all of the strings have the same number\nand type of format specifiers (most don't have any, but verify_headers()\nfor example uses %ld).\n\n-Peff\n"},{"id":"423520","messageId":"YJBdPXkogxeERdi8@coredump.intra.peff.net","threadId":"55604","inReplyTo":"874kfkxhxe.fsf@evledraar.gmail.com","subject":"Re: [PATCH 1/9] t7415: remove out-dated comment about translation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-03T20:29:49Z","receivedAt":"2021-05-03T20:29:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 03, 2021 at 11:46:52AM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> > Since GETTEXT_POISON does not exist anymore, there is no point warning\n> > people about whether we should use test_i18ngrep. This is doubly\n> > confusing because the comment was describing why it was OK to use grep,\n> > but it got caught up in the mass conversion of 674ba34038 (fsck: mark\n> > strings for translation, 2018-11-10).\n> >\n> > Note there are other uses of test_i18ngrep in this script which are now\n> > obsolete; I'll save those for a mass-cleanup. My goal here was just to\n> > fix the confusing comment in code I'm about to refactor.\n> \n> For what it's worth between [1] and [2] I'm not sure what to do about\n> the test_i18ngrep cleanup. I think your patch below is fine, but the\n> \"test_i18ngrep\" has mutated into a \"grep with debugging\", not just\n> something needed for GETTEXT_POISON.\n\nYes, I can see some value in that. Though if that's what it's going to\nbe, IMHO we should use it consistently and give a better name (test_grep\nor something).\n\n> So that part of your patch right now is making it less friendly for\n> debugging. I don't care, and think if we want that we'd be better of\n> scraping the trace ouput for such common cases and/or use \"verbose grep\n> [...]\" and teach the \"verbose\" wrapper about these common cases, but\n> knowing of that objection + having other outstanding things has been why\n> I haven't sent s/test_i18ngrep/grep/g patches.\n\nI was the one who introduced \"verbose\" long ago, and I did have dreams\nthat people would do \"verbose grep\" everywhere. But in the end, we made\n\"-x\" a lot nicer to use, and I think that largely obsoletes it. Using\n\"verbose\" makes the non-x verbose output slightly nicer, perhaps. But\nnot having to remember to sprinkle \"verbose\" through the test code (nor\nread it) seems like a bigger win to me.\n\nFor the same reason, I'm pretty ambivalent about having test_grep. I\nsuppose one could make a similar argument about \"test_path_is_missing\",\netc, though some of those helpers are also encapsulating technique\n(e.g., test_dir_is_empty).\n\n-Peff\n"},{"id":"423522","messageId":"YJBd0rd9DJSKQoL7@coredump.intra.peff.net","threadId":"55604","inReplyTo":"871raoxh09.fsf@evledraar.gmail.com","subject":"Re: [PATCH 5/9] t7450: test verify_path() handling of gitmodules","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-03T20:32:18Z","receivedAt":"2021-05-03T20:32:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 03, 2021 at 12:12:32PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> \n> On Sat, May 01 2021, Jeff King wrote:\n> \n> > +test_expect_success 'refuse to load symlinked .gitmodules into index' '\n> > +\ttest_must_fail git -C symlink read-tree $tree 2>err &&\n> > +\ttest_i18ngrep \"invalid path.*gitmodules\" err &&\n> > +\tgit -C symlink ls-files >out &&\n> > +\ttest_must_be_empty out\n> > +'\n> > +\n> \n> In 1/9 a test_i18ngrep is removed, but here's a new one.\n\nThanks. Most of these patches were written either in 2018 or 2020 and\npulled forward (and back then, this call would have been necessary). I\nfixed up a few cases as I was rebasing, but obviously missed this one.\n\n-Peff\n"},{"id":"423523","messageId":"YJBd/9rSkLQ68ECy@coredump.intra.peff.net","threadId":"55604","inReplyTo":"CAPig+cTmL3R=xB_zjFLZkZ3X4pOABj-OvLsbDsW91WKCv-bjHA@mail.gmail.com","subject":"Re: [PATCH 9/9] docs: document symlink restrictions for dot-files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-03T20:33:03Z","receivedAt":"2021-05-03T20:33:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, May 01, 2021 at 03:16:09PM -0400, Eric Sunshine wrote:\n\n> Just a really microscopic nit... feel free to ignore...\n> \n> > diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt\n> > @@ -1247,6 +1247,13 @@ to:\n> > +NOTES\n> > +-----\n> > +\n> > +Note that Git does not follow symbolic links when accessing a\n> > +`.gitattributes` file in the working tree. This keeps behavior\n> > +consistent when the file is accessed from the index or a tree versus\n> > +from the filesystem.\n> \n> We're in the \"NOTES\" section, so it feels redundant to begin the\n> sentence with \"Note that\".\n\nThanks. I think it's a good suggestion. I agree it's pretty minor, but\nI'm going to re-roll anyway to drop the test_i18ngrep, so I'll reword\nthis, too.\n\n-Peff\n"},{"id":"423524","messageId":"YJBgMP9eXq31INyN@coredump.intra.peff.net","threadId":"55604","inReplyTo":"YI12hK4X/gfl3u29@coredump.intra.peff.net","subject":"[PATCH v2 0/9] leftover bits from symlinked gitattributes, etc topics","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-03T20:42:24Z","receivedAt":"2021-05-03T20:42:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, May 01, 2021 at 11:40:52AM -0400, Jeff King wrote:\n\n> A while back, I had a topic[1] that treated symlinked .gitattributes\n> (and .gitignore and .mailmap) the same as .gitmodules: forbidding them\n> in the index, complaining about them via fsck, etc.\n> \n> In the end, we decided not to do that[2], and instead just open the\n> files with O_NOFOLLOW instead. As I said in that thread, we could\n> salvage some of the cleanups, fsck checks, and docs from the original\n> topic. So here that is. (The new topic is in master but not yet\n> released; so while this is not strictly a bug-fix for an existing topic,\n> it would be good to get especially the doc improvements into the same\n> release).\n\nHere's a re-roll with two small fixes: dropping the test_i18ngrep that\nÆvar noticed, and taking Eric's wording suggestion for the docs.\n\nI didn't take Ævar's suggestion to expand the line-wrapping fixes\nfurther. I don't mind that happening, but I'd prefer doing it as a\nseparate series.\n\nRange-diff (a little hard to read because the one-line change in the\ntests percolates through several commits, and the two-word changes in\nthe docs caused rewrapping):\n\n 1: c91ce2ed34 = 1: c91ce2ed34 t7415: remove out-dated comment about translation\n 2: 99fe934110 = 2: 99fe934110 fsck_tree(): fix shadowed variable\n 3: 9695e4370c = 3: 9695e4370c fsck_tree(): wrap some long lines\n 4: 2cf9839145 = 4: 2cf9839145 t7415: rename to expand scope\n 5: ad18686096 ! 5: 1664953e71 t7450: test verify_path() handling of gitmodules\n @@ t/t7450-bad-git-dotfiles.sh: test_expect_success 'fsck detects symlinked .gitmod\n \n +test_expect_success 'refuse to load symlinked .gitmodules into index' '\n +\ttest_must_fail git -C symlink read-tree $tree 2>err &&\n -+\ttest_i18ngrep \"invalid path.*gitmodules\" err &&\n ++\tgrep \"invalid path.*gitmodules\" err &&\n +\tgit -C symlink ls-files >out &&\n +\ttest_must_be_empty out\n +'\n 6: 9691fb8d5c ! 6: 41000ce022 t7450: test .gitmodules symlink matching against obscured names\n @@ t/t7450-bad-git-dotfiles.sh: test_expect_success 'index-pack --strict works for\n -\n -test_expect_success 'refuse to load symlinked .gitmodules into index' '\n -\ttest_must_fail git -C symlink read-tree $tree 2>err &&\n --\ttest_i18ngrep \"invalid path.*gitmodules\" err &&\n +-\tgrep \"invalid path.*gitmodules\" err &&\n -\tgit -C symlink ls-files >out &&\n -\ttest_must_be_empty out\n -'\n @@ t/t7450-bad-git-dotfiles.sh: test_expect_success 'index-pack --strict works for\n +\t\t\t -c core.protectntfs \\\n +\t\t\t -c core.protecthfs \\\n +\t\t\t read-tree $tree 2>err &&\n -+\t\ttest_i18ngrep \"invalid path.*$name\" err &&\n ++\t\tgrep \"invalid path.*$name\" err &&\n +\t\tgit -C $dir ls-files -s >out &&\n +\t\ttest_must_be_empty out\n +\t'\n 7: 670705dca2 = 7: 58efbbbbb6 t0060: test ntfs/hfs-obscured dotfiles\n 8: 422162a7ae = 8: aeff66bf1e fsck: warn about symlinked dotfiles we'll open with O_NOFOLLOW\n 9: f1b226ca4f ! 9: a8f9255d9b docs: document symlink restrictions for dot-files\n @@ Documentation/gitattributes.txt: to:\n +NOTES\n +-----\n +\n -+Note that Git does not follow symbolic links when accessing a\n -+`.gitattributes` file in the working tree. This keeps behavior\n -+consistent when the file is accessed from the index or a tree versus\n -+from the filesystem.\n ++Git does not follow symbolic links when accessing a `.gitattributes`\n ++file in the working tree. This keeps behavior consistent when the file\n ++is accessed from the index or a tree versus from the filesystem.\n \n EXAMPLES\n --------\n @@ Documentation/gitignore.txt: not tracked by Git remain untracked.\n To stop tracking a file that is currently tracked, use\n 'git rm --cached'.\n \n -+Note that Git does not follow symbolic links when accessing a\n -+`.gitignore` file in the working tree. This keeps behavior consistent\n -+when the file is accessed from the index or a tree versus from the\n -+filesystem.\n ++Git does not follow symbolic links when accessing a `.gitignore` file in\n ++the working tree. This keeps behavior consistent when the file is\n ++accessed from the index or a tree versus from the filesystem.\n +\n EXAMPLES\n --------\n @@ Documentation/gitmailmap.txt: this would also match the 'Commit Name .shallow::\n +NOTES\n +-----\n +\n -+Note that Git does not allow the `.gitmodules` file within a working\n -+tree to be a symbolic link, and will refuse to check out such a tree\n -+entry. This keeps behavior consistent when the file is accessed from the\n -+index or a tree versus from the filesystem, and helps Git reliably\n -+enforce security checks of the file contents.\n ++Git does not allow the `.gitmodules` file within a working tree to be a\n ++symbolic link, and will refuse to check out such a tree entry. This\n ++keeps behavior consistent when the file is accessed from the index or a\n ++tree versus from the filesystem, and helps Git reliably enforce security\n ++checks of the file contents.\n \n EXAMPLES\n --------\n\n [1/9]: t7415: remove out-dated comment about translation\n [2/9]: fsck_tree(): fix shadowed variable\n [3/9]: fsck_tree(): wrap some long lines\n [4/9]: t7415: rename to expand scope\n [5/9]: t7450: test verify_path() handling of gitmodules\n [6/9]: t7450: test .gitmodules symlink matching against obscured names\n [7/9]: t0060: test ntfs/hfs-obscured dotfiles\n [8/9]: fsck: warn about symlinked dotfiles we'll open with O_NOFOLLOW\n [9/9]: docs: document symlink restrictions for dot-files\n\n Documentation/gitattributes.txt | 6 +\n Documentation/gitignore.txt | 4 +\n Documentation/gitmailmap.txt | 7 ++\n Documentation/gitmodules.txt | 8 ++\n cache.h | 1 +\n fsck.c | 84 ++++++++++---\n fsck.h | 3 +\n path.c | 5 +\n t/helper/test-path-utils.c | 46 +++++--\n t/t0060-path-utils.sh | 30 +++++\n ...ule-names.sh => t7450-bad-git-dotfiles.sh} | 116 +++++++++++++-----\n utf8.c | 5 +\n utf8.h | 1 +\n 13 files changed, 255 insertions(+), 61 deletions(-)\n rename t/{t7415-submodule-names.sh => t7450-bad-git-dotfiles.sh} (70%)\n\n-Peff\n"},{"id":"423525","messageId":"YJBgWqPdQwibWYn/@coredump.intra.peff.net","threadId":"55604","inReplyTo":"YJBgMP9eXq31INyN@coredump.intra.peff.net","subject":"[PATCH v2 1/9] t7415: remove out-dated comment about translation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-03T20:43:06Z","receivedAt":"2021-05-03T20:43:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Since GETTEXT_POISON does not exist anymore, there is no point warning\npeople about whether we should use test_i18ngrep. This is doubly\nconfusing because the comment was describing why it was OK to use grep,\nbut it got caught up in the mass conversion of 674ba34038 (fsck: mark\nstrings for translation, 2018-11-10).\n\nNote there are other uses of test_i18ngrep in this script which are now\nobsolete; I'll save those for a mass-cleanup. My goal here was just to\nfix the confusing comment in code I'm about to refactor.\n\nSigned-off-by: Jeff King \n---\n t/t7415-submodule-names.sh | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t7415-submodule-names.sh b/t/t7415-submodule-names.sh\nindex f70368bc2e..fef6561d80 100755\n--- a/t/t7415-submodule-names.sh\n+++ b/t/t7415-submodule-names.sh\n@@ -151,10 +151,9 @@ test_expect_success 'fsck detects symlinked .gitmodules file' '\n \t\t} | git mktree &&\n \n \t\t# Check not only that we fail, but that it is due to the\n-\t\t# symlink detector; this grep string comes from the config\n-\t\t# variable name and will not be translated.\n+\t\t# symlink detector\n \t\ttest_must_fail git fsck 2>output &&\n-\t\ttest_i18ngrep gitmodulesSymlink output\n+\t\tgrep gitmodulesSymlink output\n \t)\n '\n \n-- \n2.31.1.926.gd12152deb6\n\n"},{"id":"423526","messageId":"YJBgXWb4Y2movw97@coredump.intra.peff.net","threadId":"55604","inReplyTo":"YJBgMP9eXq31INyN@coredump.intra.peff.net","subject":"[PATCH v2 2/9] fsck_tree(): fix shadowed variable","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-03T20:43:09Z","receivedAt":"2021-05-03T20:43:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Commit b2f2039c2b (fsck: accept an oid instead of a \"struct tree\" for\nfsck_tree(), 2019-10-18) introduced a new \"oid\" parameter to\nfsck_tree(), and we pass it to the report() function when we find\nproblems. However, that is shadowed within the tree-walking loop by the\nexisting \"oid\" variable which we use to store the oid of each tree\nentry. As a result, we may report the wrong oid for some problems we\ndetect within the loop (the entry oid, instead of the tree oid).\n\nOur tests didn't catch this because they checked only that we found the\nexpected fsck problem, not that it was attached to the correct object.\n\nLet's rename both variables in the function to avoid confusion. This\nmakes the diff a little noisy (e.g., all of the report() calls outside\nthe loop were already correct but need to be touched), but makes sure we\ncatch all cases and will avoid similar confusion in the future.\n\nAnd we can update the test to be a bit more specific and catch this\nproblem.\n\nSigned-off-by: Jeff King \n---\n fsck.c | 42 ++++++++++++++++++++------------------\n t/t7415-submodule-names.sh | 5 +++--\n 2 files changed, 25 insertions(+), 22 deletions(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex f5ed6a2635..dd31ed1413 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -558,7 +558,7 @@ static int verify_ordered(unsigned mode1, const char *name1,\n \treturn c1 < c2 ? 0 : TREE_UNORDERED;\n }\n \n-static int fsck_tree(const struct object_id *oid,\n+static int fsck_tree(const struct object_id *tree_oid,\n \t\t const char *buffer, unsigned long size,\n \t\t struct fsck_options *options)\n {\n@@ -579,7 +579,7 @@ static int fsck_tree(const struct object_id *oid,\n \tstruct name_stack df_dup_candidates = { NULL };\n \n \tif (init_tree_desc_gently(&desc, buffer, size)) {\n-\t\tretval += report(options, oid, OBJ_TREE, FSCK_MSG_BAD_TREE, \"cannot be parsed as a tree\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_BAD_TREE, \"cannot be parsed as a tree\");\n \t\treturn retval;\n \t}\n \n@@ -589,11 +589,11 @@ static int fsck_tree(const struct object_id *oid,\n \twhile (desc.size) {\n \t\tunsigned short mode;\n \t\tconst char *name, *backslash;\n-\t\tconst struct object_id *oid;\n+\t\tconst struct object_id *entry_oid;\n \n-\t\toid = tree_entry_extract(&desc, &name, &mode);\n+\t\tentry_oid = tree_entry_extract(&desc, &name, &mode);\n \n-\t\thas_null_sha1 |= is_null_oid(oid);\n+\t\thas_null_sha1 |= is_null_oid(entry_oid);\n \t\thas_full_path |= !!strchr(name, '/');\n \t\thas_empty_name |= !*name;\n \t\thas_dot |= !strcmp(name, \".\");\n@@ -603,10 +603,11 @@ static int fsck_tree(const struct object_id *oid,\n \n \t\tif (is_hfs_dotgitmodules(name) || is_ntfs_dotgitmodules(name)) {\n \t\t\tif (!S_ISLNK(mode))\n-\t\t\t\toidset_insert(&options->gitmodules_found, oid);\n+\t\t\t\toidset_insert(&options->gitmodules_found,\n+\t\t\t\t\t entry_oid);\n \t\t\telse\n \t\t\t\tretval += report(options,\n-\t\t\t\t\t\t oid, OBJ_TREE,\n+\t\t\t\t\t\t tree_oid, OBJ_TREE,\n \t\t\t\t\t\t FSCK_MSG_GITMODULES_SYMLINK,\n \t\t\t\t\t\t \".gitmodules is a symbolic link\");\n \t\t}\n@@ -617,9 +618,10 @@ static int fsck_tree(const struct object_id *oid,\n \t\t\t\thas_dotgit |= is_ntfs_dotgit(backslash);\n \t\t\t\tif (is_ntfs_dotgitmodules(backslash)) {\n \t\t\t\t\tif (!S_ISLNK(mode))\n-\t\t\t\t\t\toidset_insert(&options->gitmodules_found, oid);\n+\t\t\t\t\t\toidset_insert(&options->gitmodules_found,\n+\t\t\t\t\t\t\t entry_oid);\n \t\t\t\t\telse\n-\t\t\t\t\t\tretval += report(options, oid, OBJ_TREE,\n+\t\t\t\t\t\tretval += report(options, tree_oid, OBJ_TREE,\n \t\t\t\t\t\t\t\t FSCK_MSG_GITMODULES_SYMLINK,\n \t\t\t\t\t\t\t\t \".gitmodules is a symbolic link\");\n \t\t\t\t}\n@@ -628,7 +630,7 @@ static int fsck_tree(const struct object_id *oid,\n \t\t}\n \n \t\tif (update_tree_entry_gently(&desc)) {\n-\t\t\tretval += report(options, oid, OBJ_TREE, FSCK_MSG_BAD_TREE, \"cannot be parsed as a tree\");\n+\t\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_BAD_TREE, \"cannot be parsed as a tree\");\n \t\t\tbreak;\n \t\t}\n \n@@ -676,25 +678,25 @@ static int fsck_tree(const struct object_id *oid,\n \tname_stack_clear(&df_dup_candidates);\n \n \tif (has_null_sha1)\n-\t\tretval += report(options, oid, OBJ_TREE, FSCK_MSG_NULL_SHA1, \"contains entries pointing to null sha1\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_NULL_SHA1, \"contains entries pointing to null sha1\");\n \tif (has_full_path)\n-\t\tretval += report(options, oid, OBJ_TREE, FSCK_MSG_FULL_PATHNAME, \"contains full pathnames\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_FULL_PATHNAME, \"contains full pathnames\");\n \tif (has_empty_name)\n-\t\tretval += report(options, oid, OBJ_TREE, FSCK_MSG_EMPTY_NAME, \"contains empty pathname\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_EMPTY_NAME, \"contains empty pathname\");\n \tif (has_dot)\n-\t\tretval += report(options, oid, OBJ_TREE, FSCK_MSG_HAS_DOT, \"contains '.'\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_HAS_DOT, \"contains '.'\");\n \tif (has_dotdot)\n-\t\tretval += report(options, oid, OBJ_TREE, FSCK_MSG_HAS_DOTDOT, \"contains '..'\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_HAS_DOTDOT, \"contains '..'\");\n \tif (has_dotgit)\n-\t\tretval += report(options, oid, OBJ_TREE, FSCK_MSG_HAS_DOTGIT, \"contains '.git'\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_HAS_DOTGIT, \"contains '.git'\");\n \tif (has_zero_pad)\n-\t\tretval += report(options, oid, OBJ_TREE, FSCK_MSG_ZERO_PADDED_FILEMODE, \"contains zero-padded file modes\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_ZERO_PADDED_FILEMODE, \"contains zero-padded file modes\");\n \tif (has_bad_modes)\n-\t\tretval += report(options, oid, OBJ_TREE, FSCK_MSG_BAD_FILEMODE, \"contains bad file modes\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_BAD_FILEMODE, \"contains bad file modes\");\n \tif (has_dup_entries)\n-\t\tretval += report(options, oid, OBJ_TREE, FSCK_MSG_DUPLICATE_ENTRIES, \"contains duplicate file entries\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_DUPLICATE_ENTRIES, \"contains duplicate file entries\");\n \tif (not_properly_sorted)\n-\t\tretval += report(options, oid, OBJ_TREE, FSCK_MSG_TREE_NOT_SORTED, \"not properly sorted\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_TREE_NOT_SORTED, \"not properly sorted\");\n \treturn retval;\n }\n \ndiff --git a/t/t7415-submodule-names.sh b/t/t7415-submodule-names.sh\nindex fef6561d80..6a8cf3f47b 100755\n--- a/t/t7415-submodule-names.sh\n+++ b/t/t7415-submodule-names.sh\n@@ -148,12 +148,13 @@ test_expect_success 'fsck detects symlinked .gitmodules file' '\n \t\t{\n \t\t\tprintf \"100644 blob $content\\t$tricky\\n\" &&\n \t\t\tprintf \"120000 blob $target\\t.gitmodules\\n\"\n-\t\t} | git mktree &&\n+\t\t} >bad-tree &&\n+\t\ttree=$(git mktree output &&\n-\t\tgrep gitmodulesSymlink output\n+\t\tgrep \"tree $tree: gitmodulesSymlink\" output\n \t)\n '\n \n-- \n2.31.1.926.gd12152deb6\n\n"},{"id":"423527","messageId":"YJBgX88c+ZDOxjVD@coredump.intra.peff.net","threadId":"55604","inReplyTo":"YJBgMP9eXq31INyN@coredump.intra.peff.net","subject":"[PATCH v2 3/9] fsck_tree(): wrap some long lines","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-03T20:43:11Z","receivedAt":"2021-05-03T20:43:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Many calls to report() in fsck_tree() are kept on a single line and are\nquite long. Most were pretty big to begin with, but have gotten even\nlonger over the years as we've added more parameters. Let's accept the\nchurn of wrapping them in order to conform to our usual line limits.\n\nSigned-off-by: Jeff King \n---\n fsck.c | 48 ++++++++++++++++++++++++++++++++++++------------\n 1 file changed, 36 insertions(+), 12 deletions(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex dd31ed1413..db94817898 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -579,7 +579,9 @@ static int fsck_tree(const struct object_id *tree_oid,\n \tstruct name_stack df_dup_candidates = { NULL };\n \n \tif (init_tree_desc_gently(&desc, buffer, size)) {\n-\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_BAD_TREE, \"cannot be parsed as a tree\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE,\n+\t\t\t\t FSCK_MSG_BAD_TREE,\n+\t\t\t\t \"cannot be parsed as a tree\");\n \t\treturn retval;\n \t}\n \n@@ -630,7 +632,9 @@ static int fsck_tree(const struct object_id *tree_oid,\n \t\t}\n \n \t\tif (update_tree_entry_gently(&desc)) {\n-\t\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_BAD_TREE, \"cannot be parsed as a tree\");\n+\t\t\tretval += report(options, tree_oid, OBJ_TREE,\n+\t\t\t\t\t FSCK_MSG_BAD_TREE,\n+\t\t\t\t\t \"cannot be parsed as a tree\");\n \t\t\tbreak;\n \t\t}\n \n@@ -678,25 +682,45 @@ static int fsck_tree(const struct object_id *tree_oid,\n \tname_stack_clear(&df_dup_candidates);\n \n \tif (has_null_sha1)\n-\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_NULL_SHA1, \"contains entries pointing to null sha1\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE,\n+\t\t\t\t FSCK_MSG_NULL_SHA1,\n+\t\t\t\t \"contains entries pointing to null sha1\");\n \tif (has_full_path)\n-\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_FULL_PATHNAME, \"contains full pathnames\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE,\n+\t\t\t\t FSCK_MSG_FULL_PATHNAME,\n+\t\t\t\t \"contains full pathnames\");\n \tif (has_empty_name)\n-\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_EMPTY_NAME, \"contains empty pathname\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE,\n+\t\t\t\t FSCK_MSG_EMPTY_NAME,\n+\t\t\t\t \"contains empty pathname\");\n \tif (has_dot)\n-\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_HAS_DOT, \"contains '.'\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE,\n+\t\t\t\t FSCK_MSG_HAS_DOT,\n+\t\t\t\t \"contains '.'\");\n \tif (has_dotdot)\n-\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_HAS_DOTDOT, \"contains '..'\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE,\n+\t\t\t\t FSCK_MSG_HAS_DOTDOT,\n+\t\t\t\t \"contains '..'\");\n \tif (has_dotgit)\n-\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_HAS_DOTGIT, \"contains '.git'\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE,\n+\t\t\t\t FSCK_MSG_HAS_DOTGIT,\n+\t\t\t\t \"contains '.git'\");\n \tif (has_zero_pad)\n-\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_ZERO_PADDED_FILEMODE, \"contains zero-padded file modes\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE,\n+\t\t\t\t FSCK_MSG_ZERO_PADDED_FILEMODE,\n+\t\t\t\t \"contains zero-padded file modes\");\n \tif (has_bad_modes)\n-\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_BAD_FILEMODE, \"contains bad file modes\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE,\n+\t\t\t\t FSCK_MSG_BAD_FILEMODE,\n+\t\t\t\t \"contains bad file modes\");\n \tif (has_dup_entries)\n-\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_DUPLICATE_ENTRIES, \"contains duplicate file entries\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE,\n+\t\t\t\t FSCK_MSG_DUPLICATE_ENTRIES,\n+\t\t\t\t \"contains duplicate file entries\");\n \tif (not_properly_sorted)\n-\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_TREE_NOT_SORTED, \"not properly sorted\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE,\n+\t\t\t\t FSCK_MSG_TREE_NOT_SORTED,\n+\t\t\t\t \"not properly sorted\");\n \treturn retval;\n }\n \n-- \n2.31.1.926.gd12152deb6\n\n"},{"id":"423528","messageId":"YJBgYfXZ8yPzw1q9@coredump.intra.peff.net","threadId":"55604","inReplyTo":"YJBgMP9eXq31INyN@coredump.intra.peff.net","subject":"[PATCH v2 4/9] t7415: rename to expand scope","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-03T20:43:13Z","receivedAt":"2021-05-03T20:43:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This script has already expanded beyond its original intent of \".. in\nsubmodule names\" to include other malicious submodule bits. Let's update\nthe name and description to reflect that, as well as the fact that we'll\nsoon be adding similar tests for other dotfiles (.gitattributes, etc).\nWe'll also renumber it to move it out of the group of submodule-specific\ntests.\n\nSigned-off-by: Jeff King \n---\n ...submodule-names.sh => t7450-bad-git-dotfiles.sh} | 13 ++++++++++---\n 1 file changed, 10 insertions(+), 3 deletions(-)\n rename t/{t7415-submodule-names.sh => t7450-bad-git-dotfiles.sh} (95%)\n\ndiff --git a/t/t7415-submodule-names.sh b/t/t7450-bad-git-dotfiles.sh\nsimilarity index 95%\nrename from t/t7415-submodule-names.sh\nrename to t/t7450-bad-git-dotfiles.sh\nindex 6a8cf3f47b..34d4dc6def 100755\n--- a/t/t7415-submodule-names.sh\n+++ b/t/t7450-bad-git-dotfiles.sh\n@@ -1,9 +1,16 @@\n #!/bin/sh\n \n-test_description='check handling of .. in submodule names\n+test_description='check broken or malicious patterns in .git* files\n \n-Exercise the name-checking function on a variety of names, and then give a\n-real-world setup that confirms we catch this in practice.\n+Such as:\n+\n+ - presence of .. in submodule names;\n+ Exercise the name-checking function on a variety of names, and then give a\n+ real-world setup that confirms we catch this in practice.\n+\n+ - nested submodule names\n+\n+ - symlinked .gitmodules, etc\n '\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-pack.sh\n-- \n2.31.1.926.gd12152deb6\n\n"},{"id":"423529","messageId":"YJBgY6mHavaHNrXH@coredump.intra.peff.net","threadId":"55604","inReplyTo":"YJBgMP9eXq31INyN@coredump.intra.peff.net","subject":"[PATCH v2 5/9] t7450: test verify_path() handling of gitmodules","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-03T20:43:15Z","receivedAt":"2021-05-03T20:43:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Commit 10ecfa7649 (verify_path: disallow symlinks in .gitmodules,\n2018-05-04) made it impossible to load a symlink .gitmodules file into\nthe index. However, there are no tests of this behavior. Let's make sure\nthis case is covered. We can easily reuse the test setup created by\nthe matching b7b1fca175 (fsck: complain when .gitmodules is a symlink,\n2018-05-04).\n\nSigned-off-by: Jeff King \n---\n t/t7450-bad-git-dotfiles.sh | 19 ++++++++++++++++---\n 1 file changed, 16 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t7450-bad-git-dotfiles.sh b/t/t7450-bad-git-dotfiles.sh\nindex 34d4dc6def..eace9a2216 100755\n--- a/t/t7450-bad-git-dotfiles.sh\n+++ b/t/t7450-bad-git-dotfiles.sh\n@@ -139,7 +139,7 @@ test_expect_success 'index-pack --strict works for non-repo pack' '\n \tgrep gitmodulesName output\n '\n \n-test_expect_success 'fsck detects symlinked .gitmodules file' '\n+test_expect_success 'set up repo with symlinked .gitmodules file' '\n \tgit init symlink &&\n \t(\n \t\tcd symlink &&\n@@ -155,8 +155,14 @@ test_expect_success 'fsck detects symlinked .gitmodules file' '\n \t\t{\n \t\t\tprintf \"100644 blob $content\\t$tricky\\n\" &&\n \t\t\tprintf \"120000 blob $target\\t.gitmodules\\n\"\n-\t\t} >bad-tree &&\n-\t\ttree=$(git mktree bad-tree\n+\t) &&\n+\ttree=$(git -C symlink mktree err &&\n+\tgrep \"invalid path.*gitmodules\" err &&\n+\tgit -C symlink ls-files >out &&\n+\ttest_must_be_empty out\n+'\n+\n test_expect_success 'fsck detects non-blob .gitmodules' '\n \tgit init non-blob &&\n \t(\n-- \n2.31.1.926.gd12152deb6\n\n"},{"id":"423530","messageId":"YJBgZhZ8iyPDWe91@coredump.intra.peff.net","threadId":"55604","inReplyTo":"YJBgMP9eXq31INyN@coredump.intra.peff.net","subject":"[PATCH v2 6/9] t7450: test .gitmodules symlink matching against obscured names","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-03T20:43:18Z","receivedAt":"2021-05-03T20:43:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In t7450 we check that both verify_path() and fsck catch malformed\n.gitmodules entries in trees. However, we don't check that we catch\nfilesystem-equivalent forms of these (e.g., \".GITMOD~1\" on Windows).\nOur name-matching functions are exercised well in t0060, but there's\nnothing to test that we correctly call the matching functions from the\nactual fsck and verify_path() code.\n\nSo instead of testing just .gitmodules, let's repeat our tests for a few\nbasic cases. We don't need to be exhaustive here (t0060 handles that),\nbut just make sure we hit one name of each type.\n\nBesides pushing the tests into a function that takes the path as a\nparameter, we'll need to do a few things:\n\n - adjust the directory name to accommodate the tests running multiple\n times\n\n - set core.protecthfs for index checks. Fsck always protects all types\n by default, but we want to be able to exercise the HFS routines on\n every system. Note that core.protectntfs is already the default\n these days, but it doesn't hurt to explicitly label our need for it.\n\n - we'll also take the filename (\"gitmodules\") as a parameter. All\n calls use the same name for now, but a future patch will extend this\n to handle other .gitfoo files. Note that our fake-content symlink\n destination is somewhat .gitmodules specific. But it isn't necessary\n for other files (which don't do a content check). And it happens to\n be a valid attribute and ignore file anyway.\n\nSigned-off-by: Jeff King \n---\n t/t7450-bad-git-dotfiles.sh | 91 +++++++++++++++++++++----------------\n 1 file changed, 53 insertions(+), 38 deletions(-)\n\ndiff --git a/t/t7450-bad-git-dotfiles.sh b/t/t7450-bad-git-dotfiles.sh\nindex eace9a2216..b494d72976 100755\n--- a/t/t7450-bad-git-dotfiles.sh\n+++ b/t/t7450-bad-git-dotfiles.sh\n@@ -139,44 +139,59 @@ test_expect_success 'index-pack --strict works for non-repo pack' '\n \tgrep gitmodulesName output\n '\n \n-test_expect_success 'set up repo with symlinked .gitmodules file' '\n-\tgit init symlink &&\n-\t(\n-\t\tcd symlink &&\n-\n-\t\t# Make the tree directly to avoid index restrictions.\n-\t\t#\n-\t\t# Because symlinks store the target as a blob, choose\n-\t\t# a pathname that could be parsed as a .gitmodules file\n-\t\t# to trick naive non-symlink-aware checking.\n-\t\ttricky=\"[foo]bar=true\" &&\n-\t\tcontent=$(git hash-object -w ../.gitmodules) &&\n-\t\ttarget=$(printf \"$tricky\" | git hash-object -w --stdin) &&\n-\t\t{\n-\t\t\tprintf \"100644 blob $content\\t$tricky\\n\" &&\n-\t\t\tprintf \"120000 blob $target\\t.gitmodules\\n\"\n-\t\t} >bad-tree\n-\t) &&\n-\ttree=$(git -C symlink mktree output &&\n-\t\tgrep \"tree $tree: gitmodulesSymlink\" output\n-\t)\n-'\n-\n-test_expect_success 'refuse to load symlinked .gitmodules into index' '\n-\ttest_must_fail git -C symlink read-tree $tree 2>err &&\n-\tgrep \"invalid path.*gitmodules\" err &&\n-\tgit -C symlink ls-files >out &&\n-\ttest_must_be_empty out\n-'\n+check_dotx_symlink () {\n+\tname=$1\n+\ttype=$2\n+\tpath=$3\n+\tdir=symlink-$name-$type\n+\n+\ttest_expect_success \"set up repo with symlinked $name ($type)\" '\n+\t\tgit init $dir &&\n+\t\t(\n+\t\t\tcd $dir &&\n+\n+\t\t\t# Make the tree directly to avoid index restrictions.\n+\t\t\t#\n+\t\t\t# Because symlinks store the target as a blob, choose\n+\t\t\t# a pathname that could be parsed as a .gitmodules file\n+\t\t\t# to trick naive non-symlink-aware checking.\n+\t\t\ttricky=\"[foo]bar=true\" &&\n+\t\t\tcontent=$(git hash-object -w ../.gitmodules) &&\n+\t\t\ttarget=$(printf \"$tricky\" | git hash-object -w --stdin) &&\n+\t\t\t{\n+\t\t\t\tprintf \"100644 blob $content\\t$tricky\\n\" &&\n+\t\t\t\tprintf \"120000 blob $target\\t$path\\n\"\n+\t\t\t} >bad-tree\n+\t\t) &&\n+\t\ttree=$(git -C $dir mktree <$dir/bad-tree)\n+\t'\n+\n+\ttest_expect_success \"fsck detects symlinked $name ($type)\" '\n+\t\t(\n+\t\t\tcd $dir &&\n+\n+\t\t\t# Check not only that we fail, but that it is due to the\n+\t\t\t# symlink detector\n+\t\t\ttest_must_fail git fsck 2>output &&\n+\t\t\tgrep \"tree $tree: ${name}Symlink\" output\n+\t\t)\n+\t'\n+\n+\ttest_expect_success \"refuse to load symlinked $name into index ($type)\" '\n+\t\ttest_must_fail \\\n+\t\t\tgit -C $dir \\\n+\t\t\t -c core.protectntfs \\\n+\t\t\t -c core.protecthfs \\\n+\t\t\t read-tree $tree 2>err &&\n+\t\tgrep \"invalid path.*$name\" err &&\n+\t\tgit -C $dir ls-files -s >out &&\n+\t\ttest_must_be_empty out\n+\t'\n+}\n+\n+check_dotx_symlink gitmodules vanilla .gitmodules\n+check_dotx_symlink gitmodules ntfs \".gitmodules .\"\n+check_dotx_symlink gitmodules hfs \".${u200c}gitmodules\"\n \n test_expect_success 'fsck detects non-blob .gitmodules' '\n \tgit init non-blob &&\n-- \n2.31.1.926.gd12152deb6\n\n"},{"id":"423531","messageId":"YJBgattiouF9QlKo@coredump.intra.peff.net","threadId":"55604","inReplyTo":"YJBgMP9eXq31INyN@coredump.intra.peff.net","subject":"[PATCH v2 7/9] t0060: test ntfs/hfs-obscured dotfiles","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-03T20:43:22Z","receivedAt":"2021-05-03T20:43:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We have tests that cover various filesystem-specific spellings of\n\".gitmodules\", because we need to reliably identify that path for some\nsecurity checks. These are from dc2d9ba318 (is_{hfs,ntfs}_dotgitmodules:\nadd tests, 2018-05-12), with the actual code coming from e7cb0b4455\n(is_ntfs_dotgit: match other .git files, 2018-05-11) and 0fc333ba20\n(is_hfs_dotgit: match other .git files, 2018-05-02).\n\nThose latter two commits also added similar matching functions for\n.gitattributes and .gitignore. These ended up not being used in the\nfinal series, and are currently dead code. But in preparation for them\nbeing used in some fsck checks, let's make sure they actually work by\nthrowing a few basic tests at them. Likewise, let's cover .mailmap\n(which does need matching code added).\n\nI didn't bother with the whole battery of tests that we cover for\n.gitmodules. These functions are all based on the same generic matcher,\nso it's sufficient to test most of the corner cases just once.\n\nNote that the ntfs magic prefix names in the tests come from the\nalgorithm described in e7cb0b4455 (and are different for each file).\n\nSigned-off-by: Jeff King \n---\n cache.h | 1 +\n path.c | 5 +++++\n t/helper/test-path-utils.c | 46 +++++++++++++++++++++++++++-----------\n t/t0060-path-utils.sh | 30 +++++++++++++++++++++++++\n utf8.c | 5 +++++\n utf8.h | 1 +\n 6 files changed, 75 insertions(+), 13 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex b785ffb383..e6dda88fb0 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1271,6 +1271,7 @@ int is_ntfs_dotgit(const char *name);\n int is_ntfs_dotgitmodules(const char *name);\n int is_ntfs_dotgitignore(const char *name);\n int is_ntfs_dotgitattributes(const char *name);\n+int is_ntfs_dotmailmap(const char *name);\n \n /*\n * Returns true iff \"str\" could be confused as a command-line option when\ndiff --git a/path.c b/path.c\nindex 9e883eb524..7bccd830e9 100644\n--- a/path.c\n+++ b/path.c\n@@ -1493,6 +1493,11 @@ int is_ntfs_dotgitattributes(const char *name)\n \treturn is_ntfs_dot_str(name, \"gitattributes\", \"gi7d29\");\n }\n \n+int is_ntfs_dotmailmap(const char *name)\n+{\n+\treturn is_ntfs_dot_str(name, \"mailmap\", \"maba30\");\n+}\n+\n int looks_like_command_line_option(const char *str)\n {\n \treturn str && str[0] == '-';\ndiff --git a/t/helper/test-path-utils.c b/t/helper/test-path-utils.c\nindex 313a153209..229ed416b0 100644\n--- a/t/helper/test-path-utils.c\n+++ b/t/helper/test-path-utils.c\n@@ -172,9 +172,22 @@ static struct test_data dirname_data[] = {\n \t{ NULL, NULL }\n };\n \n-static int is_dotgitmodules(const char *path)\n+static int check_dotfile(const char *x, const char **argv,\n+\t\t\t int (*is_hfs)(const char *),\n+\t\t\t int (*is_ntfs)(const char *))\n {\n-\treturn is_hfs_dotgitmodules(path) || is_ntfs_dotgitmodules(path);\n+\tint res = 0, expect = 1;\n+\tfor (; *argv; argv++) {\n+\t\tif (!strcmp(\"--not\", *argv))\n+\t\t\texpect = !expect;\n+\t\telse if (expect != (is_hfs(*argv) || is_ntfs(*argv)))\n+\t\t\tres = error(\"'%s' is %s.git%s\", *argv,\n+\t\t\t\t expect ? \"not \" : \"\", x);\n+\t\telse\n+\t\t\tfprintf(stderr, \"ok: '%s' is %s.git%s\\n\",\n+\t\t\t\t*argv, expect ? \"\" : \"not \", x);\n+\t}\n+\treturn !!res;\n }\n \n static int cmp_by_st_size(const void *a, const void *b)\n@@ -382,17 +395,24 @@ int cmd__path_utils(int argc, const char **argv)\n \t\treturn test_function(dirname_data, posix_dirname, argv[1]);\n \n \tif (argc > 2 && !strcmp(argv[1], \"is_dotgitmodules\")) {\n-\t\tint res = 0, expect = 1, i;\n-\t\tfor (i = 2; i < argc; i++)\n-\t\t\tif (!strcmp(\"--not\", argv[i]))\n-\t\t\t\texpect = !expect;\n-\t\t\telse if (expect != is_dotgitmodules(argv[i]))\n-\t\t\t\tres = error(\"'%s' is %s.gitmodules\", argv[i],\n-\t\t\t\t\t expect ? \"not \" : \"\");\n-\t\t\telse\n-\t\t\t\tfprintf(stderr, \"ok: '%s' is %s.gitmodules\\n\",\n-\t\t\t\t\targv[i], expect ? \"\" : \"not \");\n-\t\treturn !!res;\n+\t\treturn check_dotfile(\"modules\", argv + 2,\n+\t\t\t\t is_hfs_dotgitmodules,\n+\t\t\t\t is_ntfs_dotgitmodules);\n+\t}\n+\tif (argc > 2 && !strcmp(argv[1], \"is_dotgitignore\")) {\n+\t\treturn check_dotfile(\"ignore\", argv + 2,\n+\t\t\t\t is_hfs_dotgitignore,\n+\t\t\t\t is_ntfs_dotgitignore);\n+\t}\n+\tif (argc > 2 && !strcmp(argv[1], \"is_dotgitattributes\")) {\n+\t\treturn check_dotfile(\"attributes\", argv + 2,\n+\t\t\t\t is_hfs_dotgitattributes,\n+\t\t\t\t is_ntfs_dotgitattributes);\n+\t}\n+\tif (argc > 2 && !strcmp(argv[1], \"is_dotmailmap\")) {\n+\t\treturn check_dotfile(\"mailmap\", argv + 2,\n+\t\t\t\t is_hfs_dotmailmap,\n+\t\t\t\t is_ntfs_dotmailmap);\n \t}\n \n \tif (argc > 2 && !strcmp(argv[1], \"file-size\")) {\ndiff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh\nindex 0ff06b5d1b..de4960783f 100755\n--- a/t/t0060-path-utils.sh\n+++ b/t/t0060-path-utils.sh\n@@ -468,6 +468,36 @@ test_expect_success 'match .gitmodules' '\n \t\t.gitmodules,:\\$DATA\n '\n \n+test_expect_success 'match .gitattributes' '\n+\ttest-tool path-utils is_dotgitattributes \\\n+\t\t.gitattributes \\\n+\t\t.git${u200c}attributes \\\n+\t\t.Gitattributes \\\n+\t\t.gitattributeS \\\n+\t\tGITATT~1 \\\n+\t\tGI7D29~1\n+'\n+\n+test_expect_success 'match .gitignore' '\n+\ttest-tool path-utils is_dotgitignore \\\n+\t\t.gitignore \\\n+\t\t.git${u200c}ignore \\\n+\t\t.Gitignore \\\n+\t\t.gitignorE \\\n+\t\tGITIGN~1 \\\n+\t\tGI250A~1\n+'\n+\n+test_expect_success 'match .mailmap' '\n+\ttest-tool path-utils is_dotmailmap \\\n+\t\t.mailmap \\\n+\t\t.mail${u200c}map \\\n+\t\t.Mailmap \\\n+\t\t.mailmaP \\\n+\t\tMAILMA~1 \\\n+\t\tMABA30~1\n+'\n+\n test_expect_success MINGW 'is_valid_path() on Windows' '\n \ttest-tool path-utils is_valid_path \\\n \t\twin32 \\\ndiff --git a/utf8.c b/utf8.c\nindex 5b39361ada..de4ce5c0e6 100644\n--- a/utf8.c\n+++ b/utf8.c\n@@ -777,6 +777,11 @@ int is_hfs_dotgitattributes(const char *path)\n \treturn is_hfs_dot_str(path, \"gitattributes\");\n }\n \n+int is_hfs_dotmailmap(const char *path)\n+{\n+\treturn is_hfs_dot_str(path, \"mailmap\");\n+}\n+\n const char utf8_bom[] = \"\\357\\273\\277\";\n \n int skip_utf8_bom(char **text, size_t len)\ndiff --git a/utf8.h b/utf8.h\nindex fcd5167baf..9a16c8679d 100644\n--- a/utf8.h\n+++ b/utf8.h\n@@ -61,6 +61,7 @@ int is_hfs_dotgit(const char *path);\n int is_hfs_dotgitmodules(const char *path);\n int is_hfs_dotgitignore(const char *path);\n int is_hfs_dotgitattributes(const char *path);\n+int is_hfs_dotmailmap(const char *path);\n \n typedef enum {\n \tALIGN_LEFT,\n-- \n2.31.1.926.gd12152deb6\n\n"},{"id":"423532","messageId":"YJBgbfxayKvMAXaC@coredump.intra.peff.net","threadId":"55604","inReplyTo":"YJBgMP9eXq31INyN@coredump.intra.peff.net","subject":"[PATCH v2 8/9] fsck: warn about symlinked dotfiles we'll open with O_NOFOLLOW","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-03T20:43:25Z","receivedAt":"2021-05-03T20:43:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In the commits merged in via 204333b015 (Merge branch\n'jk/open-dotgitx-with-nofollow', 2021-03-22), we stopped following\nsymbolic links for .gitattributes, .gitignore, and .mailmap files.\n\nLet's teach fsck to warn that these symlinks are not going to do\nanything. Note that this is just a warning, and won't block the objects\nvia transfer.fsckObjects, since there are reported to be cases of this\nin the wild (and even once fixed, they will continue to exist in the\ncommit history of those projects, but are not particularly dangerous).\n\nNote that we won't add these to the existing gitmodules block in the\nfsck code. The logic for gitmodules is a bit more complicated, as we\nalso check the content of non-symlink instances we find. But for these\nnew files, there is no content check; we're just looking at the name and\nmode of the tree entry (and we can avoid even the complicated name\nchecks in the common case that the mode doesn't indicate a symlink).\n\nWe can reuse the test helper function we defined for .gitmodules, though\n(it needs some slight adjustments for the fsck error code, and because\nwe don't block these symlinks via verify_path()).\n\nNote that I didn't explicitly test the transfer.fsckObjects case here\n(nor does the existing .gitmodules test that it blocks a push). The\ntranslation of fsck severities to outcomes is covered in general in\nt5504.\n\nSigned-off-by: Jeff King \n---\n fsck.c | 18 ++++++++++++++++++\n fsck.h | 3 +++\n t/t7450-bad-git-dotfiles.sh | 29 +++++++++++++++++++++++++++--\n 3 files changed, 48 insertions(+), 2 deletions(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex db94817898..3ec500d707 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -614,6 +614,24 @@ static int fsck_tree(const struct object_id *tree_oid,\n \t\t\t\t\t\t \".gitmodules is a symbolic link\");\n \t\t}\n \n+\t\tif (S_ISLNK(mode)) {\n+\t\t\tif (is_hfs_dotgitignore(name) ||\n+\t\t\t is_ntfs_dotgitignore(name))\n+\t\t\t\tretval += report(options, tree_oid, OBJ_TREE,\n+\t\t\t\t\t\t FSCK_MSG_GITIGNORE_SYMLINK,\n+\t\t\t\t\t\t \".gitignore is a symlink\");\n+\t\t\tif (is_hfs_dotgitattributes(name) ||\n+\t\t\t is_ntfs_dotgitattributes(name))\n+\t\t\t\tretval += report(options, tree_oid, OBJ_TREE,\n+\t\t\t\t\t\t FSCK_MSG_GITATTRIBUTES_SYMLINK,\n+\t\t\t\t\t\t \".gitattributes is a symlink\");\n+\t\t\tif (is_hfs_dotmailmap(name) ||\n+\t\t\t is_ntfs_dotmailmap(name))\n+\t\t\t\tretval += report(options, tree_oid, OBJ_TREE,\n+\t\t\t\t\t\t FSCK_MSG_MAILMAP_SYMLINK,\n+\t\t\t\t\t\t \".mailmap is a symlink\");\n+\t\t}\n+\n \t\tif ((backslash = strchr(name, '\\\\'))) {\n \t\t\twhile (backslash) {\n \t\t\t\tbackslash++;\ndiff --git a/fsck.h b/fsck.h\nindex 7202c3c87e..d07f7a2459 100644\n--- a/fsck.h\n+++ b/fsck.h\n@@ -67,6 +67,9 @@ enum fsck_msg_type {\n \tFUNC(NUL_IN_COMMIT, WARN) \\\n \t/* infos (reported as warnings, but ignored by default) */ \\\n \tFUNC(GITMODULES_PARSE, INFO) \\\n+\tFUNC(GITIGNORE_SYMLINK, INFO) \\\n+\tFUNC(GITATTRIBUTES_SYMLINK, INFO) \\\n+\tFUNC(MAILMAP_SYMLINK, INFO) \\\n \tFUNC(BAD_TAG_NAME, INFO) \\\n \tFUNC(MISSING_TAGGER_ENTRY, INFO) \\\n \t/* ignored (elevated when requested) */ \\\ndiff --git a/t/t7450-bad-git-dotfiles.sh b/t/t7450-bad-git-dotfiles.sh\nindex b494d72976..e2773bb06d 100755\n--- a/t/t7450-bad-git-dotfiles.sh\n+++ b/t/t7450-bad-git-dotfiles.sh\n@@ -140,6 +140,18 @@ test_expect_success 'index-pack --strict works for non-repo pack' '\n '\n \n check_dotx_symlink () {\n+\tfsck_must_fail=test_must_fail\n+\tfsck_prefix=error\n+\trefuse_index=t\n+\tcase \"$1\" in\n+\t--warning)\n+\t\tfsck_must_fail=\n+\t\tfsck_prefix=warning\n+\t\trefuse_index=\n+\t\tshift\n+\t\t;;\n+\tesac\n+\n \tname=$1\n \ttype=$2\n \tpath=$3\n@@ -172,11 +184,12 @@ check_dotx_symlink () {\n \n \t\t\t# Check not only that we fail, but that it is due to the\n \t\t\t# symlink detector\n-\t\t\ttest_must_fail git fsck 2>output &&\n-\t\t\tgrep \"tree $tree: ${name}Symlink\" output\n+\t\t\t$fsck_must_fail git fsck 2>output &&\n+\t\t\tgrep \"$fsck_prefix.*tree $tree: ${name}Symlink\" output\n \t\t)\n \t'\n \n+\ttest -n \"$refuse_index\" &&\n \ttest_expect_success \"refuse to load symlinked $name into index ($type)\" '\n \t\ttest_must_fail \\\n \t\t\tgit -C $dir \\\n@@ -193,6 +206,18 @@ check_dotx_symlink gitmodules vanilla .gitmodules\n check_dotx_symlink gitmodules ntfs \".gitmodules .\"\n check_dotx_symlink gitmodules hfs \".${u200c}gitmodules\"\n \n+check_dotx_symlink --warning gitattributes vanilla .gitattributes\n+check_dotx_symlink --warning gitattributes ntfs \".gitattributes .\"\n+check_dotx_symlink --warning gitattributes hfs \".${u200c}gitattributes\"\n+\n+check_dotx_symlink --warning gitignore vanilla .gitignore\n+check_dotx_symlink --warning gitignore ntfs \".gitignore .\"\n+check_dotx_symlink --warning gitignore hfs \".${u200c}gitignore\"\n+\n+check_dotx_symlink --warning mailmap vanilla .mailmap\n+check_dotx_symlink --warning mailmap ntfs \".mailmap .\"\n+check_dotx_symlink --warning mailmap hfs \".${u200c}mailmap\"\n+\n test_expect_success 'fsck detects non-blob .gitmodules' '\n \tgit init non-blob &&\n \t(\n-- \n2.31.1.926.gd12152deb6\n\n"},{"id":"423533","messageId":"YJBgcDM8Jt4dRdNe@coredump.intra.peff.net","threadId":"55604","inReplyTo":"YJBgMP9eXq31INyN@coredump.intra.peff.net","subject":"[PATCH v2 9/9] docs: document symlink restrictions for dot-files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-03T20:43:28Z","receivedAt":"2021-05-03T20:43:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We stopped allowing symlinks for .gitmodules files in 10ecfa7649\n(verify_path: disallow symlinks in .gitmodules, 2018-05-04), and we\nstopped following symlinks for .gitattributes, .gitignore, and .mailmap\nin the commits from 204333b015 (Merge branch 'jk/open-dotgitx-with-nofollow',\n2021-03-22). The reasons are discussed in detail there, but we never\nadjusted the documentation to let users know.\n\nThis hasn't been a big deal since the point is that such setups were\nmildly broken and thought to be unusual anyway. But it certainly doesn't\nhurt to be clear and explicit about it.\n\nSuggested-by: Philip Oakley \nSigned-off-by: Jeff King \n---\n Documentation/gitattributes.txt | 6 ++++++\n Documentation/gitignore.txt | 4 ++++\n Documentation/gitmailmap.txt | 7 +++++++\n Documentation/gitmodules.txt | 8 ++++++++\n 4 files changed, 25 insertions(+)\n\ndiff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt\nindex cfcfa800c2..83fd4e19a4 100644\n--- a/Documentation/gitattributes.txt\n+++ b/Documentation/gitattributes.txt\n@@ -1247,6 +1247,12 @@ to:\n [attr]binary -diff -merge -text\n ------------\n \n+NOTES\n+-----\n+\n+Git does not follow symbolic links when accessing a `.gitattributes`\n+file in the working tree. This keeps behavior consistent when the file\n+is accessed from the index or a tree versus from the filesystem.\n \n EXAMPLES\n --------\ndiff --git a/Documentation/gitignore.txt b/Documentation/gitignore.txt\nindex 5751603b13..53e7d5c914 100644\n--- a/Documentation/gitignore.txt\n+++ b/Documentation/gitignore.txt\n@@ -149,6 +149,10 @@ not tracked by Git remain untracked.\n To stop tracking a file that is currently tracked, use\n 'git rm --cached'.\n \n+Git does not follow symbolic links when accessing a `.gitignore` file in\n+the working tree. This keeps behavior consistent when the file is\n+accessed from the index or a tree versus from the filesystem.\n+\n EXAMPLES\n --------\n \ndiff --git a/Documentation/gitmailmap.txt b/Documentation/gitmailmap.txt\nindex 3fb39f801f..06f4af93fe 100644\n--- a/Documentation/gitmailmap.txt\n+++ b/Documentation/gitmailmap.txt\n@@ -55,6 +55,13 @@ this would also match the 'Commit Name ' above:\n \tProper Name CoMmIt NaMe \n --\n \n+NOTES\n+-----\n+\n+Git does not follow symbolic links when accessing a `.mailmap` file in\n+the working tree. This keeps behavior consistent when the file is\n+accessed from the index or a tree versus from the filesystem.\n+\n EXAMPLES\n --------\n \ndiff --git a/Documentation/gitmodules.txt b/Documentation/gitmodules.txt\nindex 8e333dde1b..dcee09b500 100644\n--- a/Documentation/gitmodules.txt\n+++ b/Documentation/gitmodules.txt\n@@ -98,6 +98,14 @@ submodule..shallow::\n \tshallow clone (with a history depth of 1) unless the user explicitly\n \tasks for a non-shallow clone.\n \n+NOTES\n+-----\n+\n+Git does not allow the `.gitmodules` file within a working tree to be a\n+symbolic link, and will refuse to check out such a tree entry. This\n+keeps behavior consistent when the file is accessed from the index or a\n+tree versus from the filesystem, and helps Git reliably enforce security\n+checks of the file contents.\n \n EXAMPLES\n --------\n-- \n2.31.1.926.gd12152deb6\n"},{"id":"423571","messageId":"87fsz2wupo.fsf@evledraar.gmail.com","threadId":"55604","inReplyTo":"YJBZYt8ivlTDHpRM@coredump.intra.peff.net","subject":"Re: [PATCH 2/9] fsck_tree(): fix shadowed variable","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-05-04T10:10:16Z","receivedAt":"2021-05-04T12:26:50Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, May 03 2021, Jeff King wrote:\n\n> On Mon, May 03, 2021 at 01:15:03PM +0200, Ævar Arnfjörð Bjarmason wrote:\n>\n>> > Commit b2f2039c2b (fsck: accept an oid instead of a \"struct tree\" for\n>> > fsck_tree(), 2019-10-18) introduced a new \"oid\" parameter to\n>> > fsck_tree(), and we pass it to the report() function when we find\n>> [...]\n>> \n>> Have you considered just passing down the \"obj\" instead of the \"oid\"?\n>> It's a bit of a bigger change, but seems to be worth it in this case as\n>> the below diff (on top of master) shows. We spend quite a bit of effort\n>> peeling an \"obj\" just to pass oid/type further down the stack.\n>\n> That would be undoing the point of the referenced commit above. More\n> discussion in this thread:\n>\n> https://lore.kernel.org/git/20191018044103.GA17625@sigill.intra.peff.net/\n>\n> but the gist is:\n>\n> - there were bugs caused by looking at the object structs; not having\n> it make sure we can't introduce similar bugs\n>\n> - using oids and buffers makes it possible for code to avoid having\n> object structs at all. index-pack could take advantage of this (to\n> reduce memory usage), but nobody has yet written that patch.\n\nAh, I'd managed to read this series + done some previous fsck hacking\nwithout noticing that recent-ish change.\n\nFWIW I'd been meaning to slowly mutate some of the fsck code somewhat in\nthe opposite direction. I.e. we have N object parsers now between\n{blob,commit,tree,tag,fsck,tree-walk}.c and elsewhere. Ideally we could\nshare all/most of the logic and piggy-back in fsck on the \"main\" parser\nin some more exhaustive mode.\n\nUnfortunately most of our fsck tests are rather non-exhaustive, so I see\ne.g. in reverting some of that on our current test suite e.g. a test in\ndirectory.t6102-rev-list-unexpected-objects was changed from:\n\n error in commit 536c6159e861ef1ac7967a68324a8e7614632970: missingParent: parent objects missing\n\nTo:\n\n error in commit 536c6159e861ef1ac7967a68324a8e7614632970: broken links\n\nSo by using the \"main\" parser in this case we lost the ability to\nselectively ignore that error, looks like nobody cared (and the OID\ncould still be added to the skiplist).\n\nAnother thing that got worse (but we arguably never supported well) is\nfsck_commit() and expecting to validate at least a single commit\nobject. Before we'd notice missing parents AFAICT, after we don't at\nthat entry point, but will of course if entering via fsck_walk().\n\nI don't think any current caller cares, but when I rewrote\nbuiltin/mktag.c recently there was a suggestion to do that \"fsck the new\nobject\" for the low-level commit/tree/blob APIs too. Before (and again,\nAFACT) that would work to a level of 1, after you'll need to instrument\nsome limited walk.\n\nAnyway. This inspired me to try re-arranging the \"struct object\" to be a\nsuperset of \"struct object_id\", i.e. to make it start with the oid\ninstead of \"parsed\" etc. It means you can do this:\n\n\tstatic inline struct object_id *object_to_oid(const struct object *obj)\n\t{\n\t return (struct object_id *)obj;\n\t}\n\nAnd change common cases like oid_to_hex(&obj->oid) to\noid_to_hex(object_to_oid(obj)), i.e. a wrapper for a pure cast. Just\nthat seems to speed up fsck by 1-3%, but I'm not 100% certain yet. The\ncachegrind numbers look unambiguously better. This is with gcc and -O3.\n\nMore generally we have various other structures that wrap an \"oid\" in\none way or the other, where the common access pattern is just grabbing\nthe OID out of it, but it's not the first entry. E.g. \"name_entry\",\n\"directory\" etc. By re-arranging it like that we could also have a\n\"object_id_type\" which is a 2-member touple of \"oid/type\", so by having\nthat \"oid/type\" first in \"struct object\" you can cast \"struct object\" to\n\"struct object_id_type\" for fsck.c use, and further cast a \"struct\nobject_id_type\" to \"struct object_id\" for anything that needs that.\n\nAnyway, right now I don't have anything conclusive. Just thought I'd\npoke you/others with the above in case it's an appraoch that's been\ntried/considered before.\n"}]}