Re: [PATCH v2 2/2] grep: fix grepping for "intent to add" files
- From
Duy Nguyen <pclouds@gmail.com>
- Date
- Jun 22, 2016, 16:11 UTC
- Message-ID
- <CACsJy8C9Dh_Owr3UFJnCtvXserG4V-e1ws8ZY52ME1yr+fefOw@mail.gmail.com>
- In-Reply-To
- <xmqqinx2nonl.fsf@gitster.mtv.corp.google.com>
On Wed, Jun 22, 2016 at 12:49 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 47 quoted lines
>> @@ -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