Re: [PATCH v4 1/3] stash: add tests to ensure reflog --rewrite --updatref behavior

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 2, 2022, 23:32 UTC
Message-ID: <xmqqlexsexse.fsf@gitster.g>
In-Reply-To: <08bb8d3a9b9cd75c8b2aed11db9456baef6f415b.1646260044.git.gitgitgadget@gmail.com>

"John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 24 quoted lines

> From: John Cai <johncai86@gmail.com>
>
> There is missing test coverage to ensure that the resulting reflogs
> after a git stash drop has had its old oid rewritten if applicable, and
> if the refs/stash has been updated if applicable.
>
> Add two tests that verify both of these happen.
>
> Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> Signed-off-by: John Cai <johncai86@gmail.com>
> ---
>  t/t3903-stash.sh | 43 ++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 42 insertions(+), 1 deletion(-)
>
> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> index b149e2af441..a2f8d0c52e7 100755
> --- a/t/t3903-stash.sh
> +++ b/t/t3903-stash.sh
> @@ -41,7 +41,7 @@ diff_cmp () {
>  	rm -f "$1.compare" "$2.compare"
>  }
>  
> -test_expect_success 'stash some dirty working directory' '
> +setup_stash() {

Style.

	setup_stash () {

but more importantly, is this really setting up "stash"? "setup_stash_test" or something, perhaps?

> +test_expect_success 'stash some dirty working directory' '
> +	setup_stash
>  '

OK.

Show 16 quoted lines

> +test_expect_success 'drop stash reflog updates refs/stash' '
> +	git reset --hard &&
> +	git rev-parse refs/stash >expect &&
> +	echo 9 >file &&
> +	git stash &&
> +	git stash drop stash@{0} &&
> +	git rev-parse refs/stash >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success REFFILES 'drop stash reflog updates refs/stash with rewrite' '
> +	git init repo &&
> +	(
> +		cd repo &&
> +		setup_stash
> +	) &&

Hmph, so this is done inside the subdirectory. The implementation of the helper in this iteration does look cleaner than in the previous iteration.

But these many references to "repo/" and "-C repo" we see below makes me wonder why we do not put the whole thing inside the subshell we started earlier.

i.e.

	git init repo &&
	(
		cd repo &&
		setup_stash_test &&

		echo 9 >file &&
		old=$(git rev-parse stash@{0}) &&
		git stash &&
		new=$(git rev-parse stash@{0}) &&
		...

		test_cmp expect actual
	)

Show 24 quoted lines

> +	echo 9 >repo/file &&
> +
> +	old_oid="$(git -C repo rev-parse stash@{0})" &&
> +	git -C repo stash &&
> +	new_oid="$(git -C repo rev-parse stash@{0})" &&
> +
> +	cat >expect <<-EOF &&
> +	$(test_oid zero) $old_oid
> +	$old_oid $new_oid
> +	EOF
> +	cut -d" " -f1-2 repo/.git/logs/refs/stash >actual &&
> +	test_cmp expect actual &&
> +
> +	git -C repo stash drop stash@{1} &&
> +	cut -d" " -f1-2 repo/.git/logs/refs/stash >actual &&
> +	cat >expect <<-EOF &&
> +	$(test_oid zero) $new_oid
> +	EOF
> +	test_cmp expect actual
> +'
> +
>  test_expect_success 'stash pop' '
>  	git reset --hard &&
>  	git stash pop &&