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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Feb 23, 2022, 23:27 UTC
Message-ID: <220224.868ru1b0ku.gmgdl@evledraar.gmail.com>
In-Reply-To: <272D2409-1CAE-4203-96F7-9B104F7E5D4D@gmail.com>

On Wed, Feb 23 2022, John Cai wrote:

Show 59 quoted lines

> Hi Junio,
>
> On 23 Feb 2022, at 17:51, Junio C Hamano wrote:
>
>> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>>
>>>> +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
>>>> +'
>>>
>>> This one will be portable to the reftable backend.
>>>
>>>> +test_expect_success 'drop stash reflog updates refs/stash with rewrite' '
>>>
>>> But as I noted in <220222.86fsob88h7.gmgdl@evledraar.gmail.com> (but it
>>> was easy to miss) this test will need to depend on REFFILES. So just
>>> changing this line to:
>>>
>>>     test_expect_success REFFILES 'drop stash[...]'
>>>
>>>> +	git reset --hard &&
>>>> +	echo 9 >file &&
>>>> +	git stash &&
>>>> +	oid="$(git rev-parse stash@{0})" &&
>>>> +	git stash drop stash@{1} &&
>>>> +	cut -d" " -f1-2 .git/logs/refs/stash >actual &&
>>>> +	cat >expect <<-EOF &&
>>>> +	$(test_oid zero) $oid
>>>> +	EOF
>>>> +	test_cmp expect actual
>>>> +'
>>
>> Why should this be tested with "cut" in the first place, though?
>>
>> If we start from
>>
>>     stash@{0} = A
>>     stash@{1} = B
>>     stash@{2} = C
>>
>> and after saying "drop stash@{1}", what we need to check is that
>>
>>     stash@{0} = A
>>     stash@{1} = C
>
> Yes, this is true but that doesn't seem to test the --rewrite functionality.
> I could be missing something, but it seems that the reflog --rewrite option
> will write the LHS old oid value in the .git/logs/refs/stash file. When
> --rewrite isn't used, the reflog delete still does the right thing to the
> RHS entry.
>
> I couldn't find any way to check this LFS value other than reaching into the
> actual file. If there is a way that would be preferable.

Thanks for that summary that's accurate as far as I know. I think that's how this all works, and I don't know of another way to extract this information than this reaching behind the curtain.

Which, I think is a lot clearer if we amend the test like this. Note that this doesn't really add anything for catching a regression goes, but I think helps guide the human reader through this step-by-step. So perhaps it would be good to fix the test up to have it (or maybe not):

diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index ec9cc5646d6..bc58e99e3e6 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -198,12 +198,25 @@ test_expect_success 'drop stash reflog updates refs/stash' '
 test_expect_success 'drop stash reflog updates refs/stash with rewrite' '
 	git reset --hard &&
 	echo 9 >file &&
+
+	# Our two stashes
+	old_oid="$(git rev-parse stash@{0})" &&
 	git stash &&
-	oid="$(git rev-parse stash@{0})" &&
+	new_oid="$(git rev-parse stash@{0})" &&
+
+	# Our stash <old oid>/<new oid> before "drop"
+	cat >expect <<-EOF &&
+	$(test_oid zero) $old_oid
+	$old_oid $new_oid
+	EOF
+	cut -d" " -f1-2 .git/logs/refs/stash >actual &&
+	test_cmp expect actual &&
+
+	# Our stash <old oid>/<new oid> after "drop"
 	git stash drop stash@{1} &&
 	cut -d" " -f1-2 .git/logs/refs/stash >actual &&
 	cat >expect <<-EOF &&
-	$(test_oid zero) $oid
+	$(test_oid zero) $new_oid
 	EOF
 	test_cmp expect actual
 '

If this series is amended to drop the "EXPIRE_REFLOGS_REWRITE" flag then
this will fail on that last test_cmp like:
    
    + diff -u expect actual
    --- expect      2022-02-23 23:37:40.438221222 +0000
    +++ actual      2022-02-23 23:37:40.434221258 +0000
    @@ -1 +1 @@
    -0000000000000000000000000000000000000000 236c59f58e239e74e90b6832a98fa4b7f4b33647
    +5c6ad4ca28e71ae3a007e6c77043d04bc42fa9ee 236c59f58e239e74e90b6832a98fa4b7f4b33647

I.e. our <old oid> is now referring to the now-deleted stash entry we
just deleted, since we didn't rewrite it.

And as we can see with some manual inspection the state before we
dropped stash@{1} was:

    0000000000000000000000000000000000000000 5c6ad4ca28e71ae3a007e6c77043d04bc42fa9ee
    5c6ad4ca28e71ae3a007e6c77043d04bc42fa9ee 236c59f58e239e74e90b6832a98fa4b7f4b33647

My usual method of checking my assumption about this not being otherwise
inspectable would be something like:
        
    diff --git a/refs/files-backend.c b/refs/files-backend.c
    index f59589d6cce..590c13e7a2b 100644
    --- a/refs/files-backend.c
    +++ b/refs/files-backend.c
    @@ -3133,7 +3133,7 @@ static int files_reflog_expire(struct ref_store *ref_store,
            const struct object_id *oid;
     
            memset(&cb, 0, sizeof(cb));
    -       cb.rewrite = !!(expire_flags & EXPIRE_REFLOGS_REWRITE);
    +       cb.rewrite = 0;
            cb.dry_run = !!(expire_flags & EXPIRE_REFLOGS_DRY_RUN);
            cb.policy_cb = policy_cb_data;
            cb.should_prune_fn = should_prune_fn;

I.e. let's intentionally break the flag, and see what else fails (it's
set in a few places, but this is the only place where it's checked).

That should normally find the other things that are testing this, maybe
there's a better way.

But, no such luck :) The only thing that'll fail is this new test being
added here.

So just like my 5ac15ad2509 (reflog tests: add --updateref tests,
2021-10-16) this is covering a true blindspot in the "git reflog"
functionality.

The only tests that used --rewrite were a test added in c41a87dd80c
(refs: make rev-parse --quiet actually quiet, 2014-09-18), which will
pass if --rewrite is omitted.

And the ones I added in 5ac15ad2509, which I added not to test --rewrite
per-se, but to test that the --updateref part of it behaved as expected
in combination with whatever effect it was having.