Re: [PATCH 04/13] revert: Propogate errors upwards from do_pick_commit

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Jun 21, 2011, 16:22 UTC
Message-ID: <20110621162232.GI15461@elie>
In-Reply-To: <1308661489-20080-5-git-send-email-artagnon@gmail.com>

Ramkumar Ramachandra wrote:

Show 10 quoted lines

> Earlier, revert_or_cherry_pick only used to return a non-negative
> number or die.  Change this so that it can return negative values too;
> postive return values indicate conflicts, while negative ones indicate
> other errors, and this return status is propogated updwards from
> do_pick_commit, to be finally handled in cmd_cherry_pick and
> cmd_revert.  While revert_or_cherry_pick can still die due to several
> other reasons, this patches attempts to factor out some of the die
> calls.  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.

The "Earlier" combined with imperfect tense somehow oddly trips me up (somehow it does not register that you mean "Before this patch"). I usually find using the present or the simple past for the status quo is clearer ("Currently the return value from revert_or_cherry_pick is a non-negative integer representing the intended exit status from git revert or git cherry-pick. Change that by ...").

Carrying forward from that confusion, it's hard to concentrate hard enough to parse and figure out the rest. That shouldn't be necessary: by setting the scene with an explanation of the problem being solved it should be possible to make this much simpler.

Suppose, forgetting the above, that I asked you to explain what this patch is about. You'd say it's about teaching "revert_or_cherry_pick" to return error() instead of die()-ing a little more often, right? Then if I asked you why, you might say one of many things --- for example, that in the long term the hope is to allow speeding up multiple-cherry-pick by not writing CHERRY_PICK_HEAD et al until a single-commit pick fails, and that die()-ing interferes with that by not moving the thread of control to the caller that wants to write files like CHERRY_PICK_HEAD on error. After that, I might ask what this patch does to address that, and you might tell me that in addition to converting various die() calls to "return error()", it updates the callers to cope with the negative return codes that could result from that. Eventually the negative value gets returned from revert_or_cherry_pick; currently the convention there is to return a nonnegative integer (representing an appropriate "normal" exit status for the command) but in the new regime it can also be -1, meaning the command failed and should die(). Finally if I asked you what the impact of applying this patch is, you might tell me that the benefits will require converting more die() calls so the only immediate impact should be to change various messages from "fatal:" to "error:" and add a new

	fatal: cherry-pick failed

at the end.

> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -250,25 +250,15 @@ static struct tree *empty_tree(void)

[...]

Show 18 quoted lines

> @@ -582,14 +572,28 @@ static int revert_or_cherry_pick(int argc, const char **argv)
>  
>  int cmd_revert(int argc, const char **argv, const char *prefix)
>  {
> +	int res;
>  	if (isatty(0))
>  		edit = 1;
>  	action = REVERT;
> -	return revert_or_cherry_pick(argc, argv);
> +	res = revert_or_cherry_pick(argc, argv);
> +	if (res > 0)
> +		/* Exit status from conflict */
> +		return res;
> +	if (res < 0)
> +		/* Other error */
> +		die(_("%s failed"), me);
> +	return 0;
>  }

With the above explanation in mind, it seems simpler to say

	if (res < 0)
		die(...);
	return res;

the idea being that there are two cases --- the new "die for failure" case and the old "pass on the exit status requested by revert_or_cherry_pick" case.

Show 12 quoted lines

>  
>  int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
>  {
> +	int res;
>  	action = CHERRY_PICK;
> -	return revert_or_cherry_pick(argc, argv);
> +	res = revert_or_cherry_pick(argc, argv);
> +	if (res > 0)
> +		return res;
> +	if (res < 0)
> +		die(_("%s failed"), me);
> +	return 0;

Should the "revert" or "cherry-pick" here be part of the message marked for translation? A translator might want to paraphrase to

	fatal: failed to cherry-pick

if that is more natural in the language at hand.