On 11/18, Junio C Hamano wrote:
> Brandon Williams <bmwill@xxxxxxxxxx> 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