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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 21, 2016, 22:49 UTC
Message-ID: <xmqqinx2nonl.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20160621211412.28752-2-charles@hashpling.org>

Charles Bailey <charles@hashpling.org> writes:

> Is "Helped-by" an appropriate attribution in this case?

Sure.

Show 22 quoted lines

> diff --git a/builtin/grep.c b/builtin/grep.c
> index 462e607..ae73831 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -386,7 +386,7 @@ static int grep_cache(struct grep_opt *opt, const struct pathspec *pathspec, int
>  
>  	for (nr = 0; nr < active_nr; nr++) {
>  		const struct cache_entry *ce = active_cache[nr];
> -		if (!S_ISREG(ce->ce_mode) || ce_intent_to_add(ce))
> +		if (!S_ISREG(ce->ce_mode))
>  			continue;
>  		if (!ce_path_match(ce, pathspec, NULL))
>  			continue;
> @@ -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 actually think (ce->ce_flags & CE_VALID) case should go to working tree for performance, but that is a separate topic.

> +test_expect_success 'grep can find things only in the work tree' '
> +	touch work-tree-only &&

Please do not use "touch" if the reason to use the command is *not* to muck with the timestamp, e.g. to create an empty file.

Show 37 quoted lines

> +	git add work-tree-only &&
> +	echo "find in work tree" >work-tree-only &&
> +	git grep --quiet "find in work tree" &&
> +	test_must_fail git grep --quiet --cached "find in work tree" &&
> +	test_must_fail git grep --quiet "find in work tree" HEAD &&
> +	git rm -f work-tree-only
> +'
> +
> +test_expect_success 'grep can find things only in the work tree (i-t-a)' '
> +	echo "intend to add this" >intend-to-add &&
> +	git add -N intend-to-add &&
> +	git grep --quiet "intend to add this" &&
> +	test_must_fail git grep --quiet --cached "intend to add this" &&
> +	test_must_fail git grep --quiet "intend to add this" HEAD &&
> +	git rm -f intend-to-add
> +'
> +
> +test_expect_success 'grep can find things only in the index' '
> +	echo "only in the index" >cache-this &&
> +	git add cache-this &&
> +	rm cache-this &&
> +	test_must_fail git grep --quiet "only in the index" &&
> +	git grep --quiet --cached "only in the index" &&
> +	test_must_fail git grep --quiet "only in the index" HEAD &&
> +	git rm --cached cache-this
> +'
> +
> +test_expect_success 'grep does not report i-t-a with -L --cached' '
> +	echo "intend to add this" >intend-to-add &&
> +	git add -N intend-to-add &&
> +	git ls-files | grep -v "^intend-to-add\$" >expected &&
> +	git grep -L --cached "nonexistent_string" >actual &&
> +	test_cmp expected actual &&
> +	git rm -f intend-to-add
> +'
> +
>  test_done