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

Commit Message

John Cai March 2, 2022, 10:27 p.m. UTC

From: John Cai <[e-mail]>

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 <[e-mail]>
Signed-off-by: John Cai <[e-mail]>
---
 t/t3903-stash.sh | 43 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 42 insertions(+), 1 deletion(-)

Comments

Junio C Hamano March 2, 2022, 11:32 p.m. UTC | #1

"John Cai via GitGitGadget" <[e-mail]> writes:

> From: John Cai <[e-mail]>
>
> 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 <[e-mail]>
> Signed-off-by: John Cai <[e-mail]>
> ---
>  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 &&

John Cai March 3, 2022, 3:22 p.m. UTC | #2

Hi Junio,

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

> "John Cai via GitGitGadget" <[e-mail]> writes:
>
>> From: John Cai <[e-mail]>
>>
>> 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 <[e-mail]>
>> Signed-off-by: John Cai <[e-mail]>
>> ---
>>  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 &&

Phillip Wood March 3, 2022, 4:11 p.m. UTC | #3

On 02/03/2022 23:32, Junio C Hamano wrote:
> "John Cai via GitGitGadget" <[e-mail]> writes:
> [...]
>> +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
> 	)
>

I wonder if we could avoid the subshell entirely and avoid relying on 
REFFILES (assuming we're not trying to test the implementation details 
of that refs backend) by doing something like

test_expect_success 'drop stash reflog updates refs/stash with rewrite' '
	old=$(git rev-parse stash@{0}) &&
	setup_stash_test &&
	git rev-list -g stash >tmp &&
	sed /$old/d tmp >expect &&
	git rev-list -g stash >actual &&
	test_cmp expect actual
'

Best Wishes

Phillip

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

Ævar Arnfjörð Bjarmason March 3, 2022, 4:52 p.m. UTC | #4

On Thu, Mar 03 2022, Phillip Wood wrote:

> On 02/03/2022 23:32, Junio C Hamano wrote:
>> "John Cai via GitGitGadget" <[e-mail]> writes:
>> [...]
>>> +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
>> 	)
>>
>
> I wonder if we could avoid the subshell entirely and avoid relying on
> REFFILES (assuming we're not trying to test the implementation details 
> of that refs backend) by doing something like
>
> test_expect_success 'drop stash reflog updates refs/stash with rewrite' '
> 	old=$(git rev-parse stash@{0}) &&
> 	setup_stash_test &&
> 	git rev-list -g stash >tmp &&
> 	sed /$old/d tmp >expect &&
> 	git rev-list -g stash >actual &&
> 	test_cmp expect actual
> '

Unless I'm missing something that "rev-list -g" will emit only the RHS
of the stash logs, i.e. no "0000..." etc.

And if we only look at that the difference with specifying the flag
isn't visible, no?

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

Phillip Wood March 3, 2022, 5:28 p.m. UTC | #5

On 03/03/2022 16:52, Ævar Arnfjörð Bjarmason wrote:
> 
> On Thu, Mar 03 2022, Phillip Wood wrote:
> 
>> On 02/03/2022 23:32, Junio C Hamano wrote:
>>> "John Cai via GitGitGadget" <[e-mail]> writes:
>>> [...]
>>>> +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
>>> 	)
>>>
>>
>> I wonder if we could avoid the subshell entirely and avoid relying on
>> REFFILES (assuming we're not trying to test the implementation details
>> of that refs backend) by doing something like
>>
>> test_expect_success 'drop stash reflog updates refs/stash with rewrite' '
>> 	old=$(git rev-parse stash@{0}) &&
>> 	setup_stash_test &&
>> 	git rev-list -g stash >tmp &&
>> 	sed /$old/d tmp >expect &&
>> 	git rev-list -g stash >actual &&
>> 	test_cmp expect actual
>> '
> 
> Unless I'm missing something that "rev-list -g" will emit only the RHS
> of the stash logs, i.e. no "0000..." etc.
> 
> And if we only look at that the difference with specifying the flag
> isn't visible, no?

