Re: [PATCH v2 1/3] repo-settings: create core.featureAdoptionRate setting

[Junio C Hamano <gitster@pobox.com>]

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

> @@ -41,7 +42,6 @@ static int aggressive_depth = 50;
>  static int aggressive_window = 250;
>  static int gc_auto_threshold = 6700;
>  static int gc_auto_pack_limit = 50;
> -static int gc_write_commit_graph;
>  static int detach_auto = 1;
>  static timestamp_t gc_log_expire_time;
>  static const char *gc_log_expire = "1.day.ago";
> @@ -148,7 +148,6 @@ static void gc_config(void)
>  	git_config_get_int("gc.aggressivedepth", &aggressive_depth);
>  	git_config_get_int("gc.auto", &gc_auto_threshold);
>  	git_config_get_int("gc.autopacklimit", &gc_auto_pack_limit);
> -	git_config_get_bool("gc.writecommitgraph", &gc_write_commit_graph);
>  	git_config_get_bool("gc.autodetach", &detach_auto);
>  	git_config_get_expiry("gc.pruneexpire", &prune_expire);
>  	git_config_get_expiry("gc.worktreepruneexpire", &prune_worktrees_expire);
> @@ -685,7 +684,8 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
>  		clean_pack_garbage();
>  	}
>  
> -	if (gc_write_commit_graph)
> +	prepare_repo_settings(the_repository);
> +	if (the_repository->settings->gc_write_commit_graph == 1)
>  		write_commit_graph_reachable(get_object_directory(), 0,
>  					     !quiet && !daemonized);

OK, so the general idea is to move per-subsystem local variables to
new fields in the repository structure, stop parsing the configuration
in per-subsystem config callback, which is how the configuration for
the writing side is done above, and ...

>  
> diff --git a/commit-graph.c b/commit-graph.c
> index 7c5e54875f..b09c465a7a 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -16,6 +16,7 @@
>  #include "hashmap.h"
>  #include "replace-object.h"
>  #include "progress.h"
> +#include "repo-settings.h"
>  
>  #define GRAPH_SIGNATURE 0x43475048 /* "CGPH" */
>  #define GRAPH_CHUNKID_OIDFANOUT 0x4f494446 /* "OIDF" */
> @@ -311,7 +312,6 @@ static void prepare_commit_graph_one(struct repository *r, const char *obj_dir)
>  static int prepare_commit_graph(struct repository *r)
>  {
>  	struct object_directory *odb;
> -	int config_value;
>  
>  	if (git_env_bool(GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD, 0))
>  		die("dying as requested by the '%s' variable on commit-graph load!",
> @@ -321,9 +321,10 @@ static int prepare_commit_graph(struct repository *r)
>  		return !!r->objects->commit_graph;
>  	r->objects->commit_graph_attempted = 1;
>  
> +	prepare_repo_settings(r);
> +
>  	if (!git_env_bool(GIT_TEST_COMMIT_GRAPH, 0) &&
> -	    (repo_config_get_bool(r, "core.commitgraph", &config_value) ||
> -	    !config_value))
> +	    r->settings->core_commit_graph != 1)
>  		/*
>  		 * This repository is not configured to use commit graphs, so
>  		 * do not load one. (But report commit_graph_attempted anyway

... how the reading side is done here.  And then instead let the new
repo-settings module read them ...

> diff --git a/repo-settings.c b/repo-settings.c
> new file mode 100644
> index 0000000000..f7fc2a1959
> --- /dev/null
> +++ b/repo-settings.c
> @@ -0,0 +1,44 @@
> +#include "cache.h"
> +#include "repository.h"
> +#include "config.h"
> +#include "repo-settings.h"
> +
> +#define UPDATE_DEFAULT(s,v) do { if (s == -1) { s = v; } } while(0)
> +
> +static int git_repo_config(const char *key, const char *value, void *cb)
> +{
> +	struct repo_settings *rs = (struct repo_settings *)cb;
> +
> +	if (!strcmp(key, "core.featureadoptionrate")) {
> +		int rate = git_config_int(key, value);
> +		if (rate >= 3) {
> +			UPDATE_DEFAULT(rs->core_commit_graph, 1);
> +			UPDATE_DEFAULT(rs->gc_write_commit_graph, 1);
> +		}
> +		return 0;
> +	}
> +	if (!strcmp(key, "core.commitgraph")) {
> +		rs->core_commit_graph = git_config_bool(key, value);
> +		return 0;
> +	}
> +	if (!strcmp(key, "gc.writecommitgraph")) {
> +		rs->gc_write_commit_graph = git_config_bool(key, value);
> +		return 0;
> +	}
> +
> +	return 1;
> +}

... like this.  If a concrete/underlying configuration variable
appears in the configuration data stream, the fields would get their
values overwritten, and if the adoption rate setting appears
_before_ concreate ones, they affect the value in the fields.  So
the assignment to these fields when the adoption rate setting is
seen survives only when the underlying variable does not appear
anywhere in the configuration dasta stream at all.

Which is exactly what we want.  Nicely written.

> +void prepare_repo_settings(struct repository *r)
> +{
> +	if (r->settings)
> +		return;
> +
> +	r->settings = xmalloc(sizeof(*r->settings));
> +
> +	/* Defaults */
> +	r->settings->core_commit_graph = -1;
> +	r->settings->gc_write_commit_graph = -1;
> +
> +	repo_config(r, git_repo_config, r->settings);
> +}
> diff --git a/repo-settings.h b/repo-settings.h
> new file mode 100644
> index 0000000000..11d08648e1
> --- /dev/null
> +++ b/repo-settings.h
> @@ -0,0 +1,13 @@
> +#ifndef REPO_SETTINGS_H
> +#define REPO_SETTINGS_H
> +
> +struct repo_settings {
> +	char core_commit_graph;
> +	char gc_write_commit_graph;
> +};

I do not see a particular reason to favor type "char" here. "char"
is wider than e.g. "signed int :2", if you wanted to save space
compared to a more obvious and naive type, e.g. "int".  More
importantly, the language does not guarantee signedness of "char",
so the sentinel value of "-1" is a bit tricky to use, as you have to
be prepared to see your code used on an "unsigned char" platform.

Use of "signed char" would be OK, but this is a singleton instance
per repository, so I am not sure how much it matters to save a few
words here by not using the most natural "int" type.