On Wed, Feb 23 2022, John Cai wrote:
> Hi Junio,
>
> On 23 Feb 2022, at 17:51, Junio C Hamano wrote:
>
>> Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx> (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.
