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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 9, 2015, 18:15 UTC
Message-ID: <xmqqk2qvq570.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <CACRoPnSPVMt+FtK6bwfa7Z3jBheTEkBnhU+B7qL8JrAsSmAmkQ@mail.gmail.com>

Paul Tan <pyokagan@gmail.com> writes:

Show 10 quoted lines

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

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.

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.