Re: [PATCH v2 07/11] bisect: libify `bisect_checkout`

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

Miriam Rubio <mirucam@gmail.com> writes:

> From: Pranit Bauva <pranit.bauva@gmail.com>
>
> Since we want to get rid of git-bisect.sh it would be necessary to
> convert those exit() calls to return statements so that errors can be
> reported.
>
> Emulate try catch in C by converting `exit(<positive-value>)` to
> `return <negative-value>`. Follow POSIX conventions to return
> <negative-value> to indicate error.
>
> Turn `exit()` to `return` calls in `bisect_checkout()`.
> Changes related to return values have no bad side effects on the
> code that calls `bisect_checkout()`.
>
> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> Mentored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
> Signed-off-by: Tanushree Tumane <tanushreetumane@gmail.com>
> Signed-off-by: Miriam Rubio <mirucam@gmail.com>
> ---
>  bisect.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/bisect.c b/bisect.c
> index a7a5d158e6..dee8318d9b 100644
> --- a/bisect.c
> +++ b/bisect.c
> @@ -713,6 +713,7 @@ static int bisect_checkout(const struct object_id *bisect_rev, int no_checkout)
>  {
>  	char bisect_rev_hex[GIT_MAX_HEXSZ + 1];
>  
> +	int res = 0;
>  	memcpy(bisect_rev_hex, oid_to_hex(bisect_rev), the_hash_algo->hexsz + 1);

Wrong placement of a new decl.  Have a block of decls and then have
a blank line before the first statement, i.e.

		char bisect_rev_hex[...];
	+	int res = 0;

		memcpy(...);

This comment probably applies to other hunks in the entire series.

> @@ -721,14 +722,14 @@ static int bisect_checkout(const struct object_id *bisect_rev, int no_checkout)
>  		update_ref(NULL, "BISECT_HEAD", bisect_rev, NULL, 0,
>  			   UPDATE_REFS_DIE_ON_ERR);
>  	} else {
> -		int res;
>  		res = run_command_v_opt(argv_checkout, RUN_GIT_CMD);
>  		if (res)
> -			exit(res);
> +			return res > 0 ? -res : res;

Hmph.  

This means that res == -1 and res == 1 from run_command_v_opt()
cannot be distinguished by our callers.  Is that what we want here?

If that is really what we want, it probably is easier to read if
this were written like so:

		return -abs(res);

>  	argv_show_branch[1] = bisect_rev_hex;
> -	return run_command_v_opt(argv_show_branch, RUN_GIT_CMD);
> +	res = run_command_v_opt(argv_show_branch, RUN_GIT_CMD);
> +	return res > 0 ? -res : res;

Likewise.