{"thread":{"id":"54365","subject":"[PATCH] maintenance: test commit-graph auto condition","startedAt":"2020-10-07T12:55:23Z","lastAt":"2020-10-08T00:50:44Z","messageCount":4,"participants":["Derrick Stolee via GitGitGadget","Junio C Hamano","Derrick Stolee"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"407050","messageId":"pull.746.git.1602075317625.gitgitgadget@gmail.com","threadId":"54365","inReplyTo":null,"subject":"[PATCH] maintenance: test commit-graph auto condition","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-10-07T12:55:17Z","receivedAt":"2020-10-07T12:55:23Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <dstolee@microsoft.com>\n\nThe auto condition for the commit-graph maintenance task walks refs\nlooking for commits that are not in the commit-graph file. This was\nadded in 4ddc79b2 (maintenance: add auto condition for commit-graph\ntask, 2020-09-17) but was left untested.\n\nThe initial goal of this change was to demonstrate the feature works\nproperly by adding tests. However, there was an off-by-one error that\ncaused the basic tests around maintenance.commit-graph.auto=1 to fail\nwhen it should work.\n\nThe subtlety is that if a ref tip is not in the commit-graph, then we\nwere not adding that to the total count. In the test, we see that we\nhave only added one commit since our last commit-graph write, so the\nauto condition would say there is nothing to do.\n\nThe fix is simple: add the check for the commit-graph position to see\nthat the tip is not in the commit-graph file before starting our walk.\nSince this happens before adding to the DFS stack, we do not need to\nclear our (currently empty) commit list.\n\nThis does add some extra complexity for the test, because we also want\nto verify that the walk along the parents actually does some work. This\nmeans we need to add at least two commits in a row without writing the\ncommit-graph. However, we also need to make sure no additional refs are\npointing to the middle of this list or else the for_each_ref() in\nshould_write_commit_graph() might visit these commits as tips instead of\ndoing a DFS walk. Hence, the last two commits are added with \"git\ncommit\" instead of \"test_commit\".\n\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n    maintenance: test commit-graph auto condition\n    \n    As promised [1], here is a test to check that\n    maintenance.commit-graph.auto behaves correctly. In the process, I found\n    a small off-by-one error that is not super-critical, but worth fixing.\n    \n    Thanks, -Stolee\n    \n    [1] \n    https://lore.kernel.org/git/cfc8a8e9-f812-2cb1-f6d8-57ef585346d1@gmail.com/\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-746%2Fderrickstolee%2Fmaintenance%2Fcg-auto-test-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-746/derrickstolee/maintenance/cg-auto-test-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/746\n\n builtin/gc.c           |  8 +++++++-\n t/t7900-maintenance.sh | 29 +++++++++++++++++++++++++++++\n 2 files changed, 36 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 090959350e..12ddb68bba 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -737,9 +737,15 @@ static int dfs_on_ref(const char *refname,\n \tcommit = lookup_commit(the_repository, oid);\n \tif (!commit)\n \t\treturn 0;\n-\tif (parse_commit(commit))\n+\tif (parse_commit(commit) ||\n+\t    commit_graph_position(commit) != COMMIT_NOT_FROM_GRAPH)\n \t\treturn 0;\n \n+\tdata->num_not_in_graph++;\n+\n+\tif (data->num_not_in_graph >= data->limit)\n+\t\treturn 1;\n+\n \tcommit_list_append(commit, &stack);\n \n \twhile (!result && stack) {\ndiff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\nindex 53c883531e..3e16439bf6 100755\n--- a/t/t7900-maintenance.sh\n+++ b/t/t7900-maintenance.sh\n@@ -52,6 +52,35 @@ test_expect_success 'run --task=<task>' '\n \ttest_subcommand git commit-graph write --split --reachable --no-progress <run-both.txt\n '\n \n+test_expect_success 'commit-graph auto condition' '\n+\tCOMMAND=\"maintenance run --task=commit-graph --auto --quiet\" &&\n+\n+\tGIT_TRACE2_EVENT=\"$(pwd)/cg-no.txt\" \\\n+\t\tgit -c maintenance.commit-graph.auto=1 $COMMAND &&\n+\tGIT_TRACE2_EVENT=\"$(pwd)/cg-negative-means-yes.txt\" \\\n+\t\tgit -c maintenance.commit-graph.auto=\"-1\" $COMMAND &&\n+\n+\ttest_commit one &&\n+\n+\tGIT_TRACE2_EVENT=\"$(pwd)/cg-zero-means-no.txt\" \\\n+\t\tgit -c maintenance.commit-graph.auto=0 $COMMAND &&\n+\tGIT_TRACE2_EVENT=\"$(pwd)/cg-one-satisfied.txt\" \\\n+\t\tgit -c maintenance.commit-graph.auto=1 $COMMAND &&\n+\n+\tgit commit --allow-empty -m \"two\" &&\n+\tgit commit --allow-empty -m \"three\" &&\n+\n+\tGIT_TRACE2_EVENT=\"$(pwd)/cg-two-satisfied.txt\" \\\n+\t\tgit -c maintenance.commit-graph.auto=2 $COMMAND &&\n+\n+\tCOMMIT_GRAPH_WRITE=\"git commit-graph write --split --reachable --no-progress\" &&\n+\ttest_subcommand ! $COMMIT_GRAPH_WRITE <cg-no.txt &&\n+\ttest_subcommand $COMMIT_GRAPH_WRITE <cg-negative-means-yes.txt &&\n+\ttest_subcommand ! $COMMIT_GRAPH_WRITE <cg-zero-means-no.txt &&\n+\ttest_subcommand $COMMIT_GRAPH_WRITE <cg-one-satisfied.txt &&\n+\ttest_subcommand $COMMIT_GRAPH_WRITE <cg-two-satisfied.txt\n+'\n+\n test_expect_success 'run --task=bogus' '\n \ttest_must_fail git maintenance run --task=bogus 2>err &&\n \ttest_i18ngrep \"is not a valid task\" err\n\nbase-commit: 25914c4fdeefd99b06e134496dfb9bbb58a5c417\n-- \ngitgitgadget\n"},{"id":"407099","messageId":"xmqqblhd23dc.fsf@gitster.c.googlers.com","threadId":"54365","inReplyTo":"pull.746.git.1602075317625.gitgitgadget@gmail.com","subject":"Re: [PATCH] maintenance: test commit-graph auto condition","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-07T20:22:07Z","receivedAt":"2020-10-07T20:22:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Derrick Stolee <dstolee@microsoft.com>\n>\n> The auto condition for the commit-graph maintenance task walks refs\n> looking for commits that are not in the commit-graph file. This was\n> added in 4ddc79b2 (maintenance: add auto condition for commit-graph\n> task, 2020-09-17) but was left untested.\n>\n> The initial goal of this change was to demonstrate the feature works\n> properly by adding tests. However, there was an off-by-one error that\n> caused the basic tests around maintenance.commit-graph.auto=1 to fail\n> when it should work.\n>\n> The subtlety is that if a ref tip is not in the commit-graph, then we\n> were not adding that to the total count. In the test, we see that we\n> have only added one commit since our last commit-graph write, so the\n> auto condition would say there is nothing to do.\n>\n> The fix is simple: add the check for the commit-graph position to see\n> that the tip is not in the commit-graph file before starting our walk.\n> Since this happens before adding to the DFS stack, we do not need to\n> clear our (currently empty) commit list.\n>\n> This does add some extra complexity for the test, because we also want\n> to verify that the walk along the parents actually does some work. This\n> means we need to add at least two commits in a row without writing the\n> commit-graph. However, we also need to make sure no additional refs are\n> pointing to the middle of this list or else the for_each_ref() in\n> should_write_commit_graph() might visit these commits as tips instead of\n> doing a DFS walk. Hence, the last two commits are added with \"git\n> commit\" instead of \"test_commit\".\n>\n> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>\n> ---\n>     maintenance: test commit-graph auto condition\n>     \n>     As promised [1], here is a test to check that\n>     maintenance.commit-graph.auto behaves correctly. In the process, I found\n>     a small off-by-one error that is not super-critical, but worth fixing.\n>     \n>     Thanks, -Stolee\n>     \n>     [1] \n>     https://lore.kernel.org/git/cfc8a8e9-f812-2cb1-f6d8-57ef585346d1@gmail.com/\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-746%2Fderrickstolee%2Fmaintenance%2Fcg-auto-test-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-746/derrickstolee/maintenance/cg-auto-test-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/746\n>\n>  builtin/gc.c           |  8 +++++++-\n>  t/t7900-maintenance.sh | 29 +++++++++++++++++++++++++++++\n>  2 files changed, 36 insertions(+), 1 deletion(-)\n\nHmph.  Something in ds/maintenance-part-2 series does not seem to\nwork well with this change.  I noticed it when testing today's first\nintegration cycle of jch and seen branches, but even when applied to\nds/maintenance-part-2 alone without any other topics in seen, it\nseems to break in an unexpected place.\n\n    $ git checkout -b ds/maintenance-commit-graph-auto-fix\n    $ git am -s $this_message\n    $ git checkout ds/maintenance-part-2^0\n    $ git cherry-pick ds/maintenance-commit-graph-auto-fix\n    $ make && (cd t && sh t7900-maintenance.sh -i -v)\n    ...\n    expecting success of 7900.9 'prefetch multiple remotes':\n    ...\n    Cloning into 'clone1'...\n    done.\n    Cloning into 'clone2'...\n    done.\n    Switched to a new branch 'one'\n    Switched to a new branch 'two'\n    On branch one\n    nothing to commit, working tree clean\n    not ok 9 - prefetch multiple remotes\n\nPerhaps the new test added here breaks the expectation of tests\nadded in the other series?\n\n> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\n> index 53c883531e..3e16439bf6 100755\n> --- a/t/t7900-maintenance.sh\n> +++ b/t/t7900-maintenance.sh\n> @@ -52,6 +52,35 @@ test_expect_success 'run --task=<task>' '\n>  \ttest_subcommand git commit-graph write --split --reachable --no-progress <run-both.txt\n>  '\n>  \n> +test_expect_success 'commit-graph auto condition' '\n> +\tCOMMAND=\"maintenance run --task=commit-graph --auto --quiet\" &&\n> +\n> +\tGIT_TRACE2_EVENT=\"$(pwd)/cg-no.txt\" \\\n> +\t\tgit -c maintenance.commit-graph.auto=1 $COMMAND &&\n> +\tGIT_TRACE2_EVENT=\"$(pwd)/cg-negative-means-yes.txt\" \\\n> +\t\tgit -c maintenance.commit-graph.auto=\"-1\" $COMMAND &&\n> +\n> +\ttest_commit one &&\n> +\n> +\tGIT_TRACE2_EVENT=\"$(pwd)/cg-zero-means-no.txt\" \\\n> +\t\tgit -c maintenance.commit-graph.auto=0 $COMMAND &&\n> +\tGIT_TRACE2_EVENT=\"$(pwd)/cg-one-satisfied.txt\" \\\n> +\t\tgit -c maintenance.commit-graph.auto=1 $COMMAND &&\n> +\n> +\tgit commit --allow-empty -m \"two\" &&\n> +\tgit commit --allow-empty -m \"three\" &&\n> +\n> +\tGIT_TRACE2_EVENT=\"$(pwd)/cg-two-satisfied.txt\" \\\n> +\t\tgit -c maintenance.commit-graph.auto=2 $COMMAND &&\n> +\n> +\tCOMMIT_GRAPH_WRITE=\"git commit-graph write --split --reachable --no-progress\" &&\n> +\ttest_subcommand ! $COMMIT_GRAPH_WRITE <cg-no.txt &&\n> +\ttest_subcommand $COMMIT_GRAPH_WRITE <cg-negative-means-yes.txt &&\n> +\ttest_subcommand ! $COMMIT_GRAPH_WRITE <cg-zero-means-no.txt &&\n> +\ttest_subcommand $COMMIT_GRAPH_WRITE <cg-one-satisfied.txt &&\n> +\ttest_subcommand $COMMIT_GRAPH_WRITE <cg-two-satisfied.txt\n> +'\n> +\n>  test_expect_success 'run --task=bogus' '\n>  \ttest_must_fail git maintenance run --task=bogus 2>err &&\n>  \ttest_i18ngrep \"is not a valid task\" err\n>\n> base-commit: 25914c4fdeefd99b06e134496dfb9bbb58a5c417\n"},{"id":"407112","messageId":"b4918483-5ce6-b002-77dc-92d3049f2069@gmail.com","threadId":"54365","inReplyTo":"xmqqblhd23dc.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] maintenance: test commit-graph auto condition","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-10-08T00:13:32Z","receivedAt":"2020-10-08T00:13:38Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 10/7/2020 4:22 PM, Junio C Hamano wrote:\n> Hmph.  Something in ds/maintenance-part-2 series does not seem to\n> work well with this change.  I noticed it when testing today's first\n> integration cycle of jch and seen branches, but even when applied to\n> ds/maintenance-part-2 alone without any other topics in seen, it\n> seems to break in an unexpected place.\n\nThank you for alerting me to this. I should have ran the tests after\nmerging with the other branches in progress.\n\nIt seems my issue is that I use \"test_commit one\" in two places now,\nso the second complains that nothing is new to commit.\n\nThe simplest fix is to use \"first\", \"second\", and \"third\" in this\npatch, so I'll send a v2 soon with that change.\n\nThanks,\n-Stolee\n\n"},{"id":"407113","messageId":"pull.746.v2.git.1602118239782.gitgitgadget@gmail.com","threadId":"54365","inReplyTo":"pull.746.git.1602075317625.gitgitgadget@gmail.com","subject":"[PATCH v2] maintenance: test commit-graph auto condition","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-10-08T00:50:39Z","receivedAt":"2020-10-08T00:50:44Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <dstolee@microsoft.com>\n\nThe auto condition for the commit-graph maintenance task walks refs\nlooking for commits that are not in the commit-graph file. This was\nadded in 4ddc79b2 (maintenance: add auto condition for commit-graph\ntask, 2020-09-17) but was left untested.\n\nThe initial goal of this change was to demonstrate the feature works\nproperly by adding tests. However, there was an off-by-one error that\ncaused the basic tests around maintenance.commit-graph.auto=1 to fail\nwhen it should work.\n\nThe subtlety is that if a ref tip is not in the commit-graph, then we\nwere not adding that to the total count. In the test, we see that we\nhave only added one commit since our last commit-graph write, so the\nauto condition would say there is nothing to do.\n\nThe fix is simple: add the check for the commit-graph position to see\nthat the tip is not in the commit-graph file before starting our walk.\nSince this happens before adding to the DFS stack, we do not need to\nclear our (currently empty) commit list.\n\nThis does add some extra complexity for the test, because we also want\nto verify that the walk along the parents actually does some work. This\nmeans we need to add at least two commits in a row without writing the\ncommit-graph. However, we also need to make sure no additional refs are\npointing to the middle of this list or else the for_each_ref() in\nshould_write_commit_graph() might visit these commits as tips instead of\ndoing a DFS walk. Hence, the last two commits are added with \"git\ncommit\" instead of \"test_commit\".\n\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n    maintenance: test commit-graph auto condition\n    \n    As promised [1], here is a test to check that\n    maintenance.commit-graph.auto behaves correctly. In the process, I found\n    a small off-by-one error that is not super-critical, but worth fixing.\n    \n    This is based on ds/maintenance-part-1, but my GGG pull request targets\n    ds/maintenance-part-3 to make sure it merges cleanly and the result\n    builds & tests cleanly.\n    \n    Thanks, -Stolee\n    \n    [1] \n    https://lore.kernel.org/git/cfc8a8e9-f812-2cb1-f6d8-57ef585346d1@gmail.com/\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-746%2Fderrickstolee%2Fmaintenance%2Fcg-auto-test-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-746/derrickstolee/maintenance/cg-auto-test-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/746\n\nRange-diff vs v1:\n\n 1:  39223f21fb ! 1:  984ee2d9c0 maintenance: test commit-graph auto condition\n     @@ t/t7900-maintenance.sh: test_expect_success 'run --task=<task>' '\n      +\tGIT_TRACE2_EVENT=\"$(pwd)/cg-negative-means-yes.txt\" \\\n      +\t\tgit -c maintenance.commit-graph.auto=\"-1\" $COMMAND &&\n      +\n     -+\ttest_commit one &&\n     ++\ttest_commit first &&\n      +\n      +\tGIT_TRACE2_EVENT=\"$(pwd)/cg-zero-means-no.txt\" \\\n      +\t\tgit -c maintenance.commit-graph.auto=0 $COMMAND &&\n      +\tGIT_TRACE2_EVENT=\"$(pwd)/cg-one-satisfied.txt\" \\\n      +\t\tgit -c maintenance.commit-graph.auto=1 $COMMAND &&\n      +\n     -+\tgit commit --allow-empty -m \"two\" &&\n     -+\tgit commit --allow-empty -m \"three\" &&\n     ++\tgit commit --allow-empty -m \"second\" &&\n     ++\tgit commit --allow-empty -m \"third\" &&\n      +\n      +\tGIT_TRACE2_EVENT=\"$(pwd)/cg-two-satisfied.txt\" \\\n      +\t\tgit -c maintenance.commit-graph.auto=2 $COMMAND &&\n\n\n builtin/gc.c           |  8 +++++++-\n t/t7900-maintenance.sh | 29 +++++++++++++++++++++++++++++\n 2 files changed, 36 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 090959350e..12ddb68bba 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -737,9 +737,15 @@ static int dfs_on_ref(const char *refname,\n \tcommit = lookup_commit(the_repository, oid);\n \tif (!commit)\n \t\treturn 0;\n-\tif (parse_commit(commit))\n+\tif (parse_commit(commit) ||\n+\t    commit_graph_position(commit) != COMMIT_NOT_FROM_GRAPH)\n \t\treturn 0;\n \n+\tdata->num_not_in_graph++;\n+\n+\tif (data->num_not_in_graph >= data->limit)\n+\t\treturn 1;\n+\n \tcommit_list_append(commit, &stack);\n \n \twhile (!result && stack) {\ndiff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\nindex 53c883531e..ee1f4a7ae4 100755\n--- a/t/t7900-maintenance.sh\n+++ b/t/t7900-maintenance.sh\n@@ -52,6 +52,35 @@ test_expect_success 'run --task=<task>' '\n \ttest_subcommand git commit-graph write --split --reachable --no-progress <run-both.txt\n '\n \n+test_expect_success 'commit-graph auto condition' '\n+\tCOMMAND=\"maintenance run --task=commit-graph --auto --quiet\" &&\n+\n+\tGIT_TRACE2_EVENT=\"$(pwd)/cg-no.txt\" \\\n+\t\tgit -c maintenance.commit-graph.auto=1 $COMMAND &&\n+\tGIT_TRACE2_EVENT=\"$(pwd)/cg-negative-means-yes.txt\" \\\n+\t\tgit -c maintenance.commit-graph.auto=\"-1\" $COMMAND &&\n+\n+\ttest_commit first &&\n+\n+\tGIT_TRACE2_EVENT=\"$(pwd)/cg-zero-means-no.txt\" \\\n+\t\tgit -c maintenance.commit-graph.auto=0 $COMMAND &&\n+\tGIT_TRACE2_EVENT=\"$(pwd)/cg-one-satisfied.txt\" \\\n+\t\tgit -c maintenance.commit-graph.auto=1 $COMMAND &&\n+\n+\tgit commit --allow-empty -m \"second\" &&\n+\tgit commit --allow-empty -m \"third\" &&\n+\n+\tGIT_TRACE2_EVENT=\"$(pwd)/cg-two-satisfied.txt\" \\\n+\t\tgit -c maintenance.commit-graph.auto=2 $COMMAND &&\n+\n+\tCOMMIT_GRAPH_WRITE=\"git commit-graph write --split --reachable --no-progress\" &&\n+\ttest_subcommand ! $COMMIT_GRAPH_WRITE <cg-no.txt &&\n+\ttest_subcommand $COMMIT_GRAPH_WRITE <cg-negative-means-yes.txt &&\n+\ttest_subcommand ! $COMMIT_GRAPH_WRITE <cg-zero-means-no.txt &&\n+\ttest_subcommand $COMMIT_GRAPH_WRITE <cg-one-satisfied.txt &&\n+\ttest_subcommand $COMMIT_GRAPH_WRITE <cg-two-satisfied.txt\n+'\n+\n test_expect_success 'run --task=bogus' '\n \ttest_must_fail git maintenance run --task=bogus 2>err &&\n \ttest_i18ngrep \"is not a valid task\" err\n\nbase-commit: 25914c4fdeefd99b06e134496dfb9bbb58a5c417\n-- \ngitgitgadget\n"}]}