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

[Ramkumar Ramachandra <artagnon@gmail.com>]

Hi,

Jonathan Nieder writes:
> 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.)

Sorry, I should have done this earlier.  Does this look alright?

revert: Propogate errors upwards from do_pick_commit

Currently, revert_or_cherry_pick can fail in two ways.  If it
encounters a conflict, it returns a positive number indicating the
intended exit status for the git wrapper to pass on; for all other
errors, it calls die().  The latter behavior is inconsiderate towards
callers, as it denies them the opportunity to do some post-processing
or cleanup before exiting.  For instance, later in the series, callers
will want to save some data about the current operation before
exiting.

Change this by replacing some of the calls to "die" with calls to
"error", so that revert_or_cherry_pick can return negative values too.
While postive return values indicate conflicts as before, 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.

In the same spirit, also introduce a new function error_dirty_index,
based on die_dirty_index, which prints some hints and returns an error
to its caller do_pick_commit.

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 "fatal:" messages to "error:" messages and
print a new "fatal: cherry-pick failed" message when the operation
fails.


>> --- 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)?

Hm, there was supposed to be a discard_cache() before this error as I
recall -- fixed now.  Thanks for catching.

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

I think the others are fairly clear actually.  Let me know if you find
something.

-- Ram