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

From: Derrick Stolee <derrickstolee@github.com>
Date: Aug 10, 2022, 12:47 UTC
Message-ID: <afc04510-3c68-0226-b366-f541ca933a14@github.com>
In-Reply-To: <20220807041335.1790658-2-shaoxuan.yuan02@gmail.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.

Show 19 quoted lines

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