Re: [PATCH v4 4/6] grep: optionally recurse into submodules

[Brandon Williams <bmwill@google.com>]

On 11/18, Junio C Hamano wrote:
> Brandon Williams <bmwill@google.com> writes:
> 
> > +static int grep_cache(struct grep_opt *opt, const struct pathspec *pathspec,
> > +		      int cached)
> >  {
> >  	int hit = 0;
> >  	int nr;
> > +	struct strbuf name = STRBUF_INIT;
> > +	int name_base_len = 0;
> > +	if (super_prefix) {
> > +		name_base_len = strlen(super_prefix);
> > +		strbuf_addstr(&name, super_prefix);
> > +	}
> > +
> >  	read_cache();
> >  
> >  	for (nr = 0; nr < active_nr; nr++) {
> >  		const struct cache_entry *ce = active_cache[nr];
> > -		if (!S_ISREG(ce->ce_mode))
> > -			continue;
> > -		if (!ce_path_match(ce, pathspec, NULL))
> > -			continue;
> > -		/*
> > -		 * If CE_VALID is on, we assume worktree file and its cache entry
> > -		 * are identical, even if worktree file has been modified, so use
> > -		 * cache version instead
> > -		 */
> > -		if (cached || (ce->ce_flags & CE_VALID) || ce_skip_worktree(ce)) {
> > -			if (ce_stage(ce) || ce_intent_to_add(ce))
> > -				continue;
> > -			hit |= grep_sha1(opt, ce->oid.hash, ce->name, 0,
> > -					 ce->name);
> > +		strbuf_setlen(&name, name_base_len);
> > +		strbuf_addstr(&name, ce->name);
> > +
> > +		if (S_ISREG(ce->ce_mode) &&
> > +		    match_pathspec(pathspec, name.buf, name.len, 0, NULL,
> > +				   S_ISDIR(ce->ce_mode) ||
> > +				   S_ISGITLINK(ce->ce_mode))) {
> > +			/*
> > +			 * If CE_VALID is on, we assume worktree file and its
> > +			 * cache entry are identical, even if worktree file has
> > +			 * been modified, so use cache version instead
> > +			 */
> > +			if (cached || (ce->ce_flags & CE_VALID) ||
> > +			    ce_skip_worktree(ce)) {
> > +				if (ce_stage(ce) || ce_intent_to_add(ce))
> > +					continue;
> > +				hit |= grep_sha1(opt, ce->oid.hash, ce->name,
> > +						 0, ce->name);
> > +			} else {
> > +				hit |= grep_file(opt, ce->name);
> > +			}
> > +		} else if (recurse_submodules && S_ISGITLINK(ce->ce_mode) &&
> > +			   submodule_path_match(pathspec, name.buf, NULL)) {
> > +			hit |= grep_submodule(opt, NULL, ce->name, ce->name);
> >  		}
> > -		else
> > -			hit |= grep_file(opt, ce->name);
> 
> We used to reject anything other than S_ISREG() upfront in the loop,
> and then either did grep_sha1() from the cache or from grep_file()
> from the working tree.
> 
> Now, the guard upfront is removed, and we do the same in the first
> part of this if/elseif.  The elseif part deals with a submodule that
> could match the pathspec.
> 
> Don't we need a final else clause that would skip the remainder of
> this loop?  What would happen to a S_ISREG() path that does *NOT*
> match the given pathspec?  We used to just "continue", but it seems
> to me that such a path will fall through the above if/elseif in the
> new code.  Would that be a problem?

It may be (Though I didn't see any issues when running tests).  It would
be easy enough to add an 'else continue;' at the end though.

-- 
Brandon Williams