"Derrick Stolee via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes: > From: Derrick Stolee <dstolee@xxxxxxxxxxxxx> > > 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 <dstolee@xxxxxxxxxxxxx> > --- > 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. > > Thanks, -Stolee > > [1] > https://lore.kernel.org/git/cfc8a8e9-f812-2cb1-f6d8-57ef585346d1@xxxxxxxxx/ > > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-746%2Fderrickstolee%2Fmaintenance%2Fcg-auto-test-v1 > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-746/derrickstolee/maintenance/cg-auto-test-v1 > Pull-Request: https://github.com/gitgitgadget/git/pull/746 > > builtin/gc.c | 8 +++++++- > t/t7900-maintenance.sh | 29 +++++++++++++++++++++++++++++ > 2 files changed, 36 insertions(+), 1 deletion(-) Hmph. Something in ds/maintenance-part-2 series does not seem to work well with this change. I noticed it when testing today's first integration cycle of jch and seen branches, but even when applied to ds/maintenance-part-2 alone without any other topics in seen, it seems to break in an unexpected place. $ git checkout -b ds/maintenance-commit-graph-auto-fix $ git am -s $this_message $ git checkout ds/maintenance-part-2^0 $ git cherry-pick ds/maintenance-commit-graph-auto-fix $ make && (cd t && sh t7900-maintenance.sh -i -v) ... expecting success of 7900.9 'prefetch multiple remotes': ... Cloning into 'clone1'... done. Cloning into 'clone2'... done. Switched to a new branch 'one' Switched to a new branch 'two' On branch one nothing to commit, working tree clean not ok 9 - prefetch multiple remotes Perhaps the new test added here breaks the expectation of tests added in the other series? > diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh > index 53c883531e..3e16439bf6 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 one && > + > + 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_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 > > base-commit: 25914c4fdeefd99b06e134496dfb9bbb58a5c417
