Re: [BUG] `git gc` or `git pack-refs` wipes all notes for `git notes` command

[Jeff King <peff@peff.net>]

On Fri, Jan 06, 2023 at 09:40:22PM +0900, Junio C Hamano wrote:

> > I don't think we have any fsck checks that the packed-refs file is in
> > sorted order. It might be reasonable to have them. Likewise, when
> > pack-refs rewrites the file, it should be able to cheaply double-check
> > that the input is sorted by comparing each entry against its previous.
> 
> True.  I would not mind a patch to make us do so in the code path
> where we rewrite the file and add "sorted" trait to the file.
> refs/packed-backend.c::sort_snapshot() seems to be already equipped
> to do this?

I think it may be a little trickier than that. Yes, sort_snapshot()
knows about sorting, but it only kicks in when the file isn't already
marked as sorted (and we sort on the fly so that the rest of the code
can use the same lookup routines).

And it's possible that we can just sort_snapshot() before writing, even
if the original claims to be sorted. But I'm not sure what performance
impact that might have on the normal case that everything is already in
good order. Maybe it's not a big deal; the write is already O(n), so
adding an O(n log n) might not be the end of the world.

But I was thinking more that write_with_updates() would, while iterating
through the existing entries, check that the values it gets from the
ref_iterator are indeed in sorted order. And if not, I think it needs to
actually bail, since we might already have written a partially-confused
result. And there "bail" may mean "write a warning to the user, abort
the current write, call sort_snapshot(), and then try again".

All of which is to say I don't think it's conceptually _too_ hard, but
it was not simple enough that I was comfortable dashing off a one-liner
and saying "probably something like this". ;)

> So we can conclude that this discussion thread has an
> incorrect Subject: and the symptom was caused by human error?

That's my read on it.

-Peff