diff mbox series

[v2,05/11] bisect--helper: introduce new `decide_next()` function

Message ID 20200128144026.53128-6-mirucam@gmail.com (mailing list archive)
State New, archived
Headers show
Series Finish converting git bisect to C part 1 | expand

Commit Message

Miriam R. Jan. 28, 2020, 2:40 p.m. UTC 
From: Tanushree Tumane <tanushreetumane@gmail.com>

Let's refactor code from bisect_next_check() into a new
decide_next() helper function.

This removes some goto statements and makes the code simpler,
clearer and easier to understand.

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Tanushree Tumane <tanushreetumane@gmail.com>
Signed-off-by: Miriam Rubio <mirucam@gmail.com>
---
 builtin/bisect--helper.c | 62 +++++++++++++++++++++-------------------
 1 file changed, 32 insertions(+), 30 deletions(-)

Comments

Johannes Schindelin Jan. 30, 2020, 12:31 p.m. UTC | #1 
Hi Miriam,

On Tue, 28 Jan 2020, Miriam Rubio wrote:

> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 21de5c096c..826fcba2ed 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -291,26 +291,14 @@ static const char need_bisect_start_warning[] =
>  	   "You then need to give me at least one %s and %s revision.\n"
>  	   "You can use \"git bisect %s\" and \"git bisect %s\" for that.");
>
> -static int bisect_next_check(const struct bisect_terms *terms,
> -			     const char *current_term)
> +static int decide_next(const struct bisect_terms *terms,
> +		       const char *current_term, int missing_good,
> +		       int missing_bad)
>  {
> -	int missing_good = 1, missing_bad = 1, res = 0;
> -	const char *bad_ref = xstrfmt("refs/bisect/%s", terms->term_bad);
> -	const char *good_glob = xstrfmt("%s-*", terms->term_good);
> -
> -	if (ref_exists(bad_ref))
> -		missing_bad = 0;
> -
> -	for_each_glob_ref_in(mark_good, good_glob, "refs/bisect/",
> -			     (void *) &missing_good);
> -
>  	if (!missing_good && !missing_bad)
> -		goto finish;
> -
> -	if (!current_term) {
> -		res = -1;
> -		goto finish;
> -	}
> +		return 0;
> +	if (!current_term)
> +		return -1;
>  [...]
> +
> +static int bisect_next_check(const struct bisect_terms *terms,
> +			     const char *current_term)
> +{
> +	int missing_good = 1, missing_bad = 1;
> +	const char *bad_ref = xstrfmt("refs/bisect/%s", terms->term_bad);
> +	const char *good_glob = xstrfmt("%s-*", terms->term_good);
> +
> +	if (ref_exists(bad_ref))
> +		missing_bad = 0;
> +
> +	for_each_glob_ref_in(mark_good, good_glob, "refs/bisect/",
> +			     (void *) &missing_good);
> +
>  	free((void *) good_glob);
>  	free((void *) bad_ref);

I know this is not something you introduced, but while you are already in
the neighborhood, why not fix the types of `bad_ref` and `good_glob`? The
`xstrfmt()` function returns `char *` for a reason: so that you do not
have to cast it when `free()`ing the memory.

Thanks,
Dscho

> -	return res;
> +
> +	return decide_next(terms, current_term, missing_good, missing_bad);
>  }
>
>  static int get_terms(struct bisect_terms *terms)
> --
> 2.21.1 (Apple Git-122.3)
>
>
Miriam R. Jan. 30, 2020, 2:05 p.m. UTC | #2 
Hi,

El jue., 30 ene. 2020 a las 13:31, Johannes Schindelin
(<Johannes.Schindelin@gmx.de>) escribió:
>
> Hi Miriam,
>
> On Tue, 28 Jan 2020, Miriam Rubio wrote:
>
> > diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> > index 21de5c096c..826fcba2ed 100644
> > --- a/builtin/bisect--helper.c
> > +++ b/builtin/bisect--helper.c
> > @@ -291,26 +291,14 @@ static const char need_bisect_start_warning[] =
> >          "You then need to give me at least one %s and %s revision.\n"
> >          "You can use \"git bisect %s\" and \"git bisect %s\" for that.");
> >
> > -static int bisect_next_check(const struct bisect_terms *terms,
> > -                          const char *current_term)
> > +static int decide_next(const struct bisect_terms *terms,
> > +                    const char *current_term, int missing_good,
> > +                    int missing_bad)
> >  {
> > -     int missing_good = 1, missing_bad = 1, res = 0;
> > -     const char *bad_ref = xstrfmt("refs/bisect/%s", terms->term_bad);
> > -     const char *good_glob = xstrfmt("%s-*", terms->term_good);
> > -
> > -     if (ref_exists(bad_ref))
> > -             missing_bad = 0;
> > -
> > -     for_each_glob_ref_in(mark_good, good_glob, "refs/bisect/",
> > -                          (void *) &missing_good);
> > -
> >       if (!missing_good && !missing_bad)
> > -             goto finish;
> > -
> > -     if (!current_term) {
> > -             res = -1;
> > -             goto finish;
> > -     }
> > +             return 0;
> > +     if (!current_term)
> > +             return -1;
> >  [...]
> > +
> > +static int bisect_next_check(const struct bisect_terms *terms,
> > +                          const char *current_term)
> > +{
> > +     int missing_good = 1, missing_bad = 1;
> > +     const char *bad_ref = xstrfmt("refs/bisect/%s", terms->term_bad);
> > +     const char *good_glob = xstrfmt("%s-*", terms->term_good);
> > +
> > +     if (ref_exists(bad_ref))
> > +             missing_bad = 0;
> > +
> > +     for_each_glob_ref_in(mark_good, good_glob, "refs/bisect/",
> > +                          (void *) &missing_good);
> > +
> >       free((void *) good_glob);
> >       free((void *) bad_ref);
>
> I know this is not something you introduced, but while you are already in
> the neighborhood, why not fix the types of `bad_ref` and `good_glob`? The
> `xstrfmt()` function returns `char *` for a reason: so that you do not
> have to cast it when `free()`ing the memory.

Sure! I will fix this.
Thank you for reviewing.
Best,
Miriam

>
> Thanks,
> Dscho
>
> > -     return res;
> > +
> > +     return decide_next(terms, current_term, missing_good, missing_bad);
> >  }
> >
> >  static int get_terms(struct bisect_terms *terms)
> > --
> > 2.21.1 (Apple Git-122.3)
> >
> >
diff mbox series

Patch

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 21de5c096c..826fcba2ed 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -291,26 +291,14 @@  static const char need_bisect_start_warning[] =
 	   "You then need to give me at least one %s and %s revision.\n"
 	   "You can use \"git bisect %s\" and \"git bisect %s\" for that.");
 
