Re: [PATCH v2 2/2] grep: fix grepping for "intent to add" files

On Wed, Jun 22, 2016 at 12:49 AM, Junio C Hamano [off-list ref] wrote:

quoted

@@ -396,7 +396,7 @@ static int grep_cache(struct grep_opt *opt, const struct pathspec *pathspec, int
               * cache version instead
               */
              if (cached || (ce->ce_flags & CE_VALID) || ce_skip_worktree(ce)) {
-                     if (ce_stage(ce))
+                     if (ce_stage(ce) || ce_intent_to_add(ce))
                              continue;
                      hit |= grep_sha1(opt, ce->sha1, ce->name, 0, ce->name);
              }

OK, so this function handles searching in either the index or the
working tree.

The first hunk used to unconditionally discard paths marked as
i-t-a, even when we are looking at the working tree, which is
clearly useless, and we stop rejecting i-t-a paths too early, which
is good.

The second hunk is for "grep --cached" but also covers two other
cases.  What are these?

CE_VALID is used by "Assume unchanged".  Because the user promised
that s/he will take responsibility of keeping the working tree
contents in sync with what is in the index by not modifying it, even
when we are not doing "grep --cached", we pick up the contents from
the index and look for the string in there, instead of going to the
working tree.  In other words, even though at the mechanical level
we are looking into the index, logically we are searching in the
working tree.  Is it sensible to skip i-t-a entries in that case?

I think the same discussion would apply to CE_SKIP_WORKTREE (see
"Skip-worktree bit" in Documentation/git-update-index.txt).

So I wonder if a better change would be more like

        for (...) {
                if (!S_ISREG(ce->ce_mode))
                        continue; /* not a regular file */
                if (!ce_path_match(ce, pathspec, NULL)
                        continue; /* uninteresting */
+               if (cached && ce_intent_to_add(ce))
+                       continue; /* path not yet in the index */

                if (cached || ...)
                        UNCHANGED FROM THE ORIGINAL

perhaps?

I did wonder a bit about these cases. But, can i-t-a really be
combined with CE_VALID or CE_SKIP_WORKTREE? CE_SKIP_... is
automatically set and should not cover i-t-a entries imo (I didn't
check the implementation). CE_VALID is about real entries, yes you
could do "git update-index --assume-unchanged <ita-path>" but it does
not feel right to me.

If cached is false and ce_ita() is true and either CE_VALID or
CE_SKIP_WORKTREE is set, we would continue to grep an _empty_ SHA-1.
But I think we should grep_file() instead, at least for CE_VALID.
-- 
Duy