Re: [PATCH v2 1/2] bisect: output state before we are ready to compute bisection

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

Chris Down <chris@chrisdown.name> writes:

> diff --git a/bisect.h b/bisect.h
> index 1015aeb8ea..ccd9ad31f6 100644
> --- a/bisect.h
> +++ b/bisect.h
> @@ -62,6 +62,12 @@ enum bisect_error {
>  	BISECT_INTERNAL_SUCCESS_MERGE_BASE = -11
>  };
>  
> +struct bisect_state {
> +	int nr_good;  /* How many good commits do we have? */
> +	int nr_bad;   /* How many bad commits do we have? (Well, you can only
> +			  have 0 or 1, but...) */
> +};

;-)  Multi-line comment style (cf. Documentation/CodingGuidelines)?

> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 8b2b259ff0..9d583f651c 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -329,12 +329,12 @@ static int check_and_set_terms(struct bisect_terms *terms, const char *cmd)
>  	return 0;
>  }
>  
> -static int mark_good(const char *refname, const struct object_id *oid,
> -		     int flag, void *cb_data)
> +static int inc_nr(const char *refname, const struct object_id *oid,
> +		  int flag, void *cb_data)
>  {
> -	int *m_good = (int *)cb_data;
> -	*m_good = 0;
> -	return 1;
> +	int *nr = (int *)cb_data;
> +	(*nr)++;
> +	return 0;
>  }

"mark_good" used to be a way to see if there is _any_ ref that match
the glob "refs/bisect/good-*"---the missing_good variable in the caller
is initialized to "true" (pessimism) and this callback is called for
each such ref.  Because the first such call is enough to say "yes,
we do have such a ref" without finding the second or subsequent "good"
ones, the function used to return 1 to stop for_each_glob_ref_in()
early.

All of that is now different here.  The helper is to "count" the
refs that match the glob.  So the caller will instead initialize the
counter to 0, and each time this callback is called, we increment it,
and because we do not want to stop the iteration early, we return 0.

All makes sense.

>  static const char need_bad_and_good_revision_warning[] =
> @@ -384,23 +384,49 @@ static int decide_next(const struct bisect_terms *terms,
>  			     vocab_good, vocab_bad, vocab_good, vocab_bad);
>  }
>  
> -static int bisect_next_check(const struct bisect_terms *terms,
> -			     const char *current_term)
> +static struct bisect_state bisect_status(const struct bisect_terms *terms)
>  {
> -	int missing_good = 1, missing_bad = 1;
>  	char *bad_ref = xstrfmt("refs/bisect/%s", terms->term_bad);
>  	char *good_glob = xstrfmt("%s-*", terms->term_good);
> +	struct bisect_state bs;
> +
> +	memset(&bs, 0, sizeof(bs));

We should just zero-initialize the struct by

	struct bisect_state bs = { 0 };

without memset().  But see below

>  	if (ref_exists(bad_ref))
> -		missing_bad = 0;
> +		bs.nr_bad = 1;
>  
> -	for_each_glob_ref_in(mark_good, good_glob, "refs/bisect/",
> -			     (void *) &missing_good);
> +	for_each_glob_ref_in(inc_nr, good_glob, "refs/bisect/",
> +			     (void *) &bs.nr_good);
>  	free(good_glob);
>  	free(bad_ref);
>  
> -	return decide_next(terms, current_term, missing_good, missing_bad);
> +	return bs;
> +}

Passing struct by value?

It is more usual in this codebase for the caller to prepare a struct
and give a pointer to it to a helper function like this one, i.e.

    static void bisect_status(struct bisect_state *state,
			      const struct bisect_terms *terms)
    {
	...
	for_each_glob_ref_in(count_matches, good_glob, "refs/bisect",
			     (void *) &state->nr_good);
    }

    static void bisect_print_status(const struct bisect_terms *terms)
    {
	struct bisect_state state = { 0 };

	bisect_status(&state, terms);
	...

would be more common.

> diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
> index 5382e5d216..a02587d1a7 100755
> --- a/t/t6030-bisect-porcelain.sh
> +++ b/t/t6030-bisect-porcelain.sh
> @@ -1025,4 +1025,22 @@ test_expect_success 'bisect visualize with a filename with dash and space' '
>  	git bisect visualize -p -- "-hello 2"
>  '
>  
> +test_expect_success 'bisect state output with multiple good commits' '
> +       git bisect reset &&
> +       git bisect start >output &&
> +       grep "waiting for both good and bad commits" output &&
> +       git bisect good "$HASH1" >output &&
> +       grep "waiting for bad commit, 1 good commit known" output &&
> +       git bisect good "$HASH2" >output &&
> +       grep "waiting for bad commit, 2 good commits known" output
> +'
> +
> +test_expect_success 'bisect state output with bad commit' '
> +       git bisect reset &&
> +       git bisect start >output &&
> +       grep "waiting for both good and bad commits" output &&
> +       git bisect bad "$HASH4" >output &&
> +       grep -F "waiting for good commit(s), bad commit known" output
> +'
> +
>  test_done

Looking good.

Thanks.