Re: [PATCH 6/6] builtin/reflog: introduce subcommand to list reflogs

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 3649 bytes --]

On Mon, Feb 19, 2024 at 04:32:43PM -0800, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> 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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]