Re: [PATCH 05/17] revert: Propogate errors upwards from do_pick_commit

[Jonathan Nieder <jrnieder@gmail.com>]

Ramkumar Ramachandra wrote:

> Currently, the return value from revert_or_cherry_pick is a
> non-negative number representing the intended exit status from `git
> revert` or `git cherry-pick`.  Change this by replacing some of the
> calls to "die" with calls to "error", so that it can return negative
> values too.  Postive return values indicate conflicts, while negative
> ones indicate other errors.  This return status is propogated updwards
> from do_pick_commit, to be finally handled in cmd_cherry_pick and
> cmd_revert.

As mentioned at [1], this accurately describes the effect, but not the
motivation.  (In tricky cases like this, knowing the motivation would
help with review immensely.)

[...]
> While the full benefits of this patch will only be seen once all the
> "die" calls are replaced with calls to "error", its immediate impact
> is to change some of the "die:" messages to "error:" messages and
> print a new "fatal: cherry-pick failed" message when the operation
> fails.

I think by "die:" you mean "fatal:".

> --- a/builtin/revert.c
> +++ b/builtin/revert.c
[...]
> @@ -373,12 +368,12 @@ static int do_pick_commit(void)
>  		 * to work on.
>  		 */
>  		if (write_cache_as_tree(head, 0, NULL))
> -			die (_("Your index file is unmerged."));
> +			return error(_("Your index file is unmerged."));

write_cache_as_tree() locks the index and does not always commit or
roll it back except on success.  Current callers aren't likely to try
to lock the index again (since they just die()), but presumably the
goal of returning error() here is to allow for callers that want to
stay alive and do something more.  How should they recover (i.e., what
is the intended API)?

Similar questions probably apply to calls to other APIs that return -1
to mean "I failed; please print an appropriate message about that and
exit".  I haven't checked carefully, since the answer to this example
could help in knowing what to look for in the others.

[1] http://thread.gmane.org/gmane.comp.version-control.git/176647/focus=176664