Re: [PATCH v2 7/8] packed-backend: check whether the "packed-refs" is sorted

From: shejialuo <shejialuo@gmail.com>
Date: Jan 31, 2025, 14:35 UTC
Message-ID: <Z5zfx0E2neO5MNKs@ArchLinux>
In-Reply-To: <xmqqwmecceh1.fsf@gitster.g>

On Thu, Jan 30, 2025 at 11:02:18AM -0800, Junio C Hamano wrote:

Show 10 quoted lines

> shejialuo <shejialuo@gmail.com> writes:
> 
> > We will always try to sort the "packed-refs" increasingly by comparing
> > the refname. So, we should add checks to verify whether the "packed-refs"
> > is sorted.
> 
> Do this _ONLY_ when the packed-refs file has a header that declares
> "sorted" trait.  Insisting on a packed-refs file that does not would
> mean you are stricter than the runtime contract allows.
>

From my perspective, we should check whether it is sorted when the header has a "sorted" trait. Actually, in the runtime, when calling `create_snapshot` method, the following would happen:

1. If there is no "sorted" trait, it will sort the "packed-refs".
2. If there is, it won't sort the "packed-refs".

So, we DO allow refs unsorted.

Actually, I have used `git show v1.5.0:builtin-pack-refs.c`, in this version, it does not sort the ref. However, I quite don't understand the comment from Patrick in the version one about this patch:

> Makes sense. It has been a source of bugs a couple years ago, and it can
> silently make you receive wrong results, so this is quite a sensible
> check to have.

Patrick, could you please help to explain this. I don't know whether we need to check whether "packed-refs" is sorted always. It seems that we truly allow refs unsorted. We need to know whether we should tighten this?

Show 9 quoted lines

> > +struct fsck_packed_ref_entry {
> > +	int line_number;
> > +
> > +	struct snapshot_record record;
> > +};
> 
> Not a huge deal, as 1 billion is still plenty of a large number, but
> the same comment on the line-number applies here.  We might want to
> consistently use ulong for line numbers of files we read from.

Yes, let me improve this.

Thanks, Jialuo