[PATCH v2 0/2] bisect: status improvements when bisect is not fully fleshed out

[PATCH v2 0/2] bisect: status improvements when bisect is not fully fleshed out

[Chris Down]

When bisecting, we currently don't output anything before bisection
starts in earnest, which can result in some confusion. For example, in
the case illustrated in the first commit in this patch series, it's
trivial to accidentally misspell a tag or branch and accidentally end up
in an unintended state with no clear indication about what happened.

This patch series makes it so that we give information about bisect
state even before the bisect is ready to begin. We also store these
changes in state to the bisect log.

v2:

- Move to improve bisect output overall, instead of just warning for the
  specific unintended pathspec case.

Chris Down (2):
  bisect: output state before we are ready to compute bisection
  bisect: output bisect setup status in bisect log

 bisect.h                    |  6 ++++
 builtin/bisect--helper.c    | 69 ++++++++++++++++++++++++++++++-------
 t/t6030-bisect-porcelain.sh | 23 +++++++++++++
 3 files changed, 85 insertions(+), 13 deletions(-)


base-commit: f5aaf72f1b5aeb3b77bccabce014ea2590e823e2
-- 
2.36.0

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

[Chris Down]

Commit 73c6de06aff8 ("bisect: don't use invalid oid as rev when
starting") changes the behaviour of `git bisect` to consider invalid
oids as pathspecs again, as in the old shell implementation.

While that behaviour may be desirable, it can also cause confusion. For
example, while bisecting in a particular repo I encountered this:

    $ git bisect start d93ff48803f0 v6.3
    $

...which led to me sitting for a few moments, wondering why there's no
printout stating the first rev to check.

It turns out that the tag was actually "6.3", not "v6.3", and thus the
bisect was still silently started with only a bad rev, because
d93ff48803f0 was a valid oid and "v6.3" was silently considered to be a
pathspec.

While this behaviour may be desirable, it can be confusing, especially
with different repo conventions either using or not using "v" before
release names, or when a branch name or tag is simply misspelled on the
command line.

In order to avoid situations like this, make it more clear what we're
waiting for:

    $ git bisect start d93ff48803f0 v6.3
    status: waiting for good commit(s), bad commit known

We already have good output once the bisect process has begun in
earnest, so we don't need to do anything more there.

Signed-off-by: Chris Down <chris@chrisdown.name>
---
 bisect.h                    |  6 +++++
 builtin/bisect--helper.c    | 54 ++++++++++++++++++++++++++++---------
 t/t6030-bisect-porcelain.sh | 18 +++++++++++++
 3 files changed, 65 insertions(+), 13 deletions(-)

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...) */
+};
+
 enum bisect_error bisect_next_all(struct repository *r, const char *prefix);
 
 int estimate_bisect_steps(int all);
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;
 }
 
 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));
 
 	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;
+}
+
+static void bisect_print_status(const struct bisect_terms *terms)
+{
+	const struct bisect_state bs = bisect_status(terms);
+
+	/* If we had both, we'd already be started, and shouldn't get here. */
+	if (bs.nr_good && bs.nr_bad)
+		return;
+
+	if (!bs.nr_good && !bs.nr_bad)
+		printf(_("status: waiting for both good and bad commits\n"));
+	else if (bs.nr_good)
+		printf(Q_("status: waiting for bad commit, %d good commit known\n",
+			  "status: waiting for bad commit, %d good commits known\n",
+			  bs.nr_good), bs.nr_good);
+	else
+		printf(_("status: waiting for good commit(s), bad commit known\n"));
+}
+
+static int bisect_next_check(const struct bisect_terms *terms,
+			     const char *current_term)
+{
+	const struct bisect_state bs = bisect_status(terms);
+	return decide_next(terms, current_term, !bs.nr_good, !bs.nr_bad);
 }
 
 static int get_terms(struct bisect_terms *terms)
