Use commit-graph by default

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

From: Derrick Stolee <[email protected]>

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.

---
 t/t6501-freshen-objects.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

index 033871ee5f..0973130f06 100755
@@ -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

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

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.

From: Derrick Stolee <[email protected]>

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.

---
 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(-)

index cfd0655ea1..f5277fafb1 100755
@@ -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
 '

index dacb440b27..dc4c19d0aa 100755
@@ -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' '

index 545b461e51..da6949743d 100755
@@ -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' \

index 27c7de90ce..fccdf96f13 100755
@@ -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

From: Derrick Stolee <[email protected]>

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

---
 Documentation/config.txt | 4 ++--
 builtin/gc.c             | 2 +-
 commit-graph.c           | 6 +++---
 3 files changed, 6 insertions(+), 6 deletions(-)

index f6f4c21a54..dc5ee7c145 100644
@@ -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::

index 871a56f1c5..77e7413f94 100644
@@ -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";

index a686758603..a459272466 100644
@@ -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

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.

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