| Message ID | e00a1be75b410694374b0d9bd60ab16d67ef6d20.1563818059.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Create 'feature.*' config area and some centralized config parsing | expand |
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. Otherwise this looks good! Dscho
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
diff --git a/Documentation/config.txt b/Documentation/config.txt index e3f5bc3396..77f3b1486b 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -345,6 +345,8 @@ include::config/difftool.txt[] include::config/fastimport.txt[] +include::config/feature.txt[] + include::config/fetch.txt[] include::config/format.txt[] diff --git a/Documentation/config/core.txt b/Documentation/config/core.txt index 75538d27e7..d80162681a 100644 --- a/Documentation/config/core.txt +++ b/Documentation/config/core.txt @@ -577,7 +577,8 @@ the `GIT_NOTES_REF` environment variable. See linkgit:git-notes[1]. core.commitGraph:: If true, then git will read the commit-graph file (if it exists) - to parse the graph structure of commits. Defaults to false. See + to parse the graph structure of commits. Defaults to false, unless + `feature.manyCommits` is enabled. See linkgit:git-commit-graph[1] for more information. core.useReplaceRefs:: diff --git a/Documentation/config/feature.txt b/Documentation/config/feature.txt new file mode 100644 index 0000000000..f74314ae90 --- /dev/null +++ b/Documentation/config/feature.txt @@ -0,0 +1,15 @@ +feature.*:: + The config settings that start with `feature.` modify the defaults of + a group of other config settings. These groups are created by the Git + developer community as recommended defaults and are subject to change. + In particular, new config options may be added with different defaults. + +feature.manyCommits:: + Enable config options that optimize for repos with many commits. This + setting is recommended for repos with at least 100,000 commits. The + new default values are: ++ +* `core.commitGraph=true` enables reading the commit-graph file. ++ +* `gc.writeCommitGraph=true` enables writing the commit-graph file during +garbage collection. \ No newline at end of file diff --git a/Documentation/config/gc.txt b/Documentation/config/gc.txt index 02b92b18b5..31a5fc4f75 100644 --- a/Documentation/config/gc.txt +++ b/Documentation/config/gc.txt @@ -63,8 +63,8 @@ gc.writeCommitGraph:: If true, then gc will rewrite the commit-graph file when linkgit:git-gc[1] is run. When using `git gc --auto` the commit-graph will be updated if housekeeping is - required. Default is false. See linkgit:git-commit-graph[1] - for details. + required. Default is false, unless `feature.manyCommits` + is enabled. See linkgit:git-commit-graph[1] for details. gc.logExpiry:: If the file gc.log exists, then `git gc --auto` will print 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;