Re: [PATCH] maintenance: test commit-graph auto condition

[Junio C Hamano <gitster@pobox.com>]

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

> From: Derrick Stolee <dstolee@microsoft.com>
>
> 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@microsoft.com>
> ---
>     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@gmail.com/
>
> 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