Re: Possible bug with git restore

From: Jeff King <peff@peff.net>
Date: Aug 24, 2020, 20:25 UTC
Message-ID: <20200824202556.GB709709@coredump.intra.peff.net>
In-Reply-To: <73bddcfc-f8c9-6ec1-328d-a2f427c4aef2@web.de>

On Sat, Aug 22, 2020 at 12:29:23PM +0200, René Scharfe wrote:

Show 9 quoted lines

> > So the fundamental issue is treating the pathspec in two different ways,
> > and then correlating the results. We need to either do a recursive match
> > for the tree match (as your patch does), or do non-recursive for this
> > index match (which I don't think is trivial, because of the way the
> > recursive flag works).
> 
> If using the same pathspec with both tree_entry_interesting and
> match_pathspec gives inconsistent results and can even lead to data loss
> as we've seen here, then we better prevent it.

Yeah, it would be nice to do away with this inconsistency entirely. I don't know if that even causes user-visible behavior changes at this point, though.

Show 5 quoted lines

> The easiest way to do that would be to BUG out in match_pathspec if
> recursive is unset, to indicate that it doesn't support non-recursive
> matching.  Finding all the places that didn't bothered to set this flag
> since it doesn't affect match_pathspec anyway would be quite tedious,
> though.

Yeah. I think it's easier to approach it from the tree-entry side, and say: which spots are not recursive, and could/should they be. Which I think is where your patch below is going.

> At least the test suite still completes with the following evil patch
> and the fix I sent earlier (evil because it ignores const), so we
> currently don't have any other mismatches in covered code.

That's kind-of good news, in that it lends support to the notion that basically everything wants to be recursive. But I'd be worried there's a lingering code path that is not covered in the test suite. Certainly shipping your BUG() patch would flush it out, but it's not very friendly to users who do run into it.

I wonder if we can drop it to a warning() and see if anybody reports it.

Or the more-work but more-responsible version: manually trace all of the pathspec calls to see which code paths might rely on leaving "recursive" unset.

-Peff