{"thread":{"id":"52643","subject":"[PATCH] clean: demonstrate a bug with pathspecs","startedAt":"2020-01-15T20:25:50Z","lastAt":"2020-01-16T20:20:11Z","messageCount":9,"participants":["Derrick Stolee via GitGitGadget","Kyle Meyer","Jonathan Nieder","Elijah Newren","Derrick Stolee","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"389820","messageId":"pull.526.git.1579119946211.gitgitgadget@gmail.com","threadId":"52643","inReplyTo":null,"subject":"[PATCH] clean: demonstrate a bug with pathspecs","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-01-15T20:25:45Z","receivedAt":"2020-01-15T20:25:50Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nb9660c1 (dir: fix checks on common prefix directory, 2019-12-19)\nmodified the way pathspecs are handled when handling a directory\nduring \"git clean -f \". While this improved the behavior\nfor known test breakages, it also regressed in how the clean\ncommand handles cleaning a specified file.\n\nAdd a test case that demonstrates this behavior. This test passes\nbefore b9660c1 then fails after.\n\nHelped-by: Kevin Willford \nSigned-off-by: Derrick Stolee \n---\n clean: demonstrate a bug with pathspecs\n \n While integrating v2.25.0 into the microsoft/git fork, one of our VFS\n for Git functional tests started failing. Looking into it, the only\n possible place could have been where one of our integration points with\n the virtualfilesystem hook was moved by c5c4edd (dir: break part of\n read_directory_recursive() out for reuse, 2019-12-10) and then used in\n the following two commits.\n \n By reverting these two commits, we stopped the failure, but it took a\n while before figuring out that it was a regression in Git and not a\n failure in our integration to the new logic. Thanks to Kevin Willford\n for producing a test case.\n \n b9660c1 (dir: fix checks on common prefix directory, 2019-12-19) is the\n culprit, so this patch is based on that. If rebased to c5c4edd, then the\n test passes.\n \n As for actually fixing this regression, I don't know how. This code is\n pretty dense and I don't have a firm grasp of what is happening in both\n b9660c1 and the following 777b420 (dir: synchronize tread_leading_path()\n and read_directory_recursive()). Elijah is CC'd in case he still has\n context on this area.\n \n Thanks, -Stolee\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-526%2Fderrickstolee%2Fclean-bug-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-526/derrickstolee/clean-bug-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/526\n\n t/t7300-clean.sh | 9 +++++++++\n 1 file changed, 9 insertions(+)\n\ndiff --git a/t/t7300-clean.sh b/t/t7300-clean.sh\nindex 6e6d24c1c3..782e125c89 100755\n--- a/t/t7300-clean.sh\n+++ b/t/t7300-clean.sh\n@@ -737,4 +737,13 @@ test_expect_success MINGW 'handle clean & core.longpaths = false nicely' '\n \ttest_i18ngrep \"too long\" .git/err\n '\n \n+test_expect_failure 'clean untracked paths by pathspec' '\n+\tgit init untracked &&\n+\tmkdir untracked/dir &&\n+\techo >untracked/dir/file.txt &&\n+\tgit -C untracked clean -f dir/file.txt &&\n+\tls untracked/dir >actual &&\n+\ttest_must_be_empty actual\n+'\n+\n test_done\n\nbase-commit: b9670c1f5e6b98837c489a03ac0d343d30e08505\n-- \ngitgitgadget\n"},{"id":"389832","messageId":"87zheo2t7b.fsf@kyleam.com","threadId":"52643","inReplyTo":"pull.526.git.1579119946211.gitgitgadget@gmail.com","subject":"Re: [PATCH] clean: demonstrate a bug with pathspecs","fromName":"Kyle Meyer","fromEmail":"kyle@kyleam.com","sentAt":"2020-01-15T23:30:48Z","receivedAt":"2020-01-15T23:31:02Z","isPatch":true,"sender":{"key":"kyle@kyleam.com","avatar":"https://avatars.githubusercontent.com/u/1297788?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" writes:\n\n> From: Derrick Stolee \n>\n> b9660c1 (dir: fix checks on common prefix directory, 2019-12-19)\n> modified the way pathspecs are handled when handling a directory\n> during \"git clean -f \".\n\nI can't find b9660c1. I think this and other references below should\npoint to b9670c1f5e (dir: fix checks on common prefix directory,\n2019-12-19), which matches the base-commit value for this patch.\n"},{"id":"389834","messageId":"20200116000312.GD146834@google.com","threadId":"52643","inReplyTo":"pull.526.git.1579119946211.gitgitgadget@gmail.com","subject":"Re: [PATCH] clean: demonstrate a bug with pathspecs","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-01-16T00:03:12Z","receivedAt":"2020-01-16T00:03:16Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nDerrick Stolee wrote:\n\n> b9660c1 (dir: fix checks on common prefix directory, 2019-12-19)\n> modified the way pathspecs are handled when handling a directory\n> during \"git clean -f \". While this improved the behavior\n> for known test breakages, it also regressed in how the clean\n> command handles cleaning a specified file.\n>\n> Add a test case that demonstrates this behavior. This test passes\n> before b9660c1 then fails after.\n\nCan this commit message say a little more about the nature of the\nbug? For example, what kind of workflow does this come up in for\nend users?\n\n[...]\n> While integrating v2.25.0 into the microsoft/git fork, one of our VFS\n> for Git functional tests started failing.\n\nThis is also useful information to put in the commit message: e.g.\n\"Noticed via VFS for Git's functional test \". It provides\nuseful context when looking at such a patch later.\n\n[...]\n> Elijah is CC'd in case he still has\n> context on this area.\n\nThanks and hope that helps,\nJonathan\n"},{"id":"389837","messageId":"CABPp-BHywo5Js0YGwDykV8G+=Y6-M_Wh3sE5BvC-7zArJd1rLw@mail.gmail.com","threadId":"52643","inReplyTo":"pull.526.git.1579119946211.gitgitgadget@gmail.com","subject":"Re: [PATCH] clean: demonstrate a bug with pathspecs","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2020-01-16T00:38:16Z","receivedAt":"2020-01-16T00:38:30Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Jan 15, 2020 at 12:25 PM Derrick Stolee via GitGitGadget\n wrote:\n>\n> From: Derrick Stolee \n>\n> b9660c1 (dir: fix checks on common prefix directory, 2019-12-19)\n> modified the way pathspecs are handled when handling a directory\n> during \"git clean -f \". While this improved the behavior\n> for known test breakages, it also regressed in how the clean\n> command handles cleaning a specified file.\n>\n> Add a test case that demonstrates this behavior. This test passes\n> before b9660c1 then fails after.\n>\n> Helped-by: Kevin Willford \n> Signed-off-by: Derrick Stolee \n> ---\n> clean: demonstrate a bug with pathspecs\n>\n> While integrating v2.25.0 into the microsoft/git fork, one of our VFS\n> for Git functional tests started failing. Looking into it, the only\n> possible place could have been where one of our integration points with\n> the virtualfilesystem hook was moved by c5c4edd (dir: break part of\n> read_directory_recursive() out for reuse, 2019-12-10) and then used in\n> the following two commits.\n>\n> By reverting these two commits, we stopped the failure, but it took a\n> while before figuring out that it was a regression in Git and not a\n> failure in our integration to the new logic. Thanks to Kevin Willford\n> for producing a test case.\n>\n> b9660c1 (dir: fix checks on common prefix directory, 2019-12-19) is the\n> culprit, so this patch is based on that. If rebased to c5c4edd, then the\n> test passes.\n>\n> As for actually fixing this regression, I don't know how. This code is\n> pretty dense and I don't have a firm grasp of what is happening in both\n> b9660c1 and the following 777b420 (dir: synchronize tread_leading_path()\n> and read_directory_recursive()). Elijah is CC'd in case he still has\n> context on this area.\n>\n> Thanks, -Stolee\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-526%2Fderrickstolee%2Fclean-bug-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-526/derrickstolee/clean-bug-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/526\n>\n> t/t7300-clean.sh | 9 +++++++++\n> 1 file changed, 9 insertions(+)\n>\n> diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh\n> index 6e6d24c1c3..782e125c89 100755\n> --- a/t/t7300-clean.sh\n> +++ b/t/t7300-clean.sh\n> @@ -737,4 +737,13 @@ test_expect_success MINGW 'handle clean & core.longpaths = false nicely' '\n> test_i18ngrep \"too long\" .git/err\n> '\n>\n> +test_expect_failure 'clean untracked paths by pathspec' '\n> + git init untracked &&\n> + mkdir untracked/dir &&\n> + echo >untracked/dir/file.txt &&\n> + git -C untracked clean -f dir/file.txt &&\n> + ls untracked/dir >actual &&\n> + test_must_be_empty actual\n> +'\n> +\n> test_done\n>\n> base-commit: b9670c1f5e6b98837c489a03ac0d343d30e08505\n> --\n> gitgitgadget\n\nIs there an inverted phrase corresponding to \"the gift that keeps on\ngiving\", something like \"the punishment that keeps on punishing\"? If\nso, it would be a very appropriate description of dir.c.\n\nYeah, I still have context. I even think I've got an idea about what\nthe fix might be, though with dir.c my ideas about fixes usually just\nserve as starting points for debugging before I find the real fix.\nI'll try to dig in.\n"},{"id":"389839","messageId":"e008da66-defe-d2b0-410b-64b7754b9c6e@gmail.com","threadId":"52643","inReplyTo":"CABPp-BHywo5Js0YGwDykV8G+=Y6-M_Wh3sE5BvC-7zArJd1rLw@mail.gmail.com","subject":"Re: [PATCH] clean: demonstrate a bug with pathspecs","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-01-16T01:23:35Z","receivedAt":"2020-01-16T01:23:40Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 1/15/2020 7:38 PM, Elijah Newren wrote:\n> Is there an inverted phrase corresponding to \"the gift that keeps on\n> giving\", something like \"the punishment that keeps on punishing\"? If\n> so, it would be a very appropriate description of dir.c.\n\nAt least we will continue adding tests until we converge towards\ncorrectness, and the behavior issues are even more contrived and\nspecial case (like this one).\n\n> Yeah, I still have context. I even think I've got an idea about what\n> the fix might be, though with dir.c my ideas about fixes usually just\n> serve as starting points for debugging before I find the real fix.\n> I'll try to dig in.\n\nThanks! I'll try to review it carefully when it arrives. Good luck.\n\n-Stolee\n\n"},{"id":"389840","messageId":"6ae84c45-d207-74a7-fbef-ddf78d30d3a1@gmail.com","threadId":"52643","inReplyTo":"87zheo2t7b.fsf@kyleam.com","subject":"Re: [PATCH] clean: demonstrate a bug with pathspecs","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-01-16T01:33:24Z","receivedAt":"2020-01-16T01:33:29Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 1/15/2020 6:30 PM, Kyle Meyer wrote:\n> \"Derrick Stolee via GitGitGadget\" writes:\n> \n>> From: Derrick Stolee \n>>\n>> b9660c1 (dir: fix checks on common prefix directory, 2019-12-19)\n>> modified the way pathspecs are handled when handling a directory\n>> during \"git clean -f \".\n> \n> I can't find b9660c1. I think this and other references below should\n> point to b9670c1f5e (dir: fix checks on common prefix directory,\n> 2019-12-19), which matches the base-commit value for this patch.\n\nSorry for the digit swap. Thanks for pointing that out!\n"},{"id":"389841","messageId":"354fa43b-0e62-1ee5-a63f-59d9b2da7d3f@gmail.com","threadId":"52643","inReplyTo":"20200116000312.GD146834@google.com","subject":"Re: [PATCH] clean: demonstrate a bug with pathspecs","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-01-16T01:43:29Z","receivedAt":"2020-01-16T01:43:34Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 1/15/2020 7:03 PM, Jonathan Nieder wrote:\n> Hi,\n> \n> Derrick Stolee wrote:\n> \n>> b9660c1 (dir: fix checks on common prefix directory, 2019-12-19)\n>> modified the way pathspecs are handled when handling a directory\n>> during \"git clean -f \". While this improved the behavior\n>> for known test breakages, it also regressed in how the clean\n>> command handles cleaning a specified file.\n>>\n>> Add a test case that demonstrates this behavior. This test passes\n>> before b9660c1 then fails after.\n> \n> Can this commit message say a little more about the nature of the\n> bug? For example, what kind of workflow does this come up in for\n> end users?\n\nI honestly don't know why anyone would call `git clean -f ` on a\nfile instead of using `rm `. But, the behavior _did_ change, which\nis why I'm bringing it up.\n\nIf the community instead said \"this is not important functionality. We\nshould just expect the given pathspec to only match directories\" then I\nwould accept that and just delete the file in another way. That seems\nunlikely.\n\n> [...]\n>> While integrating v2.25.0 into the microsoft/git fork, one of our VFS\n>> for Git functional tests started failing.\n> \n> This is also useful information to put in the commit message: e.g.\n> \"Noticed via VFS for Git's functional test \". It provides\n> useful context when looking at such a patch later.\n\nI'm not sure the test [1] will shed much light on the issue. It sort of\naccidentally reveals this bug because it happens to use \"git clean -f \".\n\nThe test itself is holding a handle on on a commit where \nis untracked, then tries to checkout a commit where is tracked. On\nWindows, this should fail. With the virtualization layer in VFS for Git,\nGit doesn't actually try to write to but instead VFS for Git tries\nto update the virtualization at , colliding with what Git is trying\nto do. Hence, we need to make sure the Git command actually fails in this\nattempt.\n\nPerhaps that context isn't actually helpful. And you could understand why\nI stared at this test for a long while before realizing that it was actually\na failure in \"git clean -f\" and then Kevin did the real work to find that\nVFS for Git wasn't causing the issue.\n\n-Stolee\n\n[1] https://github.com/microsoft/VFSForGit/blob/1aec263033cc3c05d0389e1792b7958d9a2e70c6/GVFS/GVFS.FunctionalTests.Windows/Windows/Tests/WindowsUpdatePlaceholderTests.cs#L38-L72\n\n"},{"id":"389903","messageId":"CABPp-BFX2ER9aqaHi=sbaSppGobCOisR2a8z1mTGqbQ8xS_WCA@mail.gmail.com","threadId":"52643","inReplyTo":"e008da66-defe-d2b0-410b-64b7754b9c6e@gmail.com","subject":"Re: [PATCH] clean: demonstrate a bug with pathspecs","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2020-01-16T18:01:23Z","receivedAt":"2020-01-16T18:01:42Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Jan 15, 2020 at 5:23 PM Derrick Stolee wrote:\n>\n> On 1/15/2020 7:38 PM, Elijah Newren wrote:\n> > Is there an inverted phrase corresponding to \"the gift that keeps on\n> > giving\", something like \"the punishment that keeps on punishing\"? If\n> > so, it would be a very appropriate description of dir.c.\n>\n> At least we will continue adding tests until we converge towards\n> correctness, and the behavior issues are even more contrived and\n> special case (like this one).\n\nThis doesn't seem any more contrived or special case than most my\nprevious fixes for dir.c...\n\n> > Yeah, I still have context. I even think I've got an idea about what\n> > the fix might be, though with dir.c my ideas about fixes usually just\n> > serve as starting points for debugging before I find the real fix.\n> > I'll try to dig in.\n>\n> Thanks! I'll try to review it carefully when it arrives. Good luck.\n\nMan, I'm such a bozo. It turns out, for once, that my idea for the\nfix was correct but after digging a bit I realized that it was\nessentially a bug I fixed not that long ago once already -- and that I\nmyself re-introduced it (for a slightly different case) in some\ncommits where I used some strongly worded disgust that \"this bad code\nstructure is going to cause someone to mess up in \" and then\nI made that exact kind of mistake I was complaining about in the\ncommit message...as part of that EXACT commit, to boot.\n\nAt least it'll make for a fun new commit message explaining it all...\n\n\nAnyway, I'm going to pull your commit into my series so I can put my\nfix on top, and lump it in with Peff's two patches over at\nhttps://lore.kernel.org/git/20200115202146.GA4091171@coredump.intra.peff.net/\nsince all these patches are basically \"more fill_directory() fixes\".\nLet me know if you have any concerns with that.\n\nElijah\n"},{"id":"389917","messageId":"xmqqk15rgnm5.fsf@gitster-ct.c.googlers.com","threadId":"52643","inReplyTo":"CABPp-BFX2ER9aqaHi=sbaSppGobCOisR2a8z1mTGqbQ8xS_WCA@mail.gmail.com","subject":"Re: [PATCH] clean: demonstrate a bug with pathspecs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-01-16T20:20:02Z","receivedAt":"2020-01-16T20:20:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n> Anyway, I'm going to pull your commit into my series so I can put my\n> fix on top, and lump it in with Peff's two patches over at\n> https://lore.kernel.org/git/20200115202146.GA4091171@coredump.intra.peff.net/\n> since all these patches are basically \"more fill_directory() fixes\".\n\nThanks. Then I'll refrain from applying those two patches we saw\nearlier (including the one you have the URL in your message).\n\n"}]}