Maybe I'm missing what this test is actually needs to do. I thought it 
just needed to check that the deleted stash is removed from the reflog 
and the others are unchanged. You're right that it wont show the LHS and 
if that is important then you need to read the log file directly.

Best Wishes

Phillip


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

John Cai March 3, 2022, 7:12 p.m. UTC | #6

Hi Phillip,

On 3 Mar 2022, at 12:28, Phillip Wood wrote:

> On 03/03/2022 16:52, Ævar Arnfjörð Bjarmason wrote:
>>
>> On Thu, Mar 03 2022, Phillip Wood wrote:
>>
>>> On 02/03/2022 23:32, Junio C Hamano wrote:
>>>> "John Cai via GitGitGadget" <[e-mail]> writes:
>>>> [...]
>>>>> +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
>>>> 	)
>>>>
>>>
>>> I wonder if we could avoid the subshell entirely and avoid relying on
>>> REFFILES (assuming we're not trying to test the implementation details
>>> of that refs backend) by doing something like
>>>
>>> test_expect_success 'drop stash reflog updates refs/stash with rewrite' '
>>> 	old=$(git rev-parse stash@{0}) &&
>>> 	setup_stash_test &&
>>> 	git rev-list -g stash >tmp &&
>>> 	sed /$old/d tmp >expect &&
>>> 	git rev-list -g stash >actual &&
>>> 	test_cmp expect actual
>>> '
>>
>> Unless I'm missing something that "rev-list -g" will emit only the RHS
>> of the stash logs, i.e. no "0000..." etc.
>>
>> And if we only look at that the difference with specifying the flag
>> isn't visible, no?
>
> Maybe I'm missing what this test is actually needs to do. I thought it just needed to check that the deleted stash is removed from the reflog and the others are unchanged. You're right that it wont show the LHS and if that is important then you need to read the log file directly.

We had discussed this briefly in [1], but the --rewrite option for reflog delete will rewrite the LHS, which is not visible to normal ref API users. So the only way to test that this happened is to reach inside of the file.

1. https://lore.kernel.org/git/xmqqczjdp2g8.fsf@gitster.g/
>
> Best Wishes
>
> Phillip

thanks,
John

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

Phillip Wood March 8, 2022, 10:39 a.m. UTC | #7

Hi John

On 03/03/2022 19:12, John Cai wrote:
> Hi Phillip,
> 
> On 3 Mar 2022, at 12:28, Phillip Wood wrote:
> 
>> On 03/03/2022 16:52, Ævar Arnfjörð Bjarmason wrote:
>>>
>>> On Thu, Mar 03 2022, Phillip Wood wrote:
>>>
>>>> On 02/03/2022 23:32, Junio C Hamano wrote:
>>>>> "John Cai via GitGitGadget" <[e-mail]> writes:
>>>>> [...]
>>>>>> +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
>>>>> 	)
>>>>>
>>>>
>>>> I wonder if we could avoid the subshell entirely and avoid relying on
>>>> REFFILES (assuming we're not trying to test the implementation details
>>>> of that refs backend) by doing something like
>>>>
>>>> test_expect_success 'drop stash reflog updates refs/stash with rewrite' '
>>>> 	old=$(git rev-parse stash@{0}) &&
>>>> 	setup_stash_test &&
>>>> 	git rev-list -g stash >tmp &&
>>>> 	sed /$old/d tmp >expect &&
>>>> 	git rev-list -g stash >actual &&
>>>> 	test_cmp expect actual
>>>> '
>>>
>>> Unless I'm missing something that "rev-list -g" will emit only the RHS
>>> of the stash logs, i.e. no "0000..." etc.
>>>
>>> And if we only look at that the difference with specifying the flag
>>> isn't visible, no?
>>
>> Maybe I'm missing what this test is actually needs to do. I thought it just needed to check that the deleted stash is removed from the reflog and the others are unchanged. You're right that it wont show the LHS and if that is important then you need to read the log file directly.
> 
> We had discussed this briefly in [1], but the --rewrite option for reflog delete will rewrite the LHS, which is not visible to normal ref API users. So the only way to test that this happened is to reach inside of the file.
> 
> 1. https://lore.kernel.org/git/xmqqczjdp2g8.fsf@gitster.g/

