Hi Junio & Paul, On 2015-10-09 03:40, Paul Tan wrote: > On Fri, Oct 9, 2015 at 8:52 AM, Junio C Hamano <gits...@pobox.com> wrote: >> Johannes Schindelin <johannes.schinde...@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. >> 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. > 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
