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 <dsto...@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
