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

[Junio C Hamano <gitster@pobox.com>]

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

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

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

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