Re: [PATCH 11/29] bisect: libify `bisect_next_all`

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Miriam,

On Mon, 20 Jan 2020, Miriam Rubio wrote:

> 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_next_all()`.
>
> All the functions calling `bisect_next_all()` are already able to
> handle return values from it.

So this patch brings more magic values that really would like to become
`enum` values. At this point, we're talking about `bisect_next_all()`
which is _not_ a file-local (`static`) function. Therefore, we now really
wade into API territory where an `enum` is no longer just a nice thing,
but a necessary thing: `bisect_next_all()` is a function that can be
called from other source files.

As far as I can see, this patch concludes the "libify" part of the patch
series (read: it would have been the second patch series I would have
suggested to split out from an otherwise too-long-to-be-reviewed set of
patches, if I had been your mentor), and I'll leave it at that for
tonight; Hopefully I will find some time to review more of these patches
tomorrow.

Ciao,
Johannes

>
> 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 | 40 +++++++++++++++++++++++++++++-----------
>  1 file changed, 29 insertions(+), 11 deletions(-)
>
> diff --git a/bisect.c b/bisect.c
> index acb5a13911..33f2829c19 100644
> --- a/bisect.c
> +++ b/bisect.c
> @@ -967,10 +967,10 @@ void read_bisect_terms(const char **read_bad, const char **read_good)
>  }
>
>  /*
> - * We use the convention that exiting with an exit code 10 means that
> - * the bisection process finished successfully.
> - * In this case the calling shell script should exit 0.
> - *
> + * We use the convention that return -10 means the bisection process
> + * finished successfully.
> + * In this case the calling function or command should not turn a -10
> + * return code into an error or a non zero exit code.
>   * If no_checkout is non-zero, the bisection process does not
>   * checkout the trial commit but instead simply updates BISECT_HEAD.
>   */
> @@ -1000,23 +1000,35 @@ int bisect_next_all(struct repository *r, const char *prefix, int no_checkout)
>
>  	if (!revs.commits) {
>  		/*
> -		 * We should exit here only if the "bad"
> +		 * We should return error here only if the "bad"
>  		 * commit is also a "skip" commit.
>  		 */
>  		res = error_if_skipped_commits(tried, NULL);
>  		if (res)
> -			exit(-res);
> +			return res;
>  		printf(_("%s was both %s and %s\n"),
>  		       oid_to_hex(current_bad_oid),
>  		       term_good,
>  		       term_bad);
> -		exit(1);
> +
> +		/*
> +		 * We don't want to clean the bisection state
> +		 * as we need to get back to where we started
> +		 * by using `git bisect reset`.
> +		 */
> +		return -1;
>  	}
>
>  	if (!all) {
>  		fprintf(stderr, _("No testable commit found.\n"
>  			"Maybe you started with bad path parameters?\n"));
> -		exit(4);
> +
> +		/*
> +		 * We don't want to clean the bisection state
> +		 * as we need to get back to where we started
> +		 * by using `git bisect reset`.
> +		 */
> +		return -4;
>  	}
>
>  	bisect_rev = &revs.commits->item->object.oid;
> @@ -1024,12 +1036,18 @@ int bisect_next_all(struct repository *r, const char *prefix, int no_checkout)
>  	if (oideq(bisect_rev, current_bad_oid)) {
>  		res = error_if_skipped_commits(tried, current_bad_oid);
>  		if (res)
> -			exit(-res);
> +			return res;
>  		printf("%s is the first %s commit\n", oid_to_hex(bisect_rev),
>  			term_bad);
> +
>  		show_diff_tree(r, prefix, revs.commits->item);
> -		/* This means the bisection process succeeded. */
> -		exit(10);
> +		/*
> +		 * This means the bisection process succeeded.
> +		 * Using -10 so that the call chain can simply check
> +		 * for negative return values for early returns up
> +		 * until the cmd_bisect__helper() caller.
> +		 */
> +		return -10;
>  	}
>
>  	nr = all - reaches - 1;
> --
> 2.21.1 (Apple Git-122.3)
>
>