commit-graph: fix bug around octopus merges

I come, hat in hand, with a bug-fix for a bug that I wrote. I just happened to discover this issue while playing around with turning the commit-graph on by default and stumbled into this warning.

Please see the commit message for full details, but putting octopus merges in a tip commit-graph file in the incremental format [1] would cause the file to not load because Git thinks the base commit-graphs do not have the right hashes.

It is very likely that a more careful patch would allow this warning to become an error during git commit-graph verify, but I wanted to minimize the change for this quick bug fix.

Thanks, -Stolee

[1] https://public-inbox.org/git/[email protected]/

Derrick Stolee (1):
commit-graph: fix bug around octopus merges

 commit-graph.c                | 2 +-
 t/t5324-split-commit-graph.sh | 4 +++-
 2 files changed, 4 insertions(+), 2 deletions(-)

base-commit: 5b15eb397d176c557a82e872d6b4043aa7589874
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-308%2Fderrickstolee%2Fcommit-graph-octopus-bug-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-308/derrickstolee/commit-graph-octopus-bug-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/308

gitgitgadget

From: Derrick Stolee <[email protected]>

In 1771be90 "commit-graph: merge commit-graph chains" (2019-06-18), the method sort_and_scan_merged_commits() was added to merge the commit lists of two commit-graph files in the incremental format. Unfortunately, there was an off-by-one error in that method around incrementing num_extra_edges, which leads to an incorrect offset for the base graph chunk.

When we store an octopus merge in the commit-graph file, we store the first parent in the normal place, but use the second parent position to point into the "extra edges" chunk where the remaining parents exist. This means we should be adding "num_parents - 1" edges to this list, not "num_parents - 2". That is the basic error.

The reason this was not caught in the test suite is more subtle. In 5324-split-commit-graph.sh, we test creating an octopus merge and adding it to the tip of a commit-graph chain, then verify the result. This _should_ have caught the problem, except that when we load the commit-graph files we were overly careful to not fail when the commit-graph chain does not match. This care was on purpose to avoid race conditions as one process reads the chain and another process modifies it. In such a case, the reading process outputs the following message to stderr:

warning: commit-graph chain does not match

These warnings are output in the test suite, but ignored. By checking the stderr of git commit-graph verify to include the expected progress output, it will now catch this error.

---
 commit-graph.c                | 2 +-
 t/t5324-split-commit-graph.sh | 4 +++-
 2 files changed, 4 insertions(+), 2 deletions(-)

index c91e6f0fb8..1aaf02879b 100644
@@ -1629,7 +1629,7 @@ static void sort_and_scan_merged_commits(struct write_commit_graph_context *ctx)
 				num_parents++;
 
 			if (num_parents > 2)
-				ctx->num_extra_edges += num_parents - 2;
+				ctx->num_extra_edges += num_parents - 1;
 		}
 	}
 

index 03f45a1ed9..99f4ef4c19 100755
@@ -319,7 +319,9 @@ test_expect_success 'add octopus merge' '
 	git merge commits/3 commits/4 &&
 	git branch merge/octopus &&
 	git commit-graph write --reachable --split &&
-	git commit-graph verify &&
+	git commit-graph verify 2>err &&
+	test_line_count = 3 err &&
+	test_i18ngrep ! warning err &&
 	test_line_count = 3 $graphdir/commit-graph-chain
 '
 

gitgitgadget

There should be a better way to check this than relying on the number of progress lines, e.g. 'git commit-graph verify' could print something specific, like "Verified 3 commit-graph files, all OK", to its standard output, and the test should check only that. Or check only that there is no warning on standard error.

On one hand, see the recent commits 077b979891 (t3404: make the 'rebase.missingCommitsCheck=ignore' test more focused, 2019-06-24) and bb431c3dad (t3420: remove progress lines before comparing output, 2019-07-04), where we started to exclude the progress output from verification in tests like this that are not specifically about progress output.

On the other hand, one of my fun side-projects is to run some semi-automated and souped-up CI builds of currently cooking topics, which include a custom GETTEXT_POISON mode [1], where the poisoned translated progress output in this new test looks like this [2]:

V.e.r.i.f.y.i.n.g. .c.o.m.m.i.t.s. .i.n. .c.o.m.m.i.t. .g.r.a.p.h.: 100% (1/1), d.o.n.e.. V.e.r.i.f.y.i.n.g. .c.o.m.m.i.t.s. .i.n. .c.o.m.m.i.t. .g.r.a.p.h.: 100% (5/5), d.o.n.e.. V.e.r.i.f.y.i.n.g. .c.o.m.m.i.t.s. .i.n. .c.o.m.m.i.t. .g.r.a.p.h.:

    100% (12/12),  d.o.n.e..

So this makes the progress lines almost twice as long, and as the last commit-graph file contains double-digit commits, the length of the progress line just happens to exceed the width of a 80 character terminal, and gets line wrapped. This throws off the line count check, as there are now 4 lines on standard error instead of the expected 3.

[1] https://public-inbox.org/git/[email protected]/
[2] https://travis-ci.org/szeder/git-cooking-topics-for-travis-ci/jobs/568120252#L4120
Note that the highlighted line is in the 'after failure' fold, and your browser might unhelpfully fold it up before you could take a good look.

Thanks for pointing out that the poison runs can change the number of lines. My intention was to ensure "we got the amount of output we expected" but it is enough to only use test_i18ngrep to check that no warnings occurred.

Thanks,
-Stolee

Hm, I'm afraid I was not clear enough, so let me clarify.

A "regular" GIT_TEST_GETTEXT_POISON=1 test run does not change the number of lines. After it replaces the translated string with the '# GETTEXT POISON #' magic, the progress output will look like this:

# GETTEXT POISON #: 41% (5/12)

This will never grow long enough to trigger a line break.

It's just one of my fun experiments [1] that makes translated strings twice as long, and thus triggers that line break.

[1] https://github.com/szeder/git gettext-poison-scrambled-msg