[v2] maintenance: test commit-graph auto condition

Commit Message

Jean-Noël Avila via GitGitGadget Oct. 8, 2020, 12:50 a.m. UTC

From: Derrick Stolee <[e-mail]>

The auto condition for the commit-graph maintenance task walks refs
looking for commits that are not in the commit-graph file. This was
added in 4ddc79b2 (maintenance: add auto condition for commit-graph
task, 2020-09-17) but was left untested.

The initial goal of this change was to demonstrate the feature works
properly by adding tests. However, there was an off-by-one error that
caused the basic tests around maintenance.commit-graph.auto=1 to fail
when it should work.

The subtlety is that if a ref tip is not in the commit-graph, then we
were not adding that to the total count. In the test, we see that we
have only added one commit since our last commit-graph write, so the
auto condition would say there is nothing to do.

The fix is simple: add the check for the commit-graph position to see
that the tip is not in the commit-graph file before starting our walk.
Since this happens before adding to the DFS stack, we do not need to
clear our (currently empty) commit list.

This does add some extra complexity for the test, because we also want
to verify that the walk along the parents actually does some work. This
means we need to add at least two commits in a row without writing the
commit-graph. However, we also need to make sure no additional refs are
pointing to the middle of this list or else the for_each_ref() in
should_write_commit_graph() might visit these commits as tips instead of
doing a DFS walk. Hence, the last two commits are added with "git
commit" instead of "test_commit".

Signed-off-by: Derrick Stolee <[e-mail]>
---
    maintenance: test commit-graph auto condition
    
    As promised [1], here is a test to check that
    maintenance.commit-graph.auto behaves correctly. In the process, I found
    a small off-by-one error that is not super-critical, but worth fixing.
    
    This is based on ds/maintenance-part-1, but my GGG pull request targets
    ds/maintenance-part-3 to make sure it merges cleanly and the result
    builds & tests cleanly.
    
    Thanks, -Stolee
    
    [1] 
    https://lore.kernel.org/git/[e-mail]/

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-746%2Fderrickstolee%2Fmaintenance%2Fcg-auto-test-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-746/derrickstolee/maintenance/cg-auto-test-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/746

Range-diff vs v1:

 1:  39223f21fb ! 1:  984ee2d9c0 maintenance: test commit-graph auto condition
     @@ t/t7900-maintenance.sh: test_expect_success 'run --task=<task>' '
      +	GIT_TRACE2_EVENT="$(pwd)/cg-negative-means-yes.txt" \
      +		git -c maintenance.commit-graph.auto="-1" $COMMAND &&
      +
     -+	test_commit one &&
     ++	test_commit first &&
      +
      +	GIT_TRACE2_EVENT="$(pwd)/cg-zero-means-no.txt" \
      +		git -c maintenance.commit-graph.auto=0 $COMMAND &&
      +	GIT_TRACE2_EVENT="$(pwd)/cg-one-satisfied.txt" \
      +		git -c maintenance.commit-graph.auto=1 $COMMAND &&
      +
     -+	git commit --allow-empty -m "two" &&
     -+	git commit --allow-empty -m "three" &&
     ++	git commit --allow-empty -m "second" &&
     ++	git commit --allow-empty -m "third" &&
      +
      +	GIT_TRACE2_EVENT="$(pwd)/cg-two-satisfied.txt" \
      +		git -c maintenance.commit-graph.auto=2 $COMMAND &&


 builtin/gc.c           |  8 +++++++-
 t/t7900-maintenance.sh | 29 +++++++++++++++++++++++++++++
 2 files changed, 36 insertions(+), 1 deletion(-)


base-commit: 25914c4fdeefd99b06e134496dfb9bbb58a5c417

Patch

diff --git a/builtin/gc.c b/builtin/gc.c
index 090959350e..12ddb68bba 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -737,9 +737,15 @@  static int dfs_on_ref(const char *refname,
 	commit = lookup_commit(the_repository, oid);
 	if (!commit)
 		return 0;
-	if (parse_commit(commit))
+	if (parse_commit(commit) ||
+	    commit_graph_position(commit) != COMMIT_NOT_FROM_GRAPH)
 		return 0;
 
+	data->num_not_in_graph++;
+
+	if (data->num_not_in_graph >= data->limit)
+		return 1;
+
 	commit_list_append(commit, &stack);
 
 	while (!result && stack) {
diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index 53c883531e..ee1f4a7ae4 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -52,6 +52,35 @@  test_expect_success 'run --task=<task>' '
 	test_subcommand git commit-graph write --split --reachable --no-progress <run-both.txt
 '
 
+test_expect_success 'commit-graph auto condition' '
+	COMMAND="maintenance run --task=commit-graph --auto --quiet" &&
+
+	GIT_TRACE2_EVENT="$(pwd)/cg-no.txt" \
+		git -c maintenance.commit-graph.auto=1 $COMMAND &&
+	GIT_TRACE2_EVENT="$(pwd)/cg-negative-means-yes.txt" \
+		git -c maintenance.commit-graph.auto="-1" $COMMAND &&
+
+	test_commit first &&
+
+	GIT_TRACE2_EVENT="$(pwd)/cg-zero-means-no.txt" \
+		git -c maintenance.commit-graph.auto=0 $COMMAND &&
+	GIT_TRACE2_EVENT="$(pwd)/cg-one-satisfied.txt" \
+		git -c maintenance.commit-graph.auto=1 $COMMAND &&
+
+	git commit --allow-empty -m "second" &&
+	git commit --allow-empty -m "third" &&
+
+	GIT_TRACE2_EVENT="$(pwd)/cg-two-satisfied.txt" \
+		git -c maintenance.commit-graph.auto=2 $COMMAND &&
+
+	COMMIT_GRAPH_WRITE="git commit-graph write --split --reachable --no-progress" &&
+	test_subcommand ! $COMMIT_GRAPH_WRITE <cg-no.txt &&
+	test_subcommand $COMMIT_GRAPH_WRITE <cg-negative-means-yes.txt &&
+	test_subcommand ! $COMMIT_GRAPH_WRITE <cg-zero-means-no.txt &&
+	test_subcommand $COMMIT_GRAPH_WRITE <cg-one-satisfied.txt &&
+	test_subcommand $COMMIT_GRAPH_WRITE <cg-two-satisfied.txt
+'
+
 test_expect_success 'run --task=bogus' '
 	test_must_fail git maintenance run --task=bogus 2>err &&
 	test_i18ngrep "is not a valid task" err