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

[Jonathan Nieder <jrnieder@gmail.com>]

Hi,

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

The above seems to be suggesting that the current return value is a
_problem_, and that this change _fixes_ it.

But I had thought that the bulk of this patch's changes (die-to-error
conversions) were not meant as a means to that end but an end in
themselves.  Wouldn't a clearer problem statement be "Currently,
revert_or_cherry_pick can fail in two ways.  If it encounters
conflicts, it returns a positive number indicating the intended exit
status for the git wrapper to pass on; for all other errors, it
die()s.  Some callers may not like the latter behavior because of
<reasons here>"?

Only after the reader understands that, she will be ready to
appreciate the value of the proposed alternate return value
convention.  Similar comments apply to the commit messages of the few
patches before --- they are not terribly confusing, but they could
still easily be improved by mentioning what problem the patches are
supposed to solve.

> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -250,25 +250,20 @@ static struct tree *empty_tree(void)
[...]
> +	if (action == CHERRY_PICK)
> +		error(_("Your local changes would be overwritten by %s."), me);
> +	else
> +		error(_("Your local changes would be overwritten by %s."), me);

gettext creates one msgid for these two strings, so translators have
no choice but to give them the same translation.  Is that the intent?

[...]
> +	if (res < 0)
> +		die(_("%s failed"), me);
> +	return res;
>  }

Likewise.

I haven't looked carefully again at the unwinding-after-error, but
aside from that and except as noted above this looks good.