Re: [PATCH v6 1/1] builtin/grep.c: integrate with sparse index

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 23, 2022, 16:58 UTC
Message-ID: <xmqqy1uauixc.fsf@gitster.g>
In-Reply-To: <20220923041842.27817-2-shaoxuan.yuan02@gmail.com>

Shaoxuan Yuan <shaoxuan.yuan02@gmail.com> writes:

> +test_expect_success 'grep with and --cached' '

"with and --cached"? "with and without --cached" is probably a good thing to test but you may need to add tests for "with" case, too?

Show 5 quoted lines

> +	init_repos &&
> +
> +	test_all_match git grep --cached a &&
> +	test_all_match git grep --cached a -- "folder1/*"
> +'

The above is very relevant for the purpose of ...

> -	/* TODO: audit for interaction with sparse-index. */
> -	ensure_full_index(repo->index);

... auditing. Run the command with a pathspec that specify areas inside and outside the sparse cone(s) and ensure the result match those in a non-sparse-index, with test_all_match().

As to the lack of the tests WITHOUT "--cached", I suspect that it is omitted because there is no checked-out copies to grep in, but I suspect that it is papering over a buggy design. If we do not by default limit the operation only to paths inside sparse cone(s), shouldn't we be treating the paths outside as if they exist with the same contents as they are in the index (and unmodified)? If we take the position that "working tree files on paths outside the sparse cone(s) do not exist", "git diff" would need to say that they are all removed to be consistent, which probably is not what we want to see.

Show 18 quoted lines

> +test_expect_success 'grep is not expanded' '
> +	init_repos &&
> +
> +	ensure_not_expanded grep a &&
> +	ensure_not_expanded grep a -- deep/* &&
> +
> +	# All files within the folder1/* pathspec are sparse,
> +	# so this command does not find any matches
> +	ensure_not_expanded ! grep a -- folder1/* &&
> +
> +	# test out-of-cone pathspec with or without wildcard
> +	ensure_not_expanded grep --cached a -- "folder1/a" &&
> +	ensure_not_expanded grep --cached a -- "folder1/*" &&
> +
> +	# test in-cone pathspec with or without wildcard
> +	ensure_not_expanded grep --cached a -- "deep/a" &&
> +	ensure_not_expanded grep --cached a -- "deep/*"
> +'

It is not wrong per-se, but I am not sure how relevant these tests are.

The implementation of ensure_not_expanded very intimately knows that a call to ensure_full_index() is the one we are trying to avoid (and we do not even detect if another way to fully expand the index is invented and used), and we know we are removing the only call to the function in "git grep".

Thanks.