Re: [PATCH v3 2/2] config: let feature.experimental imply gc.cruftPacks=true

From: Derrick Stolee <derrickstolee@github.com>
Date: Oct 31, 2022, 10:46 UTC
Message-ID: <fe5658f8-ac50-b328-f286-ef0971455046@github.com>
In-Reply-To: <13a25a425bc0901ee38760095eb1bcf314c00da2.1666819953.git.me@ttaylorr.com>

On 10/26/22 5:32 PM, Taylor Blau wrote:

Show 25 quoted lines

> From: Emily Shaffer <emilyshaffer@google.com>
> 
> We are interested in exploring whether gc.cruftPacks=true should become
> the default value.
> 
> To determine whether it is safe to do so, let's encourage more users to
> try it out.
> 
> Users who have set feature.experimental=true have already volunteered to
> try new and possibly-breaking config changes, so let's try this new
> default with that set of users.
> 
> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
>  Documentation/config/feature.txt |  3 ++
>  builtin/gc.c                     |  7 +++--
>  repo-settings.c                  |  1 +
>  repository.h                     |  1 +
>  t/t6500-gc.sh                    | 53 ++++++++++++++++++++++++++++++++
>  5 files changed, 63 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/config/feature.txt b/Documentation/config/feature.txt
> index cdecd04e5b..95975e5091 100644
> --- a/Documentation/config/feat

> +	prepare_repo_settings(the_repository);
> +	if (cruft_packs < 0)
> +		cruft_packs = the_repository->settings.gc_cruft_packs;
> +

Here, we are checking if cruft_packs hasn't been set by the command-line arguments _or_ the gc.cruftPacks config option (as checked in gc_config()). However, we now have another parameter saying "enable it by the experimental flag".

You are applying things in the correct order here, but I wanted to point out...

Show 8 quoted lines

> @@ -43,6 +43,7 @@ void prepare_repo_settings(struct repository *r)
>  	/* Defaults modified by feature.* */
>  	if (experimental) {
>  		r->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_SKIPPING;
> +		r->settings.gc_cruft_packs = 1;
>  	}
>  	if (manyfiles) {
>  		r->settings.index_version = 4;

That later in prepare_repo_settings(), we check the exact config values so that the repo_settings reflects the full implications from config:

	/* Commit graph config or default, does not cascade (simple) */
	repo_cfg_bool(r, "core.commitgraph", &r->settings.core_commit_graph, 1);
	repo_cfg_int(r, "commitgraph.generationversion", &r->settings.commit_graph_generation_version, 2);
	repo_cfg_bool(r, "commitgraph.readchangedpaths", &r->settings.commit_graph_read_changed_paths, 1);
	repo_cfg_bool(r, "gc.writecommitgraph", &r->settings.gc_write_commit_graph, 1);
	repo_cfg_bool(r, "fetch.writecommitgraph", &r->settings.fetch_write_commit_graph, 0);

	/* Boolean config or default, does not cascade (simple)  */
	repo_cfg_bool(r, "pack.usesparse", &r->settings.pack_use_sparse, 1);
	repo_cfg_bool(r, "core.multipackindex", &r->settings.core_multi_pack_index, 1);
	repo_cfg_bool(r, "index.sparse", &r->settings.sparse_index, 0);

This allows us to use the_repository->settings.* members as placeholders for the full implications from config.

So this implementation has some slight differences from other repo_settings implementations. It's correct, and the use of gc.cruftPacks is pretty isolated, but I wonder if we should be sticklers about this pattern in repo_settings.

Thanks, -Stolee

P.S. I found this draft open on my laptop when I opened it this morning. Hopefully it's not too late.