Thanks for the pointer, that was useful context that could maybe be 
added to the commit message to explain why the test needs to check the 
lhs of the reflog if you reroll.

Best Wishes

Phillip

>>
>> Best Wishes
>>
>> Phillip
> 
> thanks,
> John
> 
>>
>>
>>>>>> +	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 &&
>>>

John Cai March 8, 2022, 6:09 p.m. UTC | #8

Hi Phillip,

On 8 Mar 2022, at 5:39, Phillip Wood wrote:

> Hi John
>
> On 03/03/2022 19:12, John Cai wrote:
>> Hi Phillip,
>>
>> On 3 Mar 2022, at 12:28, Phillip Wood wrote:
>>
>>> On 03/03/2022 16:52, Ævar Arnfjörð Bjarmason wrote:
>>>>
>>>> On Thu, Mar 03 2022, Phillip Wood wrote:
>>>>
>>>>> On 02/03/2022 23:32, Junio C Hamano wrote:
>>>>>> "John Cai via GitGitGadget" <[e-mail]> writes:
>>>>>> [...]
>>>>>>> +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
>>>>>> 	)
>>>>>>
>>>>>
>>>>> I wonder if we could avoid the subshell entirely and avoid relying on
>>>>> REFFILES (assuming we're not trying to test the implementation details
>>>>> of that refs backend) by doing something like
>>>>>
>>>>> test_expect_success 'drop stash reflog updates refs/stash with rewrite' '
>>>>> 	old=$(git rev-parse stash@{0}) &&
>>>>> 	setup_stash_test &&
>>>>> 	git rev-list -g stash >tmp &&
>>>>> 	sed /$old/d tmp >expect &&
>>>>> 	git rev-list -g stash >actual &&
>>>>> 	test_cmp expect actual
>>>>> '
>>>>
>>>> Unless I'm missing something that "rev-list -g" will emit only the RHS
>>>> of the stash logs, i.e. no "0000..." etc.
>>>>
>>>> And if we only look at that the difference with specifying the flag
>>>> isn't visible, no?
>>>
>>> Maybe I'm missing what this test is actually needs to do. I thought it just needed to check that the deleted stash is removed from the reflog and the others are unchanged. You're right that it wont show the LHS and if that is important then you need to read the log file directly.
>>
>> We had discussed this briefly in [1], but the --rewrite option for reflog delete will rewrite the LHS, which is not visible to normal ref API users. So the only way to test that this happened is to reach inside of the file.
>>
>> 1. https://lore.kernel.org/git/xmqqczjdp2g8.fsf@gitster.g/
>
> Thanks for the pointer, that was useful context that could maybe be added to the commit message to explain why the test needs to check the lhs of the reflog if you reroll.

Good point. that would be helpful context since it took me a while to figure it out myself--will re-roll, thanks!

>
> Best Wishes
>
> Phillip
>
>>>
>>> Best Wishes
>>>
>>> Phillip
>>
>> thanks,
>> John
>>
>>>
>>>
>>>>>>> +	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 &&
>>>>

Patch

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() {
 	echo 1 >file &&
 	git add file &&
 	echo unrelated >other-file &&
@@ -55,6 +55,10 @@  test_expect_success 'stash some dirty working directory' '
 	git stash &&
 	git diff-files --quiet &&
 	git diff-index --cached --quiet HEAD
+}
+
+test_expect_success 'stash some dirty working directory' '
+	setup_stash
 '
 
 cat >expect <<EOF
@@ -185,6 +189,43 @@  test_expect_success 'drop middle stash by index' '
 	test 1 = $(git show HEAD:file)
 '
 
+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
+	) &&
+	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 &&