-static int bisect_next_check(const struct bisect_terms *terms,
-			     const char *current_term)
+static int decide_next(const struct bisect_terms *terms,
+		       const char *current_term, int missing_good,
+		       int missing_bad)
 {
-	int missing_good = 1, missing_bad = 1, res = 0;
-	const char *bad_ref = xstrfmt("refs/bisect/%s", terms->term_bad);
-	const char *good_glob = xstrfmt("%s-*", terms->term_good);
-
-	if (ref_exists(bad_ref))
-		missing_bad = 0;
-
-	for_each_glob_ref_in(mark_good, good_glob, "refs/bisect/",
-			     (void *) &missing_good);
-
 	if (!missing_good && !missing_bad)
-		goto finish;
-
-	if (!current_term) {
-		res = -1;
-		goto finish;
-	}
+		return 0;
+	if (!current_term)
+		return -1;
 
 	if (missing_good && !missing_bad &&
 	    !strcmp(current_term, terms->term_good)) {
@@ -321,7 +309,7 @@  static int bisect_next_check(const struct bisect_terms *terms,
 		 */
 		warning(_("bisecting only with a %s commit"), terms->term_bad);
 		if (!isatty(0))
-			goto finish;
+			return 0;
 		/*
 		 * TRANSLATORS: Make sure to include [Y] and [n] in your
 		 * translation. The program will only accept English input
@@ -329,21 +317,35 @@  static int bisect_next_check(const struct bisect_terms *terms,
 		 */
 		yesno = git_prompt(_("Are you sure [Y/n]? "), PROMPT_ECHO);
 		if (starts_with(yesno, "N") || starts_with(yesno, "n"))
-			res = -1;
-		goto finish;
-	}
-	if (!is_empty_or_missing_file(git_path_bisect_start())) {
-		res = error(_(need_bad_and_good_revision_warning),
-			       vocab_bad, vocab_good, vocab_bad, vocab_good);
-	} else {
-		res = error(_(need_bisect_start_warning),
-			       vocab_good, vocab_bad, vocab_good, vocab_bad);
+			return -1;
+		return 0;
 	}
 
-finish:
+	if (!is_empty_or_missing_file(git_path_bisect_start()))
+		return error(_(need_bad_and_good_revision_warning),
+			     vocab_bad, vocab_good, vocab_bad, vocab_good);
+	else
+		return error(_(need_bisect_start_warning),
+			     vocab_good, vocab_bad, vocab_good, vocab_bad);
+}
+
+static int bisect_next_check(const struct bisect_terms *terms,
+			     const char *current_term)
+{
+	int missing_good = 1, missing_bad = 1;
+	const char *bad_ref = xstrfmt("refs/bisect/%s", terms->term_bad);
+	const char *good_glob = xstrfmt("%s-*", terms->term_good);
+
+	if (ref_exists(bad_ref))
+		missing_bad = 0;
+
+	for_each_glob_ref_in(mark_good, good_glob, "refs/bisect/",
+			     (void *) &missing_good);
+
 	free((void *) good_glob);
 	free((void *) bad_ref);
-	return res;
+
+	return decide_next(terms, current_term, missing_good, missing_bad);
 }
 
 static int get_terms(struct bisect_terms *terms)