On Tue, May 24, 2022 at 11:55:02PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
> On Tue, May 24 2022, Taylor Blau wrote:
>
> > On Tue, May 24, 2022 at 12:39:11PM -0700, Jonathan Nieder wrote:
> >> Ævar Arnfjörð Bjarmason wrote:
> >> > On Fri, May 20 2022, Taylor Blau wrote:
> >> > On Mon, Nov 29 2021, Taylor Blau wrote:
> >>
> >> > > +static void write_mtimes_header(struct hashfile *f)
> >> > > +{
> >> > > + hashwrite_be32(f, MTIMES_SIGNATURE);
> >> > > + hashwrite_be32(f, MTIMES_VERSION);
> >> > > + hashwrite_be32(f, oid_version(the_hash_algo));
> >> > > +}
> >> [...]
> >> > But since this is a new format I think it's worth considering not using
> >> > the 1 or 2 you get from oid_version(), but the "format_id",
> >> > i.e. GIT_SHA1_FORMAT_ID or GIT_SHA256_FORMAT_ID.
> >> >
> >> > You'll use the same space in the format for it, but we'll end up with
> >> > something more obvious (as the integer encodes the sha1 or sha256 name).
> >>
> >> Agreed.
> >
> > I know we recommend using the format_id for on-disk formats, but I think
> > there is enough existing uses of "1" or "2" that either are acceptable
> > in practice.
> >
> > E.g., grepping around for "hashwrite.*oid_version", there are three
> > existing formats that use "1" or "2" instead of the format_id. They are:
> >
> > - the commit-graph format
> > - the midx format
> > - the .rev format
> >
> > Moreover, I can't seem to find any formats that _don't_ use that
> > convention.
>
> It's used in the reftable format.
Ah, thanks for pointing it out. Still, I think there's enough uses of
"1" and "2" over format_id that I'm not convinced here.
> > So I have a vague preference towards using the values "1"
> > and "2" as we currently do in these patches.
>
> I suspect that's less "vague" and more "c'mon, I'm using it in
> production already" :)
No, this wasn't a veil over anything. Yes, GitHub is using this in
production already, but that isn't why I'm opposed here. I'm opposed for
the reasons I explained in the quoted bits (and would happily carry a
small amount of custom code in GitHub's fork to continue to recognize
the "1" or "2" values if this ever changed to use format_id).
> Anyway, I'm fine with leaving it be as you have it currently. I'd first
> encountered this magic with reftable I think, and only recently found
> that we use 1 and 2 in these other more recent places.
Sounds good. Unless others have a very strong opinion, let's leave it as
is.
Thanks,
Taylor
