Re: [PATCH] branch: error codes for "edit_description"

[Junio C Hamano <gitster@pobox.com>]

Rubén Justo <rjusto@gmail.com> writes:

> Stick to the convention of exit with 1 on error.  May break some
> third party tool that expects -1 on "No branch named 'bla'" or
> "No commit on branch 'bla' yet." errors.
>
> Test added to avoid regresion on this behaviour.
> ---
>
> Junio, here is the patch for the change you suggested [1].  My concern is
> this might break some third party tool or script.  If no one thinks this
> is an issue, I can squash it with the patch I'm responding to.
>
>
> [1] https://lore.kernel.org/git/xmqq7d2fszhk.fsf@gitster.g/
>
>  builtin/branch.c  | 6 +++---
>  t/t3200-branch.sh | 8 ++++++++
>  2 files changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/branch.c b/builtin/branch.c
> index b1f6519cd9..01b6364f58 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -810,11 +810,11 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
>  			else
>  				ret = error(_("No branch named '%s'."),
>  					     branch_name);
> -		} else if (edit_branch_description(branch_name))
> -			ret = 1;
> +		} else
> +			ret = edit_branch_description(branch_name);
>  
>  		strbuf_release(&branch_ref);
> -		return ret;
> +		return -ret;

If edit_branch_description() returns -1 on failure, this would
consistently exit with 1 on failure, so it does make sense.  

It is bad to exit with a value outside 0..255 for portability
reasons, but we wrap exit() to chomp off the higher bits, so in
practice it may be fine to exit(-1) but still the above is a good
clean-up, I would think.