On 11/18, Junio C Hamano wrote: > Brandon Williams <[email protected]> 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
