Re: [PATCH 02/10] revert: Propogate errors upwards from do_pick_commit
- From
Ramkumar Ramachandra <artagnon@gmail.com>
- Date
- May 26, 2011, 09:34 UTC
- Message-ID
- <BANLkTi==NG2mTARtVC4vBD8BebMpdRv6tg@mail.gmail.com>
- In-Reply-To
- <7v62oy1juh.fsf@alter.siamese.dyndns.org>
Hi Junio,
Junio C Hamano writes:
Show 14 quoted lines
> 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.
Show 36 quoted lines
>> @@ -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.
Show 10 quoted lines
> - 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; }