{"thread":{"id":"51591","subject":"[PATCH 0/1] [v2.23.0 BUG] commit-graph: fix bug around octopus merges","startedAt":"2019-08-05T16:43:45Z","lastAt":"2019-08-06T15:19:56Z","messageCount":5,"participants":["Derrick Stolee via GitGitGadget","SZEDER Gábor","Derrick Stolee"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"379940","messageId":"pull.308.git.gitgitgadget@gmail.com","threadId":"51591","inReplyTo":null,"subject":"[PATCH 0/1] [v2.23.0 BUG] commit-graph: fix bug around octopus merges","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-08-05T16:43:40Z","receivedAt":"2019-08-05T16:43:45Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"I come, hat in hand, with a bug-fix for a bug that I wrote. I just happened\nto discover this issue while playing around with turning the commit-graph on\nby default and stumbled into this warning.\n\nPlease see the commit message for full details, but putting octopus merges\nin a tip commit-graph file in the incremental format [1] would cause the\nfile to not load because Git thinks the base commit-graphs do not have the\nright hashes.\n\nIt is very likely that a more careful patch would allow this warning to\nbecome an error during git commit-graph verify, but I wanted to minimize the\nchange for this quick bug fix.\n\nThanks, -Stolee\n\n[1] https://public-inbox.org/git/pull.184.v6.git.gitgitgadget@gmail.com/\n\nDerrick Stolee (1):\n  commit-graph: fix bug around octopus merges\n\n commit-graph.c                | 2 +-\n t/t5324-split-commit-graph.sh | 4 +++-\n 2 files changed, 4 insertions(+), 2 deletions(-)\n\n\nbase-commit: 5b15eb397d176c557a82e872d6b4043aa7589874\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-308%2Fderrickstolee%2Fcommit-graph-octopus-bug-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-308/derrickstolee/commit-graph-octopus-bug-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/308\n-- \ngitgitgadget\n"},{"id":"379941","messageId":"6e913ac2b6ddc080c7796e93f019243bbdce1dff.1565023420.git.gitgitgadget@gmail.com","threadId":"51591","inReplyTo":"pull.308.git.gitgitgadget@gmail.com","subject":"[PATCH 1/1] commit-graph: fix bug around octopus merges","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-08-05T16:43:41Z","receivedAt":"2019-08-05T16:43:47Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <dstolee@microsoft.com>\n\nIn 1771be90 \"commit-graph: merge commit-graph chains\" (2019-06-18),\nthe method sort_and_scan_merged_commits() was added to merge the\ncommit lists of two commit-graph files in the incremental format.\nUnfortunately, there was an off-by-one error in that method around\nincrementing num_extra_edges, which leads to an incorrect offset\nfor the base graph chunk.\n\nWhen we store an octopus merge in the commit-graph file, we store\nthe first parent in the normal place, but use the second parent\nposition to point into the \"extra edges\" chunk where the remaining\nparents exist. This means we should be adding \"num_parents - 1\"\nedges to this list, not \"num_parents - 2\". That is the basic error.\n\nThe reason this was not caught in the test suite is more subtle.\nIn 5324-split-commit-graph.sh, we test creating an octopus merge\nand adding it to the tip of a commit-graph chain, then verify the\nresult. This _should_ have caught the problem, except that when\nwe load the commit-graph files we were overly careful to not fail\nwhen the commit-graph chain does not match. This care was on\npurpose to avoid race conditions as one process reads the chain\nand another process modifies it. In such a case, the reading\nprocess outputs the following message to stderr:\n\n\twarning: commit-graph chain does not match\n\nThese warnings are output in the test suite, but ignored. By\nchecking the stderr of `git commit-graph verify` to include\nthe expected progress output, it will now catch this error.\n\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n commit-graph.c                | 2 +-\n t/t5324-split-commit-graph.sh | 4 +++-\n 2 files changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex c91e6f0fb8..1aaf02879b 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -1629,7 +1629,7 @@ static void sort_and_scan_merged_commits(struct write_commit_graph_context *ctx)\n \t\t\t\tnum_parents++;\n \n \t\t\tif (num_parents > 2)\n-\t\t\t\tctx->num_extra_edges += num_parents - 2;\n+\t\t\t\tctx->num_extra_edges += num_parents - 1;\n \t\t}\n \t}\n \ndiff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh\nindex 03f45a1ed9..99f4ef4c19 100755\n--- a/t/t5324-split-commit-graph.sh\n+++ b/t/t5324-split-commit-graph.sh\n@@ -319,7 +319,9 @@ test_expect_success 'add octopus merge' '\n \tgit merge commits/3 commits/4 &&\n \tgit branch merge/octopus &&\n \tgit commit-graph write --reachable --split &&\n-\tgit commit-graph verify &&\n+\tgit commit-graph verify 2>err &&\n+\ttest_line_count = 3 err &&\n+\ttest_i18ngrep ! warning err &&\n \ttest_line_count = 3 $graphdir/commit-graph-chain\n '\n \n-- \ngitgitgadget\n"},{"id":"379988","messageId":"20190806092159.GE20404@szeder.dev","threadId":"51591","inReplyTo":"6e913ac2b6ddc080c7796e93f019243bbdce1dff.1565023420.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/1] commit-graph: fix bug around octopus merges","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-08-06T09:21:59Z","receivedAt":"2019-08-06T09:22:07Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Mon, Aug 05, 2019 at 09:43:41AM -0700, Derrick Stolee via GitGitGadget wrote:\n\n> In such a case, the reading\n> process outputs the following message to stderr:\n> \n> \twarning: commit-graph chain does not match\n> \n> These warnings are output in the test suite, but ignored. By\n> checking the stderr of `git commit-graph verify` to include\n> the expected progress output, it will now catch this error.\n\nThere should be a better way to check this than relying on the number\nof progress lines, e.g. 'git commit-graph verify' could print\nsomething specific, like \"Verified 3 commit-graph files, all OK\", to\nits standard output, and the test should check only that.  Or check\nonly that there is no warning on standard error.\n\nOn one hand, see the recent commits 077b979891 (t3404: make the\n'rebase.missingCommitsCheck=ignore' test more focused, 2019-06-24) and\nbb431c3dad (t3420: remove progress lines before comparing output,\n2019-07-04), where we started to exclude the progress output from\nverification in tests like this that are not specifically about\nprogress output.\n\nOn the other hand, one of my fun side-projects is to run some\nsemi-automated and souped-up CI builds of currently cooking topics,\nwhich include a custom GETTEXT_POISON mode [1], where the poisoned\ntranslated progress output in this new test looks like this [2]:\n\n  V.e.r.i.f.y.i.n.g. .c.o.m.m.i.t.s. .i.n. .c.o.m.m.i.t. .g.r.a.p.h.: 100% (1/1),  d.o.n.e..\n  V.e.r.i.f.y.i.n.g. .c.o.m.m.i.t.s. .i.n. .c.o.m.m.i.t. .g.r.a.p.h.: 100% (5/5),  d.o.n.e..\n  V.e.r.i.f.y.i.n.g. .c.o.m.m.i.t.s. .i.n. .c.o.m.m.i.t. .g.r.a.p.h.:\n    100% (12/12),  d.o.n.e..\n\nSo this makes the progress lines almost twice as long, and as the last\ncommit-graph file contains double-digit commits, the length of the\nprogress line just happens to exceed the width of a 80 character\nterminal, and gets line wrapped.  This throws off the line count\ncheck, as there are now 4 lines on standard error instead of the\nexpected 3.\n\n\n[1] https://public-inbox.org/git/20181022202241.18629-1-szeder.dev@gmail.com/\n[2] https://travis-ci.org/szeder/git-cooking-topics-for-travis-ci/jobs/568120252#L4120\n    Note that the highlighted line is in the 'after failure' fold, and\n    your browser might unhelpfully fold it up before you could take a\n    good look.\n\n\n> diff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh\n> index 03f45a1ed9..99f4ef4c19 100755\n> --- a/t/t5324-split-commit-graph.sh\n> +++ b/t/t5324-split-commit-graph.sh\n> @@ -319,7 +319,9 @@ test_expect_success 'add octopus merge' '\n>  \tgit merge commits/3 commits/4 &&\n>  \tgit branch merge/octopus &&\n>  \tgit commit-graph write --reachable --split &&\n> -\tgit commit-graph verify &&\n> +\tgit commit-graph verify 2>err &&\n> +\ttest_line_count = 3 err &&\n> +\ttest_i18ngrep ! warning err &&\n>  \ttest_line_count = 3 $graphdir/commit-graph-chain\n>  '\n>  \n> -- \n> gitgitgadget\n"},{"id":"379989","messageId":"ec14865f-98cb-5e1a-b580-8b6fddaa6217@gmail.com","threadId":"51591","inReplyTo":"20190806092159.GE20404@szeder.dev","subject":"Re: [PATCH 1/1] commit-graph: fix bug around octopus merges","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-08-06T12:03:07Z","receivedAt":"2019-08-06T12:03:11Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/6/2019 5:21 AM, SZEDER Gábor wrote:\n> On Mon, Aug 05, 2019 at 09:43:41AM -0700, Derrick Stolee via GitGitGadget wrote:\n> \n>> In such a case, the reading\n>> process outputs the following message to stderr:\n>>\n>> \twarning: commit-graph chain does not match\n>>\n>> These warnings are output in the test suite, but ignored. By\n>> checking the stderr of `git commit-graph verify` to include\n>> the expected progress output, it will now catch this error.\n> \n> There should be a better way to check this than relying on the number\n> of progress lines, e.g. 'git commit-graph verify' could print\n> something specific, like \"Verified 3 commit-graph files, all OK\", to\n> its standard output, and the test should check only that.  Or check\n> only that there is no warning on standard error.\n> \n> On one hand, see the recent commits 077b979891 (t3404: make the\n> 'rebase.missingCommitsCheck=ignore' test more focused, 2019-06-24) and\n> bb431c3dad (t3420: remove progress lines before comparing output,\n> 2019-07-04), where we started to exclude the progress output from\n> verification in tests like this that are not specifically about\n> progress output.\n> \n> On the other hand, one of my fun side-projects is to run some\n> semi-automated and souped-up CI builds of currently cooking topics,\n> which include a custom GETTEXT_POISON mode [1], where the poisoned\n> translated progress output in this new test looks like this [2]:\n> \n>   V.e.r.i.f.y.i.n.g. .c.o.m.m.i.t.s. .i.n. .c.o.m.m.i.t. .g.r.a.p.h.: 100% (1/1),  d.o.n.e..\n>   V.e.r.i.f.y.i.n.g. .c.o.m.m.i.t.s. .i.n. .c.o.m.m.i.t. .g.r.a.p.h.: 100% (5/5),  d.o.n.e..\n>   V.e.r.i.f.y.i.n.g. .c.o.m.m.i.t.s. .i.n. .c.o.m.m.i.t. .g.r.a.p.h.:\n>     100% (12/12),  d.o.n.e..\n> \n> So this makes the progress lines almost twice as long, and as the last\n> commit-graph file contains double-digit commits, the length of the\n> progress line just happens to exceed the width of a 80 character\n> terminal, and gets line wrapped.  This throws off the line count\n> check, as there are now 4 lines on standard error instead of the\n> expected 3.\n\nThanks for pointing out that the poison runs can change the number of lines.\nMy intention was to ensure \"we got the amount of output we expected\" but it\nis enough to only use test_i18ngrep to check that no warnings occurred.\n\nThanks,\n-Stolee\n"},{"id":"380004","messageId":"20190806151949.GF20404@szeder.dev","threadId":"51591","inReplyTo":"ec14865f-98cb-5e1a-b580-8b6fddaa6217@gmail.com","subject":"Re: [PATCH 1/1] commit-graph: fix bug around octopus merges","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-08-06T15:19:49Z","receivedAt":"2019-08-06T15:19:56Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Tue, Aug 06, 2019 at 08:03:07AM -0400, Derrick Stolee wrote:\n> > On the other hand, one of my fun side-projects is to run some\n> > semi-automated and souped-up CI builds of currently cooking topics,\n> > which include a custom GETTEXT_POISON mode [1], where the poisoned\n> > translated progress output in this new test looks like this [2]:\n> > \n> >   V.e.r.i.f.y.i.n.g. .c.o.m.m.i.t.s. .i.n. .c.o.m.m.i.t. .g.r.a.p.h.: 100% (1/1),  d.o.n.e..\n> >   V.e.r.i.f.y.i.n.g. .c.o.m.m.i.t.s. .i.n. .c.o.m.m.i.t. .g.r.a.p.h.: 100% (5/5),  d.o.n.e..\n> >   V.e.r.i.f.y.i.n.g. .c.o.m.m.i.t.s. .i.n. .c.o.m.m.i.t. .g.r.a.p.h.:\n> >     100% (12/12),  d.o.n.e..\n> > \n> > So this makes the progress lines almost twice as long, and as the last\n> > commit-graph file contains double-digit commits, the length of the\n> > progress line just happens to exceed the width of a 80 character\n> > terminal, and gets line wrapped.  This throws off the line count\n> > check, as there are now 4 lines on standard error instead of the\n> > expected 3.\n> \n> Thanks for pointing out that the poison runs can change the number of lines.\n\nHm, I'm afraid I was not clear enough, so let me clarify.\n\nA \"regular\" GIT_TEST_GETTEXT_POISON=1 test run does not change the\nnumber of lines.  After it replaces the translated string with the '#\nGETTEXT POISON #' magic, the progress output will look like this:\n\n  # GETTEXT POISON #:  41% (5/12)\n\nThis will never grow long enough to trigger a line break.\n\nIt's just one of my fun experiments [1] that makes translated strings\ntwice as long, and thus triggers that line break.\n\n\n[1] https://github.com/szeder/git gettext-poison-scrambled-msg\n\n\n"}]}