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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Aug 27, 2018, 12:49 UTC
Message-ID: <nycvar.QRO.7.76.6.1808271442010.73@tvgsbejvaqbjf.bet>
In-Reply-To: <xmqqzhxmfgbb.fsf@gitster-ct.c.googlers.com>

Hi,

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

Show 11 quoted lines

> 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`).

Show 26 quoted lines

> > 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