@@ -606,8 +632,10 @@ static enum bisect_error bisect_next(struct bisect_terms *terms, const char *pre
 
 static enum bisect_error bisect_auto_next(struct bisect_terms *terms, const char *prefix)
 {
-	if (bisect_next_check(terms, NULL))
+	if (bisect_next_check(terms, NULL)) {
+		bisect_print_status(terms);
 		return BISECT_OK;
+	}
 
 	return bisect_next(terms, prefix);
 }
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
-- 
2.36.0

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

[Taylor Blau]

On Fri, May 06, 2022 at 01:52:46AM +0100, Chris Down wrote:
> In order to avoid situations like this, make it more clear what we're
> waiting for:
>
>     $ git bisect start d93ff48803f0 v6.3
>     status: waiting for good commit(s), bad commit known

Makes sense. It would be kind of nice to realize that (in your example)
"v6.3" likely wasn't a pathspec that matched any files, either, and
after trying to DWIM it into something sensible, printed an error and
quit.

But I think the behavior here is slightly more subtle, since we really
care about whether or not a pathspec would match any revisions along the
bisection, not just the tips or the currently checked-out revision.

So I think the approach here makes sense.

> +static void bisect_print_status(const struct bisect_terms *terms)
> +{
> +	const struct bisect_state bs = bisect_status(terms);
> +
> +	/* If we had both, we'd already be started, and shouldn't get here. */
> +	if (bs.nr_good && bs.nr_bad)
> +		return;
> +
> +	if (!bs.nr_good && !bs.nr_bad)
> +		printf(_("status: waiting for both good and bad commits\n"));
> +	else if (bs.nr_good)
> +		printf(Q_("status: waiting for bad commit, %d good commit known\n",
> +			  "status: waiting for bad commit, %d good commits known\n",
> +			  bs.nr_good), bs.nr_good);
> +	else
> +		printf(_("status: waiting for good commit(s), bad commit known\n"));
> +}

Could or should these printf()'s be advise() calls instead? That would
make it easier for users to turn them off if they don't want the extra
output. At the very least, we should make sure that they are sent to
stderr to discourage scripting around them.

Thanks,
Taylor

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

[Chris Down]

Taylor Blau writes:
>Could or should these printf()'s be advise() calls instead? That would
>make it easier for users to turn them off if they don't want the extra
>output. At the very least, we should make sure that they are sent to
>stderr to discourage scripting around them.

Ah, I didn't know about advise(); looks like it could be reasonable to use 
here. If nobody objects, I'll change to advise() when the rest of the feedback 
has come in and v3 is sent.

Thanks!

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

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

>> +	if (!bs.nr_good && !bs.nr_bad)
>> +		printf(_("status: waiting for both good and bad commits\n"));
>> +	else if (bs.nr_good)
>> +		printf(Q_("status: waiting for bad commit, %d good commit known\n",
>> +			  "status: waiting for bad commit, %d good commits known\n",
>> +			  bs.nr_good), bs.nr_good);
>> +	else
>> +		printf(_("status: waiting for good commit(s), bad commit known\n"));
>> +}
>
> Could or should these printf()'s be advise() calls instead?

Given that existing bisect_next_all() mesasge to give estimates come
to the standard output, I do not think so.

	/*
	 * TRANSLATORS: the last %s will be replaced with "(roughly %d
	 * steps)" translation.
	 */
	printf(Q_("Bisecting: %d revision left to test after this %s\n",
		  "Bisecting: %d revisions left to test after this %s\n",
		  nr), nr, steps_msg);

I view these new messages as merely correcting the gap we used to
have.  We should have been giving feedback to the end-user when they
did something, but instead we were giving feedback only when we did
something, which resulted in the original "Huh?" that motivated this
series.

I actually wonder if we should do s/status:/Bisecting:/ to make the
messages even more uniform, but if we were to go in that direction
in the longer term, we'd probably be downcasing "Bisecting" to match
our error/warning/info messages.

Thanks.

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

[Junio C Hamano]

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.

[PATCH v2 2/2] bisect: output bisect setup status in bisect log

[Chris Down]

This allows seeing the current intermediate status without adding a new
good or bad commit:

    $ git bisect log | tail -1
    # status: waiting for bad commit, 1 good commit known

Signed-off-by: Chris Down <chris@chrisdown.name>
---
 builtin/bisect--helper.c    | 25 ++++++++++++++++++++-----
 t/t6030-bisect-porcelain.sh |  9 +++++++--
 2 files changed, 27 insertions(+), 7 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 9d583f651c..ef75f0a0ce 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -404,6 +404,21 @@ static struct bisect_state bisect_status(const struct bisect_terms *terms)
 	return bs;
 }
 
