[PATCH 0/2] rebase -i: fix SIGSEGV when 'merge <branch>' fails

[PATCH 0/2] rebase -i: fix SIGSEGV when 'merge <branch>' fails

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

As they fix a bug these patches are based on maint. Unfortunately
the second patch has semantic conflicts with master

s/git_path_merge_msg()/git_path_merge_msg(the_repository)/

There are additional textual conflicts with pu/next due to some
messages being marked for translation. The new message here should be
marked for translation to match them.

Phillip Wood (2):
  t3430: add conflicting commit
  rebase -i: fix SIGSEGV when 'merge <branch>' fails

 sequencer.c              | 24 +++++++++++++++++++-----
 t/t3430-rebase-merges.sh | 30 +++++++++++++++++++++++-------
 2 files changed, 42 insertions(+), 12 deletions(-)

-- 
2.18.0

[PATCH 1/2] t3430: add conflicting commit

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Move the creation of conflicting-G from a test to the setup so that it
can be used in subsequent tests without creating the kind of implicit
dependencies that plague t3404. While we're at it simplify the
arguments to the test_commit() call the creates the conflicting commit.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 t/t3430-rebase-merges.sh | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
index 78f7c99580..31fe4268d5 100755
--- a/t/t3430-rebase-merges.sh
+++ b/t/t3430-rebase-merges.sh
@@ -13,8 +13,10 @@ Initial setup:
     -- B --                   (first)
    /       \
  A - C - D - E - H            (master)
-       \       /
-         F - G                (second)
+   \    \       /
+    \    F - G                (second)
+     \
+      Conflicting-G
 '
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-rebase.sh
@@ -49,7 +51,9 @@ test_expect_success 'setup' '
 	git merge --no-commit G &&
 	test_tick &&
 	git commit -m H &&
-	git tag -m H H
+	git tag -m H H &&
+	git checkout A &&
+	test_commit conflicting-G G.t
 '
 
 test_expect_success 'create completely different structure' '
@@ -72,7 +76,7 @@ test_expect_success 'create completely different structure' '
 	EOF
 	test_config sequence.editor \""$PWD"/replace-editor.sh\" &&
 	test_tick &&
-	git rebase -i -r A &&
+	git rebase -i -r A master &&
 	test_cmp_graph <<-\EOF
 	*   Merge the topic branch '\''onebranch'\''
 	|\
@@ -141,8 +145,7 @@ test_expect_success 'failed `merge` writes patch (may be rescheduled, too)' '
 
 	: fail because of merge conflict &&
 	rm G.t .git/rebase-merge/patch &&
-	git reset --hard &&
-	test_commit conflicting-G G.t not-G conflicting-G &&
+	git reset --hard conflicting-G &&
 	test_must_fail git rebase --continue &&
 	! grep "^merge -C .* G$" .git/rebase-merge/git-rebase-todo &&
 	test_path_is_file .git/rebase-merge/patch
-- 
2.18.0

[PATCH 2/2] rebase -i: fix SIGSEGV when 'merge <branch>' fails

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

If a merge command in the todo list specifies just a branch to merge
with no -C/-c argument then item->commit is NULL. This means that if
there are merge conflicts error_with_patch() is passed a NULL commit
which causes a segmentation fault when make_patch() tries to look it up.

This commit implements a minimal fix which fixes the crash and allows
the user to successfully commit a conflict resolution with 'git rebase
--continue'. It does not write .git/rebase-merge/patch,
.git/rebase-merge/stopped-sha or update REBASE_HEAD. To sensibly get the
hashes of the merge parents would require refactoring do_merge() to
extract the code that parses the merge parents into a separate function
which error_with_patch() could then use to write the parents into the
stopped-sha file. To create meaningful output make_patch() and 'git
rebase --show-current-patch' would also need to be modified to diff the
merge parent and merge base in this case.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 sequencer.c              | 24 +++++++++++++++++++-----
 t/t3430-rebase-merges.sh | 15 ++++++++++++++-
 2 files changed, 33 insertions(+), 6 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 4034c0461b..df49199175 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2590,8 +2590,12 @@ static int error_with_patch(struct commit *commit,
 	const char *subject, int subject_len,
 	struct replay_opts *opts, int exit_code, int to_amend)
 {
-	if (make_patch(commit, opts))
-		return -1;
+	if (commit) {
+		if (make_patch(commit, opts))
+			return -1;
+	} else if (copy_file(rebase_path_message(), git_path_merge_msg(), 0666))
+		return error(_("unable to copy '%s' to '%s'"),
+			     git_path_merge_msg(), rebase_path_message());
 
 	if (to_amend) {
 		if (intend_to_amend())
@@ -2604,9 +2608,19 @@ static int error_with_patch(struct commit *commit,
 			"Once you are satisfied with your changes, run\n"
 			"\n"
 			"  git rebase --continue\n", gpg_sign_opt_quoted(opts));
-	} else if (exit_code)
-		fprintf(stderr, "Could not apply %s... %.*s\n",
-			short_commit_name(commit), subject_len, subject);
+	} else if (exit_code) {
+		if (commit)
+			fprintf(stderr, "Could not apply %s... %.*s\n",
+				short_commit_name(commit),
+				subject_len, subject);
+		else
+			/*
+			 * We don't have the hash of the parent so
+			 * just print the line from the todo file.
+			 */
+			fprintf(stderr, "Could not merge %.*s\n",
+				subject_len, subject);
+	}
 
 	return exit_code;
 }
diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
index 31fe4268d5..2e767d4f1e 100755
--- a/t/t3430-rebase-merges.sh
+++ b/t/t3430-rebase-merges.sh
@@ -129,7 +129,7 @@ test_expect_success '`reset` refuses to overwrite untracked files' '
 	git rebase --abort
 '
 
-test_expect_success 'failed `merge` writes patch (may be rescheduled, too)' '
+test_expect_success 'failed `merge -C` writes patch (may be rescheduled, too)' '
 	test_when_finished "test_might_fail git rebase --abort" &&
 	git checkout -b conflicting-merge A &&
 
@@ -151,6 +151,19 @@ test_expect_success 'failed `merge` writes patch (may be rescheduled, too)' '
 	test_path_is_file .git/rebase-merge/patch
 '
 
+SQ="'"
+test_expect_success 'failed `merge <branch>` does not crash' '
+	test_when_finished "test_might_fail git rebase --abort" &&
+	git checkout conflicting-G &&
+
+	echo "merge G" >script-from-scratch &&
+	test_config sequence.editor \""$PWD"/replace-editor.sh\" &&
+	test_tick &&
+	test_must_fail git rebase -ir HEAD &&
+	! grep "^merge G$" .git/rebase-merge/git-rebase-todo &&
+	grep "^Merge branch ${SQ}G${SQ}$" .git/rebase-merge/message
+'
+
 test_expect_success 'with a branch tip that was cherry-picked already' '
 	git checkout -b already-upstream master &&
 	base="$(git rev-parse --verify HEAD)" &&
-- 
2.18.0

Re: [PATCH 2/2] rebase -i: fix SIGSEGV when 'merge <branch>' fails

[Johannes Schindelin]

Hi Phillip,

On Wed, 15 Aug 2018, Phillip Wood wrote:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
> 
> If a merge command in the todo list specifies just a branch to merge
> with no -C/-c argument then item->commit is NULL. This means that if
> there are merge conflicts error_with_patch() is passed a NULL commit
> which causes a segmentation fault when make_patch() tries to look it up.
> 
> This commit implements a minimal fix which fixes the crash and allows
> the user to successfully commit a conflict resolution with 'git rebase
> --continue'. It does not write .git/rebase-merge/patch,
> .git/rebase-merge/stopped-sha or update REBASE_HEAD. To sensibly get the
> hashes of the merge parents would require refactoring do_merge() to
> extract the code that parses the merge parents into a separate function
> which error_with_patch() could then use to write the parents into the
> stopped-sha file. To create meaningful output make_patch() and 'git
> rebase --show-current-patch' would also need to be modified to diff the
> merge parent and merge base in this case.
> 
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>

ACK!

Thanks,
Dscho

Re: [PATCH 2/2] rebase -i: fix SIGSEGV when 'merge <branch>' fails

[Junio C Hamano]

Phillip Wood <phillip.wood@talktalk.net> writes:

> This commit implements a minimal fix which fixes the crash and allows
> the user to successfully commit a conflict resolution with 'git rebase
> --continue'. It does not write .git/rebase-merge/patch,
> .git/rebase-merge/stopped-sha or update REBASE_HEAD.

I think that should be OK.  When merging, a patch that shows the
diff from the merge base to the tip indeed is an interesting and
useful reference material to help the conflict resolution, but it is
not even clear what the latter two should mean while merging.

> diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
> index 31fe4268d5..2e767d4f1e 100755
> --- a/t/t3430-rebase-merges.sh
> +++ b/t/t3430-rebase-merges.sh
> @@ -129,7 +129,7 @@ test_expect_success '`reset` refuses to overwrite untracked files' '
>  	git rebase --abort
>  '
>  
> -test_expect_success 'failed `merge` writes patch (may be rescheduled, too)' '
> +test_expect_success 'failed `merge -C` writes patch (may be rescheduled, too)' '
>  	test_when_finished "test_might_fail git rebase --abort" &&
>  	git checkout -b conflicting-merge A &&
>  
> @@ -151,6 +151,19 @@ test_expect_success 'failed `merge` writes patch (may be rescheduled, too)' '
>  	test_path_is_file .git/rebase-merge/patch
>  '
>  
> +SQ="'"

A low-hanging fruit tangent, but somebody may want to go through the
output from

    $ git grep '[Ss][Qq]_*=' t

and come up with a shared "convenience" definition of this, which
perhaps sits next to the definition of $_x40 etc.

> +test_expect_success 'failed `merge <branch>` does not crash' '
> +	test_when_finished "test_might_fail git rebase --abort" &&
> +	git checkout conflicting-G &&
> +
> +	echo "merge G" >script-from-scratch &&
> +	test_config sequence.editor \""$PWD"/replace-editor.sh\" &&
> +	test_tick &&
> +	test_must_fail git rebase -ir HEAD &&
> +	! grep "^merge G$" .git/rebase-merge/git-rebase-todo &&
> +	grep "^Merge branch ${SQ}G${SQ}$" .git/rebase-merge/message
> +'
> +
>  test_expect_success 'with a branch tip that was cherry-picked already' '
>  	git checkout -b already-upstream master &&
>  	base="$(git rev-parse --verify HEAD)" &&

Re: [PATCH 2/2] rebase -i: fix SIGSEGV when 'merge <branch>' fails

[Johannes Schindelin]

Hi,

On Thu, 16 Aug 2018, Junio C Hamano wrote:

> Phillip Wood <phillip.wood@talktalk.net> writes:
> 
> > This commit implements a minimal fix which fixes the crash and allows
> > the user to successfully commit a conflict resolution with 'git rebase
> > --continue'. It does not write .git/rebase-merge/patch,
> > .git/rebase-merge/stopped-sha or update REBASE_HEAD.
> 
> I think that should be OK.  When merging, a patch that shows the
> diff from the merge base to the tip indeed is an interesting and
> useful reference material to help the conflict resolution, but it is
> not even clear what the latter two should mean while merging.

Late reply, I know.

It is indeed ambiguous what the REBASE_HEAD should be... While a natural
choice would be the commit to be merged, that would be inconsistent with
the `-c`/`-C` version of `merge`.

But yes, the `patch` should not be written, and the `stopped-sha` does not
make sense with a `merge` that has neither `-c <commit>` nor `-C <commit>`
(although, please note, that this breaks a subsequent `fixup` if there is
one directly after the `merge`).

> > diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
> > index 31fe4268d5..2e767d4f1e 100755
> > --- a/t/t3430-rebase-merges.sh
> > +++ b/t/t3430-rebase-merges.sh
> > @@ -129,7 +129,7 @@ test_expect_success '`reset` refuses to overwrite untracked files' '
> >  	git rebase --abort
> >  '
> >  
> > -test_expect_success 'failed `merge` writes patch (may be rescheduled, too)' '
> > +test_expect_success 'failed `merge -C` writes patch (may be rescheduled, too)' '
> >  	test_when_finished "test_might_fail git rebase --abort" &&
> >  	git checkout -b conflicting-merge A &&
> >  
> > @@ -151,6 +151,19 @@ test_expect_success 'failed `merge` writes patch (may be rescheduled, too)' '
> >  	test_path_is_file .git/rebase-merge/patch
> >  '
> >  
> > +SQ="'"
> 
> A low-hanging fruit tangent, but somebody may want to go through the
> output from
> 
>     $ git grep '[Ss][Qq]_*=' t
> 
> and come up with a shared "convenience" definition of this, which
> perhaps sits next to the definition of $_x40 etc.

If only we had a nice bug tracker with labels. I guess
https://github.com/git/git would be a good place, but it *is* discouraged
by us, which is a pity.

Ciao,
Dscho