From: shejialuo <shejialuo@gmail.com>
Date: Fri, 31 Jan 2025 14:35:51 GMT
Subject: Re: [PATCH v2 7/8] packed-backend: check whether the "packed-refs" is sorted
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:
> 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?

> > +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