Re: [PATCH v5 5/6] grep: enable recurse-submodules to work on <tree> objects

[Brandon Williams <bmwill@google.com>]

On 11/22, Brandon Williams wrote:
> On 11/22, Junio C Hamano wrote:
> > Brandon Williams <bmwill@google.com> writes:
> > 
> > > diff --git a/tree-walk.c b/tree-walk.c
> > > index 828f435..ff77605 100644
> > > --- a/tree-walk.c
> > > +++ b/tree-walk.c
> > > @@ -1004,6 +1004,19 @@ static enum interesting do_match(const struct name_entry *entry,
> > >  				 */
> > >  				if (ps->recursive && S_ISDIR(entry->mode))
> > >  					return entry_interesting;
> > > +
> > > +				/*
> > > +				 * When matching against submodules with
> > > +				 * wildcard characters, ensure that the entry
> > > +				 * at least matches up to the first wild
> > > +				 * character.  More accurate matching can then
> > > +				 * be performed in the submodule itself.
> > > +				 */
> > > +				if (ps->recursive && S_ISGITLINK(entry->mode) &&
> > > +				    !ps_strncmp(item, match + baselen,
> > > +						entry->path,
> > > +						item->nowildcard_len - baselen))
> > > +					return entry_interesting;
> > >  			}
> > 
> > This one (and the other hunk) feels more correct than the previous
> > round.  One thing to keep in mind however is that ps->recursive is
> > about "do we show a tree as a tree aka 040000, or do we descend into
> > it to show its contents?", not about "do we recurse into submodules?",
> > AFAICT.
> > 
> > So this change may have an impact on "git ls-tree -r" with pathspec;
> > I offhand do not know if that impact is undesirable or not.  A test
> > or two may be in order to illustrate what happens?  With a submodule
> > at "sub/module", running "git ls-tree -r HEAD -- sub/module/*" or
> > something like that, perhaps?
> 
> Maybe unrelated, but it looks like wildcard characters are overridden in
> ls-tree.c per '170260ae'.  As such wildmatching just doesn't work with
> ls-tree.  so `git ls-tree -r HEAD -- "*"` results in no hits.

Wrong commit.  Its this one (f0096c06bcdeb7aa6ae8a749ddc9d6d4a2c381d1)
that disabled wildmatching since it is 'plumbing'

-- 
Brandon Williams