Re: [PATCH v5 00/17] cruft packs
- From
Taylor Blau <me@ttaylorr.com>
- Date
- May 24, 2022, 22:12 UTC
- Message-ID
- <Yo1YZM2dI6t+RsWv@nand.local>
- In-Reply-To
- <220525.86sfoytwjn.gmgdl@evledraar.gmail.com>
On Tue, May 24, 2022 at 11:55:02PM +0200, Ævar Arnfjörð Bjarmason wrote:
Show 39 quoted lines
>
> 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.
Show 5 quoted lines
> > 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