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

[Brandon Williams <bmwill@google.com>]

On 11/18, Junio C Hamano wrote:
> Brandon Williams <bmwill@google.com> writes:
> 
> > @@ -671,12 +707,29 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,
> >  	enum interesting match = entry_not_interesting;
> >  	struct name_entry entry;
> >  	int old_baselen = base->len;
> > +	struct strbuf name = STRBUF_INIT;
> > +	int name_base_len = 0;
> > +	if (super_prefix) {
> > +		strbuf_addstr(&name, super_prefix);
> > +		name_base_len = name.len;
> > +	}
> >  
> >  	while (tree_entry(tree, &entry)) {
> >  		int te_len = tree_entry_len(&entry);
> >  
> >  		if (match != all_entries_interesting) {
> > -			match = tree_entry_interesting(&entry, base, tn_len, pathspec);
> > +			strbuf_setlen(&name, name_base_len);
> > +			strbuf_addstr(&name, base->buf + tn_len);
> > +
> > +			if (recurse_submodules && S_ISGITLINK(entry.mode)) {
> > +				strbuf_addstr(&name, entry.path);
> > +				match = submodule_path_match(pathspec, name.buf,
> > +							     NULL);
> 
> The vocabulary from submodule_path_match() returns is the same as
> that of do_match_pathspec() and match_pathspec_item() which is
> MATCHED_{EXACTLY,FNMATCH,RECURSIVELY}, which is different from the
> vocabulary of the variable "match" which is "enum interesting" that
> is used by the tree-walk infrastructure.
> 
> I doubt they are compatible to be usable like this.  Am I missing
> something?

I think i initially must have thought it would work out, but looking
back at this I can clearly see that they aren't 100% compatible...

It slightly feels odd to me that we have so many different means for
checking pathspecs, all of which pretty much duplicate some of the
functionality of the other.  Is there any reason there are these two
different code paths?  Do we want them to remain separate or have them
be unified at some point?

Also, in order to use the tree_entry_interesting code it looks like I'll
either have to pipe through a flag saying 'yes i want to match against
submodules' like I did for the other pathspec codepath.  Either that or
add functionality to perform wildmatching against partial matches (ie
directories and submodules) since currently the tree_entry_interesting
code path just punts and says 'well say it matches for now and check
again later' whenever it runs into a directory (I can't really make it
do that for submodules without a flag of somesort as tests could break).
Or maybe both?

-- 
Brandon Williams