Re: [PATCH v2 0/7] reflog: introduce subcommand to list reflogs

From: Patrick Steinhardt <ps@pks.im>
Date: Feb 21, 2024, 11:48 UTC
Message-ID: <ZdXi8agx5oxKfwrD@tanuki>
In-Reply-To: <xmqq34tnrqxv.fsf@gitster.g>

On Tue, Feb 20, 2024 at 09:22:36AM -0800, Junio C Hamano wrote:

> Patrick Steinhardt <ps@pks.im> writes:

[snip]

Show 21 quoted lines

> > 3:  e4e4fac05c ! 3:  32b24a3d4b refs/files: sort reflogs returned by the reflog iterator
> >     @@ refs/files-backend.c: static struct ref_iterator *reflog_iterator_begin(struct r
> >       	iter->dir_iterator = diter;
> >       	iter->ref_store = ref_store;
> >       	strbuf_release(&sb);
> >     +@@ refs/files-backend.c: static struct ref_iterator *files_reflog_iterator_begin(struct ref_store *ref_st
> >     + 		return reflog_iterator_begin(ref_store, refs->gitcommondir);
> >     + 	} else {
> >     + 		return merge_ref_iterator_begin(
> >     +-			0, reflog_iterator_begin(ref_store, refs->base.gitdir),
> >     ++			1, reflog_iterator_begin(ref_store, refs->base.gitdir),
> >     + 			reflog_iterator_begin(ref_store, refs->gitcommondir),
> >     + 			reflog_iterator_select, refs);
> >     + 	}
> 
> This hunk is new.  Is there a downside to force merged iterators to
> always be sorted?  The ones that are combined are all sorted so it
> is natural to force sorting like this code does?  It might deserve
> explaining, and would certainly help future readers who runs "blame"
> on this code to figure out what made us think always sorting is a
> good direction forward.

Not really -- it merely gets passed down to the base ref iterator to indicate that the entries are returned in lexicographic order. But I've been jumping the gun here: the `reflog_iterator_select()` function does not ensure lexicographic ordering between the two merged iterators right now. I was assuming so because I implemented it in the reftable backend like that. Should've double checked.

It's an easy fix though, which I'll add as another patch on top. Thanks for making me think twice.

Patrick