+__attribute__((format (printf, 1, 2)))
+static void bisect_log_printf(const char *fmt, ...)
+{
+	va_list ap;
+	char buf[1024];
+
+	va_start(ap, fmt);
+	if (vsnprintf(buf, sizeof(buf), fmt, ap) < 0)
+		*buf = '\0';
+	va_end(ap);
+
+	printf("%s", buf);
+	append_to_file(git_path_bisect_log(), "# %s", buf);
+}
+
 static void bisect_print_status(const struct bisect_terms *terms)
 {
 	const struct bisect_state bs = bisect_status(terms);
@@ -413,13 +428,13 @@ static void bisect_print_status(const struct bisect_terms *terms)
 		return;
 
 	if (!bs.nr_good && !bs.nr_bad)
-		printf(_("status: waiting for both good and bad commits\n"));
+		bisect_log_printf(_("status: waiting for both good and bad commits\n"));
 	else if (bs.nr_good)
-		printf(Q_("status: waiting for bad commit, %d good commit known\n",
-			  "status: waiting for bad commit, %d good commits known\n",
-			  bs.nr_good), bs.nr_good);
+		bisect_log_printf(Q_("status: waiting for bad commit, %d good commit known\n",
+				     "status: waiting for bad commit, %d good commits known\n",
+				     bs.nr_good), bs.nr_good);
 	else
-		printf(_("status: waiting for good commit(s), bad commit known\n"));
+		bisect_log_printf(_("status: waiting for good commit(s), bad commit known\n"));
 }
 
 static int bisect_next_check(const struct bisect_terms *terms,
diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index a02587d1a7..d16730a2e2 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -1029,18 +1029,23 @@ 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 log | grep "waiting for both good and bad commits" &&
        git bisect good "$HASH1" >output &&
        grep "waiting for bad commit, 1 good commit known" output &&
+       git bisect log | grep "waiting for bad commit, 1 good commit known" &&
        git bisect good "$HASH2" >output &&
-       grep "waiting for bad commit, 2 good commits known" output
+       grep "waiting for bad commit, 2 good commits known" output &&
+       git bisect log | grep "waiting for bad commit, 2 good commits known"
 '
 
 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 log | grep "waiting for both good and bad commits" &&
        git bisect bad "$HASH4" >output &&
-       grep -F "waiting for good commit(s), bad commit known" output
+       grep -F "waiting for good commit(s), bad commit known" output &&
+       git bisect log | grep -F "waiting for good commit(s), bad commit known"
 '
 
 test_done
-- 
2.36.0

Re: [PATCH v2 2/2] bisect: output bisect setup status in bisect log

[Taylor Blau]

On Fri, May 06, 2022 at 01:52:54AM +0100, Chris Down wrote:
> This allows seeing the current intermediate status without adding a new
> good or bad commit:
>
>     $ git bisect log | tail -1
>     # status: waiting for bad commit, 1 good commit known

Hmm. I was worried that this would make it harder to turn the output of
"git bisect log" into something you can inject into "git bisect replay
<log>". But it doesn't, because you prefix the status lines with a '#'
character.

That's good, and I think it's an improvement over what I'd currently
recommend, which would be something like:

    git bisect log | grep '^# bad:'
    git bisect log | grep '^# good:'

to see the state of our good and bad endpoints.

I'm not totally convinced it _needs_ to live in "git bisect log",
though, since it feels like additional information that is just added
for convenience. That's not the worst thing in the world, but I think
it would be fine to just take the first patch (with my suggestions
applied) as well.

> Signed-off-by: Chris Down <chris@chrisdown.name>
> ---
>  builtin/bisect--helper.c    | 25 ++++++++++++++++++++-----
>  t/t6030-bisect-porcelain.sh |  9 +++++++--
>  2 files changed, 27 insertions(+), 7 deletions(-)
>
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 9d583f651c..ef75f0a0ce 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -404,6 +404,21 @@ static struct bisect_state bisect_status(const struct bisect_terms *terms)
>  	return bs;
>  }
>
> +__attribute__((format (printf, 1, 2)))
> +static void bisect_log_printf(const char *fmt, ...)
> +{
> +	va_list ap;
> +	char buf[1024];
> +
> +	va_start(ap, fmt);
> +	if (vsnprintf(buf, sizeof(buf), fmt, ap) < 0)
> +		*buf = '\0';
> +	va_end(ap);
> +
> +	printf("%s", buf);
> +	append_to_file(git_path_bisect_log(), "# %s", buf);
> +}

This direct use of vsnprintf might be avoided by preparing the output in
bisect_print_status() via a strbuf and then calling:

    append_to_file(git_path_bisect_log(), "# %s", buf.buf).

