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

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.

[...]

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