{"thread":{"id":"53436","subject":"invalid tree and commit object","startedAt":"2020-05-09T06:19:51Z","lastAt":"2020-05-21T18:01:22Z","messageCount":23,"participants":["Brandon Williams","René Scharfe","Johannes Schindelin","Junio C Hamano","Denton Liu"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"397446","messageId":"CALN-EhTpiLERuB16-WPZaLub6GdaRHJW8xDeaOEqSFtKe0kCYw@mail.gmail.com","threadId":"53436","inReplyTo":null,"subject":"invalid tree and commit object","fromName":"Brandon Williams","fromEmail":"bwilliamseng@gmail.com","sentAt":"2020-05-09T06:19:38Z","receivedAt":"2020-05-09T06:19:51Z","isPatch":false,"sender":{"key":"bwilliamseng@gmail.com","avatar":null},"body":"Hey!\n\nIts been a minute since I've written to the list but I was recently looking\ninto the rules fsck uses to identify valid or invalid objects and I believe I\nfound a case that I believe fsck is currently missing. One of the things fsck\nlooks for when validating a tree object is that it doesn't contain any\nduplicate entries. It even has a nice comment about how `git-write-tree` used\nto write out trees with duplicate entries:\n\n    /*\n     * git-write-tree used to write out a nonsense tree that has\n     * entries with the same name, one blob and one tree.  Make\n     * sure we do not have duplicate entries.\n     */\n\nHere's the setup:\n    tree c63d067eaeed0cbc68b7e4fdf40d267c6b152fe8\n    tree 6241ab2a5314798183b5c4ee8a7b0ccd12c651e6\n    blob 5e1c309dae7f45e0f39b1bf3ac3cd9db12e7d689\n\n    $ git ls-tree c63d067eaeed0cbc68b7e4fdf40d267c6b152fe8\n    100644 blob 5e1c309dae7f45e0f39b1bf3ac3cd9db12e7d689    hello\n    100644 blob 5e1c309dae7f45e0f39b1bf3ac3cd9db12e7d689    hello.c\n    040000 tree 6241ab2a5314798183b5c4ee8a7b0ccd12c651e6    hello\n\n    $ git ls-tree 6241ab2a5314798183b5c4ee8a7b0ccd12c651e6\n    100644 blob 5e1c309dae7f45e0f39b1bf3ac3cd9db12e7d689    hello\n\n    # '%' here indicates that there is no newline at the end of the object\n    $ git cat-file blob 5e1c309dae7f45e0f39b1bf3ac3cd9db12e7d689\n    Hello World%\n\nfsck currently passes when being passed these objects despite c63d067eae having\na duplicate entry. This seems to be due to the duplicate entry check in\n`fsck_tree` only checking if adjacent entries are duplicates but due to the\nsorting rules its unable to realize that there is both a blob and a tree with\nthe name \"hello\".\n\nI was even able to produce a commit and push it to Github[1] (which\ndidn't complain)\n\n    $ git show --pretty=raw 62f1ff6e109f8b77edd7eeb65f6634faa76a93b2\n    commit 62f1ff6e109f8b77edd7eeb65f6634faa76a93b2\n    tree c63d067eaeed0cbc68b7e4fdf40d267c6b152fe8\n    author Brandon Williams <bwilliams.eng@gmail.com> 1589004242 -0700\n    committer Brandon Williams <bwilliams.eng@gmail.com> 1589004242 -0700\n\n        hello\n\nChecking out that commit leaves your working directory in a somewhat\nbroken and 'unclean' state (although Github's UI seems to be able to handle\ndisplaying it properly).\n\nAm I correct in assuming that this object is indeed invalid and should be\nrejected by fsck?\n\n-Brandon\n\n[1]: https://github.com/bmwill/invalid-commit\n"},{"id":"397447","messageId":"d963242a-72f3-7f42-7c95-ea5148f74804@web.de","threadId":"53436","inReplyTo":"CALN-EhTpiLERuB16-WPZaLub6GdaRHJW8xDeaOEqSFtKe0kCYw@mail.gmail.com","subject":"Re: invalid tree and commit object","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2020-05-09T10:16:08Z","receivedAt":"2020-05-09T10:16:22Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 09.05.20 um 08:19 schrieb Brandon Williams:\n> Here's the setup:\n>     tree c63d067eaeed0cbc68b7e4fdf40d267c6b152fe8\n>     tree 6241ab2a5314798183b5c4ee8a7b0ccd12c651e6\n>     blob 5e1c309dae7f45e0f39b1bf3ac3cd9db12e7d689\n>\n>     $ git ls-tree c63d067eaeed0cbc68b7e4fdf40d267c6b152fe8\n>     100644 blob 5e1c309dae7f45e0f39b1bf3ac3cd9db12e7d689    hello\n>     100644 blob 5e1c309dae7f45e0f39b1bf3ac3cd9db12e7d689    hello.c\n>     040000 tree 6241ab2a5314798183b5c4ee8a7b0ccd12c651e6    hello\n\n> Am I correct in assuming that this object is indeed invalid and should be\n> rejected by fsck?\n\nI'd say yes twice -- what good is a tree that you can't check out because\nit contains a d/f conflict?\n\nSo I got curious if such trees might be in popular repos, wrote the patch\nbelow and checked around a bit, but couldn't find any.\n\nIs there a smarter way to check for duplicates?  One that doesn't need\nallocations?  Perhaps by having a version of tree_entry_extract() that\nseeks backwards somehow?\n\n---\n fsck.c          | 10 ++++++++++\n t/t1450-fsck.sh | 16 ++++++++++++++++\n 2 files changed, 26 insertions(+)\n\ndiff --git a/fsck.c b/fsck.c\nindex 087a7f1ffc..f47b35fee8 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -587,6 +587,8 @@ static int fsck_tree(const struct object_id *oid,\n \tstruct tree_desc desc;\n \tunsigned o_mode;\n \tconst char *o_name;\n+\tstruct string_list names = STRING_LIST_INIT_NODUP;\n+\tsize_t nr;\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@@ -680,8 +682,16 @@ static int fsck_tree(const struct object_id *oid,\n\n \t\to_mode = mode;\n \t\to_name = name;\n+\t\tstring_list_append(&names, name);\n \t}\n\n+\tnr = names.nr;\n+\tstring_list_sort(&names);\n+\tstring_list_remove_duplicates(&names, 0);\n+\tif (names.nr != nr)\n+\t\thas_dup_entries = 1;\n+\tstring_list_clear(&names, 0);\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 \tif (has_full_path)\ndiff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\nindex 449ebc5657..91a6e34f38 100755\n--- a/t/t1450-fsck.sh\n+++ b/t/t1450-fsck.sh\n@@ -257,6 +257,22 @@ test_expect_success 'tree object with duplicate entries' '\n \ttest_i18ngrep \"error in tree .*contains duplicate file entries\" out\n '\n\n+test_expect_success 'tree object with dublicate names' '\n+\ttest_when_finished \"remove_object \\$blob\" &&\n+\ttest_when_finished \"remove_object \\$tree\" &&\n+\ttest_when_finished \"remove_object \\$badtree\" &&\n+\tblob=$(echo blob | git hash-object -w --stdin) &&\n+\tprintf \"100644 blob %s\\t%s\\n\" $blob x.2 >tree &&\n+\ttree=$(git mktree <tree) &&\n+\tprintf \"100644 blob %s\\t%s\\n\" $blob x.1 >badtree &&\n+\tprintf \"100644 blob %s\\t%s\\n\" $blob x >>badtree &&\n+\tprintf \"040000 tree %s\\t%s\\n\" $tree x >>badtree &&\n+\tbadtree=$(git mktree <badtree) &&\n+\ttest_must_fail git fsck 2>out &&\n+\ttest_i18ngrep \"$badtree\" out &&\n+\ttest_i18ngrep \"error in tree .*contains duplicate file entries\" out\n+'\n+\n test_expect_success 'unparseable tree object' '\n \ttest_oid_cache <<-\\EOF &&\n \tjunk sha1:twenty-bytes-of-junk\n--\n2.26.2\n"},{"id":"397449","messageId":"nycvar.QRO.7.76.6.2005090915020.56@tvgsbejvaqbjf.bet","threadId":"53436","inReplyTo":"d963242a-72f3-7f42-7c95-ea5148f74804@web.de","subject":"Re: invalid tree and commit object","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-05-09T07:16:00Z","receivedAt":"2020-05-09T11:35:38Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sat, 9 May 2020, René Scharfe wrote:\n\n> Am 09.05.20 um 08:19 schrieb Brandon Williams:\n> > Here's the setup:\n> >     tree c63d067eaeed0cbc68b7e4fdf40d267c6b152fe8\n> >     tree 6241ab2a5314798183b5c4ee8a7b0ccd12c651e6\n> >     blob 5e1c309dae7f45e0f39b1bf3ac3cd9db12e7d689\n> >\n> >     $ git ls-tree c63d067eaeed0cbc68b7e4fdf40d267c6b152fe8\n> >     100644 blob 5e1c309dae7f45e0f39b1bf3ac3cd9db12e7d689    hello\n> >     100644 blob 5e1c309dae7f45e0f39b1bf3ac3cd9db12e7d689    hello.c\n> >     040000 tree 6241ab2a5314798183b5c4ee8a7b0ccd12c651e6    hello\n>\n> > Am I correct in assuming that this object is indeed invalid and should be\n> > rejected by fsck?\n>\n> I'd say yes twice -- what good is a tree that you can't check out because\n> it contains a d/f conflict?\n>\n> So I got curious if such trees might be in popular repos, wrote the patch\n> below and checked around a bit, but couldn't find any.\n>\n> Is there a smarter way to check for duplicates?  One that doesn't need\n> allocations?  Perhaps by having a version of tree_entry_extract() that\n> seeks backwards somehow?\n\nMaybe we should verify that the entries are sorted? That would not need\nany allocation, and it could even use the return value of the same\ncomparison we already perform to check for duplicates.\n\nCiao,\nDscho\n"},{"id":"397450","messageId":"142b7944-e151-2160-2fbe-e277f16033c9@web.de","threadId":"53436","inReplyTo":"nycvar.QRO.7.76.6.2005090915020.56@tvgsbejvaqbjf.bet","subject":"Re: invalid tree and commit object","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2020-05-09T11:51:26Z","receivedAt":"2020-05-09T11:52:03Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 09.05.20 um 09:16 schrieb Johannes Schindelin:\n> Hi,\n>\n> On Sat, 9 May 2020, René Scharfe wrote:\n>\n>> Am 09.05.20 um 08:19 schrieb Brandon Williams:\n>>> Here's the setup:\n>>>     tree c63d067eaeed0cbc68b7e4fdf40d267c6b152fe8\n>>>     tree 6241ab2a5314798183b5c4ee8a7b0ccd12c651e6\n>>>     blob 5e1c309dae7f45e0f39b1bf3ac3cd9db12e7d689\n>>>\n>>>     $ git ls-tree c63d067eaeed0cbc68b7e4fdf40d267c6b152fe8\n>>>     100644 blob 5e1c309dae7f45e0f39b1bf3ac3cd9db12e7d689    hello\n>>>     100644 blob 5e1c309dae7f45e0f39b1bf3ac3cd9db12e7d689    hello.c\n>>>     040000 tree 6241ab2a5314798183b5c4ee8a7b0ccd12c651e6    hello\n>>\n>>> Am I correct in assuming that this object is indeed invalid and should be\n>>> rejected by fsck?\n>>\n>> I'd say yes twice -- what good is a tree that you can't check out because\n>> it contains a d/f conflict?\n>>\n>> So I got curious if such trees might be in popular repos, wrote the patch\n>> below and checked around a bit, but couldn't find any.\n>>\n>> Is there a smarter way to check for duplicates?  One that doesn't need\n>> allocations?  Perhaps by having a version of tree_entry_extract() that\n>> seeks backwards somehow?\n>\n> Maybe we should verify that the entries are sorted? That would not need\n> any allocation, and it could even use the return value of the same\n> comparison we already perform to check for duplicates.\n\nYes, but the key question is: Sorted by what?  Tree entries are sorted\nby name, only that a slash is appended implicitly to the name of entries\nof type tree.  The current code already checks for duplicates by\ncomparing neighboring entries' names.  That's not sufficient to find\nduplicates that differ in type, as Brandon's example above shows (which\nis sorted correctly).\n\nRené\n"},{"id":"397460","messageId":"xmqqpnbduiec.fsf@gitster.c.googlers.com","threadId":"53436","inReplyTo":"d963242a-72f3-7f42-7c95-ea5148f74804@web.de","subject":"Re: invalid tree and commit object","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-09T17:28:59Z","receivedAt":"2020-05-09T17:29:06Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> So I got curious if such trees might be in popular repos, wrote the patch\n> below and checked around a bit, but couldn't find any.\n>\n> Is there a smarter way to check for duplicates?  One that doesn't need\n> allocations?  Perhaps by having a version of tree_entry_extract() that\n> seeks backwards somehow?\n\nI've never looked into seeking backwards in a tree object, but in\nunpack-trees, I had to deal with this exact problem that a blob\n'hello' sorts before 'hello.c' which in turn sorts after a tree\n'hello' because of the \"implicit slash after the name for an entry\nfor a tree object\" rule by introducing the \"cache_bottom\" hack in\nthe traversal logic to limit how far we must scan back.\n\nWe may be able to limit the list of \"seen recently\" names in a\nsimilar way.\n\nIf the tree we are scanning has 'hello' (blob), 'hello.c' and\n'hellp', the bottom pointer initially would be at 'hello' (blob),\nthen stay there when we see 'hello.c' (because the next entry might\nbe 'hello' (tree) that would crash with 'hello'), and when we see\nthe entry 'hellp', we know that the entry at the bottom pointer\n'hello' (blob) cannot crash with any entry that comes later in the\ntree object we are scanning, so we can advance the bottom pointer\nforward.  To decide if we can advance the bottom pointer beyond\n'hello.c' (blob), we see if 'hello.c' (tree) can appear after the\ncurrent entry we are looking at (i.e. 'hellp'), and we know it\ncannot without violating the sort order.  So the bottom would move\nto point at 'hellp' we just saw.\n\nIf we had 'hello' (tree) instead of 'hellp', when we look at it\nafter looking at 'hello' (blob) and 'hello.c', we scan from the\nbottom pointer up to the previous entry, which is still pointing at\n'hello' (blob), and notice the crash.\n\n"},{"id":"397469","messageId":"938f0818-7e57-b883-009f-01db88ef8f65@web.de","threadId":"53436","inReplyTo":"xmqqpnbduiec.fsf@gitster.c.googlers.com","subject":"Re: invalid tree and commit object","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2020-05-09T19:24:43Z","receivedAt":"2020-05-09T19:24:54Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 09.05.20 um 19:28 schrieb Junio C Hamano:\n> René Scharfe <l.s.r@web.de> writes:\n>\n>> So I got curious if such trees might be in popular repos, wrote the patch\n>> below and checked around a bit, but couldn't find any.\n>>\n>> Is there a smarter way to check for duplicates?  One that doesn't need\n>> allocations?  Perhaps by having a version of tree_entry_extract() that\n>> seeks backwards somehow?\n>\n> I've never looked into seeking backwards in a tree object, but in\n> unpack-trees, I had to deal with this exact problem that a blob\n> 'hello' sorts before 'hello.c' which in turn sorts after a tree\n> 'hello' because of the \"implicit slash after the name for an entry\n> for a tree object\" rule by introducing the \"cache_bottom\" hack in\n> the traversal logic to limit how far we must scan back.\n>\n> We may be able to limit the list of \"seen recently\" names in a\n> similar way.\n>\n> If the tree we are scanning has 'hello' (blob), 'hello.c' and\n> 'hellp', the bottom pointer initially would be at 'hello' (blob),\n> then stay there when we see 'hello.c' (because the next entry might\n> be 'hello' (tree) that would crash with 'hello'), and when we see\n> the entry 'hellp', we know that the entry at the bottom pointer\n> 'hello' (blob) cannot crash with any entry that comes later in the\n> tree object we are scanning, so we can advance the bottom pointer\n> forward.  To decide if we can advance the bottom pointer beyond\n> 'hello.c' (blob), we see if 'hello.c' (tree) can appear after the\n> current entry we are looking at (i.e. 'hellp'), and we know it\n> cannot without violating the sort order.  So the bottom would move\n> to point at 'hellp' we just saw.\n>\n> If we had 'hello' (tree) instead of 'hellp', when we look at it\n> after looking at 'hello' (blob) and 'hello.c', we scan from the\n> bottom pointer up to the previous entry, which is still pointing at\n> 'hello' (blob), and notice the crash.\n>\n\nHmm, this could lead to quadratic behavior in the worst case, can't it?\nImagine you have:\n\n  a\n  a.b\n  a.b.c\n  ...\n  a.b.c/\n  a.b/\n  a/\n\nIf you have a single bottom pointer, it needs to stay at \"a\" the whole\ntime, so that you can detect the d/f conflict with \"a/\" at the end.\nAnd for all entries in between you need to scan from \"a\" on and compare\neach entry -- quadratic.  The blobs \"a.b\" and \"a.b.c\" don't need to\nactually be present, but we need to scan all entries to determine\n\"a.b/\" and \"a.b.c/\" are not conflicting anyway.  Right?\n\nWe could, however, reduce the names we add to the string_list to\nthose that are possible candidates for conflict -- blobs followed by an\nentry whose name starts with the blob name followed by a dot and trees\nthat follow an entry whose name matches in the same way.\n\nRené\n"},{"id":"397470","messageId":"xmqqh7wovoop.fsf@gitster.c.googlers.com","threadId":"53436","inReplyTo":"938f0818-7e57-b883-009f-01db88ef8f65@web.de","subject":"Re: invalid tree and commit object","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-09T20:27:50Z","receivedAt":"2020-05-09T20:27:58Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> Hmm, this could lead to quadratic behavior in the worst case, can't it?\n\nOh, absolutely.  But as you have shown, you'd need a specially\ncrafted tree with early entries that are prefixes of later ones,\nwhich would rather be rare, and most of the time the bottom pointer\nwould advance by one every time we consume one path.  \n\nSo it is trading (hopefully rare) worst-case runtime with reduced\nstorage cost.\n\n> We could, however, reduce the names we add to the string_list to\n> those that are possible candidates for conflict -- blobs followed by an\n> entry whose name starts with the blob name followed by a dot and trees\n> that follow an entry whose name matches in the same way.\n\nYes, that is a valid solution that strikes a different balance\nbetween allocation and runtime.\n\nWe may want to survey how commonly \"bad\" trees appear in real\nprojects.  Depending on the result, we might want to update the\n\"limit re-scanning using the bottom pointer\" hack we have been using\nin the unpack-trees code.\n\nThanks.\n"},{"id":"397487","messageId":"aab9512b-a70a-0f5b-5cdc-5d40acd343d0@web.de","threadId":"53436","inReplyTo":"xmqqh7wovoop.fsf@gitster.c.googlers.com","subject":"Re: invalid tree and commit object","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2020-05-10T09:07:25Z","receivedAt":"2020-05-10T09:07:47Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 09.05.20 um 22:27 schrieb Junio C Hamano:\n> René Scharfe <l.s.r@web.de> writes:\n>\n>> Hmm, this could lead to quadratic behavior in the worst case, can't it?\n>\n> Oh, absolutely.  But as you have shown, you'd need a specially\n> crafted tree with early entries that are prefixes of later ones,\n> which would rather be rare, and most of the time the bottom pointer\n> would advance by one every time we consume one path.\n>\n> So it is trading (hopefully rare) worst-case runtime with reduced\n> storage cost.\n>\n>> We could, however, reduce the names we add to the string_list to\n>> those that are possible candidates for conflict -- blobs followed by an\n>> entry whose name starts with the blob name followed by a dot and trees\n>> that follow an entry whose name matches in the same way.\n>\n> Yes, that is a valid solution that strikes a different balance\n> between allocation and runtime.\n\nfsck should ideally handle the worst cases gracefully -- I assume it has\nto deal with the weirdest \"natural\" corruptions and the \"best\" that\npranksters and DoSers can come up with.\n\n> We may want to survey how commonly \"bad\" trees appear in real\n> projects.  Depending on the result, we might want to update the\n> \"limit re-scanning using the bottom pointer\" hack we have been using\n> in the unpack-trees code.\n\nThey are surprisingly common in Git's own repo.  Ca. 74% of the trees\nin my clone had at least one candidate and the average number of\ncandidates per tree in those was ca. 8.9.  We have e.g. the t/ tree\nwith its several tnnnn/ vs. tnnnn-foo.sh entries, or builtin/ vs.\nbuiltin.h.\n\nIn the Linux repo ca. 20% of the checked trees have at least a\ncandidate and the average number of candidates in those is ca. 2.4.\n\nSo the patch for only adding possible candidates to the string_list\nis below (on top of my earlier one), but I wonder if we can do\nbetter.\n\nWhen we look for a d/f name clash we don't necessarily have to look\nback upon finding a candidate directory -- we can also scan ahead\nupon finding a candidate non-directory.  That could be done using\nrepeated update_tree_entry_gently() calls on a private copy.  It\nwouldn't require any string_list allocations, but its worst case\ncomplexity would still be quadratic, of course.\n\nWould a stack work?  When we see a candidate non-directory, we put\nit on the stack.  When we see a candidate directory, we compare it\nto the entry at the top of the stack using strcmp().  Equality\nindicates a duplicate and we are done.  If the directory name is\nless then we can pop the entry from the stack and check again, as\nwe're past the point where a duplicate would be.  Makes sense?\n\nThe candidate stack solution would have linear complexity and\nrequire less memory than a full list of candidates.\n\nIt would rely on the entries to be in the correct order (same as\nthe patch below, come to think of it), but that's probably OK.\nWe may miss DUPLICATE_ENTRIES (just like today :), but\nTREE_NOT_SORTED would still be reported.\n\n---\n fsck.c | 25 +++++++++++++++++++++++--\n 1 file changed, 23 insertions(+), 2 deletions(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex f47b35fee8..7d5bfef804 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -569,6 +569,25 @@ static int verify_ordered(unsigned mode1, const char *name1, unsigned mode2, con\n \treturn c1 < c2 ? 0 : TREE_UNORDERED;\n }\n\n+/*\n+ * Consecutive entries are checked for duplicates in verify_ordered().\n+ * However, there can be non-consecutive duplicates because a slash\n+ * ('/') is appended implicitly to directory names during sorting, so\n+ * there can be other names between a non-directory entry and a\n+ * directory entry with the same name.  E.g.:\n+ *\n+ *    foo\n+ *    foo.bar\n+ *    foo/\n+ *\n+ */\n+static int may_be_df_dup(const char *candidate, const char *interloper)\n+{\n+\tconst char *p;\n+\n+\treturn skip_prefix(interloper, candidate, &p) && '\\0' < *p && *p < '/';\n+}\n+\n static int fsck_tree(const struct object_id *oid,\n \t\t     const char *buffer, unsigned long size,\n \t\t     struct fsck_options *options)\n@@ -678,11 +697,14 @@ static int fsck_tree(const struct object_id *oid,\n \t\t\tdefault:\n \t\t\t\tbreak;\n \t\t\t}\n+\t\t\tif (!S_ISDIR(o_mode) && may_be_df_dup(o_name, name))\n+\t\t\t\tstring_list_append(&names, o_name);\n+\t\t\tif (S_ISDIR(mode) && may_be_df_dup(name, o_name))\n+\t\t\t\tstring_list_append(&names, name);\n \t\t}\n\n \t\to_mode = mode;\n \t\to_name = name;\n-\t\tstring_list_append(&names, name);\n \t}\n\n \tnr = names.nr;\n@@ -1221,7 +1243,6 @@ int fsck_finish(struct fsck_options *options)\n \t\tfree(buf);\n \t}\n\n-\n \toidset_clear(&gitmodules_found);\n \toidset_clear(&gitmodules_done);\n \treturn ret;\n--\n2.26.2\n\n"},{"id":"397502","messageId":"2937d635-52a9-5e69-b3d2-fbde415b7315@web.de","threadId":"53436","inReplyTo":"aab9512b-a70a-0f5b-5cdc-5d40acd343d0@web.de","subject":"Re: invalid tree and commit object","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2020-05-10T16:12:16Z","receivedAt":"2020-05-10T16:12:27Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 10.05.20 um 11:07 schrieb René Scharfe:\n> Would a stack work?  When we see a candidate non-directory, we put\n> it on the stack.  When we see a candidate directory, we compare it\n> to the entry at the top of the stack using strcmp().  Equality\n> indicates a duplicate and we are done.  If the directory name is\n> less then we can pop the entry from the stack and check again, as\n> we're past the point where a duplicate would be.  Makes sense?\n>\n> The candidate stack solution would have linear complexity and\n> require less memory than a full list of candidates.\n>\n> It would rely on the entries to be in the correct order (same as\n> the patch below, come to think of it), but that's probably OK.\n> We may miss DUPLICATE_ENTRIES (just like today :), but\n> TREE_NOT_SORTED would still be reported.\n\nHere's what I mean.  It seems to work, but could use some critical\nthought and testing.\n\n-- >8 --\nSubject: [PATCH] fsck: report non-consecutive duplicate names in trees\n\nTree entries are sorted in path order, meaning that directory names get\na slash ('/') appended implicitly.  Git fsck checks if trees contains\nconsecutive duplicates, but due to that ordering there can be\nnon-consecutive duplicates as well if one of them is a directory and the\nother one isn't.  Such a tree cannot be fully checked out.\n\nFind these duplicates by recording candidate file names on a stack and\ncheck candidate directory names against that stack to find matches.\n\nSuggested-by: Brandon Williams <bwilliamseng@gmail.com>\nOriginal-test-by: Brandon Williams <bwilliamseng@gmail.com>\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n fsck.c          | 72 +++++++++++++++++++++++++++++++++++++++++++++++--\n t/t1450-fsck.sh | 16 +++++++++++\n 2 files changed, 86 insertions(+), 2 deletions(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex 087a7f1ffc..8bb3ecf282 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -523,6 +523,28 @@ int fsck_walk(struct object *obj, void *data, struct fsck_options *options)\n \t}\n }\n\n+struct name_stack {\n+\tconst char **names;\n+\tsize_t nr, alloc;\n+};\n+\n+static void name_stack_push(struct name_stack *stack, const char *name)\n+{\n+\tALLOC_GROW(stack->names, stack->nr + 1, stack->alloc);\n+\tstack->names[stack->nr++] = name;\n+}\n+\n+static const char *name_stack_pop(struct name_stack *stack)\n+{\n+\treturn stack->nr ? stack->names[--stack->nr] : NULL;\n+}\n+\n+static void name_stack_clear(struct name_stack *stack)\n+{\n+\tFREE_AND_NULL(stack->names);\n+\tstack->nr = stack->alloc = 0;\n+}\n+\n /*\n  * The entries in a tree are ordered in the _path_ order,\n  * which means that a directory entry is ordered by adding\n@@ -534,7 +556,14 @@ int fsck_walk(struct object *obj, void *data, struct fsck_options *options)\n #define TREE_UNORDERED (-1)\n #define TREE_HAS_DUPS  (-2)\n\n-static int verify_ordered(unsigned mode1, const char *name1, unsigned mode2, const char *name2)\n+static int is_less_than_slash(unsigned char c)\n+{\n+\treturn '\\0' < c && c < '/';\n+}\n+\n+static int verify_ordered(unsigned mode1, const char *name1,\n+\t\t\t  unsigned mode2, const char *name2,\n+\t\t\t  struct name_stack *candidates)\n {\n \tint len1 = strlen(name1);\n \tint len2 = strlen(name2);\n@@ -566,6 +595,41 @@ static int verify_ordered(unsigned mode1, const char *name1, unsigned mode2, con\n \t\tc1 = '/';\n \tif (!c2 && S_ISDIR(mode2))\n \t\tc2 = '/';\n+\n+\t/*\n+\t * There can be non-consecutive duplicates due to the implicitly\n+\t * add slash, e.g.:\n+\t *\n+\t *   foo\n+\t *   foo.bar\n+\t *   foo.bar.baz\n+\t *   foo.bar/\n+\t *   foo/\n+\t *\n+\t * Record non-directory candidates (like \"foo\" and \"foo.bar\" in\n+\t * the example) on a stack and check directory candidates (like\n+\t * foo/\" and \"foo.bar/\") against that stack.\n+\t */\n+\tif (!c1 && is_less_than_slash(c2)) {\n+\t\tname_stack_push(candidates, name1);\n+\t} else if (c2 == '/' && is_less_than_slash(c1)) {\n+\t\tfor (;;) {\n+\t\t\tconst char *p;\n+\t\t\tconst char *f_name = name_stack_pop(candidates);\n+\n+\t\t\tif (!f_name)\n+\t\t\t\tbreak;\n+\t\t\tif (!skip_prefix(name2, f_name, &p))\n+\t\t\t\tbreak;\n+\t\t\tif (!*p)\n+\t\t\t\treturn TREE_HAS_DUPS;\n+\t\t\tif (is_less_than_slash(*p)) {\n+\t\t\t\tname_stack_push(candidates, f_name);\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t}\n+\t}\n+\n \treturn c1 < c2 ? 0 : TREE_UNORDERED;\n }\n\n@@ -587,6 +651,7 @@ static int fsck_tree(const struct object_id *oid,\n \tstruct tree_desc desc;\n \tunsigned o_mode;\n \tconst char *o_name;\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@@ -666,7 +731,8 @@ static int fsck_tree(const struct object_id *oid,\n \t\t}\n\n \t\tif (o_name) {\n-\t\t\tswitch (verify_ordered(o_mode, o_name, mode, name)) {\n+\t\t\tswitch (verify_ordered(o_mode, o_name, mode, name,\n+\t\t\t\t\t       &df_dup_candidates)) {\n \t\t\tcase TREE_UNORDERED:\n \t\t\t\tnot_properly_sorted = 1;\n \t\t\t\tbreak;\n@@ -682,6 +748,8 @@ static int fsck_tree(const struct object_id *oid,\n \t\to_name = name;\n \t}\n\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 \tif (has_full_path)\ndiff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\nindex 449ebc5657..91a6e34f38 100755\n--- a/t/t1450-fsck.sh\n+++ b/t/t1450-fsck.sh\n@@ -257,6 +257,22 @@ test_expect_success 'tree object with duplicate entries' '\n \ttest_i18ngrep \"error in tree .*contains duplicate file entries\" out\n '\n\n+test_expect_success 'tree object with dublicate names' '\n+\ttest_when_finished \"remove_object \\$blob\" &&\n+\ttest_when_finished \"remove_object \\$tree\" &&\n+\ttest_when_finished \"remove_object \\$badtree\" &&\n+\tblob=$(echo blob | git hash-object -w --stdin) &&\n+\tprintf \"100644 blob %s\\t%s\\n\" $blob x.2 >tree &&\n+\ttree=$(git mktree <tree) &&\n+\tprintf \"100644 blob %s\\t%s\\n\" $blob x.1 >badtree &&\n+\tprintf \"100644 blob %s\\t%s\\n\" $blob x >>badtree &&\n+\tprintf \"040000 tree %s\\t%s\\n\" $tree x >>badtree &&\n+\tbadtree=$(git mktree <badtree) &&\n+\ttest_must_fail git fsck 2>out &&\n+\ttest_i18ngrep \"$badtree\" out &&\n+\ttest_i18ngrep \"error in tree .*contains duplicate file entries\" out\n+'\n+\n test_expect_success 'unparseable tree object' '\n \ttest_oid_cache <<-\\EOF &&\n \tjunk sha1:twenty-bytes-of-junk\n--\n2.26.2\n"},{"id":"397503","messageId":"xmqq1rnrvj8d.fsf@gitster.c.googlers.com","threadId":"53436","inReplyTo":"aab9512b-a70a-0f5b-5cdc-5d40acd343d0@web.de","subject":"Re: invalid tree and commit object","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-10T16:37:54Z","receivedAt":"2020-05-10T16:38:03Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> Would a stack work?  When we see a candidate non-directory, we put\n> it on the stack.  When we see a candidate directory, we compare it\n> to the entry at the top of the stack using strcmp().  Equality\n> indicates a duplicate and we are done.  If the directory name is\n> less then we can pop the entry from the stack and check again, as\n> we're past the point where a duplicate would be.  Makes sense?\n\nPerfectly and quite excited ;-)  I wonder if we can do the same in\nthe unpack-trees side.\n\n\n"},{"id":"397556","messageId":"xmqq4ksmsaks.fsf@gitster.c.googlers.com","threadId":"53436","inReplyTo":"2937d635-52a9-5e69-b3d2-fbde415b7315@web.de","subject":"Re: invalid tree and commit object","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-11T16:25:23Z","receivedAt":"2020-05-11T16:25:36Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n>  fsck.c          | 72 +++++++++++++++++++++++++++++++++++++++++++++++--\n>  t/t1450-fsck.sh | 16 +++++++++++\n>  2 files changed, 86 insertions(+), 2 deletions(-)\n>\n> diff --git a/fsck.c b/fsck.c\n> index 087a7f1ffc..8bb3ecf282 100644\n> --- a/fsck.c\n> +++ b/fsck.c\n> @@ -523,6 +523,28 @@ int fsck_walk(struct object *obj, void *data, struct fsck_options *options)\n>  \t}\n>  }\n>\n> +struct name_stack {\n> +\tconst char **names;\n> +\tsize_t nr, alloc;\n> +};\n> +\n> +static void name_stack_push(struct name_stack *stack, const char *name)\n> +{\n> +\tALLOC_GROW(stack->names, stack->nr + 1, stack->alloc);\n> +\tstack->names[stack->nr++] = name;\n> +}\n> +\n> +static const char *name_stack_pop(struct name_stack *stack)\n> +{\n> +\treturn stack->nr ? stack->names[--stack->nr] : NULL;\n> +}\n> +\n> +static void name_stack_clear(struct name_stack *stack)\n> +{\n> +\tFREE_AND_NULL(stack->names);\n> +\tstack->nr = stack->alloc = 0;\n> +}\n\nOK, names are just pointing into tree objects' buffer, and will be\nthere until we no longer need the stack and call stack_clear().\nGood.\n\n> @@ -534,7 +556,14 @@ int fsck_walk(struct object *obj, void *data, struct fsck_options *options)\n>  #define TREE_UNORDERED (-1)\n>  #define TREE_HAS_DUPS  (-2)\n>\n> -static int verify_ordered(unsigned mode1, const char *name1, unsigned mode2, const char *name2)\n> +static int is_less_than_slash(unsigned char c)\n> +{\n> +\treturn '\\0' < c && c < '/';\n> +}\n\nMental note: the terminating byte does not count as\n\"less than\".\n\n> +static int verify_ordered(unsigned mode1, const char *name1,\n> +\t\t\t  unsigned mode2, const char *name2,\n> +\t\t\t  struct name_stack *candidates)\n\nMental note: the caller is iterating over tree entries, and calls\nthis helper with two adjacent entries (mode2/name2 is what it just\nsaw).  The function wants to see if name1 has duplicate in the tree\nobject, and before this fix, we thought that it is sufficient to\ncompare it with name2, but now we realized that an entry with the\nsame name as name1 can come much later than name2.  But it is still\nthe same in spirit---we want to make sure name1 does not have any\nduplicate (name2 will get its turn by the next call to this function\nthe caller makes).\n\n>  {\n>  \tint len1 = strlen(name1);\n>  \tint len2 = strlen(name2);\n> @@ -566,6 +595,41 @@ static int verify_ordered(unsigned mode1, const char *name1, unsigned mode2, con\n>  \t\tc1 = '/';\n>  \tif (!c2 && S_ISDIR(mode2))\n>  \t\tc2 = '/';\n\nMental note: at this point, we have rejected two adjacent and\nidentical names, and c1 and c2 are the first byte that are different\nbetween these two names; but if it is NUL at the end of the name,\nand the object is a tree, c1/c2 is changed to a slash.\n\n> +\n> +\t/*\n> +\t * There can be non-consecutive duplicates due to the implicitly\n> +\t * add slash, e.g.:\n\ns/add slash/added slash/, or even \"added slash at the end of the\nname of a tree object\".\n\n> +\t *\n> +\t *   foo\n> +\t *   foo.bar\n> +\t *   foo.bar.baz\n> +\t *   foo.bar/\n> +\t *   foo/\n> +\t *\n> +\t * Record non-directory candidates (like \"foo\" and \"foo.bar\" in\n> +\t * the example) on a stack and check directory candidates (like\n> +\t * foo/\" and \"foo.bar/\") against that stack.\n> +\t */\n> +\tif (!c1 && is_less_than_slash(c2)) {\n> +\t\tname_stack_push(candidates, name1);\n\nIf name1 is a blob and the name2 sorts before a hypothetical entry\nthat is a tree object with the same name as name1, we need to\nremember that we saw name1.  We need to keep that record until we\nsee an entry that sorts after such a hypothetical tree.\n\nWe earlier made a mental note that c2==NUL does not count as being\nless than slash, but it does not matter, as we rejected !c1 && !c2\nmuch earlier.\n\nWhen we remember name1 this way, we cannot yet tell if it is\nduplicate, so we don't do anything unusual.\n\n> +\t} else if (c2 == '/' && is_less_than_slash(c1)) {\n\nNow we are seeing a tree.  Does it crash with the last \"suspicious\"\nname we saw?\n\n> +\t\tfor (;;) {\n> +\t\t\tconst char *p;\n> +\t\t\tconst char *f_name = name_stack_pop(candidates);\n\nWe pop one name.  We know that \n\n - it is a name of a blob object\n - all we have seen since then are blobs and commits and no tree\n\n> +\t\t\tif (!f_name)\n> +\t\t\t\tbreak;\n\nIf there is no remembered name, we are done.\n\n> +\t\t\tif (!skip_prefix(name2, f_name, &p))\n> +\t\t\t\tbreak;\n\nIf the remembered name (e.g. \"foo\") is not a prefix of the current\nname (e.g. \"fop\"), we cannot have \"foo/\" after \"fop\" we are seeing\nwithout violating ordering constraints that we will notice while\ninspecting the later entries of this tree.  So \"foo\" can be\ndiscarded at this point.\n\n> +\t\t\tif (!*p)\n> +\t\t\t\treturn TREE_HAS_DUPS;\n\nIf name2 and f_name are the same, we have found a duplicate.\n\n> +\t\t\tif (is_less_than_slash(*p)) {\n\nIf name2 (the one we are currently looking at) has f_name as its\nprefix, and the first byte that differs is less than slash (again,\nour earlier observation that NUL is not counted as less than slash\ndoes not matter here, as we just handled NUL case before this\ncheck), then it still is possible that a tree entry with the same\nname as f_name is hiding behind name2, so we push it back to the\nstack (i.e.  we cannot tell f_name is a duplicat).  Entries in the\nstack below f_name sorts earlier than f_name, are they are prefixes\nof f_name in the ascending order of their length (due to the \"if not\nprefix we can pop\" logic we saw earlier), and a tree with the same\nname as stack[n] should sort earlier than a tree with the same name\nas stack[n-1], so after seeing that we are not ready to determine\nif f_name has duplicate, we know we are not ready for the names\ndeeper in the stack.\n\n\tSide note. this is nice but is subtle.  I'd need to retrace\n\tthe thoughts on this part again later to convince myself\n\tthat we are not missing anything.\n\n> +\t\t\t\tname_stack_push(candidates, f_name);\n> +\t\t\t\tbreak;\n> +\t\t\t}\n\nOtherwise, we know name2 sorts later than the hypothetical entry\nthat is a tree with the same name as f_name, so f_name can be\ndiscarded (i.e. we do not push it again).  Further, we may notice\nthat name2 makes it impossile for other names in the stack to have\nentries with the same name, so we iterate.\n\n> +\t\t}\n> +\t}\n> +\n>  \treturn c1 < c2 ? 0 : TREE_UNORDERED;\n>  }\n>\n> @@ -587,6 +651,7 @@ static int fsck_tree(const struct object_id *oid,\n>  \tstruct tree_desc desc;\n>  \tunsigned o_mode;\n>  \tconst char *o_name;\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> @@ -666,7 +731,8 @@ static int fsck_tree(const struct object_id *oid,\n>  \t\t}\n>\n>  \t\tif (o_name) {\n> -\t\t\tswitch (verify_ordered(o_mode, o_name, mode, name)) {\n> +\t\t\tswitch (verify_ordered(o_mode, o_name, mode, name,\n> +\t\t\t\t\t       &df_dup_candidates)) {\n>  \t\t\tcase TREE_UNORDERED:\n>  \t\t\t\tnot_properly_sorted = 1;\n>  \t\t\t\tbreak;\n> @@ -682,6 +748,8 @@ static int fsck_tree(const struct object_id *oid,\n>  \t\to_name = name;\n>  \t}\n>\n> +\tname_stack_clear(&df_dup_candidates);\n> +\n\nOK.  Nicely done.\n\nThanks.\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>  \tif (has_full_path)\n> diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\n> index 449ebc5657..91a6e34f38 100755\n> --- a/t/t1450-fsck.sh\n> +++ b/t/t1450-fsck.sh\n> @@ -257,6 +257,22 @@ test_expect_success 'tree object with duplicate entries' '\n>  \ttest_i18ngrep \"error in tree .*contains duplicate file entries\" out\n>  '\n>\n> +test_expect_success 'tree object with dublicate names' '\n> +\ttest_when_finished \"remove_object \\$blob\" &&\n> +\ttest_when_finished \"remove_object \\$tree\" &&\n> +\ttest_when_finished \"remove_object \\$badtree\" &&\n> +\tblob=$(echo blob | git hash-object -w --stdin) &&\n> +\tprintf \"100644 blob %s\\t%s\\n\" $blob x.2 >tree &&\n> +\ttree=$(git mktree <tree) &&\n> +\tprintf \"100644 blob %s\\t%s\\n\" $blob x.1 >badtree &&\n> +\tprintf \"100644 blob %s\\t%s\\n\" $blob x >>badtree &&\n> +\tprintf \"040000 tree %s\\t%s\\n\" $tree x >>badtree &&\n> +\tbadtree=$(git mktree <badtree) &&\n> +\ttest_must_fail git fsck 2>out &&\n> +\ttest_i18ngrep \"$badtree\" out &&\n> +\ttest_i18ngrep \"error in tree .*contains duplicate file entries\" out\n> +'\n> +\n>  test_expect_success 'unparseable tree object' '\n>  \ttest_oid_cache <<-\\EOF &&\n>  \tjunk sha1:twenty-bytes-of-junk\n> --\n> 2.26.2\n"},{"id":"397720","messageId":"CALN-EhQ91HDQ_VZ_+y2bPgFPhZ-_YWPAMZry4AhHTx+4w-DSJA@mail.gmail.com","threadId":"53436","inReplyTo":"xmqq4ksmsaks.fsf@gitster.c.googlers.com","subject":"Re: invalid tree and commit object","fromName":"Brandon Williams","fromEmail":"bwilliamseng@gmail.com","sentAt":"2020-05-13T16:27:16Z","receivedAt":"2020-05-13T16:27:30Z","isPatch":false,"sender":{"key":"bwilliamseng@gmail.com","avatar":null},"body":"Thanks for coming up with a solution so quickly! Glad it wasn't too\ndifficult to solve.\n"},{"id":"398334","messageId":"8cdbcd9b-b18d-d1d0-eec7-16f4139dbaf9@web.de","threadId":"53436","inReplyTo":"xmqq4ksmsaks.fsf@gitster.c.googlers.com","subject":"Re: invalid tree and commit object","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2020-05-21T09:51:33Z","receivedAt":"2020-05-21T09:51:52Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 11.05.20 um 18:25 schrieb Junio C Hamano:\n> René Scharfe <l.s.r@web.de> writes:\n>> +\n>> +\t/*\n>> +\t * There can be non-consecutive duplicates due to the implicitly\n>> +\t * add slash, e.g.:\n>\n> s/add slash/added slash/, or even \"added slash at the end of the\n> name of a tree object\".\n\nThanks for spotting the typo.  I think the short one suffices as the\nsentence below (and earlier comments) already cover it.\n\n>> +\t *\n>> +\t *   foo\n>> +\t *   foo.bar\n>> +\t *   foo.bar.baz\n>> +\t *   foo.bar/\n>> +\t *   foo/\n>> +\t *\n>> +\t * Record non-directory candidates (like \"foo\" and \"foo.bar\" in\n>> +\t * the example) on a stack and check directory candidates (like\n>> +\t * foo/\" and \"foo.bar/\") against that stack.\n>> +\t */\n\n> \tSide note. this is nice but is subtle.  I'd need to retrace\n> \tthe thoughts on this part again later to convince myself\n> \tthat we are not missing anything.\n\nYes, it's tricky, and we do miss something, as I mentioned in the test\ncoverage thread.  I'll reply with patches.\n\nRené\n"},{"id":"398335","messageId":"b0dd3b35-5875-8d4a-e078-26c368b81506@web.de","threadId":"53436","inReplyTo":"xmqq1rnrvj8d.fsf@gitster.c.googlers.com","subject":"Re: invalid tree and commit object","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2020-05-21T09:51:51Z","receivedAt":"2020-05-21T09:52:04Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 10.05.20 um 18:37 schrieb Junio C Hamano:\n> René Scharfe <l.s.r@web.de> writes:\n>\n>> Would a stack work?  When we see a candidate non-directory, we put\n>> it on the stack.  When we see a candidate directory, we compare it\n>> to the entry at the top of the stack using strcmp().  Equality\n>> indicates a duplicate and we are done.  If the directory name is\n>> less then we can pop the entry from the stack and check again, as\n>> we're past the point where a duplicate would be.  Makes sense?\n>\n> Perfectly and quite excited ;-)  I wonder if we can do the same in\n> the unpack-trees side.\n\nWalking multiple trees in lockstep should be possible as well.  We may\nwant to include a bitmap indicating which trees contain the name in the\nstack entry, or any other necessary information.\n\nAnd output of candidates would be delayed until the matching directory\nname is either reached or found to be missing, so it can be out of\norder.  Sorting like in diffcore_fix_diff_index() is an option if the\norder is important and the entries are collected before being used\nfurther.  Not sure if streaming in the right order could be done\nefficiently.\n\nBut lets see if that scheme works in fsck first.  I'll send a patch for\nmaking testing a bit easier.\n\nRené\n"},{"id":"398336","messageId":"0e34ddca-1e29-17ed-278c-4f94ed6436b3@web.de","threadId":"53436","inReplyTo":"xmqq4ksmsaks.fsf@gitster.c.googlers.com","subject":"[PATCH 1/4] fsck: fix a typo in a comment","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2020-05-21T09:52:04Z","receivedAt":"2020-05-21T09:52:13Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Reported-by: Junio C Hamano <gitster@pobox.com>\n---\n fsck.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex 8bb3ecf282..b48426262c 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -598,7 +598,7 @@ static int verify_ordered(unsigned mode1, const char *name1,\n\n \t/*\n \t * There can be non-consecutive duplicates due to the implicitly\n-\t * add slash, e.g.:\n+\t * added slash, e.g.:\n \t *\n \t *   foo\n \t *   foo.bar\n--\n2.26.2\n"},{"id":"398337","messageId":"106c58e1-9c74-46e3-c83a-88eee114d9d6@web.de","threadId":"53436","inReplyTo":"xmqq4ksmsaks.fsf@gitster.c.googlers.com","subject":"[PATCH 2/4] t1450: increase test coverage of in-tree d/f detection","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2020-05-21T09:52:28Z","receivedAt":"2020-05-21T09:52:37Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Exercise the case of putting a conflict candidate file name back on the\nstack because a matching directory might yet come up later.\n\nDo that by factoring out the test code into a function to allow for more\nconcise notation in the form of parameters indicating names of trees\n(with trailing slash) and blobs (without trailing slash) in no\nparticular order (they are sorted by git mktree).  Then add the new test\ncase as a second function call.\n\nFix a typo in the test title while at it (\"dublicate\").\n\nReported-by: Derrick Stolee <stolee@gmail.com>\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n t/t1450-fsck.sh | 42 +++++++++++++++++++++++++++---------------\n 1 file changed, 27 insertions(+), 15 deletions(-)\n\ndiff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\nindex 91a6e34f38..9640ac8ff2 100755\n--- a/t/t1450-fsck.sh\n+++ b/t/t1450-fsck.sh\n@@ -257,21 +257,33 @@ test_expect_success 'tree object with duplicate entries' '\n \ttest_i18ngrep \"error in tree .*contains duplicate file entries\" out\n '\n\n-test_expect_success 'tree object with dublicate names' '\n-\ttest_when_finished \"remove_object \\$blob\" &&\n-\ttest_when_finished \"remove_object \\$tree\" &&\n-\ttest_when_finished \"remove_object \\$badtree\" &&\n-\tblob=$(echo blob | git hash-object -w --stdin) &&\n-\tprintf \"100644 blob %s\\t%s\\n\" $blob x.2 >tree &&\n-\ttree=$(git mktree <tree) &&\n-\tprintf \"100644 blob %s\\t%s\\n\" $blob x.1 >badtree &&\n-\tprintf \"100644 blob %s\\t%s\\n\" $blob x >>badtree &&\n-\tprintf \"040000 tree %s\\t%s\\n\" $tree x >>badtree &&\n-\tbadtree=$(git mktree <badtree) &&\n-\ttest_must_fail git fsck 2>out &&\n-\ttest_i18ngrep \"$badtree\" out &&\n-\ttest_i18ngrep \"error in tree .*contains duplicate file entries\" out\n-'\n+check_duplicate_names () {\n+\texpect=$1 &&\n+\tshift &&\n+\tnames=$@ &&\n+\ttest_expect_$expect \"tree object with duplicate names: $names\" '\n+\t\ttest_when_finished \"remove_object \\$blob\" &&\n+\t\ttest_when_finished \"remove_object \\$tree\" &&\n+\t\ttest_when_finished \"remove_object \\$badtree\" &&\n+\t\tblob=$(echo blob | git hash-object -w --stdin) &&\n+\t\tprintf \"100644 blob %s\\t%s\\n\" $blob x.2 >tree &&\n+\t\ttree=$(git mktree <tree) &&\n+\t\tfor name in $names\n+\t\tdo\n+\t\t\tcase \"$name\" in\n+\t\t\t*/) printf \"040000 tree %s\\t%s\\n\" $tree \"${name%/}\" ;;\n+\t\t\t*)  printf \"100644 blob %s\\t%s\\n\" $blob \"$name\" ;;\n+\t\t\tesac\n+\t\tdone >badtree &&\n+\t\tbadtree=$(git mktree <badtree) &&\n+\t\ttest_must_fail git fsck 2>out &&\n+\t\ttest_i18ngrep \"$badtree\" out &&\n+\t\ttest_i18ngrep \"error in tree .*contains duplicate file entries\" out\n+\t'\n+}\n+\n+check_duplicate_names success x x.1 x/\n+check_duplicate_names success x x.1.2 x.1/ x/\n\n test_expect_success 'unparseable tree object' '\n \ttest_oid_cache <<-\\EOF &&\n--\n2.26.2\n"},{"id":"398338","messageId":"f5ec1823-438d-58ee-a63a-f3f0d0836e82@web.de","threadId":"53436","inReplyTo":"xmqq4ksmsaks.fsf@gitster.c.googlers.com","subject":"[PATCH 3/4] t1450: demonstrate undetected in-tree d/f conflict","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2020-05-21T09:52:43Z","receivedAt":"2020-05-21T09:52:50Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Signed-off-by: René Scharfe <l.s.r@web.de>\n---\n t/t1450-fsck.sh | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\nindex 9640ac8ff2..5780e10cbc 100755\n--- a/t/t1450-fsck.sh\n+++ b/t/t1450-fsck.sh\n@@ -284,6 +284,7 @@ check_duplicate_names () {\n\n check_duplicate_names success x x.1 x/\n check_duplicate_names success x x.1.2 x.1/ x/\n+check_duplicate_names failure x x.1 x.1.2 x/\n\n test_expect_success 'unparseable tree object' '\n \ttest_oid_cache <<-\\EOF &&\n--\n2.26.2\n"},{"id":"398339","messageId":"36ba44ed-d076-12fb-3d28-7b5989c736b7@web.de","threadId":"53436","inReplyTo":"xmqq4ksmsaks.fsf@gitster.c.googlers.com","subject":"[PATCH 4/4] fsck: detect more in-tree d/f conflicts","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2020-05-21T09:52:54Z","receivedAt":"2020-05-21T09:53:01Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"If the conflict candidate file name from the top of the stack is not a\nprefix of the current candiate directory then we can discard it as no\nmatching directory can come up later.  But we are not done checking the\ncandidate directory -- the stack might still hold a matching file name,\nso stay in the loop and check the next candidate file name.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n fsck.c          | 2 +-\n t/t1450-fsck.sh | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex b48426262c..f82e2fe9e3 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -620,7 +620,7 @@ static int verify_ordered(unsigned mode1, const char *name1,\n \t\t\tif (!f_name)\n \t\t\t\tbreak;\n \t\t\tif (!skip_prefix(name2, f_name, &p))\n-\t\t\t\tbreak;\n+\t\t\t\tcontinue;\n \t\t\tif (!*p)\n \t\t\t\treturn TREE_HAS_DUPS;\n \t\t\tif (is_less_than_slash(*p)) {\ndiff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\nindex 5780e10cbc..344a2aad82 100755\n--- a/t/t1450-fsck.sh\n+++ b/t/t1450-fsck.sh\n@@ -284,7 +284,7 @@ check_duplicate_names () {\n\n check_duplicate_names success x x.1 x/\n check_duplicate_names success x x.1.2 x.1/ x/\n-check_duplicate_names failure x x.1 x.1.2 x/\n+check_duplicate_names success x x.1 x.1.2 x/\n\n test_expect_success 'unparseable tree object' '\n \ttest_oid_cache <<-\\EOF &&\n--\n2.26.2\n"},{"id":"398340","messageId":"20200521101021.GA578798@generichostname","threadId":"53436","inReplyTo":"0e34ddca-1e29-17ed-278c-4f94ed6436b3@web.de","subject":"Re: [PATCH 1/4] fsck: fix a typo in a comment","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-05-21T10:10:21Z","receivedAt":"2020-05-21T10:10:26Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Hi René,\n\nOn Thu, May 21, 2020 at 11:52:04AM +0200, René Scharfe wrote:\n> Reported-by: Junio C Hamano <gitster@pobox.com>\n\nMissing sign-off? I'm not sure it matters since I don't think there's\nanything here copyrightable but just pointing it out.\n\n-Denton\n"},{"id":"398347","messageId":"20200521102053.GA578930@generichostname","threadId":"53436","inReplyTo":"106c58e1-9c74-46e3-c83a-88eee114d9d6@web.de","subject":"Re: [PATCH 2/4] t1450: increase test coverage of in-tree d/f detection","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-05-21T10:20:53Z","receivedAt":"2020-05-21T10:21:06Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Hi René,\n\nOn Thu, May 21, 2020 at 11:52:28AM +0200, René Scharfe wrote:\n> diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\n> index 91a6e34f38..9640ac8ff2 100755\n> --- a/t/t1450-fsck.sh\n> +++ b/t/t1450-fsck.sh\n> @@ -257,21 +257,33 @@ test_expect_success 'tree object with duplicate entries' '\n>  \ttest_i18ngrep \"error in tree .*contains duplicate file entries\" out\n>  '\n> \n> -test_expect_success 'tree object with dublicate names' '\n> -\ttest_when_finished \"remove_object \\$blob\" &&\n> -\ttest_when_finished \"remove_object \\$tree\" &&\n> -\ttest_when_finished \"remove_object \\$badtree\" &&\n> -\tblob=$(echo blob | git hash-object -w --stdin) &&\n> -\tprintf \"100644 blob %s\\t%s\\n\" $blob x.2 >tree &&\n> -\ttree=$(git mktree <tree) &&\n> -\tprintf \"100644 blob %s\\t%s\\n\" $blob x.1 >badtree &&\n> -\tprintf \"100644 blob %s\\t%s\\n\" $blob x >>badtree &&\n> -\tprintf \"040000 tree %s\\t%s\\n\" $tree x >>badtree &&\n> -\tbadtree=$(git mktree <badtree) &&\n> -\ttest_must_fail git fsck 2>out &&\n> -\ttest_i18ngrep \"$badtree\" out &&\n> -\ttest_i18ngrep \"error in tree .*contains duplicate file entries\" out\n> -'\n> +check_duplicate_names () {\n> +\texpect=$1 &&\n> +\tshift &&\n> +\tnames=$@ &&\n\nIt doesn't really make sense to use $@ here since we're not using the\nargument list behaviour of $@; we're just expanding it normally. I would\nreplace this with $* instead.\n\n> +\ttest_expect_$expect \"tree object with duplicate names: $names\" '\n> +\t\ttest_when_finished \"remove_object \\$blob\" &&\n> +\t\ttest_when_finished \"remove_object \\$tree\" &&\n> +\t\ttest_when_finished \"remove_object \\$badtree\" &&\n> +\t\tblob=$(echo blob | git hash-object -w --stdin) &&\n> +\t\tprintf \"100644 blob %s\\t%s\\n\" $blob x.2 >tree &&\n> +\t\ttree=$(git mktree <tree) &&\n> +\t\tfor name in $names\n> +\t\tdo\n> +\t\t\tcase \"$name\" in\n> +\t\t\t*/) printf \"040000 tree %s\\t%s\\n\" $tree \"${name%/}\" ;;\n> +\t\t\t*)  printf \"100644 blob %s\\t%s\\n\" $blob \"$name\" ;;\n> +\t\t\tesac\n> +\t\tdone >badtree &&\n> +\t\tbadtree=$(git mktree <badtree) &&\n> +\t\ttest_must_fail git fsck 2>out &&\n> +\t\ttest_i18ngrep \"$badtree\" out &&\n> +\t\ttest_i18ngrep \"error in tree .*contains duplicate file entries\" out\n> +\t'\n> +}\n> +\n> +check_duplicate_names success x x.1 x/\n> +check_duplicate_names success x x.1.2 x.1/ x/\n> \n>  test_expect_success 'unparseable tree object' '\n>  \ttest_oid_cache <<-\\EOF &&\n> --\n> 2.26.2\n"},{"id":"398350","messageId":"acd2c0f1-af5d-f234-ad71-b55c0c505e0e@web.de","threadId":"53436","inReplyTo":"0e34ddca-1e29-17ed-278c-4f94ed6436b3@web.de","subject":"Re: [PATCH 1/4] fsck: fix a typo in a comment","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2020-05-21T11:15:51Z","receivedAt":"2020-05-21T11:16:05Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 21.05.20 um 11:52 schrieb René Scharfe:\n> Reported-by: Junio C Hamano <gitster@pobox.com>\n\nForgot to add:\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n\nThanks for noticing, Denton!\n\n> ---\n>  fsck.c | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/fsck.c b/fsck.c\n> index 8bb3ecf282..b48426262c 100644\n> --- a/fsck.c\n> +++ b/fsck.c\n> @@ -598,7 +598,7 @@ static int verify_ordered(unsigned mode1, const char *name1,\n>\n>  \t/*\n>  \t * There can be non-consecutive duplicates due to the implicitly\n> -\t * add slash, e.g.:\n> +\t * added slash, e.g.:\n>  \t *\n>  \t *   foo\n>  \t *   foo.bar\n> --\n> 2.26.2\n>\n"},{"id":"398358","messageId":"1f982a11-358c-195d-21f4-0299f5b60ff2@web.de","threadId":"53436","inReplyTo":"20200521102053.GA578930@generichostname","subject":"Re: [PATCH 2/4] t1450: increase test coverage of in-tree d/f detection","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2020-05-21T13:31:26Z","receivedAt":"2020-05-21T13:31:43Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 21.05.20 um 12:20 schrieb Denton Liu:\n> Hi René,\n>\n> On Thu, May 21, 2020 at 11:52:28AM +0200, René Scharfe wrote:\n>> diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\n>> index 91a6e34f38..9640ac8ff2 100755\n>> --- a/t/t1450-fsck.sh\n>> +++ b/t/t1450-fsck.sh\n>> @@ -257,21 +257,33 @@ test_expect_success 'tree object with duplicate entries' '\n>>  \ttest_i18ngrep \"error in tree .*contains duplicate file entries\" out\n>>  '\n>>\n>> -test_expect_success 'tree object with dublicate names' '\n>> -\ttest_when_finished \"remove_object \\$blob\" &&\n>> -\ttest_when_finished \"remove_object \\$tree\" &&\n>> -\ttest_when_finished \"remove_object \\$badtree\" &&\n>> -\tblob=$(echo blob | git hash-object -w --stdin) &&\n>> -\tprintf \"100644 blob %s\\t%s\\n\" $blob x.2 >tree &&\n>> -\ttree=$(git mktree <tree) &&\n>> -\tprintf \"100644 blob %s\\t%s\\n\" $blob x.1 >badtree &&\n>> -\tprintf \"100644 blob %s\\t%s\\n\" $blob x >>badtree &&\n>> -\tprintf \"040000 tree %s\\t%s\\n\" $tree x >>badtree &&\n>> -\tbadtree=$(git mktree <badtree) &&\n>> -\ttest_must_fail git fsck 2>out &&\n>> -\ttest_i18ngrep \"$badtree\" out &&\n>> -\ttest_i18ngrep \"error in tree .*contains duplicate file entries\" out\n>> -'\n>> +check_duplicate_names () {\n>> +\texpect=$1 &&\n>> +\tshift &&\n>> +\tnames=$@ &&\n>\n> It doesn't really make sense to use $@ here since we're not using the\n> argument list behaviour of $@; we're just expanding it normally. I would\n> replace this with $* instead.\n\nThe assignment to $names flattens the list, so $@ and $* behave the same\nhere.  I didn't think much about it, but it would be nice to support names\nthat contain spaces, which we could do by writing the arguments to a file.\nAnd if we go that route then support for names with newlines would be nice\nas well.  I'm not sure it's worth it, but a patch for that is below.\n\nAt least I'd like to keep the $@ as kind of a reminder that we want to\npass on arguments (full names), not words.\n\n>\n>> +\ttest_expect_$expect \"tree object with duplicate names: $names\" '\n>> +\t\ttest_when_finished \"remove_object \\$blob\" &&\n>> +\t\ttest_when_finished \"remove_object \\$tree\" &&\n>> +\t\ttest_when_finished \"remove_object \\$badtree\" &&\n>> +\t\tblob=$(echo blob | git hash-object -w --stdin) &&\n>> +\t\tprintf \"100644 blob %s\\t%s\\n\" $blob x.2 >tree &&\n>> +\t\ttree=$(git mktree <tree) &&\n>> +\t\tfor name in $names\n>> +\t\tdo\n>> +\t\t\tcase \"$name\" in\n>> +\t\t\t*/) printf \"040000 tree %s\\t%s\\n\" $tree \"${name%/}\" ;;\n>> +\t\t\t*)  printf \"100644 blob %s\\t%s\\n\" $blob \"$name\" ;;\n>> +\t\t\tesac\n>> +\t\tdone >badtree &&\n>> +\t\tbadtree=$(git mktree <badtree) &&\n>> +\t\ttest_must_fail git fsck 2>out &&\n>> +\t\ttest_i18ngrep \"$badtree\" out &&\n>> +\t\ttest_i18ngrep \"error in tree .*contains duplicate file entries\" out\n>> +\t'\n>> +}\n>> +\n>> +check_duplicate_names success x x.1 x/\n>> +check_duplicate_names success x x.1.2 x.1/ x/\n>>\n>>  test_expect_success 'unparseable tree object' '\n>>  \ttest_oid_cache <<-\\EOF &&\n>> --\n>> 2.26.2\n\n-- >8 --\nSubject: [PATCH 5/4] t1450: support names with spaces in check_duplicate_names()\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n t/t1450-fsck.sh | 22 +++++++++++++---------\n 1 file changed, 13 insertions(+), 9 deletions(-)\n\ndiff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\nindex 344a2aad82..b1766c8e11 100755\n--- a/t/t1450-fsck.sh\n+++ b/t/t1450-fsck.sh\n@@ -257,10 +257,20 @@ test_expect_success 'tree object with duplicate entries' '\n \ttest_i18ngrep \"error in tree .*contains duplicate file entries\" out\n '\n\n+names_to_tree () {\n+\tawk -v blob=\"$1\" -v tree=\"$2\" -v RS='\\0' -v FS='/' '\n+\t\tNF == 1 {printf \"100644 blob %s\\t%s%c\", blob, $1, 0}\n+\t\tNF == 2 {printf \"040000 tree %s\\t%s%c\", tree, $1, 0}\n+\t'\n+}\n+\n check_duplicate_names () {\n \texpect=$1 &&\n \tshift &&\n-\tnames=$@ &&\n+\tfor name in \"$@\"\n+\tdo\n+\t\tprintf \"%s\\0\" \"$name\"\n+\tdone >names &&\n \ttest_expect_$expect \"tree object with duplicate names: $names\" '\n \t\ttest_when_finished \"remove_object \\$blob\" &&\n \t\ttest_when_finished \"remove_object \\$tree\" &&\n@@ -268,14 +278,8 @@ check_duplicate_names () {\n \t\tblob=$(echo blob | git hash-object -w --stdin) &&\n \t\tprintf \"100644 blob %s\\t%s\\n\" $blob x.2 >tree &&\n \t\ttree=$(git mktree <tree) &&\n-\t\tfor name in $names\n-\t\tdo\n-\t\t\tcase \"$name\" in\n-\t\t\t*/) printf \"040000 tree %s\\t%s\\n\" $tree \"${name%/}\" ;;\n-\t\t\t*)  printf \"100644 blob %s\\t%s\\n\" $blob \"$name\" ;;\n-\t\t\tesac\n-\t\tdone >badtree &&\n-\t\tbadtree=$(git mktree <badtree) &&\n+\t\tnames_to_tree $blob $tree <names >badtree &&\n+\t\tbadtree=$(git mktree -z <badtree) &&\n \t\ttest_must_fail git fsck 2>out &&\n \t\ttest_i18ngrep \"$badtree\" out &&\n \t\ttest_i18ngrep \"error in tree .*contains duplicate file entries\" out\n--\n2.26.2\n"},{"id":"398373","messageId":"xmqqwo55rwuq.fsf@gitster.c.googlers.com","threadId":"53436","inReplyTo":"1f982a11-358c-195d-21f4-0299f5b60ff2@web.de","subject":"Re: [PATCH 2/4] t1450: increase test coverage of in-tree d/f detection","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-21T18:01:17Z","receivedAt":"2020-05-21T18:01:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n>>> +check_duplicate_names () {\n>>> +\texpect=$1 &&\n>>> +\tshift &&\n>>> +\tnames=$@ &&\n>>\n>> It doesn't really make sense to use $@ here since we're not using the\n>> argument list behaviour of $@; we're just expanding it normally. I would\n>> replace this with $* instead.\n>\n> The assignment to $names flattens the list, so $@ and $* behave the same\n> here.\n> ...\n> At least I'd like to keep the $@ as kind of a reminder that we want to\n> pass on arguments (full names), not words.\n\nI personally prefer to use \"$*\" when we are not invoking the \"list\"\nmagic of \"$@\" and switch it to \"$@\" when it starts to matter, but I\ncan also understand your \"reminder value\" reasoning, so I am on the\nfence.\n"}]}