Re: Possible bug with git restore
- From
René Scharfe <l.s.r@web.de>
- Date
- Aug 22, 2020, 10:29 UTC
- Message-ID
- <73bddcfc-f8c9-6ec1-328d-a2f427c4aef2@web.de>
- In-Reply-To
- <20200820182720.GA2537643@coredump.intra.peff.net>
Am 20.08.20 um 20:27 schrieb Jeff King:
Show 15 quoted lines
> On Thu, Aug 20, 2020 at 07:48:48PM +0200, René Scharfe wrote:
>
>>> - shouldn't that wildcard pathspec match those files? I've confirmed
>>> that the glob characters make it into Git's pathspec machinery, and
>>> since it doesn't have slashes, I think we'd match a basename (and
>>> certainly "git ls-files *test_file.*" does what I expect).
>>
>> No, because restore doesn't interpret pathspecs recursively. I don't
>> know why that causes files to disappear, though. But here's a fix.
>
> I think it's because of this comment from bc96cc87dbb:
>
> When pathspec.recursive == 0, the behavior depends on match functions:
> non-recursive for tree_entry_interesting() and recursive for
> match_pathspec{,_depth}Show 5 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.
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.
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.
René
--- dir.c | 4 ++++ pathspec.h | 2 ++ tree-walk.c | 5 +++++ 3 files changed, 11 insertions(+)
diff --git a/dir.c b/dir.c index fe64be30ed6..87d5ffa62d0 100644 --- a/dir.c +++ b/dir.c @@ -432,6 +432,10 @@ static int do_match_pathspec(const struct index_state *istate, { int i, retval = 0, exclude = flags & DO_MATCH_EXCLUDE; + ((struct pathspec *)ps)->match_pathspec = 1; + if (ps->tree_entry_interesting && !ps->recursive) + BUG("match_pathspec tree_entry_interesting !recursive"); + GUARD_PATHSPEC(ps, PATHSPEC_FROMTOP | PATHSPEC_MAXDEPTH | diff --git a/pathspec.h b/pathspec.h index 454ce364fac..bbae0abb249 100644 --- a/pathspec.h +++ b/pathspec.h @@ -32,6 +32,8 @@ struct pathspec { unsigned int has_wildcard:1; unsigned int recursive:1; unsigned int recurse_submodules:1; + unsigned int match_pathspec:1; + unsigned int tree_entry_interesting:1; unsigned magic; int max_depth; struct pathspec_item { diff --git a/tree-walk.c b/tree-walk.c index 0160294712b..f6465cd9cf4 100644 --- a/tree-walk.c +++ b/tree-walk.c @@ -1185,6 +1185,11 @@ enum interesting tree_entry_interesting(struct index_state *istate, const struct pathspec *ps) { enum interesting positive, negative; + + ((struct pathspec *)ps)->tree_entry_interesting = 1; + if (ps->match_pathspec && !ps->recursive) + BUG("match_pathspec tree_entry_interesting !recursive"); + positive = do_match(istate, entry, base, base_offset, ps, 0); /* -- 2.28.0