From: Ramkumar Ramachandra <artagnon@gmail.com>
Date: Thu, 26 May 2011 09:34:16 GMT
Subject: Re: [PATCH 02/10] revert: Propogate errors upwards from do_pick_commit
Message-ID: <BANLkTi==NG2mTARtVC4vBD8BebMpdRv6tg@mail.gmail.com>
In-Reply-To: <7v62oy1juh.fsf@alter.siamese.dyndns.org>

Hi Junio,

Junio C Hamano writes:
> Ramkumar Ramachandra <artagnon@gmail.com> 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?

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

>> @@ -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 <artagnon@gmail.com>

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;
 }