Re: Possible bug with git restore

["René Scharfe" <l.s.r@web.de>]

Am 20.08.20 um 20:27 schrieb Jeff King:
> 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}

> 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