Re: [PATCH 0/2] Reinstate the helpful message when `git pull --rebase` fails

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Oct 9, 2015, 09:50 UTC
Message-ID: <4a99ca8724b3604cbbec48d559e134fd@dscho.org>
In-Reply-To: <CACRoPnSPVMt+FtK6bwfa7Z3jBheTEkBnhU+B7qL8JrAsSmAmkQ@mail.gmail.com>

Hi Junio & Paul,

On 2015-10-09 03:40, Paul Tan wrote:

Show 15 quoted lines

> On Fri, Oct 9, 2015 at 8:52 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>>
>>> Brendan Forster noticed that we no longer see the helpful message after
>>> a failed `git pull --rebase`. It turns out that the builtin `am` calls
>>> the recursive merge function directly, not via a separate process.
>>>
>>> But that function was not really safe to be called that way, as it
>>> die()s pretty liberally.
> 
> I'm not too familiar with the merge-recursive.c code, but I was under
> the impression that it only called die() under fatal conditions. In
> common use cases, such as merge conflicts, it just errors out and the
> helpful error message does get printed. Is there a reproduction recipe
> for this?

Yes. Sorry, I should have added that as part of the patch series. Actually, I should have written it *before* making those patches. Because it revealed that the underlying problem is completely different: *Normally* you are correct, if `pull --rebase` fails with a merge conflict, the advice is shown.

The problem occurs with CR/LF.

I have a reproducer and will wiggle it down to a proper test case.

Show 7 quoted lines

>> If that is the case, I'd thinkg that we'd prefer, as a regression
>> fix to correct "that", i.e., let recursive-merge die and let the
>> caller catch its exit status.
> 
> We could do that, but I don't think it would be worth the overhead to
> spawn an additional process for every patch just to print an
> additional message should merge_recursive() call die().

I would hope that we do not go that direction! The benefit of making `am` a builtin was to *avoid* spawning, after all. Let's not make the experience for Windows users *worse* again.

Show 5 quoted lines

> Instead, stepping back a bit, I wonder if we can extend coverage of
> the helpful message to all die() calls when running git-am. We could
> just install a die routine with set_die_routine() in builtin/am.c.
> Then, should die() be called anywhere, the helpful error message will
> be printed as well. fast-import.c and http-backend.c seem to do this.

This looks more like a work-around to me. In general, I think it is not really a good idea to treat each and every code path as if it is safe to die(). That would just preclude the code from being used as a library function.

But it looks more and more as if the problem lies with the CR/LF handling of Git. Will keep you posted.

Ciao, Dscho