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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Oct 12, 2015, 09:40 UTC
Message-ID: <ed70803ecd73415f1bbafb68502fbbda@dscho.org>
In-Reply-To: <xmqq4mhzq41e.fsf@gitster.mtv.corp.google.com>

Hi Junio,

On 2015-10-09 20:40, Junio C Hamano wrote:

Show 23 quoted lines

> Junio C Hamano <gitster@pobox.com> writes:
> 
>>> 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.
>>
>> That could certainly be a valid approach and may give us a better
>> end result.  If it works, it could be a change that is localized
>> with a lot less impact.
> 
> I looked at the codepath involved, and I do not think that is a
> feasible way forward in this case.  It is not about a "helpful
> message" at all.  You would have to do everything that is done in
> the error codepath in your custom die routine, which does not make
> much sense.
> 
> I think the most sensible regression fix as the first step at this
> point is to call it as a separate process, just like the code calls
> "apply" as a separate process for each patch.  Optimization can come
> later when it is shown that it matters---we need to regain
> correctness first.

I fear that you might underestimate the finality of this "first step". If you reintroduce that separate process, not only is it a performance regression, but could we really realistically expect any further steps to happen after that? I do not think so.

Also, please let me clarify why I called reintroducing the separate process "heavy-handed" in an earlier message. As pointed out by Paul, the dying code paths indicate non-recoverable problems, i.e. serious problems that not even a rerere could fix. Modulo bugs, of course, but those bugs need to be fixed and not papered over. The real bug, after all, is that a non-recoverable code path is taken when it should just return with an error code.

Reintroducing the separate process would not help the endeavor to fix those code paths. Indeed, if we still had the separate process, I would never have discovered that bug!

And we should also keep in mind that this whole story demonstrates the rather serious shortcomings of the mindset we display throughout libgit.a where it does not behave like a library at all. Of course, hindsight is 20/20, so it is all too easy, and not exactly fair, to criticise the short-sightedness of writing code that does not clean up after itself "because it is a short-running process anyway". I certainly have contributed to these problems myself! All the more eager am I to help *increase* the number of functions in libgit.a that are reentrant, eventually making libgit.a behave like a true library. And in that light, what you called "the first step" appears like it would be a huge step backwards.

In contrast, introducing the "gentle" flag would be a step in the right direction. It is a much lighter stop-gap solution, too.

For the above reasons, I respectfully remain convinced that reintroducing the separate process would be a mistake.

Ciao, Dscho