Hi Junio,
Junio C Hamano writes:
> Ramkumar Ramachandra <artagnon@xxxxxxxxx> 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@xxxxxxxxx>
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;
}
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html