Re: [PATCH 1/8] revert: Improve error handling by cascading errors upwards

From: Jonathan Nieder <jrnieder@gmail.com>
Date: May 19, 2011, 18:03 UTC
Message-ID: <20110519180314.GA26248@elie>
In-Reply-To: <20110519091831.GA28723@ramkum.desktop.amazon.com>

Ramkumar Ramachandra wrote:

> I'm preparing a large series dedicated to solving error-handling
> issues before getting to the sequencer series.  I plan to post some
> quick-and-dirty diffs of various things and ask for feedback.

Sounds good. I wonder if there's a good way to test this (maybe I'll try making an XS module so Git.pm can take advantage of similar changes).

> Jonathan Nieder writes:
>> Ramkumar Ramachandra wrote:

Show 10 quoted lines

>>> @@ -418,19 +434,19 @@ static int do_pick_commit(void)
>>>  		struct commit_list *p;
>>>  
>>>  		if (!mainline)
>>> -			die(_("Commit %s is a merge but no -m option was given."),
>>> -			    sha1_to_hex(commit->object.sha1));
>>> +			return error(_("Commit %s is a merge but no -m option was given."),
>>> +				sha1_to_hex(commit->object.sha1));
>>
>> This is not a conflict but an error in usage.

[...]

Show 6 quoted lines

> For this part, I think the correct way to handle the usage error is to
> print a message like this:
>
>     usage: cherry-pick: Commit b8bf32 is a merge but no -m option was given.
>
> And exit with status 129. Is this acceptable?

On second thought, status 128 seems appropriate --- the caller made a mistake, but it's more analagous to merging with a dirty index than (i.e., the command was not able to be fulfilled as desired) than to misspelling a command-line option (i.e., broken script). I suppose treating it as an error (as in your "return error") would work, and the caller can transform -1 to exit(128).

Sorry for the thinko.