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

Paul Tan [off-list ref] writes:

That said, I do agree that even if we die(), we could try to be more
helpful by printing additional helpful instructions.

quoted

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.