>  static void bisect_print_status(const struct bisect_terms *terms)
>  {
>  	const struct bisect_state bs = bisect_status(terms);
> @@ -413,13 +428,13 @@ static void bisect_print_status(const struct bisect_terms *terms)
>  		return;
>
>  	if (!bs.nr_good && !bs.nr_bad)
> -		printf(_("status: waiting for both good and bad commits\n"));
> +		bisect_log_printf(_("status: waiting for both good and bad commits\n"));
>  	else if (bs.nr_good)
> -		printf(Q_("status: waiting for bad commit, %d good commit known\n",
> -			  "status: waiting for bad commit, %d good commits known\n",
> -			  bs.nr_good), bs.nr_good);
> +		bisect_log_printf(Q_("status: waiting for bad commit, %d good commit known\n",
> +				     "status: waiting for bad commit, %d good commits known\n",
> +				     bs.nr_good), bs.nr_good);
>  	else
> -		printf(_("status: waiting for good commit(s), bad commit known\n"));
> +		bisect_log_printf(_("status: waiting for good commit(s), bad commit known\n"));
>  }

Interesting; this patch removes the output that we were giving to users
in the last patch. Should it go to both places?

Thanks,
Taylor

Re: [PATCH v2 2/2] bisect: output bisect setup status in bisect log

[Chris Down]

Taylor Blau writes:
>On Fri, May 06, 2022 at 01:52:54AM +0100, Chris Down wrote:
>> This allows seeing the current intermediate status without adding a new
>> good or bad commit:
>>
>>     $ git bisect log | tail -1
>>     # status: waiting for bad commit, 1 good commit known
>
>Hmm. I was worried that this would make it harder to turn the output of
>"git bisect log" into something you can inject into "git bisect replay
><log>". But it doesn't, because you prefix the status lines with a '#'
>character.
>
>That's good, and I think it's an improvement over what I'd currently
>recommend, which would be something like:
>
>    git bisect log | grep '^# bad:'
>    git bisect log | grep '^# good:'
>
>to see the state of our good and bad endpoints.

>
>I'm not totally convinced it _needs_ to live in "git bisect log",
>though, since it feels like additional information that is just added
>for convenience. That's not the worst thing in the world, but I think
>it would be fine to just take the first patch (with my suggestions
>applied) as well.

There was some discussion in the v1 thread (Message-ID: 
<xmqqv8uo1mk6.fsf@gitster.g>) about adding an additional `git bisect 
status' command, but while writing it my immediate thoughts were that it 
doesn't make much sense to separate from the rest of the log. I'm curious what 
Junio's thoughts are on that, happy to do it whichever way is preferred. :-)

>> Signed-off-by: Chris Down <chris@chrisdown.name>
>> ---
>>  builtin/bisect--helper.c    | 25 ++++++++++++++++++++-----
>>  t/t6030-bisect-porcelain.sh |  9 +++++++--
>>  2 files changed, 27 insertions(+), 7 deletions(-)
>>
>> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
>> index 9d583f651c..ef75f0a0ce 100644
>> --- a/builtin/bisect--helper.c
>> +++ b/builtin/bisect--helper.c
>> @@ -404,6 +404,21 @@ static struct bisect_state bisect_status(const struct bisect_terms *terms)
>>  	return bs;
>>  }
>>
>> +__attribute__((format (printf, 1, 2)))
>> +static void bisect_log_printf(const char *fmt, ...)
>> +{
>> +	va_list ap;
>> +	char buf[1024];
>> +
>> +	va_start(ap, fmt);
>> +	if (vsnprintf(buf, sizeof(buf), fmt, ap) < 0)
>> +		*buf = '\0';
>> +	va_end(ap);
>> +
>> +	printf("%s", buf);
>> +	append_to_file(git_path_bisect_log(), "# %s", buf);
>> +}
>
>This direct use of vsnprintf might be avoided by preparing the output in
>bisect_print_status() via a strbuf and then calling:
>
>    append_to_file(git_path_bisect_log(), "# %s", buf.buf).

I'm not intimately familiar with the codebase, so maybe I'm missing something, 
but isn't it overkill to use a string buffer for something which is isn't going 
to then be used as a mutable buffer?

Happy to do it whichever way works for you folks, but would be good to 
understand the rationale so that I can write better patches next time :-)

>>  static void bisect_print_status(const struct bisect_terms *terms)
>>  {
>>  	const struct bisect_state bs = bisect_status(terms);
>> @@ -413,13 +428,13 @@ static void bisect_print_status(const struct bisect_terms *terms)
>>  		return;
>>
>>  	if (!bs.nr_good && !bs.nr_bad)
>> -		printf(_("status: waiting for both good and bad commits\n"));
>> +		bisect_log_printf(_("status: waiting for both good and bad commits\n"));
>>  	else if (bs.nr_good)
>> -		printf(Q_("status: waiting for bad commit, %d good commit known\n",
>> -			  "status: waiting for bad commit, %d good commits known\n",
>> -			  bs.nr_good), bs.nr_good);
>> +		bisect_log_printf(Q_("status: waiting for bad commit, %d good commit known\n",
>> +				     "status: waiting for bad commit, %d good commits known\n",
>> +				     bs.nr_good), bs.nr_good);
>>  	else
>> -		printf(_("status: waiting for good commit(s), bad commit known\n"));
>> +		bisect_log_printf(_("status: waiting for good commit(s), bad commit known\n"));
>>  }
>
>Interesting; this patch removes the output that we were giving to users
>in the last patch. Should it go to both places?

Not sure I understand, we printf() in bisect_log_printf, no?

Re: [PATCH v2 2/2] bisect: output bisect setup status in bisect log

[Taylor Blau]

On Fri, May 06, 2022 at 11:09:46AM +0100, Chris Down wrote:
> > I'm not totally convinced it _needs_ to live in "git bisect log",
> > though, since it feels like additional information that is just added
> > for convenience. That's not the worst thing in the world, but I think
> > it would be fine to just take the first patch (with my suggestions
> > applied) as well.
>
> There was some discussion in the v1 thread (Message-ID:
> <xmqqv8uo1mk6.fsf@gitster.g>) about adding an additional `git bisect status'
> command, but while writing it my immediate thoughts were that it doesn't
> make much sense to separate from the rest of the log. I'm curious what
> Junio's thoughts are on that, happy to do it whichever way is preferred. :-)

