Re: [PATCH 2/5] repo-settings: add feature.manyCommits setting

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Jul 23, 2019, 14:53 UTC
Message-ID: <nycvar.QRO.7.76.6.1907231544200.21907@tvgsbejvaqbjf.bet>
In-Reply-To: <e00a1be75b410694374b0d9bd60ab16d67ef6d20.1563818059.git.gitgitgadget@gmail.com>

Hi Stolee,

On Mon, 22 Jul 2019, Derrick Stolee via GitGitGadget wrote:

Show 5 quoted lines

> From: Derrick Stolee <dstolee@microsoft.com>
>
> When a repo has many commits, it is helpful to write and read the
> commit-graph file. Future changes to Git may include new config
> settings that are benefitial in this scenario.

s/benefitial/beneficial/

>
> Create the 'feature.manyCommits' config setting that changes the
> default values of 'core.commitGraph' and 'gc.writeCommitGraph' to
> true.

Great!

Show 22 quoted lines

> diff --git a/repo-settings.c b/repo-settings.c
> index 13a9128f62..f328602fd7 100644
> --- a/repo-settings.c
> +++ b/repo-settings.c
> @@ -3,10 +3,17 @@
>  #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, "feature.manycommits")) {
> +		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;

Okay, this one is tricky. The behavior I would want is for `feature.manycommits` to override the _default_. And if I set `feature.manycommits = false` (e.g. via `git -c feature.manycommits=false ...`), I would want the default to "go back".

So I'd really rather see this as

	if (!repo_config_get_bool(r, "feature.manycommits", &b) && b) {
		rs->core_commit_graph = 1;
		rs->gc_write_commit_graph = 1;
	}

	[...]

	repo_config_get_bool(r, "core.commitgraph", &rs->core_commit_graph);

I.e. override the default only if `feature.manyCommits` was true *in the end*.

In any case, we really need to make sure to handle the `= false` case correctly here. We might want to override the setting from the command-line, or it might be set in `~/.gitconfig` and need to be overridden in a local repository. We need to handle it.

Otherwise this looks good! Dscho