Re: [PATCH v2 1/4] t1092: add tests for `git-rm`

[Derrick Stolee <derrickstolee@github.com>]

On 8/7/22 12:13 AM, Shaoxuan Yuan wrote:

> +test_expect_failure 'rm pathspec outside sparse definition' '

My only concern with this version is a minor one, and I didn't
notice it until this version: this test_expect_failure.

test_expect_failure doesn't help too much except to say "something
fails in this test". It could be the very first command, or it
could be the last.

> +	init_repos &&
> +
> +	for file in folder1/a folder1/0/1
> +	do
> +		test_sparse_match test_must_fail git rm $file &&
> +		test_sparse_match test_must_fail git rm --cached $file &&
> +		test_sparse_match git rm --sparse $file &&
> +		test_sparse_match git status --porcelain=v2
> +	done &&
> +
> +	cat >folder1-full <<-EOF &&
> +	rm ${SQ}folder1/0/0/0${SQ}
> +	rm ${SQ}folder1/0/1${SQ}
> +	rm ${SQ}folder1/a${SQ}
> +	EOF
> +
> +	cat >folder1-sparse <<-EOF &&
> +	rm ${SQ}folder1/${SQ}
> +	EOF

The difference you are demonstrating is that this output is
different. I think that at the point of this patch, they are
the same. The goal of this patch is to establish a common
point of reference for the full index and sparse index cases.

If everything below was "test_sparse_match" in this patch,
then I believe the test would pass.

The behavior changes when we enable the sparse index in the
'rm' builtin. Demonstrating the changes to the test at that
time helps collect all of the different ways behavior changes
with a sparse index, making it really easy to audit what
exactly is different between the modes.

Another approach would be to integrate the sparse index with
the builtin early, but keep the ensure_full_index() calls in
certain places (so we still expand to a full index) and slowly
add modes that do not expand. This is even trickier to do than
to delay the test changes to the end.

That said, finding out how to organize these tests is very
difficult because there is a bit of a chicken-or-egg problem:
How can we test the custom integration logic without enabling
the sparse index across the entire builtin? How can we enable
the sparse index across the builtin without having all of the
integration logic implemented?

So please take my ramblings here as food for thought, but not
any need to make changes to this series. v2 looks good to me.

Thanks,
-Stolee