On Mon, Feb 19, 2024 at 04:32:43PM -0800, Junio C Hamano wrote:
> Patrick Steinhardt <ps@xxxxxx> writes:
>
> > diff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh
> > index d2f5f42e67..6d8d5a253d 100755
> > --- a/t/t1410-reflog.sh
> > +++ b/t/t1410-reflog.sh
> > @@ -436,4 +436,73 @@ test_expect_success 'empty reflog' '
> > test_must_be_empty err
> > '
> >
> > +test_expect_success 'list reflogs' '
> > + test_when_finished "rm -rf repo" &&
> > + git init repo &&
> > + (
> > + cd repo &&
> > + git reflog list >actual &&
> > + test_must_be_empty actual &&
> > +
> > + test_commit A &&
> > + cat >expect <<-EOF &&
> > + HEAD
> > + refs/heads/main
> > + EOF
> > + git reflog list >actual &&
> > + test_cmp expect actual &&
> > +
> > + git branch b &&
> > + cat >expect <<-EOF &&
> > + HEAD
> > + refs/heads/b
> > + refs/heads/main
> > + EOF
> > + git reflog list >actual &&
> > + test_cmp expect actual
> > + )
> > +'
>
> OK. This is a quite boring baseline.
>
> > +test_expect_success 'reflog list returns error with additional args' '
> > + cat >expect <<-EOF &&
> > + error: list does not accept arguments: ${SQ}bogus${SQ}
> > + EOF
> > + test_must_fail git reflog list bogus 2>err &&
> > + test_cmp expect err
> > +'
>
> Makes sense.
>
> > +test_expect_success 'reflog for symref with unborn target can be listed' '
> > + test_when_finished "rm -rf repo" &&
> > + git init repo &&
> > + (
> > + cd repo &&
> > + test_commit A &&
> > + git symbolic-ref HEAD refs/heads/unborn &&
> > + cat >expect <<-EOF &&
> > + HEAD
> > + refs/heads/main
> > + EOF
> > + git reflog list >actual &&
> > + test_cmp expect actual
> > + )
> > +'
>
> Should this be under REFFILES? Ah, no, "git symbolic-ref" is valid
> under reftable as well, so there is no need to.
>
> Without [5/6], would it have failed to show the reflog for HEAD?
I initially thought so, but no. `refs_resolve_ref_unsafe()` is weird as
it returns successfully even if a symref cannot be resolved unless you
pass `RESOLVE_REF_READING`, which we didn't.
The case where it does make a difference is if we had a corrupt ref. So
if you "echo garbage >.git/refs/heads/branch", then the corresponding
reflog would not have been listed. Even worse, even after this patch
series it's still impossible to `git reflog show` the reflog because we
fail to resolve the ref itself, which basically breaks the whole point
of the reflog.
This is something that I plan to address in a follow-up patch series.
> > +test_expect_success 'reflog with invalid object ID can be listed' '
> > + test_when_finished "rm -rf repo" &&
> > + git init repo &&
> > + (
> > + cd repo &&
> > + test_commit A &&
> > + test-tool ref-store main update-ref msg refs/heads/missing \
> > + $(test_oid deadbeef) "$ZERO_OID" REF_SKIP_OID_VERIFICATION &&
> > + cat >expect <<-EOF &&
> > + HEAD
> > + refs/heads/main
> > + refs/heads/missing
> > + EOF
> > + git reflog list >actual &&
> > + test_cmp expect actual
> > + )
> > +'
>
> OK.
>
> > test_done
>
> It would have been "interesting" to see an example of "there is a
> reflog but the underlying ref for it is missing" case, but I think
> that falls into a minor repository corruption category, so lack of
> such a test is also fine.
The reason why I didn't include such a test is that it's by necessity
specific to the backend: we don't have any way to delete a ref without
also deleting the corresponding reflog. So we'd have to manually delete
it, which only works with the REFFILES backend.
Patrick
Attachment:
signature.asc
Description: PGP signature
