Skip to content

[v2.23.0 BUG] commit-graph: fix bug around octopus merges #308

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

[v2.23.0 BUG] commit-graph: fix bug around octopus merges #308

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion commit-graph.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1629,7 +1629,7 @@ static void sort_and_scan_merged_commits(struct write_commit_graph_context *ctx)
num_parents++;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, SZEDER Gábor wrote (reply to this):

On Mon, Aug 05, 2019 at 09:43:41AM -0700, Derrick Stolee via GitGitGadget wrote:

> In such a case, the reading
> process outputs the following message to stderr:
> 
> 	warning: commit-graph chain does not match
> 
> These warnings are output in the test suite, but ignored. By
> checking the stderr of `git commit-graph verify` to include
> the expected progress output, it will now catch this error.

There should be a better way to check this than relying on the number
of progress lines, e.g. 'git commit-graph verify' could print
something specific, like "Verified 3 commit-graph files, all OK", to
its standard output, and the test should check only that.  Or check
only that there is no warning on standard error.

On one hand, see the recent commits 077b979891 (t3404: make the
'rebase.missingCommitsCheck=ignore' test more focused, 2019-06-24) and
bb431c3dad (t3420: remove progress lines before comparing output,
2019-07-04), where we started to exclude the progress output from
verification in tests like this that are not specifically about
progress output.

On the other hand, one of my fun side-projects is to run some
semi-automated and souped-up CI builds of currently cooking topics,
which include a custom GETTEXT_POISON mode [1], where the poisoned
translated progress output in this new test looks like this [2]:

  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..
  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..
  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% (12/12),  d.o.n.e..

So this makes the progress lines almost twice as long, and as the last
commit-graph file contains double-digit commits, the length of the
progress line just happens to exceed the width of a 80 character
terminal, and gets line wrapped.  This throws off the line count
check, as there are now 4 lines on standard error instead of the
expected 3.


[1] https://public-inbox.org/git/[email protected]/
[2] https://travis-ci.org/szeder/git-cooking-topics-for-travis-ci/jobs/568120252#L4120
    Note that the highlighted line is in the 'after failure' fold, and
    your browser might unhelpfully fold it up before you could take a
    good look.


> diff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh
> index 03f45a1ed9..99f4ef4c19 100755
> --- a/t/t5324-split-commit-graph.sh
> +++ b/t/t5324-split-commit-graph.sh
> @@ -319,7 +319,9 @@ test_expect_success 'add octopus merge' '
>  	git merge commits/3 commits/4 &&
>  	git branch merge/octopus &&
>  	git commit-graph write --reachable --split &&
> -	git commit-graph verify &&
> +	git commit-graph verify 2>err &&
> +	test_line_count = 3 err &&
> +	test_i18ngrep ! warning err &&
>  	test_line_count = 3 $graphdir/commit-graph-chain
>  '
>  
> -- 
> gitgitgadget

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 8/6/2019 5:21 AM, SZEDER Gábor wrote:
> On Mon, Aug 05, 2019 at 09:43:41AM -0700, Derrick Stolee via GitGitGadget wrote:
> 
>> In such a case, the reading
>> process outputs the following message to stderr:
>>
>> 	warning: commit-graph chain does not match
>>
>> These warnings are output in the test suite, but ignored. By
>> checking the stderr of `git commit-graph verify` to include
>> the expected progress output, it will now catch this error.
> 
> There should be a better way to check this than relying on the number
> of progress lines, e.g. 'git commit-graph verify' could print
> something specific, like "Verified 3 commit-graph files, all OK", to
> its standard output, and the test should check only that.  Or check
> only that there is no warning on standard error.
> 
> On one hand, see the recent commits 077b979891 (t3404: make the
> 'rebase.missingCommitsCheck=ignore' test more focused, 2019-06-24) and
> bb431c3dad (t3420: remove progress lines before comparing output,
> 2019-07-04), where we started to exclude the progress output from
> verification in tests like this that are not specifically about
> progress output.
> 
> On the other hand, one of my fun side-projects is to run some
> semi-automated and souped-up CI builds of currently cooking topics,
> which include a custom GETTEXT_POISON mode [1], where the poisoned
> translated progress output in this new test looks like this [2]:
> 
>   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..
>   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..
>   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% (12/12),  d.o.n.e..
> 
> So this makes the progress lines almost twice as long, and as the last
> commit-graph file contains double-digit commits, the length of the
> progress line just happens to exceed the width of a 80 character
> terminal, and gets line wrapped.  This throws off the line count
> check, as there are now 4 lines on standard error instead of the
> expected 3.

Thanks for pointing out that the poison runs can change the number of lines.
My intention was to ensure "we got the amount of output we expected" but it
is enough to only use test_i18ngrep to check that no warnings occurred.

Thanks,
-Stolee

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, SZEDER Gábor wrote (reply to this):

On Tue, Aug 06, 2019 at 08:03:07AM -0400, Derrick Stolee wrote:
> > On the other hand, one of my fun side-projects is to run some
> > semi-automated and souped-up CI builds of currently cooking topics,
> > which include a custom GETTEXT_POISON mode [1], where the poisoned
> > translated progress output in this new test looks like this [2]:
> > 
> >   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..
> >   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..
> >   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% (12/12),  d.o.n.e..
> > 
> > So this makes the progress lines almost twice as long, and as the last
> > commit-graph file contains double-digit commits, the length of the
> > progress line just happens to exceed the width of a 80 character
> > terminal, and gets line wrapped.  This throws off the line count
> > check, as there are now 4 lines on standard error instead of the
> > expected 3.
> 
> Thanks for pointing out that the poison runs can change the number of lines.

Hm, I'm afraid I was not clear enough, so let me clarify.

A "regular" GIT_TEST_GETTEXT_POISON=1 test run does not change the
number of lines.  After it replaces the translated string with the '#
GETTEXT POISON #' magic, the progress output will look like this:

  # GETTEXT POISON #:  41% (5/12)

This will never grow long enough to trigger a line break.

It's just one of my fun experiments [1] that makes translated strings
twice as long, and thus triggers that line break.


[1] https://github.com/szeder/git gettext-poison-scrambled-msg


if (num_parents > 2)
ctx->num_extra_edges += num_parents - 2;
ctx->num_extra_edges += num_parents - 1;
}
}

Expand Down
4 changes: 3 additions & 1 deletion t/t5324-split-commit-graph.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,7 +319,9 @@ test_expect_success 'add octopus merge' '
git merge commits/3 commits/4 &&
git branch merge/octopus &&
git commit-graph write --reachable --split &&
git commit-graph verify &&
git commit-graph verify 2>err &&
test_line_count = 3 err &&
test_i18ngrep ! warning err &&
test_line_count = 3 $graphdir/commit-graph-chain
'

Expand Down