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

From: Junio C Hamano <[email-masked]>
Date: Fri, 23 Sep 2022 09:58:23 -0700

Shaoxuan Yuan <[email-masked]> 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?

> +	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.

> +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.