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

[Junio C Hamano <gitster@pobox.com>]

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?