I don't have a strong feeling either way. The way that you have
incorporated it into the output of "git bisect log" feels simple and
sane to me.

The most important thing is that you didn't break the ability to pipe
the unmodified output of "git bisect log" into "git bisect replay", so I
beyond that I don't have any strong feelings about it.

> > > Signed-off-by: Chris Down <chris@chrisdown.name>
> > > ---
> > >  builtin/bisect--helper.c    | 25 ++++++++++++++++++++-----
> > >  t/t6030-bisect-porcelain.sh |  9 +++++++--
> > >  2 files changed, 27 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> > > index 9d583f651c..ef75f0a0ce 100644
> > > --- a/builtin/bisect--helper.c
> > > +++ b/builtin/bisect--helper.c
> > > @@ -404,6 +404,21 @@ static struct bisect_state bisect_status(const struct bisect_terms *terms)
> > >  	return bs;
> > >  }
> > >
> > > +__attribute__((format (printf, 1, 2)))
> > > +static void bisect_log_printf(const char *fmt, ...)
> > > +{
> > > +	va_list ap;
> > > +	char buf[1024];
> > > +
> > > +	va_start(ap, fmt);
> > > +	if (vsnprintf(buf, sizeof(buf), fmt, ap) < 0)
> > > +		*buf = '\0';
> > > +	va_end(ap);
> > > +
> > > +	printf("%s", buf);
> > > +	append_to_file(git_path_bisect_log(), "# %s", buf);
> > > +}
> >
> > This direct use of vsnprintf might be avoided by preparing the output in
> > bisect_print_status() via a strbuf and then calling:
> >
> >    append_to_file(git_path_bisect_log(), "# %s", buf.buf).
>
> I'm not intimately familiar with the codebase, so maybe I'm missing
> something, but isn't it overkill to use a string buffer for something which
> is isn't going to then be used as a mutable buffer?
>
> Happy to do it whichever way works for you folks, but would be good to
> understand the rationale so that I can write better patches next time :-)

I'm trying to avoid having a vsnprintf() when one isn't strictly needed.
This part of bisect isn't performance critical code, so having a strbuf
around doesn't bother me in the slightest.

If you really wanted to, you could treat the strbuf like a static buffer
by declaring it as such and calling strbuf_reset() before or after using
it. But allocating little bits of memory in bisect_print_status()
doesn't bother me much, either.

At the very least, it would dramatically simplify the implementation of
this patch with a negligible cost, so I'd recommend doing it.

