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

[Junio C Hamano <gitster@pobox.com>]

Paul Tan <pyokagan@gmail.com> writes:

> That said, I do agree that even if we die(), we could try to be more
> helpful by printing additional helpful instructions.
>
>> 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().

For a thing that (1) has to be run every time in the whole operation
and (2) is a very small operation itself whose runtime cost can be
dwarfed by cost of spawning on some platforms, it is clearly better
to run it internally instead of running it via run_command()
interface.  This is especially so if it (3) wants to just kill the
whole operation when it finds a problem anyway.  For example, it
would be crazy to run "update-ref" via run_command() in the "am"
that is rewritten in C.

But does the same reasoning apply to the use of merge-recursive in
"am -3" codepath, where it (1) runs only as a fallback when straight
application of the patch fails, (2) runs a heavy-weight recursive
merge machinery, and (3) now a regression is pointed out that it
wants to do more than just calling die() when there is a problem?

You seem to be viewing the world in black-and-white and thinking
that run_command() is unconditionally bad.  You need to stop doing
that.  Instead, view it as another tool that gives a much better
isolation from the main flow of logic (hence flexiblity) that comes
with a bigger cost.  I am not convinced with your "I don't think it
would be worth".

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

Thanks.