[PATCH 0/3] Use commit-graph by default

[PATCH 0/3] Use commit-graph by default

[Derrick Stolee via GitGitGadget]

The commit-graph feature is starting to stabilize. Based on what is in
master right now, we have:

Git 2.18:

 * Ability to write commit-graph (requires user interaction).
   
   
 * Commit parsing is faster when commit-graph exists.
   
   
 * Must have core.commitGraph true to use.
   
   

Git 2.19:

 * Ability to write commit-graph on GC with gc.writeCommitGraph.
   
   
 * Generation numbers written in commit-graph
   
   
 * A few reachability algorithms make use of generation numbers.
   
   

(queued for) master:

 * The test suite passes with GIT_TEST_COMMIT_GRAPH=1
   
   
 * 'git commit-graph write' has progress indicators.
   
   
 * The commit-graph is automatically disabled when grafts or replace-objects
   exist.
   
   

There are some other things coming that are in review (like 'git log
--graph' speedups), but it is probably time to consider enabling the
commit-graph by default. This series does that.

For timing, I'm happy to leave this queued for a merge after the Git 2.20
release. There are enough things in master to justify not enabling this by
default until that goes out and more people use it.

PATCH 3/3 is rather simple, and is the obvious thing to do to achieve
enabling these config values by default.

PATCH 1/3 is a required change to make the test suite work with this change.
This change isn't needed with GIT_TEST_COMMIT_GRAPH=1 because the
commit-graph is up-to-date for these 'git gc' calls, so no progress is
output.

PATCH 2/3 is also a necessary evil, since we already had to disable
GIT_TEST_COMMIT_GRAPH for some tests, we now also need to turn off
core.commitGraph.

Thanks, -Stolee

Derrick Stolee (3):
  t6501: use --quiet when testing gc stderr
  t: explicitly turn off core.commitGraph as needed
  commit-graph: Use commit-graph by default

 Documentation/config.txt            | 4 ++--
 builtin/gc.c                        | 2 +-
 commit-graph.c                      | 6 +++---
 t/t0410-partial-clone.sh            | 3 ++-
 t/t5307-pack-missing-commit.sh      | 3 ++-
 t/t6011-rev-list-with-bad-commit.sh | 3 ++-
 t/t6024-recursive-merge.sh          | 3 ++-
 t/t6501-freshen-objects.sh          | 6 +++---
 8 files changed, 17 insertions(+), 13 deletions(-)


base-commit: a4b8ab5363a32f283a61ef3a962853556d136c0e
Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-50%2Fderrickstolee%2Fcommit-graph-default-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-50/derrickstolee/commit-graph-default-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/50
-- 
gitgitgadget

[PATCH 1/3] t6501: use --quiet when testing gc stderr

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <dstolee@microsoft.com>

The test script t6501-freshen-objects.sh has some tests that care
if 'git gc' has any output to stderr. This is intended to say that
no warnings occurred related to broken links. However, when we
have operations that output progress (like writing the commit-graph)
this causes the test to fail.

Use 'git gc --quiet' to avoid these progress indicators from causing
a test failure.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 t/t6501-freshen-objects.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/t6501-freshen-objects.sh b/t/t6501-freshen-objects.sh
index 033871ee5f..0973130f06 100755
--- a/t/t6501-freshen-objects.sh
+++ b/t/t6501-freshen-objects.sh
@@ -137,7 +137,7 @@ test_expect_success 'do not complain about existing broken links (commit)' '
 	some message
 	EOF
 	commit=$(git hash-object -t commit -w broken-commit) &&
-	git gc 2>stderr &&
+	git gc --quiet 2>stderr &&
 	verbose git cat-file -e $commit &&
 	test_must_be_empty stderr
 '
@@ -147,7 +147,7 @@ test_expect_success 'do not complain about existing broken links (tree)' '
 	100644 blob 0000000000000000000000000000000000000003	foo
 	EOF
 	tree=$(git mktree --missing <broken-tree) &&
-	git gc 2>stderr &&
+	git gc --quiet 2>stderr &&
 	git cat-file -e $tree &&
 	test_must_be_empty stderr
 '
@@ -162,7 +162,7 @@ test_expect_success 'do not complain about existing broken links (tag)' '
 	this is a broken tag
 	EOF
 	tag=$(git hash-object -t tag -w broken-tag) &&
-	git gc 2>stderr &&
+	git gc --quiet 2>stderr &&
 	git cat-file -e $tag &&
 	test_must_be_empty stderr
 '
-- 
gitgitgadget

Re: [PATCH 1/3] t6501: use --quiet when testing gc stderr

[Junio C Hamano]

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Derrick Stolee <dstolee@microsoft.com>
>
> The test script t6501-freshen-objects.sh has some tests that care
> if 'git gc' has any output to stderr. This is intended to say that
> no warnings occurred related to broken links. However, when we
> have operations that output progress (like writing the commit-graph)
> this causes the test to fail.

I see that the descriptor #2 is redirected into a regular file.  Why
should we be writing the progress indicator in that case in the
first place?  Shoudln't we be doing the usual "are we showing this
to an interactive terminal?" test?

Re: [PATCH 1/3] t6501: use --quiet when testing gc stderr

[Derrick Stolee]

On 10/18/2018 1:23 AM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> From: Derrick Stolee <dstolee@microsoft.com>
>>
>> The test script t6501-freshen-objects.sh has some tests that care
>> if 'git gc' has any output to stderr. This is intended to say that
>> no warnings occurred related to broken links. However, when we
>> have operations that output progress (like writing the commit-graph)
>> this causes the test to fail.
> I see that the descriptor #2 is redirected into a regular file.  Why
> should we be writing the progress indicator in that case in the
> first place?  Shoudln't we be doing the usual "are we showing this
> to an interactive terminal?" test?

This code from builtin/gc.c makes it look like we are doing that:

         if (gc_write_commit_graph)
                 write_commit_graph_reachable(get_object_directory(), 0,
                                              !quiet && !daemonized);

But really, daemonized is only for when running in the background.

Do you have an example of a builtin that checks this interactive 
terminal behavior?

Thanks,
-Stolee

Re: [PATCH 1/3] t6501: use --quiet when testing gc stderr

[Junio C Hamano]

Derrick Stolee <stolee@gmail.com> writes:

> This code from builtin/gc.c makes it look like we are doing that:
>
>         if (gc_write_commit_graph)
>                 write_commit_graph_reachable(get_object_directory(), 0,
>                                              !quiet && !daemonized);
>
> But really, daemonized is only for when running in the background.

But that is something we can easily fix, no?

"git grep isatty" tells you that we use isatty(2) to decide if we
want to go verbose or show progress.  Another frequent use of isatty
in our codebase of course is to use of a pager and columnar output
formats.

[PATCH 2/3] t: explicitly turn off core.commitGraph as needed

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <dstolee@microsoft.com>

There are a few tests that already require GIT_TEST_COMMIT_GRAPH=0
as they rely on an interaction with the commits in the object store
that is circumvented by parsing commit information from the
commit-graph instead. Before enabling core.commitGraph as true by
default, explicitly turn the setting off for these tests.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 t/t0410-partial-clone.sh            | 3 ++-
 t/t5307-pack-missing-commit.sh      | 3 ++-
 t/t6011-rev-list-with-bad-commit.sh | 3 ++-
 t/t6024-recursive-merge.sh          | 3 ++-
 4 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh
index cfd0655ea1..f5277fafb1 100755
--- a/t/t0410-partial-clone.sh
+++ b/t/t0410-partial-clone.sh
@@ -193,7 +193,8 @@ test_expect_success 'rev-list stops traversal at missing and promised commit' '
 
 	git -C repo config core.repositoryformatversion 1 &&
 	git -C repo config extensions.partialclone "arbitrary string" &&
-	GIT_TEST_COMMIT_GRAPH=0 git -C repo rev-list --exclude-promisor-objects --objects bar >out &&
+	GIT_TEST_COMMIT_GRAPH=0 git -c core.commitGraph=false \
+		-C repo rev-list --exclude-promisor-objects --objects bar >out &&
 	grep $(git -C repo rev-parse bar) out &&
 	! grep $FOO out
 '
diff --git a/t/t5307-pack-missing-commit.sh b/t/t5307-pack-missing-commit.sh
index dacb440b27..dc4c19d0aa 100755
--- a/t/t5307-pack-missing-commit.sh
+++ b/t/t5307-pack-missing-commit.sh
@@ -16,7 +16,8 @@ test_expect_success setup '
 	obj=$(git rev-parse --verify tag3) &&
 	fanout=$(expr "$obj" : "\(..\)") &&
 	remainder=$(expr "$obj" : "..\(.*\)") &&
-	rm -f ".git/objects/$fanout/$remainder"
+	rm -f ".git/objects/$fanout/$remainder" &&
+	git config core.commitGraph false
 '
 
 test_expect_success 'check corruption' '
diff --git a/t/t6011-rev-list-with-bad-commit.sh b/t/t6011-rev-list-with-bad-commit.sh
index 545b461e51..da6949743d 100755
--- a/t/t6011-rev-list-with-bad-commit.sh
+++ b/t/t6011-rev-list-with-bad-commit.sh
@@ -42,7 +42,8 @@ test_expect_success 'corrupt second commit object' \
    '
 
 test_expect_success 'rev-list should fail' '
-	test_must_fail env GIT_TEST_COMMIT_GRAPH=0 git rev-list --all > /dev/null
+	test_must_fail env GIT_TEST_COMMIT_GRAPH=0 \
+		git -c core.commitGraph=false rev-list --all > /dev/null
 '
 
 test_expect_success 'git repack _MUST_ fail' \
diff --git a/t/t6024-recursive-merge.sh b/t/t6024-recursive-merge.sh
index 27c7de90ce..fccdf96f13 100755
--- a/t/t6024-recursive-merge.sh
+++ b/t/t6024-recursive-merge.sh
@@ -61,7 +61,8 @@ GIT_AUTHOR_DATE="2006-12-12 23:00:08" git commit -m F
 '
 
 test_expect_success 'combined merge conflicts' '
-	test_must_fail env GIT_TEST_COMMIT_GRAPH=0 git merge -m final G
+	test_must_fail env GIT_TEST_COMMIT_GRAPH=0 \
+		git -c core.commitGraph=false merge -m final G
 '
 
 cat > expect << EOF
-- 
gitgitgadget

[PATCH 3/3] commit-graph: Use commit-graph by default

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <dstolee@microsoft.com>

The config setting "core.commitGraph" enables using the commit-graph
file to accelerate commit walks through parsing speed and generation
numbers. The setting "gc.writeCommitGraph" enables writing the
commit-graph file on every non-trivial 'git gc' operation. Together,
they help users automatically improve their performance.

By setting these config variables to true by default, we make the
commit-graph feature an "opt-out" feature instead of "opt-in".

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 Documentation/config.txt | 4 ++--
 builtin/gc.c             | 2 +-
 commit-graph.c           | 6 +++---
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index f6f4c21a54..dc5ee7c145 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -923,7 +923,7 @@ 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 true. See
 	linkgit:git-commit-graph[1] for more information.
 
 core.useReplaceRefs::
@@ -1639,7 +1639,7 @@ gc.writeCommitGraph::
 	If true, then gc will rewrite the commit-graph file when
 	linkgit:git-gc[1] is run. When using linkgit:git-gc[1]
 	'--auto' the commit-graph will be updated if housekeeping is
-	required. Default is false. See linkgit:git-commit-graph[1]
+	required. Default is true. See linkgit:git-commit-graph[1]
 	for details.
 
 gc.logExpiry::
diff --git a/builtin/gc.c b/builtin/gc.c
index 871a56f1c5..77e7413f94 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -41,7 +41,7 @@ static int aggressive_depth = 50;
 static int aggressive_window = 250;
 static int gc_auto_threshold = 6700;
 static int gc_auto_pack_limit = 50;
-static int gc_write_commit_graph;
+static int gc_write_commit_graph = 1;
 static int detach_auto = 1;
 static timestamp_t gc_log_expire_time;
 static const char *gc_log_expire = "1.day.ago";
diff --git a/commit-graph.c b/commit-graph.c
index a686758603..a459272466 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -232,15 +232,15 @@ static int prepare_commit_graph(struct repository *r)
 {
 	struct alternate_object_database *alt;
 	char *obj_dir;
-	int config_value;
+	int config_value = 1;
 
 	if (r->objects->commit_graph_attempted)
 		return !!r->objects->commit_graph;
 	r->objects->commit_graph_attempted = 1;
 
+	repo_config_get_bool(r, "core.commitgraph", &config_value);
 	if (!git_env_bool(GIT_TEST_COMMIT_GRAPH, 0) &&
-	    (repo_config_get_bool(r, "core.commitgraph", &config_value) ||
-	    !config_value))
+	    !config_value)
 		/*
 		 * This repository is not configured to use commit graphs, so
 		 * do not load one. (But report commit_graph_attempted anyway
-- 
gitgitgadget

Re: [PATCH 0/3] Use commit-graph by default

[Junio C Hamano]

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

> The commit-graph feature is starting to stabilize. Based on what is in
> master right now, we have:
>
> Git 2.18:
>
>  * Ability to write commit-graph (requires user interaction).
>    
>    
>  * Commit parsing is faster when commit-graph exists.
>    
>    
>  * Must have core.commitGraph true to use.
>    
>    
>
> Git 2.19:
>
>  * Ability to write commit-graph on GC with gc.writeCommitGraph.
>    
>    
>  * Generation numbers written in commit-graph
>    
>    
>  * A few reachability algorithms make use of generation numbers.
>    
>    
>
> (queued for) master:
>
>  * The test suite passes with GIT_TEST_COMMIT_GRAPH=1
>    
>    
>  * 'git commit-graph write' has progress indicators.
>    
>    
>  * The commit-graph is automatically disabled when grafts or replace-objects
>    exist.

If I recall correctly, one more task that was discussed but hasn't
been addressed well is how the generation and incremental update of
it should integrate with the normal repository maintenance workflow
(perhaps "gc --auto").  If we are going to turn it on by default, it
would be good to see if we can avoid multiple independent walks done
over the same history graph by repack, prune and now commit-graph,
before such a change happens.

Thanks.

Re: [PATCH 0/3] Use commit-graph by default

[Derrick Stolee]

On 10/17/2018 11:47 PM, Junio C Hamano wrote:
> If I recall correctly, one more task that was discussed but hasn't
> been addressed well is how the generation and incremental update of
> it should integrate with the normal repository maintenance workflow
> (perhaps "gc --auto").  If we are going to turn it on by default, it
> would be good to see if we can avoid multiple independent walks done
> over the same history graph by repack, prune and now commit-graph,
> before such a change happens.
I don't remember a discussion on this, but it is an interesting point. 
I'll give it some thought to see if we can combine these walks.

Thanks,
-Stolee