On Tue, Feb 20, 2024 at 09:22:36AM -0800, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
[snip]
> > 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