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

[Derrick Stolee <stolee@gmail.com>]

On 7/23/2019 10:53 AM, Johannes Schindelin wrote:
> Hi Stolee,
> 
> On Mon, 22 Jul 2019, Derrick Stolee via GitGitGadget wrote:
> 
>> 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!
> 
>> 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.

I had forgotten about this interaction. Thanks for finding it!

The way I would fix it is to add an "int feature_many_commits" to
the repo_settings struct, then check its value at the very end of
prepare_repo_settings() and then UPDATE_DEFAULT() for the settings
we want.

Thanks!
-Stolee