> > >  static void bisect_print_status(const struct bisect_terms *terms)
> > >  {
> > >  	const struct bisect_state bs = bisect_status(terms);
> > > @@ -413,13 +428,13 @@ static void bisect_print_status(const struct bisect_terms *terms)
> > >  		return;
> > >
> > >  	if (!bs.nr_good && !bs.nr_bad)
> > > -		printf(_("status: waiting for both good and bad commits\n"));
> > > +		bisect_log_printf(_("status: waiting for both good and bad commits\n"));
> > >  	else if (bs.nr_good)
> > > -		printf(Q_("status: waiting for bad commit, %d good commit known\n",
> > > -			  "status: waiting for bad commit, %d good commits known\n",
> > > -			  bs.nr_good), bs.nr_good);
> > > +		bisect_log_printf(Q_("status: waiting for bad commit, %d good commit known\n",
> > > +				     "status: waiting for bad commit, %d good commits known\n",
> > > +				     bs.nr_good), bs.nr_good);
> > >  	else
> > > -		printf(_("status: waiting for good commit(s), bad commit known\n"));
> > > +		bisect_log_printf(_("status: waiting for good commit(s), bad commit known\n"));
> > >  }
> >
> > Interesting; this patch removes the output that we were giving to users
> > in the last patch. Should it go to both places?
>
> Not sure I understand, we printf() in bisect_log_printf, no?

I missed that, thanks for pointing it out.

Thanks,
Taylor

Re: [PATCH v2 2/2] bisect: output bisect setup status in bisect log

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

> On Fri, May 06, 2022 at 01:52:54AM +0100, Chris Down wrote:
>> This allows seeing the current intermediate status without adding a new
>> good or bad commit:
>>
>>     $ git bisect log | tail -1
>>     # status: waiting for bad commit, 1 good commit known
>
> Hmm. I was worried that this would make it harder to turn the output of
> "git bisect log" into something you can inject into "git bisect replay
> <log>". But it doesn't, because you prefix the status lines with a '#'
> character.

;-) 

It has always been the basic design of "git bisect replay" since the
old days back when the command was scripted.  The log is designed to
be an executable shell script and we do not need to use "replay" at
all.

> I'm not totally convinced it _needs_ to live in "git bisect log",

It is a convenient place and other "helpful" comments are also
stored there, so the existing users may already know to look into it
to reorient where they were.

The original suggestion during the previous round of this series was
to come up with a way to help those who get lost during a long
session of spelunking the history to start bisection, like so:

 * start a bisection session
 * find one end (perhaps bad end)
 * say "git bisect bad $this" and be told "now go find a good one"
 * ran around various versions with "git reset --hard $that &&
   make" to see the other end (perhaps good end).
 * forget where they were---was it their turn to feed a good
   revision or bad revision?
 * "git bisect status" can be used to remind them that they were
   told "now go find a good one".

And I actually find that "go to log and see what progress you made
so far" as implemented in this series a far easier and scalable
solution than "git bisect status".  Next time we add more "here is
an extra piece of information to help reorient yourself", we do not
have to worry about how to present it in "git bisect status" output
(and we do not have to worry about teaching "bisect status"
subcommand about it in the first place).  As long as we make sure
that everybody uses the new "bisect_log_printf()" wrapper for
informational messages, we get it for free.

Thanks.

Re: [PATCH v2 2/2] bisect: output bisect setup status in bisect log

[Junio C Hamano]

Chris Down <chris@chrisdown.name> writes:

> This allows seeing the current intermediate status without adding a new
> good or bad commit:
>
>     $ git bisect log | tail -1
>     # status: waiting for bad commit, 1 good commit known

Clever.  As we are keeping the "log" of what we have done so far,
it is a good place to record this new piece of info, too.  I find
it much better than my earlier suggestion to add "bisect status"
subcommand.

