[11/29] bisect: libify `bisect_next_all`

Commit Message

Miriam R. Jan. 20, 2020, 2:37 p.m. UTC

From: Pranit Bauva <[e-mail]>

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.

Mentored-by: Christian Couder <[e-mail]>
Mentored-by: Johannes Schindelin <[e-mail]>
Signed-off-by: Pranit Bauva <[e-mail]>
Signed-off-by: Tanushree Tumane <[e-mail]>
Signed-off-by: Miriam Rubio <[e-mail]>
---
 bisect.c | 40 +++++++++++++++++++++++++++++-----------
 1 file changed, 29 insertions(+), 11 deletions(-)

Comments

Johannes Schindelin Jan. 20, 2020, 10:29 p.m. UTC | #1

Hi Miriam,

On Mon, 20 Jan 2020, Miriam Rubio wrote:

> From: Pranit Bauva <[e-mail]>
>
> 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 <[e-mail]>
> Mentored-by: Johannes Schindelin <[e-mail]>
> Signed-off-by: Pranit Bauva <[e-mail]>
> Signed-off-by: Tanushree Tumane <[e-mail]>
> Signed-off-by: Miriam Rubio <[e-mail]>
> ---
>  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)
>
>

Christian Couder Jan. 21, 2020, 7:15 a.m. UTC | #2

On Mon, Jan 20, 2020 at 11:29 PM Johannes Schindelin
<[e-mail]> wrote:

> On Mon, 20 Jan 2020, Miriam Rubio wrote:

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

I agree that return values could be better documented. About enum
though, I am perhaps wrong but I don't think that we use them often
for error codes. Do you have examples in the code base where they are
used for such a purpose along with regular `error(...)` calls?

> 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),

I am ok with saying that "part 1" stops after this patch and that the
rest will be "part 2" and will not be resent in the version 2 of the
"part 1" patch series, but will rather be resent later, when "part 1"
will be in next for example.

> and I'll leave it at that for
> tonight; Hopefully I will find some time to review more of these patches
> tomorrow.

That would be great!

Thanks,
Christian.

Johannes Schindelin Jan. 30, 2020, 3:18 p.m. UTC | #3

Hi Chris,

On Tue, 21 Jan 2020, Christian Couder wrote:

> On Mon, Jan 20, 2020 at 11:29 PM Johannes Schindelin
> <[e-mail]> wrote:
>
> > On Mon, 20 Jan 2020, Miriam Rubio wrote:
>
> > > 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.
>
> I agree that return values could be better documented. About enum
> though, I am perhaps wrong but I don't think that we use them often
> for error codes.

It does not matter how often we actually use them, it matters more whether
we _want_ to use them. And in the recent years, we have definitely made
more use of enums than before, to allow the compiler to catch mistakes
earlier. We even started to convert existing constants to enums, for
example.

So I don't think that it is sound advice here to point to the code base.

If you _must_ value the existing practice over a clearly communicated
review, then please look no further than at the declaration of
`safe_create_leading_directories()`.

> Do you have examples in the code base where they are used for such a
> purpose along with regular `error(...)` calls?

It's all in the code. You don't need me to read it aloud for you.

:-)

Ciao,
Dscho

Patch

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;