{"thread":{"id":"56713","subject":"[PATCH 0/3] commit-graph: make \"verify\" work with replace refs","startedAt":"2021-10-14T23:37:21Z","lastAt":"2021-10-15T16:18:52Z","messageCount":7,"participants":["Ævar Arnfjörð Bjarmason","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"438797","messageId":"cover-0.3-00000000000-20211014T233343Z-avarab@gmail.com","threadId":"56713","inReplyTo":null,"subject":"[PATCH 0/3] commit-graph: make \"verify\" work with replace refs","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-14T23:37:13Z","receivedAt":"2021-10-14T23:37:21Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"This changes \"commit-graph verify\" to ignore replace refs, as\n\"commit-graph write\" already does.\n\nThe 3/3 is a rather simple fix, and unblocks another series that wants\nto fix the GIT_TEST_COMMIT_GRAPH mode.\n\n1-2/3 are fixes to existing commit-graph tests. I just wanted to use\nthe \"graph_git_two_modes\" helper, but it was broken, including in a\nway that hid an existing failure in a years-old test.\n\nGlen: I've tested this with your v4 series with that\n\"GIT_TEST_COMMIT_GRAPH=0\" part of the mktag tests removed, and they\npass on top of this with GIT_TEST_COMMIT_GRAPH=true.\n\nÆvar Arnfjörð Bjarmason (3):\n commit-graph tests: fix error-hiding graph_git_two_modes() helper\n commit-graph tests: fix another graph_git_two_modes() helper\n commit-graph: don't consider \"replace\" objects with \"verify\"\n\n builtin/commit-graph.c | 2 +-\n t/t5318-commit-graph.sh | 5 +++--\n t/t5324-split-commit-graph.sh | 20 ++++++++++++--------\n 3 files changed, 16 insertions(+), 11 deletions(-)\n\n-- \n2.33.1.1338.g20da966911a\n\n"},{"id":"438798","messageId":"patch-1.3-f74cc32ceea-20211014T233343Z-avarab@gmail.com","threadId":"56713","inReplyTo":"cover-0.3-00000000000-20211014T233343Z-avarab@gmail.com","subject":"[PATCH 1/3] commit-graph tests: fix error-hiding graph_git_two_modes() helper","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-14T23:37:14Z","receivedAt":"2021-10-14T23:37:23Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"The graph_git_two_modes() helper added in 177722b3442 (commit:\nintegrate commit graph with commit parsing, 2018-04-10) didn't\n&&-chain its \"git commit-graph\" invocations, which as can be seen with\nSANITIZE=leak will happily mark tests as passing if both of these\ncommands die, since test_cmp() will be comparing two empty files.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t5318-commit-graph.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex 295c5bd94d2..88fbe004a38 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -70,8 +70,8 @@ test_expect_success 'create commits and repack' '\n '\n \n graph_git_two_modes() {\n-\tgit -c core.commitGraph=true $1 >output\n-\tgit -c core.commitGraph=false $1 >expect\n+\tgit -c core.commitGraph=true $1 >output &&\n+\tgit -c core.commitGraph=false $1 >expect &&\n \ttest_cmp expect output\n }\n \n-- \n2.33.1.1338.g20da966911a\n\n"},{"id":"438799","messageId":"patch-2.3-d6934031882-20211014T233343Z-avarab@gmail.com","threadId":"56713","inReplyTo":"cover-0.3-00000000000-20211014T233343Z-avarab@gmail.com","subject":"[PATCH 2/3] commit-graph tests: fix another graph_git_two_modes() helper","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-14T23:37:15Z","receivedAt":"2021-10-14T23:37:25Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"In 135a7123755 (commit-graph: add --split option to builtin,\n2019-06-18) this function was copy/pasted to the split commit-graph\ntests, as in the preceding commit we need to fix this to use\n&&-chaining, so it won't be hiding errors.\n\nUnlike its sister function in \"t5318-commit-graph.sh\", which we got\nlucky with, this one was hiding a real test failure. A tests added in\nc523035cbd8 (commit-graph: allow cross-alternate chains, 2019-06-18)\nhas never worked as intended. Unlike most other graph_git_behavior\nuses in this file it clones the repository into a sub-directory, so\nwe'll need to refer to \"commits/6\" as \"origin/commits/6\".\n\nIt's not easy to simply move the \"graph_git_behavior\" to the test\nabove it, since it itself spawns a \"test_expect_success\". Let's\ninstead add support to \"graph_git_behavior()\" and\n\"graph_git_two_modes()\" to pass a \"-C\" argument to git.\n\nWe also need to add a \"test -d fork\" here, because otherwise we'll\nfail on e.g.:\n\n GIT_SKIP_TESTS=t5324.13 ./t5324-split-commit-graph.sh\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t5324-split-commit-graph.sh | 20 ++++++++++++--------\n 1 file changed, 12 insertions(+), 8 deletions(-)\n\ndiff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh\nindex 587226ed103..847b8097109 100755\n--- a/t/t5324-split-commit-graph.sh\n+++ b/t/t5324-split-commit-graph.sh\n@@ -55,8 +55,8 @@ test_expect_success 'create commits and write commit-graph' '\n '\n \n graph_git_two_modes() {\n-\tgit -c core.commitGraph=true $1 >output\n-\tgit -c core.commitGraph=false $1 >expect\n+\tgit ${2:+ -C \"$2\"} -c core.commitGraph=true $1 >output &&\n+\tgit ${2:+ -C \"$2\"} -c core.commitGraph=false $1 >expect &&\n \ttest_cmp expect output\n }\n \n@@ -64,12 +64,13 @@ graph_git_behavior() {\n \tMSG=$1\n \tBRANCH=$2\n \tCOMPARE=$3\n+\tDIR=$4\n \ttest_expect_success \"check normal git operations: $MSG\" '\n-\t\tgraph_git_two_modes \"log --oneline $BRANCH\" &&\n-\t\tgraph_git_two_modes \"log --topo-order $BRANCH\" &&\n-\t\tgraph_git_two_modes \"log --graph $COMPARE..$BRANCH\" &&\n-\t\tgraph_git_two_modes \"branch -vv\" &&\n-\t\tgraph_git_two_modes \"merge-base -a $BRANCH $COMPARE\"\n+\t\tgraph_git_two_modes \"log --oneline $BRANCH\" \"$DIR\" &&\n+\t\tgraph_git_two_modes \"log --topo-order $BRANCH\" \"$DIR\" &&\n+\t\tgraph_git_two_modes \"log --graph $COMPARE..$BRANCH\" \"$DIR\" &&\n+\t\tgraph_git_two_modes \"branch -vv\" \"$DIR\" &&\n+\t\tgraph_git_two_modes \"merge-base -a $BRANCH $COMPARE\" \"$DIR\"\n \t'\n }\n \n@@ -187,7 +188,10 @@ test_expect_success 'create fork and chain across alternate' '\n \t)\n '\n \n-graph_git_behavior 'alternate: commit 13 vs 6' commits/13 commits/6\n+if test -d fork\n+then\n+\tgraph_git_behavior 'alternate: commit 13 vs 6' commits/13 origin/commits/6 \"fork\"\n+fi\n \n test_expect_success 'test merge stragety constants' '\n \tgit clone . merge-2 &&\n-- \n2.33.1.1338.g20da966911a\n\n"},{"id":"438800","messageId":"patch-3.3-56bb70518cb-20211014T233343Z-avarab@gmail.com","threadId":"56713","inReplyTo":"cover-0.3-00000000000-20211014T233343Z-avarab@gmail.com","subject":"[PATCH 3/3] commit-graph: don't consider \"replace\" objects with \"verify\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-14T23:37:16Z","receivedAt":"2021-10-14T23:37:25Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Extend the code added in d6538246d3d (commit-graph: not compatible\nwith replace objects, 2018-08-20) which ignored replace objects in the\n\"write\" command to ignore it in the \"verify\" command too.\n\nWe can just move this assignment to the cmd_commit_graph(), it\ndispatches to \"write\" and \"verify\", and we're unlikely to ever get a\nsub-command that would like to consider replace refs.\n\nThis will make tests added in eddc1f556cd (mktag tests: test\nupdate-ref and reachable fsck, 2021-06-17) pass in combination with\nthe \"GIT_TEST_COMMIT_GRAPH\" mode added in 859fdc0c3cf (commit-graph:\ndefine GIT_TEST_COMMIT_GRAPH, 2018-08-29), except that mode is\ncurrently broken (but is being fixed concurrently). See the discussion\nstarting at [1].\n\n1. https://lore.kernel.org/git/87wnmihswp.fsf@evledraar.gmail.com/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/commit-graph.c | 2 +-\n t/t5318-commit-graph.sh | 1 +\n 2 files changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex 3c3de3a156f..fb8e166a26f 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -263,7 +263,6 @@ static int graph_write(int argc, const char **argv)\n \t git_env_bool(GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS, 0))\n \t\tflags |= COMMIT_GRAPH_WRITE_BLOOM_FILTERS;\n \n-\tread_replace_refs = 0;\n \todb = find_odb(the_repository, opts.obj_dir);\n \n \tif (opts.reachable) {\n@@ -318,6 +317,7 @@ int cmd_commit_graph(int argc, const char **argv, const char *prefix)\n \tif (!argc)\n \t\tgoto usage;\n \n+\tread_replace_refs = 0;\n \tsave_commit_buffer = 0;\n \n \tif (!strcmp(argv[0], \"verify\"))\ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex 88fbe004a38..84d122a7ae7 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -385,6 +385,7 @@ test_expect_success 'replace-objects invalidates commit-graph' '\n \t\tgit commit-graph write --reachable &&\n \t\ttest_path_is_file .git/objects/info/commit-graph &&\n \t\tgit replace HEAD~1 HEAD~2 &&\n+\t\tgraph_git_two_modes \"commit-graph verify\" &&\n \t\tgit -c core.commitGraph=false log >expect &&\n \t\tgit -c core.commitGraph=true log >actual &&\n \t\ttest_cmp expect actual &&\n-- \n2.33.1.1338.g20da966911a\n\n"},{"id":"438858","messageId":"xmqq1r4m9sxt.fsf@gitster.g","threadId":"56713","inReplyTo":"patch-1.3-f74cc32ceea-20211014T233343Z-avarab@gmail.com","subject":"Re: [PATCH 1/3] commit-graph tests: fix error-hiding graph_git_two_modes() helper","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-15T16:03:10Z","receivedAt":"2021-10-15T16:03:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> The graph_git_two_modes() helper added in 177722b3442 (commit:\n> integrate commit graph with commit parsing, 2018-04-10) didn't\n> &&-chain its \"git commit-graph\" invocations, which as can be seen with\n> SANITIZE=leak will happily mark tests as passing if both of these\n> commands die, since test_cmp() will be comparing two empty files.\n\nAs the chains the four invocation of this helper with &&- correctly,\nthis does make a difference. Nicely spotted.\n\n\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> t/t5318-commit-graph.sh | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\n> index 295c5bd94d2..88fbe004a38 100755\n> --- a/t/t5318-commit-graph.sh\n> +++ b/t/t5318-commit-graph.sh\n> @@ -70,8 +70,8 @@ test_expect_success 'create commits and repack' '\n> '\n> \n> graph_git_two_modes() {\n> -\tgit -c core.commitGraph=true $1 >output\n> -\tgit -c core.commitGraph=false $1 >expect\n> +\tgit -c core.commitGraph=true $1 >output &&\n> +\tgit -c core.commitGraph=false $1 >expect &&\n> \ttest_cmp expect output\n> }\n"},{"id":"438863","messageId":"xmqqwnme8dsp.fsf@gitster.g","threadId":"56713","inReplyTo":"patch-2.3-d6934031882-20211014T233343Z-avarab@gmail.com","subject":"Re: [PATCH 2/3] commit-graph tests: fix another graph_git_two_modes() helper","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-15T16:15:34Z","receivedAt":"2021-10-15T16:15:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> In 135a7123755 (commit-graph: add --split option to builtin,\n> 2019-06-18) this function was copy/pasted to the split commit-graph\n> tests, as in the preceding commit we need to fix this to use\n> &&-chaining, so it won't be hiding errors.\n>\n> Unlike its sister function in \"t5318-commit-graph.sh\", which we got\n> lucky with, this one was hiding a real test failure. A tests added in\n> c523035cbd8 (commit-graph: allow cross-alternate chains, 2019-06-18)\n> has never worked as intended. Unlike most other graph_git_behavior\n> uses in this file it clones the repository into a sub-directory, so\n> we'll need to refer to \"commits/6\" as \"origin/commits/6\".\n\nInteresting. The original created \"fork\" prepared the alternates\nstructure in the preceding test, but tested the \"behavour\" of the\ncommands outside the \"fork\" it just prepared?\n\n> graph_git_two_modes() {\n> -\tgit -c core.commitGraph=true $1 >output\n> -\tgit -c core.commitGraph=false $1 >expect\n> +\tgit ${2:+ -C \"$2\"} -c core.commitGraph=true $1 >output &&\n> +\tgit ${2:+ -C \"$2\"} -c core.commitGraph=false $1 >expect &&\n\nOK, it was a bit curious to see :+ (instead of just +), but the\ncaller unconditionally passes \"$DIR\" (with double quotes), so it is\nunderstandable. Much more concise than having the caller to repeat\n${4+\"$4\"} where it says \"$DIR\".\n\n> \ttest_cmp expect output\n> }\n> \n> @@ -64,12 +64,13 @@ graph_git_behavior() {\n> \tMSG=$1\n> \tBRANCH=$2\n> \tCOMPARE=$3\n> +\tDIR=$4\n> \ttest_expect_success \"check normal git operations: $MSG\" '\n> -\t\tgraph_git_two_modes \"log --oneline $BRANCH\" &&\n> -\t\tgraph_git_two_modes \"log --topo-order $BRANCH\" &&\n> -\t\tgraph_git_two_modes \"log --graph $COMPARE..$BRANCH\" &&\n> -\t\tgraph_git_two_modes \"branch -vv\" &&\n> -\t\tgraph_git_two_modes \"merge-base -a $BRANCH $COMPARE\"\n> +\t\tgraph_git_two_modes \"log --oneline $BRANCH\" \"$DIR\" &&\n> +\t\tgraph_git_two_modes \"log --topo-order $BRANCH\" \"$DIR\" &&\n> +\t\tgraph_git_two_modes \"log --graph $COMPARE..$BRANCH\" \"$DIR\" &&\n> +\t\tgraph_git_two_modes \"branch -vv\" \"$DIR\" &&\n> +\t\tgraph_git_two_modes \"merge-base -a $BRANCH $COMPARE\" \"$DIR\"\n> \t'\n> }\n> \n> @@ -187,7 +188,10 @@ test_expect_success 'create fork and chain across alternate' '\n> \t)\n> '\n> \n> -graph_git_behavior 'alternate: commit 13 vs 6' commits/13 commits/6\n> +if test -d fork\n> +then\n> +\tgraph_git_behavior 'alternate: commit 13 vs 6' commits/13 origin/commits/6 \"fork\"\n> +fi\n> \n> test_expect_success 'test merge stragety constants' '\n> \tgit clone . merge-2 &&\n"},{"id":"438864","messageId":"xmqqr1cm8dne.fsf@gitster.g","threadId":"56713","inReplyTo":"patch-3.3-56bb70518cb-20211014T233343Z-avarab@gmail.com","subject":"Re: [PATCH 3/3] commit-graph: don't consider \"replace\" objects with \"verify\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-15T16:18:45Z","receivedAt":"2021-10-15T16:18:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Extend the code added in d6538246d3d (commit-graph: not compatible\n> with replace objects, 2018-08-20) which ignored replace objects in the\n> \"write\" command to ignore it in the \"verify\" command too.\n>\n> We can just move this assignment to the cmd_commit_graph(), it\n> dispatches to \"write\" and \"verify\", and we're unlikely to ever get a\n> sub-command that would like to consider replace refs.\n>\n> This will make tests added in eddc1f556cd (mktag tests: test\n> update-ref and reachable fsck, 2021-06-17) pass in combination with\n> the \"GIT_TEST_COMMIT_GRAPH\" mode added in 859fdc0c3cf (commit-graph:\n> define GIT_TEST_COMMIT_GRAPH, 2018-08-29), except that mode is\n> currently broken (but is being fixed concurrently). See the discussion\n> starting at [1].\n>\n> 1. https://lore.kernel.org/git/87wnmihswp.fsf@evledraar.gmail.com/\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> builtin/commit-graph.c | 2 +-\n> t/t5318-commit-graph.sh | 1 +\n> 2 files changed, 2 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\n> index 3c3de3a156f..fb8e166a26f 100644\n> --- a/builtin/commit-graph.c\n> +++ b/builtin/commit-graph.c\n> @@ -263,7 +263,6 @@ static int graph_write(int argc, const char **argv)\n> \t git_env_bool(GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS, 0))\n> \t\tflags |= COMMIT_GRAPH_WRITE_BLOOM_FILTERS;\n> \n> -\tread_replace_refs = 0;\n> \todb = find_odb(the_repository, opts.obj_dir);\n> \n> \tif (opts.reachable) {\n> @@ -318,6 +317,7 @@ int cmd_commit_graph(int argc, const char **argv, const char *prefix)\n> \tif (!argc)\n> \t\tgoto usage;\n> \n> +\tread_replace_refs = 0;\n> \tsave_commit_buffer = 0;\n> \n> \tif (!strcmp(argv[0], \"verify\"))\n\nOK. The only question I have is if this deserves some kind of a\nwarning. If the user has replacement defined and makes an explicit\nrequest to work with the commit-graph, silently ignoring the request\ninstead of telling them why we are ignoring may lead to confusion.\n\n\tSide note. It is a \"question\", not \"objection\".\n\nOther than that, nicely done.\n\nThanks.\n\n> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\n> index 88fbe004a38..84d122a7ae7 100755\n> --- a/t/t5318-commit-graph.sh\n> +++ b/t/t5318-commit-graph.sh\n> @@ -385,6 +385,7 @@ test_expect_success 'replace-objects invalidates commit-graph' '\n> \t\tgit commit-graph write --reachable &&\n> \t\ttest_path_is_file .git/objects/info/commit-graph &&\n> \t\tgit replace HEAD~1 HEAD~2 &&\n> +\t\tgraph_git_two_modes \"commit-graph verify\" &&\n> \t\tgit -c core.commitGraph=false log >expect &&\n> \t\tgit -c core.commitGraph=true log >actual &&\n> \t\ttest_cmp expect actual &&\n"}]}