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

From: Junio C Hamano <gitster@pobox.com>
Date: May 25, 2011, 22:44 UTC
Message-ID: <7v62oy1juh.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1306333025-29893-3-git-send-email-artagnon@gmail.com>

Ramkumar Ramachandra <artagnon@gmail.com> writes:

Show 10 quoted lines

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

Show 31 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?

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