diff mbox series

[1/5] rebase -r: demonstrate bug with conflicting merges

Message ID efdd3736a96f90a4ab52acaf2e5efbe3435bcb89.1542065154.git.gitgitgadget@gmail.com
State New, archived
Headers show
Series Assorted fixes revolving around rebase and merges | expand

Commit Message

Elijah Newren via GitGitGadget Nov. 12, 2018, 11:25 p.m. UTC
From: Johannes Schindelin <johannes.schindelin@gmx.de>

When calling `merge` on a branch that has already been merged, that
`merge` is skipped quietly, but currently a MERGE_HEAD file is being
left behind and will then be grabbed by the next `pick` (that did
not want to create a *merge* commit).

Demonstrate this.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t3430-rebase-merges.sh | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

Comments

Junio C Hamano Nov. 13, 2018, 2:29 a.m. UTC | #1
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> When calling `merge` on a branch that has already been merged, that
> `merge` is skipped quietly, but currently a MERGE_HEAD file is being
> left behind and will then be grabbed by the next `pick` (that did
> not want to create a *merge* commit).
>
> Demonstrate this.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  t/t3430-rebase-merges.sh | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)

For a trivially small change/fix like this, it is OK and even
preferrable to make 1+2 a single step, as applying t/ part only to
try to see the breakage (or "am"ing everything and then "diff |
apply -R" the part outside t/ for the same purpose) is easy enough.

Because the patch 2 with your method ends up showing only the test
set-up part in the context by changing _failure to _success, without
showing what end-user visible breakage the step fixed, which usually
comes near the end of the added test piece.  A single patch that
gives tests that ought to succeed would not force the readers to
switch between patches 1 and 2 while reading the fix.

Of course, the above would not apply for a more involved case where
the actual fix to the code needs to span multiple patches.

Thanks.

> diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
> index aa7bfc88ec..1f08a33687 100755
> --- a/t/t3430-rebase-merges.sh
> +++ b/t/t3430-rebase-merges.sh
> @@ -396,4 +396,20 @@ test_expect_success 'with --autosquash and --exec' '
>  	grep "G: +G" actual
>  '
>  
> +test_expect_failure '--continue after resolving conflicts after a merge' '
> +	git checkout -b already-has-g E &&
> +	git cherry-pick E..G &&
> +	test_commit H2 &&
> +
> +	git checkout -b conflicts-in-merge H &&
> +	test_commit H2 H2.t conflicts H2-conflict &&
> +	test_must_fail git rebase -r already-has-g &&
> +	grep conflicts H2.t &&

Is this making sure that the above test_must_fail succeeded because
of a conflict and not due to any other failure?  I would have used
"ls-files -u H2.t" to see if the index is unmerged, which probably
is a more direct way to test what this is trying to test, but if we
are in the conflicted state, the one side of << == >> has this
string (the other has "H2" in it, presumably?), so in practice this
should be good enough.

> +	echo resolved >H2.t &&
> +	git add -u &&

and we resolve to continue.

> +	git rebase --continue &&
> +	test_must_fail git rev-parse --verify HEAD^2 &&

Even if we made an octopus by mistake, the above will catch it,
which is good.

> +	test_path_is_missing .git/MERGE_HEAD
> +'
> +
>  test_done

And from the proposed log message, I am reading that the last two
things (i.e. resulting tip is a child with a single parent and there
is no leftover MERGE_HEAD file) fail without the fix.  

This is enough material to convince me or anybody that the bug is
worth fixing.  Thanks for being careful noticing a glitch during
your real (and otherwise unrelated to the bug) work and following
through.
Johannes Schindelin Nov. 13, 2018, 10:12 a.m. UTC | #2
Hi Junio,

On Tue, 13 Nov 2018, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
> 
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > When calling `merge` on a branch that has already been merged, that
> > `merge` is skipped quietly, but currently a MERGE_HEAD file is being
> > left behind and will then be grabbed by the next `pick` (that did
> > not want to create a *merge* commit).
> >
> > Demonstrate this.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  t/t3430-rebase-merges.sh | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> 
> For a trivially small change/fix like this, it is OK and even
> preferrable to make 1+2 a single step, as applying t/ part only to
> try to see the breakage (or "am"ing everything and then "diff |
> apply -R" the part outside t/ for the same purpose) is easy enough.

I disagree. It helps both development and porting to different branches to
be able to cherry-pick the regression test individually. Please do not ask
me to violate this hard-learned principle.

> Because the patch 2 with your method ends up showing only the test
> set-up part in the context by changing _failure to _success, without
> showing what end-user visible breakage the step fixed, which usually
> comes near the end of the added test piece.  A single patch that
> gives tests that ought to succeed would not force the readers to
> switch between patches 1 and 2 while reading the fix.

That is why I put in a verbose commit message, so that you do not have to
guess. And even the test title talks about this.

Seriously, I am very much opposed to changing the patches in the direction
you suggested. In my mind, they would make the story substantially worse.

Thank you for your review,
Dscho

> 
> Of course, the above would not apply for a more involved case where
> the actual fix to the code needs to span multiple patches.
> 
> Thanks.
> 
> > diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
> > index aa7bfc88ec..1f08a33687 100755
> > --- a/t/t3430-rebase-merges.sh
> > +++ b/t/t3430-rebase-merges.sh
> > @@ -396,4 +396,20 @@ test_expect_success 'with --autosquash and --exec' '
> >  	grep "G: +G" actual
> >  '
> >  
> > +test_expect_failure '--continue after resolving conflicts after a merge' '
> > +	git checkout -b already-has-g E &&
> > +	git cherry-pick E..G &&
> > +	test_commit H2 &&
> > +
> > +	git checkout -b conflicts-in-merge H &&
> > +	test_commit H2 H2.t conflicts H2-conflict &&
> > +	test_must_fail git rebase -r already-has-g &&
> > +	grep conflicts H2.t &&
> 
> Is this making sure that the above test_must_fail succeeded because
> of a conflict and not due to any other failure?  I would have used
> "ls-files -u H2.t" to see if the index is unmerged, which probably
> is a more direct way to test what this is trying to test, but if we
> are in the conflicted state, the one side of << == >> has this
> string (the other has "H2" in it, presumably?), so in practice this
> should be good enough.
> 
> > +	echo resolved >H2.t &&
> > +	git add -u &&
> 
> and we resolve to continue.
> 
> > +	git rebase --continue &&
> > +	test_must_fail git rev-parse --verify HEAD^2 &&
> 
> Even if we made an octopus by mistake, the above will catch it,
> which is good.
> 
> > +	test_path_is_missing .git/MERGE_HEAD
> > +'
> > +
> >  test_done
> 
> And from the proposed log message, I am reading that the last two
> things (i.e. resulting tip is a child with a single parent and there
> is no leftover MERGE_HEAD file) fail without the fix.  
> 
> This is enough material to convince me or anybody that the bug is
> worth fixing.  Thanks for being careful noticing a glitch during
> your real (and otherwise unrelated to the bug) work and following
> through.
>
Junio C Hamano Nov. 13, 2018, 12:06 p.m. UTC | #3
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> For a trivially small change/fix like this, it is OK and even
>> preferrable to make 1+2 a single step, as applying t/ part only to
>> try to see the breakage (or "am"ing everything and then "diff |
>> apply -R" the part outside t/ for the same purpose) is easy enough.
>
> I disagree. It helps both development and porting to different branches to
> be able to cherry-pick the regression test individually. Please do not ask
> me to violate this hard-learned principle.

A trivially small change/fix like this, by definition (of "trivial"
and "small" ness), it is trivial to develop and port to different
branches a single patch, and test with just one half (either the
test part or the code-change part) of the change reversed, to ensure
that the codebase is broken without the code-change and to
demonstrate that the code-change does fix the problem revealed by
the test change.  And "porting" by cherry-picking a single patch is
always easier than two patch series.

So you may disagree all you want in your project, but do not make
reviewer's lives unnecessarily harder in this project.

Thanks.
Johannes Schindelin Nov. 13, 2018, 12:47 p.m. UTC | #4
Hi Junio,

On Tue, 13 Nov 2018, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> For a trivially small change/fix like this, it is OK and even
> >> preferrable to make 1+2 a single step, as applying t/ part only to
> >> try to see the breakage (or "am"ing everything and then "diff |
> >> apply -R" the part outside t/ for the same purpose) is easy enough.
> >
> > I disagree. It helps both development and porting to different branches to
> > be able to cherry-pick the regression test individually. Please do not ask
> > me to violate this hard-learned principle.
> 
> A trivially small change/fix like this, by definition (of "trivial"
> and "small" ness), it is trivial to develop and port to different
> branches a single patch, and test with just one half (either the
> test part or the code-change part) of the change reversed, to ensure
> that the codebase is broken without the code-change and to
> demonstrate that the code-change does fix the problem revealed by
> the test change.  And "porting" by cherry-picking a single patch is
> always easier than two patch series.
> 
> So you may disagree all you want in your project, but do not make
> reviewer's lives unnecessarily harder in this project.

You misunderstand. In this case it is crucial to read the regression test
first. The fix does not make much sense before one understands the
condition under which the order of the code statements matters.

By trying to force me to smoosh them together, you are trying to force me
to combine them in one patch where you would read the (now seemingly
non-sensical) fix first, and only then the test.

That's just really unhelpful. If I were a reviewer, I would want it
presented in the way it *was* presented. I firmly believe most reviewers
would.

Ciao,
Dscho
Junio C Hamano Nov. 13, 2018, 12:56 p.m. UTC | #5
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> You misunderstand. In this case it is crucial to read the regression test
> first. The fix does not make much sense before one understands the
> condition under which the order of the code statements matters.

I am not sure what you mean.  It sounds as if you want to use
diff-orderfile to present change for t/ before changes to other
files are presented in format-patch output to help readers, and I
think that may make sense for certain cases.  It may even include
this case.

But that is not incompatible with "avoid showing the patch that
updates the code to fix breakages separately, ending up with showing
the changes to t/ that are mostly about s/_failure/_success/ and
readers are forced to go back to the previous patch to remind
themselves what the broken scenario was about; by keeping it in a
single patch, the readers can get the tests in one piece".
diff mbox series

Patch

diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
index aa7bfc88ec..1f08a33687 100755
--- a/t/t3430-rebase-merges.sh
+++ b/t/t3430-rebase-merges.sh
@@ -396,4 +396,20 @@  test_expect_success 'with --autosquash and --exec' '
 	grep "G: +G" actual
 '
 
+test_expect_failure '--continue after resolving conflicts after a merge' '
+	git checkout -b already-has-g E &&
+	git cherry-pick E..G &&
+	test_commit H2 &&
+
+	git checkout -b conflicts-in-merge H &&
+	test_commit H2 H2.t conflicts H2-conflict &&
+	test_must_fail git rebase -r already-has-g &&
+	grep conflicts H2.t &&
+	echo resolved >H2.t &&
+	git add -u &&
+	git rebase --continue &&
+	test_must_fail git rev-parse --verify HEAD^2 &&
+	test_path_is_missing .git/MERGE_HEAD
+'
+
 test_done