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

[John Cai <johncai86@gmail.com>]

Hi Junio,

On 2 Mar 2022, at 18:32, Junio C Hamano wrote:

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

makes sense to me. Is this worth a re-roll and sending out another series to the list? or is it sufficient to make the change on my branch?

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