Re: [PATCH v5 00/17] cruft packs

Hi,

Æ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.

[...]

Other than that the only question I have (I think) on this series is if
Jonathan Nieder is happy with it. I looked back in my logs and there was
an extensive on-IRC discussion about it at the end of March, which ended
in you sending: https://lore.kernel.org/git/YkICkpttOujOKeT3@nand.local/ (local)

But it seems Jonathan didn't chime in since then, and he had some major
issues with the approach here. I think those should have been addressed
by that discussion, but it would be nice to get a confirmation.

I would still prefer if this used a repository format extension, but
that preference is not strong enough that I'd say "this must not go in
without one".  What I think would help would be some information in
the user-facing documentation for commands that create and work with
cruft packs.  In other words, if our take on people sharing
repositories between implementations that understand and don't
understand cruft packs and get objects moving back and forth between
packed and loose objects is "you should have known you were doing
something strange", the least we can do is to warn them.

I don't see a config to enable PACK_CRUFT by default yet in this
series.  I'd like one, so that people can turn it on and get the good
new behavior. :)

Thanks,
Jonathan