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

[Jonathan Nieder <jrnieder@gmail.com>]

Ramkumar Ramachandra wrote:

> 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)
[...]
> @@ -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.

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