Re: [PATCH 02/10] revert: Propogate errors upwards from do_pick_commit

Ramkumar Ramachandra [off-list ref] writes:

+static int error_dirty_index(const char *me)
 {
+	if (read_cache_unmerged())
+		return error_resolve_conflict(me);
+
+	int ret = error(_("Your local changes would be overwritten by %s.\n"), me);
+	if (advice_commit_before_merge)
+		advise(_("Please, commit your changes or stash them to proceed."));
+	return ret;
 }

I like this rewrite whose result is short-and-sweet, but you do not even
need the "ret" variable. error() always yields -1, no?

This hunk is dubious.

 - Why initialize res to zero if it always is assigned the return value of
   revert_or_cherry_pick() before it is used?

 - The called function seems to return errors from various places but as
   far as I see they are all return value of error(), so it would be
   equivalent to

	if (r_o_c_p(...))
		exit(128);
	return 0;

If you are going to introduce different return values from r-o-c-p() in a
later patch, these functions should be updated in that patch, I think.

Hi Junio,

Junio C Hamano writes:

Ramkumar Ramachandra [off-list ref] writes:

quoted

+static int error_dirty_index(const char *me)
 {
+     if (read_cache_unmerged())
+             return error_resolve_conflict(me);
+
+     int ret = error(_("Your local changes would be overwritten by %s.\n"), me);
+     if (advice_commit_before_merge)
+             advise(_("Please, commit your changes or stash them to proceed."));
+     return ret;
 }

I like this rewrite whose result is short-and-sweet, but you do not even
need the "ret" variable. error() always yields -1, no?

Okay; I didn't do this in the first place because I thought it would
be inelegant to hardcode '-1'. Fixed anyway.

quoted

@@ -594,14 +584,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 = 0;
      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 */
+             exit(128);
+     return 0;
 }

 int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
 {
+     int res = 0;
      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)
+             exit(128);
+     return 0;
 }

This hunk is dubious.

 - Why initialize res to zero if it always is assigned the return value of
  revert_or_cherry_pick() before it is used?

Okay. Fixed.

 - The called function seems to return errors from various places but as
  far as I see they are all return value of error(), so it would be
  equivalent to

       if (r_o_c_p(...))
               exit(128);
       return 0;

If you are going to introduce different return values from r-o-c-p() in a
later patch, these functions should be updated in that patch, I think.

revert_or_cherry_pick *does* return different values in this patch! As
I've pointed out in the comment, positive exit status indicates a
conflict, while a negative one indicates an error. To prove to myself
that this is case, I applied this diff temporarily and ran all tests
-- and viola, t3505-cherry-pick-empty.sh broke. Is there something I'm
not understanding correctly?

Thanks for the review.

Signed-off-by: Ramkumar Ramachandra <redacted>

diff --git a/builtin/revert.c b/builtin/revert.c
index 523d41a..9c7921b 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c

@@ -584,28 +584,22 @@ static int revert_or_cherry_pick(int argc, const

char **argv)

 int cmd_revert(int argc, const char **argv, const char *prefix)
 {
-	int res = 0;
+	int res;
 	if (isatty(0))
 		edit = 1;
 	action = REVERT;
 	res = revert_or_cherry_pick(argc, argv);
-	if (res > 0)
-		/* Exit status from conflict */
-		return res;
-	if (res < 0)
-		/* Other error */
+	if (res)
 		exit(128);
 	return 0;
 }

 int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
 {
-	int res = 0;
+	int res;
 	action = CHERRY_PICK;
 	res = revert_or_cherry_pick(argc, argv);
-	if (res > 0)
-		return res;
-	if (res < 0)
+	if (res)
 		exit(128);
 	return 0;
 }