Re: [PATCH 2/2] pack-objects: enable multi-pack reuse via `feature.experimental`

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 2641 bytes --]

On Tue, Jan 16, 2024 at 02:03:47PM -0500, Taylor Blau wrote:
> Now that multi-pack reuse is supported, enable it via the
> feature.experimental configuration in addition to the classic
> `pack.allowPackReuse`.
> 
> This will allow more users to experiment with the new behavior who might
> not otherwise be aware of the existing `pack.allowPackReuse`
> configuration option.
> 
> The enum with values NO_PACK_REUSE, SINGLE_PACK_REUSE, and
> MULTI_PACK_REUSE is defined statically in builtin/pack-objects.c's
> compilation unit. We could hoist that enum into a scope visible from the
> repository_settings struct, and then use that enum value in
> pack-objects. Instead, define a single int that indicates what
> pack-objects's default value should be to avoid additional unnecessary
> code movement.
> 
> Though `feature.experimental` implies `pack.allowPackReuse=multi`, this
> can still be overridden by explicitly setting the latter configuration
> to either "single" or "false". Tests covering all of these cases are
> showin t5332.

I do not mind adding more configs to `feature.experimental` because it
is the best mechanism we have for a staged rollout of features. It is
not ideal by any means as we have no way to tell how many people enable
this, or whether they hit any bugs. But we do not really have any
alternatives.

But one thing I would like to see is to have a clear plan for how
experimental features become stable. It's not a huge problem (yet)
because there are only two experimental features. One of them
("pack.useBitmapBoundaryTraversal=true") was recently added by you via
b0afdce5da (pack-bitmap.c: use commit boundary during bitmap traversal,
2023-05-08), which is perfectly fine. But the other one
("fetch.negotiationAlgorithm=skipping") has been added has been added
via b5651a2092 (experimental: default to fetch.writeCommitGraph=false,
2020-07-06), so it's been experimental for ~3.5 years by now.

I would like to avoid cases like this by laying out a plan for when
experimental features become the new default. It could be as simple as
"Let's wait N releases and then mark it stable". But having something
and documenting such a plan in our code makes it a lot more actionable
to promote those features to become stable eventually.

I know that this is not in any way specific to your patch, but I thought
this was a good opportunity to start this discussion. If we can agree on
my opinion then it would be great to add a comment to the experimental
feature to explain such an exit criterion.

Other than that this patch looks good to me, thanks!

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]