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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Jul 6, 2011, 08:50 UTC
Message-ID: <20110706085007.GB15682@elie>
In-Reply-To: <1309938868-2028-6-git-send-email-artagnon@gmail.com>

Hi,

Ramkumar Ramachandra wrote:

Show 5 quoted lines

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