Re: [PATCH] clean: demonstrate a bug with pathspecs

From: Derrick Stolee <stolee@gmail.com>
Date: Jan 16, 2020, 01:43 UTC
Message-ID: <354fa43b-0e62-1ee5-a63f-59d9b2da7d3f@gmail.com>
In-Reply-To: <20200116000312.GD146834@google.com>

On 1/15/2020 7:03 PM, Jonathan Nieder wrote:

Show 16 quoted lines

> Hi,
> 
> Derrick Stolee wrote:
> 
>> b9660c1 (dir: fix checks on common prefix directory, 2019-12-19)
>> modified the way pathspecs are handled when handling a directory
>> during "git clean -f <path>". While this improved the behavior
>> for known test breakages, it also regressed in how the clean
>> command handles cleaning a specified file.
>>
>> Add a test case that demonstrates this behavior. This test passes
>> before b9660c1 then fails after.
> 
> Can this commit message say a little more about the nature of the
> bug?  For example, what kind of workflow does this come up in for
> end users?

I honestly don't know why anyone would call `git clean -f <path>` on a file instead of using `rm <path>`. But, the behavior _did_ change, which is why I'm bringing it up.

If the community instead said "this is not important functionality. We should just expect the given pathspec to only match directories" then I would accept that and just delete the file in another way. That seems unlikely.

Show 7 quoted lines

> [...]
>>     While integrating v2.25.0 into the microsoft/git fork, one of our VFS
>>     for Git functional tests started failing.
> 
> This is also useful information to put in the commit message: e.g.
> "Noticed via VFS for Git's functional test <test name>".  It provides
> useful context when looking at such a patch later.

I'm not sure the test [1] will shed much light on the issue. It sort of accidentally reveals this bug because it happens to use "git clean -f <path>".

The test itself is holding a handle on <path> on a commit where <path> is untracked, then tries to checkout a commit where <path> is tracked. On Windows, this should fail. With the virtualization layer in VFS for Git, Git doesn't actually try to write to <path> but instead VFS for Git tries to update the virtualization at <path>, colliding with what Git is trying to do. Hence, we need to make sure the Git command actually fails in this attempt.

Perhaps that context isn't actually helpful. And you could understand why I stared at this test for a long while before realizing that it was actually a failure in "git clean -f" and then Kevin did the real work to find that VFS for Git wasn't causing the issue.

-Stolee

[1] https://github.com/microsoft/VFSForGit/blob/1aec263033cc3c05d0389e1792b7958d9a2e70c6/GVFS/GVFS.FunctionalTests.Windows/Windows/Tests/WindowsUpdatePlaceholderTests.cs#L38-L72