diff mbox series

[v3,2/5] repo-settings: add feature.manyCommits setting

Message ID c0129066a02b39535110ae592c16ca0e5d6d6c24.1564515324.git.gitgitgadget@gmail.com (mailing list archive)
State New, archived
Headers show
Series Create 'feature.*' config area and some centralized config parsing | expand

Commit Message

John Passaro via GitGitGadget July 30, 2019, 7:35 p.m. UTC 
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 beneficial in this scenario.

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

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 Documentation/config.txt         |  2 ++
 Documentation/config/core.txt    |  3 ++-
 Documentation/config/feature.txt | 15 +++++++++++++++
 Documentation/config/gc.txt      |  4 ++--
 repo-settings.c                  |  7 +++++++
 5 files changed, 28 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/config/feature.txt

Comments

Junio C Hamano July 30, 2019, 8:57 p.m. UTC | #1 
"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

> 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.*::
> +...
> +* `gc.writeCommitGraph=true` enables writing the commit-graph file during
> +garbage collection.
> \ No newline at end of file

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 309577f6bc..fc05f4fbc4 100644
> --- a/repo-settings.c
> +++ b/repo-settings.c
> @@ -2,6 +2,8 @@
>  #include "config.h"
>  #include "repository.h"
>  
> +#define UPDATE_DEFAULT(s,v) do { if (s == -1) { s = v; } } while(0)

A few points:

 1. give 's' and 'v' a bit better name, perhaps 'slot' and 'value'?

 2. "do { if ((s) == -1) { (s) = (v); } } while(0)"

 3. When we learn to set default values for variables that are not
    boolean in the future, we will regret that we did not name it
    UPDATE_DEFAULT_BOOL(slot, value).
Johannes Schindelin July 31, 2019, 1:17 p.m. UTC | #2 
Hi Junio,

On Tue, 30 Jul 2019, Junio C Hamano wrote:

> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > +#define UPDATE_DEFAULT(s,v) do { if (s == -1) { s = v; } } while(0)
>
> [...]
>  3. When we learn to set default values for variables that are not
>     boolean in the future, we will regret that we did not name it
>     UPDATE_DEFAULT_BOOL(slot, value).

On the other hand, as we never promised any kind of API (and this is not
even an internal API to begin with), it will be _easy_ to rename it in
the unlikely event that we would ever introduce non-boolean defaults to
override, wouldn't you agree?

We have plenty of precedent where patch series start by refactoring,
whether it is to rename functions or variables or files or extracting
functions. Preparing for a future that might never come strikes me as
premature optimization.

Ciao,
Dscho
Ævar Arnfjörð Bjarmason July 31, 2019, 3:01 p.m. UTC | #3 
On Tue, Jul 30 2019, Derrick Stolee via GitGitGadget wrote:

> +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.

During the whole new commit graph format discussion (which has now
landed) we discussed just auto toggling this:
https://public-inbox.org/git/87zhobr4fl.fsf@evledraar.gmail.com/

This looks fine, but have we backed out of simply enabling this at this
point? I don't see why not, regardless of commit count...
Junio C Hamano July 31, 2019, 3:48 p.m. UTC | #4 
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> On Tue, 30 Jul 2019, Junio C Hamano wrote:
>
>> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
>>
>> > +#define UPDATE_DEFAULT(s,v) do { if (s == -1) { s = v; } } while(0)
>>
>> [...]
>>  3. When we learn to set default values for variables that are not
>>     boolean in the future, we will regret that we did not name it
>>     UPDATE_DEFAULT_BOOL(slot, value).
>
> On the other hand, as we never promised any kind of API (and this is not
> even an internal API to begin with), it will be _easy_ to rename it in
> the unlikely event that we would ever introduce non-boolean defaults to
> override, wouldn't you agree?

I agree that it is easy to say that it is easy to rename it later
and burden somebody else with the task.

I know that the renaming itself is easy, when you limit yourself
within the scope of a single topic, whether done now or later.  I
also know that having to worry about other topics in flight has
non-zero cost.  I also know that you are not the one who will bear
it---I will be.

So from my point of view, if we can make a prediction, even with
limited knowledge that a name may need to be renamed in the future,
it is better not pick such a name and instead use one that we think
it has a better chance of surviving without needing a rename.
Derrick Stolee Aug. 1, 2019, 6:27 p.m. UTC | #5 
On 7/31/2019 11:01 AM, Ævar Arnfjörð Bjarmason wrote:
> 
> On Tue, Jul 30 2019, Derrick Stolee via GitGitGadget wrote:
> 
>> +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.
> 
> During the whole new commit graph format discussion (which has now
> landed) we discussed just auto toggling this:
> https://public-inbox.org/git/87zhobr4fl.fsf@evledraar.gmail.com/
> 
> This looks fine, but have we backed out of simply enabling this at this
> point? I don't see why not, regardless of commit count...

I would be happy to drop feature.manyCommits and instead swap the
defaults of core.commitGraph and gc.writeCommitGraph to true if we think
that is what we want to do for 2.24.0. We can use the repo settings and
UPDATE_DEFAULT[_BOOL] for this purpose.

Thanks,
-Stolee
diff mbox series

Patch

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 309577f6bc..fc05f4fbc4 100644
--- a/repo-settings.c
+++ b/repo-settings.c
@@ -2,6 +2,8 @@ 
 #include "config.h"
 #include "repository.h"
 
+#define UPDATE_DEFAULT(s,v) do { if (s == -1) { s = v; } } while(0)
+
 void prepare_repo_settings(struct repository *r)
 {
 	int value;
@@ -22,4 +24,9 @@  void prepare_repo_settings(struct repository *r)
 
 	if (!repo_config_get_bool(r, "pack.usesparse", &value))
 		r->settings.pack_use_sparse = value;
+
+	if (!repo_config_get_bool(r, "feature.manycommits", &value) && value) {
+		UPDATE_DEFAULT(r->settings.core_commit_graph, 1);
+		UPDATE_DEFAULT(r->settings.gc_write_commit_graph, 1);
+	}
 }