> +__attribute__((format (printf, 1, 2)))
> +static void bisect_log_printf(const char *fmt, ...)
> +{
> +	va_list ap;
> +	char buf[1024];
> +
> +	va_start(ap, fmt);
> +	if (vsnprintf(buf, sizeof(buf), fmt, ap) < 0)
> +		*buf = '\0';
> +	va_end(ap);

We probably should use strbuf_addf() or something so that we do not
even have to wonder if 1024 is large enough for everybody.  That
would be in line with what is done in other parts of the codebase,
e.g. bisect_next_all() uses xstrfmt() that is a thin wrapper around
strbuf to format the estimated steps.

Thanks.

Re: [PATCH v2 2/2] bisect: output bisect setup status in bisect log

[Junio C Hamano]

Chris Down <chris@chrisdown.name> writes:

> +__attribute__((format (printf, 1, 2)))
> +static void bisect_log_printf(const char *fmt, ...)
> +{
> +	va_list ap;
> +	char buf[1024];
> +
> +	va_start(ap, fmt);
> +	if (vsnprintf(buf, sizeof(buf), fmt, ap) < 0)
> +		*buf = '\0';
> +	va_end(ap);

We should just do

	struct strbuf buf = STRBUF_INIT;

	va_start(ap, fmt);
	strbuf_vaddf(&buf, fmt, ap);
	va_end(ap);

> +	printf("%s", buf);
> +	append_to_file(git_path_bisect_log(), "# %s", buf);

and free the resource with

	strbuf_release(&buf);

I think.

> +}

> diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
> index a02587d1a7..d16730a2e2 100755
> --- a/t/t6030-bisect-porcelain.sh
> +++ b/t/t6030-bisect-porcelain.sh
> @@ -1029,18 +1029,23 @@ 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 log | grep "waiting for both good and bad commits" &&

Having "git" command on the left hand side of pipe would hide a
failure signalled by its exit status from the command.  The "but if
the command fails, how likely would we see the expected output to
its standard ouput?" argument aside, it is more common to write

	  git bisect log >output &&
	  grep "..." &&

instead.

Thanks.

Re: [PATCH v2 2/2] bisect: output bisect setup status in bisect log

[Taylor Blau]

On Fri, May 06, 2022 at 11:18:25AM -0700, Junio C Hamano wrote:
> Chris Down <chris@chrisdown.name> writes:
>
> > +__attribute__((format (printf, 1, 2)))
> > +static void bisect_log_printf(const char *fmt, ...)
> > +{
> > +	va_list ap;
> > +	char buf[1024];
> > +
> > +	va_start(ap, fmt);
> > +	if (vsnprintf(buf, sizeof(buf), fmt, ap) < 0)
> > +		*buf = '\0';
> > +	va_end(ap);
>
> We should just do
>
> 	struct strbuf buf = STRBUF_INIT;
>
> 	va_start(ap, fmt);
> 	strbuf_vaddf(&buf, fmt, ap);
> 	va_end(ap);
>
> > +	printf("%s", buf);
> > +	append_to_file(git_path_bisect_log(), "# %s", buf);
>
> and free the resource with
>
> 	strbuf_release(&buf);
>
> I think.

I don't think so. bisect_log_printf() has only one caller, which is
bisect_print_status(). Couldn't the latter manage its own strbuf without
the need to introduce a new varargs function?

Thanks,
Taylor

Re: [PATCH v2 2/2] bisect: output bisect setup status in bisect log

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

> On Fri, May 06, 2022 at 11:18:25AM -0700, Junio C Hamano wrote:
>> Chris Down <chris@chrisdown.name> writes:
>>
>> > +__attribute__((format (printf, 1, 2)))
>> > +static void bisect_log_printf(const char *fmt, ...)
>> > +{
>> > +	va_list ap;
>> > +	char buf[1024];
>> > +
>> > +	va_start(ap, fmt);
>> > +	if (vsnprintf(buf, sizeof(buf), fmt, ap) < 0)
>> > +		*buf = '\0';
>> > +	va_end(ap);
>>
>> We should just do
>>
>> 	struct strbuf buf = STRBUF_INIT;
>>
>> 	va_start(ap, fmt);
>> 	strbuf_vaddf(&buf, fmt, ap);
>> 	va_end(ap);
>>
>> > +	printf("%s", buf);
>> > +	append_to_file(git_path_bisect_log(), "# %s", buf);
>>
>> and free the resource with
>>
>> 	strbuf_release(&buf);
>>
>> I think.
>
> I don't think so. bisect_log_printf() has only one caller, which is
> bisect_print_status(). Couldn't the latter manage its own strbuf without
> the need to introduce a new varargs function?

I actually was hoping that other existing informational messages
prefixed with "Bisecting:" (i.e. those that tells you how many steps
are remaining) can go as similar comments to the log file; they are
currently written with plain-vanilla printf(3), and could use this
one instead.

Re: [PATCH v2 2/2] bisect: output bisect setup status in bisect log

[Taylor Blau]

On Mon, May 09, 2022 at 09:08:46AM -0700, Junio C Hamano wrote:
> > I don't think so. bisect_log_printf() has only one caller, which is
> > bisect_print_status(). Couldn't the latter manage its own strbuf without
> > the need to introduce a new varargs function?
>
> I actually was hoping that other existing informational messages
> prefixed with "Bisecting:" (i.e. those that tells you how many steps
> are remaining) can go as similar comments to the log file; they are
> currently written with plain-vanilla printf(3), and could use this
> one instead.

I see. In that case I agree that we this function should take varargs,
and it should pass that va_list to strbuf_vaddf().

But this patch shouldn't reimplement the wheel here with a bare
vsnprintf() call that could be replaced with a strbuf.

Thanks,
Taylor

Re: [PATCH v2 0/2] bisect: status improvements when bisect is not fully fleshed out

[Chris Down]

Thanks Junio and Taylor for reviewing. I have the following action items for 
v3:

# [1/3] bisect: lowercase "Bisect:" to "bisect:" prior to wider use

- New patch

# [2/3] bisect: output state before we are ready to compute bisection 

- Fix multiline comment style in bisect.h
- Zero-initialise bisect_state directly, don't use memset()
- Pass the bisect state struct as an argument into bisect_print_status
- Change from "status:" to "bisecting:"

# [3/3] bisect: output bisect setup status in bisect log

- Use strbuf in bisect_log_printf
- Change `git bisect log' use an output file in tests instead of piping

I'll wait a bit to see if there's any further feedback and then will send v3.

Thanks!

Re: [PATCH v2 0/2] bisect: status improvements when bisect is not fully fleshed out

[Junio C Hamano]

Chris Down <chris@chrisdown.name> writes:

> Thanks Junio and Taylor for reviewing. I have the following action
> items for v3:
>
> # [1/3] bisect: lowercase "Bisect:" to "bisect:" prior to wider use
>
> - New patch

My preference actually were to leave this change out of the topic.
That is, instead of using "status:" in newer messages, have them use
the same "Bisecting:", so that all the "where we are in the bisect
session?" messages from the command use that same prefix.

I also wonder if the existing "Bisecting:" messages should also be
sent as comment to the log file, using the same bisect_log_printf()
helper (with the v2 patches, they are still using printf() and sent
only to the standard output).

But this, just like "status:" -> "Bisecting:" -> "bisecting:" you
reacted to, is just "I wonder...", and is not a suggestion to make
changes as part of this series.  Something to think about for a
possible follow-up after we complete this topic.

But I do not mind if you want to go the extra mile to do all of the
above as part of the series.  It would make the series to require
more reviews, which is why I generally recommend against extending
the scope of the (initial) topic too much and instead leave as much
additional changes to follow-up series after the initial series is
done.

> # [2/3] bisect: output state before we are ready to compute bisection 
> - Fix multiline comment style in bisect.h
> - Zero-initialise bisect_state directly, don't use memset()
> - Pass the bisect state struct as an argument into bisect_print_status
> - Change from "status:" to "bisecting:"
>
> # [3/3] bisect: output bisect setup status in bisect log
>
> - Use strbuf in bisect_log_printf
> - Change `git bisect log' use an output file in tests instead of piping
>
> I'll wait a bit to see if there's any further feedback and then will send v3.
>
> Thanks!

Re: [PATCH v2 0/2] bisect: status improvements when bisect is not fully fleshed out

[Chris Down]

Junio C Hamano writes:
>Chris Down <chris@chrisdown.name> writes:
>
>> Thanks Junio and Taylor for reviewing. I have the following action
>> items for v3:
>>
>> # [1/3] bisect: lowercase "Bisect:" to "bisect:" prior to wider use
>>
>> - New patch
>
>My preference actually were to leave this change out of the topic.
>That is, instead of using "status:" in newer messages, have them use
>the same "Bisecting:", so that all the "where we are in the bisect
>session?" messages from the command use that same prefix.

Sure, that also works.

>I also wonder if the existing "Bisecting:" messages should also be
>sent as comment to the log file, using the same bisect_log_printf()
>helper (with the v2 patches, they are still using printf() and sent
>only to the standard output).
>
>But this, just like "status:" -> "Bisecting:" -> "bisecting:" you
>reacted to, is just "I wonder...", and is not a suggestion to make
>changes as part of this series.  Something to think about for a
>possible follow-up after we complete this topic.
>
>But I do not mind if you want to go the extra mile to do all of the
>above as part of the series.  It would make the series to require
>more reviews, which is why I generally recommend against extending
>the scope of the (initial) topic too much and instead leave as much
>additional changes to follow-up series after the initial series is
>done.

Sounds good then, I'll send another one after this goes in :-)

Thanks!