[PATCH] status: fix branch shown when not only bisecting

[PATCH] status: fix branch shown when not only bisecting

[Rubén Justo]

In 83c750acde (wt-status.*: better advice for git status added,
2012-06-05), git-status received new informative messages to describe
the ongoing work in a worktree.

Multiple operations can be performed concurrently in a worktree.  For
example, during a rebase, the user can also perform a cherry-pick.  In
that situation, git-status only shows information about one of them,
prioritizing which one, in order: merge, am, rebase, cherry-pick.

However, when a bisect is also in progress, git-status includes, in
addition to the description of any other ongoing operation, the
description of the bisect.  This means that, in this case, it shows
description for two ongoing operations.

In 0722c805d6 (status: show the branch name if possible in in-progress
info, 2013-02-03), the messages introduced in 83c750acde were improved
to show, if possible, the branch in which the described operation was
initiated.

Unfortunately, git-status only records one branch related to one
operation.  However, in the situation described above, when a bisect is
combined with another operation, it is necessary to record two branches,
which, it is important to note, may be different.

Therefore, when that happens, we show incorrect information:

   $ git checkout -b bisect-branch
   $ git bisect start
   $ git checkout -b rebase-branch
   $ GIT_SEQUENCE_EDITOR='echo break >>' git rebase -i HEAD~
   $ git status
   interactive rebase in progress ...
   ...

   You are currently editing a commit while rebasing branch 'bisect-branch' on '...'.

   You are currently bisecting, started from branch 'bisect-branch'.

   ...

Days after 0722c805d6, this bisect circumstance hit us again; a leak was
introduced in 8b87cfd000 (wt-status: move strbuf into
read_and_strip_branch(), 2013-03-16).

Let's fix git-status to display accurate information and also fix the
leak, considering the branch where the bisect was started separately
from the branch related to other ongoing operations.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 branch.c               |  4 ++--
 ref-filter.c           |  2 +-
 t/t7512-status-help.sh | 30 ++++++++++++++++++++++++++++++
 worktree.c             |  4 ++--
 wt-status.c            |  7 ++++---
 wt-status.h            |  1 +
 6 files changed, 40 insertions(+), 8 deletions(-)

diff --git a/branch.c b/branch.c
index cdf70b0ef0..81c03b81b7 100644
--- a/branch.c
+++ b/branch.c
@@ -420,9 +420,9 @@ static void prepare_checked_out_branches(void)
 		wt_status_state_free_buffers(&state);
 
 		if (wt_status_check_bisect(wt, &state) &&
-		    state.branch) {
+		    state.bisecting_from) {
 			struct strbuf ref = STRBUF_INIT;
-			strbuf_addf(&ref, "refs/heads/%s", state.branch);
+			strbuf_addf(&ref, "refs/heads/%s", state.bisecting_from);
 			old = strmap_put(&current_checked_out_branches,
 					 ref.buf,
 					 xstrdup(wt->path));
diff --git a/ref-filter.c b/ref-filter.c
index f0a8e55270..526eebffad 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1894,7 +1894,7 @@ char *get_head_description(void)
 				    state.detached_from);
 	} else if (state.bisect_in_progress)
 		strbuf_addf(&desc, _("(no branch, bisect started on %s)"),
-			    state.branch);
+			    state.bisecting_from);
 	else if (state.detached_from) {
 		if (state.detached_at)
 			strbuf_addf(&desc, _("(HEAD detached at %s)"),
diff --git a/t/t7512-status-help.sh b/t/t7512-status-help.sh
index c2ab8a444a..bbb8e9b8b0 100755
--- a/t/t7512-status-help.sh
+++ b/t/t7512-status-help.sh
@@ -692,6 +692,36 @@ EOF
 '
 
 
+test_expect_success 'status when bisecting while rebasing' '
+	git reset --hard main &&
+	FAKE_LINES="exec_exit_15" &&
+	export FAKE_LINES &&
+	test_when_finished "git rebase --abort" &&
+	ONTO=$(git rev-parse --short HEAD^) &&
+	test_must_fail git rebase -i HEAD^ &&
+	git checkout -b bisect_while_rebasing &&
+	test_when_finished "git bisect reset" &&
+	git bisect start &&
+	TGT=$(git rev-parse --short two_bisect) &&
+	cat >expected <<EOF &&
+On branch bisect_while_rebasing
+Last command done (1 command done):
+   exec exit 15
+No commands remaining.
+You are currently editing a commit while rebasing branch '\''bisect'\'' on '\''$ONTO'\''.
+  (use "git commit --amend" to amend the current commit)
+  (use "git rebase --continue" once you are satisfied with your changes)
+
+You are currently bisecting, started from branch '\''bisect_while_rebasing'\''.
+  (use "git bisect reset" to get back to the original branch)
+
+nothing to commit (use -u to show untracked files)
+EOF
+	git status --untracked-files=no >actual &&
+	test_cmp expected actual
+'
+
+
 test_expect_success 'status when rebase --apply conflicts with statushints disabled' '
 	git reset --hard main &&
 	git checkout -b statushints_disabled &&
diff --git a/worktree.c b/worktree.c
index b8cf29e6a1..360e2b1866 100644
--- a/worktree.c
+++ b/worktree.c
@@ -395,9 +395,9 @@ int is_worktree_being_bisected(const struct worktree *wt,
 
 	memset(&state, 0, sizeof(state));
 	found_bisect = wt_status_check_bisect(wt, &state) &&
-		       state.branch &&
+		       state.bisecting_from &&
 		       skip_prefix(target, "refs/heads/", &target) &&
-		       !strcmp(state.branch, target);
+		       !strcmp(state.bisecting_from, target);
 	wt_status_state_free_buffers(&state);
 	return found_bisect;
 }
diff --git a/wt-status.c b/wt-status.c
index 5b1378965c..0eb2af63b6 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -861,6 +861,7 @@ void wt_status_state_free_buffers(struct wt_status_state *state)
 	FREE_AND_NULL(state->branch);
 	FREE_AND_NULL(state->onto);
 	FREE_AND_NULL(state->detached_from);
+	FREE_AND_NULL(state->bisecting_from);
 }
 
 static void wt_longstatus_print_unmerged(struct wt_status *s)
@@ -1569,10 +1570,10 @@ static void show_revert_in_progress(struct wt_status *s,
 static void show_bisect_in_progress(struct wt_status *s,
 				    const char *color)
 {
-	if (s->state.branch)
+	if (s->state.bisecting_from)
 		status_printf_ln(s, color,
 				 _("You are currently bisecting, started from branch '%s'."),
-				 s->state.branch);
+				 s->state.bisecting_from);
 	else
 		status_printf_ln(s, color,
 				 _("You are currently bisecting."));
@@ -1733,7 +1734,7 @@ int wt_status_check_bisect(const struct worktree *wt,
 
 	if (!stat(worktree_git_path(wt, "BISECT_LOG"), &st)) {
 		state->bisect_in_progress = 1;
-		state->branch = get_branch(wt, "BISECT_START");
+		state->bisecting_from = get_branch(wt, "BISECT_START");
 		return 1;
 	}
 	return 0;
diff --git a/wt-status.h b/wt-status.h
index ab9cc9d8f0..819dcad723 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -94,6 +94,7 @@ struct wt_status_state {
 	char *branch;
 	char *onto;
 	char *detached_from;
+	char *bisecting_from;
 	struct object_id detached_oid;
 	struct object_id revert_head_oid;
 	struct object_id cherry_pick_head_oid;
-- 
2.40.1

Re: [PATCH] status: fix branch shown when not only bisecting

[Junio C Hamano]

Rubén Justo <rjusto@gmail.com> writes:

> In 83c750acde (wt-status.*: better advice for git status added,
> 2012-06-05), git-status received new informative messages to describe
> the ongoing work in a worktree.
>
> Multiple operations can be performed concurrently in a worktree.  For
> example, during a rebase, the user can also perform a cherry-pick.

Hmph ...

> In
> that situation, git-status only shows information about one of them,
> prioritizing which one, in order: merge, am, rebase, cherry-pick.

... I have to wonder if it is a bug that "cherry-pick" proceeds when
there is an ongoing "rebase" going on, though.  When a sequencer is
driving a cherry-pick of master..topic1 and the user gets control
back in the middle, perhaps due to a conflict, should the user be
allowed to do "cherry-pick master..topic2", splicing these commits
from the other topic in the middle of the first cherry-pick session
the user started?

Re: [PATCH] status: fix branch shown when not only bisecting

[Rubén Justo]

On 31-jul-2023 16:45:56, Junio C Hamano wrote:
> Rubén Justo <rjusto@gmail.com> writes:
> 
> > In 83c750acde (wt-status.*: better advice for git status added,
> > 2012-06-05), git-status received new informative messages to describe
> > the ongoing work in a worktree.
> >
> > Multiple operations can be performed concurrently in a worktree.  For
> > example, during a rebase, the user can also perform a cherry-pick.
> 
> Hmph ...
> 
> > In
> > that situation, git-status only shows information about one of them,
> > prioritizing which one, in order: merge, am, rebase, cherry-pick.
> 
> ... I have to wonder if it is a bug that "cherry-pick" proceeds when
> there is an ongoing "rebase" going on, though.

Doing an interactive rebase, stop at some point, cherry-pick some commit
and let the rebase continue, it's not optimal but does not seem to me to
be bad workflow.

> When a sequencer is
> driving a cherry-pick of master..topic1 and the user gets control
> back in the middle, perhaps due to a conflict, should the user be
> allowed to do "cherry-pick master..topic2", splicing these commits
> from the other topic in the middle of the first cherry-pick session
> the user started?

We already prevent this to happen.  Maybe because we do not want to
support multiple .git/CHERRY_PICK_HEAD files.  Anyway, to me, sounds
like a reasonable thing to have: that nesting limit of 1.  The same for
the other operations.

Re: [PATCH] status: fix branch shown when not only bisecting

[Junio C Hamano]

Rubén Justo <rjusto@gmail.com> writes:

>> When a sequencer is
>> driving a cherry-pick of master..topic1 and the user gets control
>> back in the middle, perhaps due to a conflict, should the user be
>> allowed to do "cherry-pick master..topic2", splicing these commits
>> from the other topic in the middle of the first cherry-pick session
>> the user started?
>
> We already prevent this to happen.  Maybe because we do not want to
> support multiple .git/CHERRY_PICK_HEAD files.  Anyway, to me, sounds
> like a reasonable thing to have: that nesting limit of 1.  The same for
> the other operations.

OK, as long as we prevent such kinds of questionable combinations
("two multi-commit cherry-picks" was merely an example---I did not
mean that is the only problematic case), I do not see much problem
with it.  In any case, teaching "status" how to show such a state
with less information loss, which is the theme of this patch, is not
making things worse---even if some of the states may be nonsense and
should be prevented, "git status" is not the place to do so anyway.

I didn't see if the proposed output from the command makes sense
(yet), but somebody else may already have done so and writing their
reviews on their findings.  Let's see if we get any positive reviews
and move it to 'next' after that.

Will queue in the meantime not to lose it in 'seen'.

Thanks.

Re: [PATCH] status: fix branch shown when not only bisecting

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> I didn't see if the proposed output from the command makes sense
> (yet), but somebody else may already have done so and writing their
> reviews on their findings.  Let's see if we get any positive reviews
> and move it to 'next' after that.

And unfortunately nothing has happened.  Granted that summer in the
northern hemisphere is a slow season, but it is a bit disappointing.

[PATCH v2] status: fix branch shown when not only bisecting

[Rubén Justo]

In 83c750acde (wt-status.*: better advice for git status added,
2012-06-05), git-status received new informative messages to describe
the ongoing work in a worktree.

These messages were enhanced in 0722c805d6 (status: show the branch name
if possible in in-progress info, 2013-02-03), to show, if possible, the
branch where the operation was initiated.

Since then, we show incorrect information when several operations are in
progress and one of them is bisect:

   $ git checkout -b foo
   $ GIT_SEQUENCE_EDITOR='echo break >' git rebase -i HEAD~
   $ git checkout -b bar
   $ git bisect start
   $ git status
   ...

   You are currently editing a commit while rebasing branch 'bar' on '...'.

   You are currently bisecting, started from branch 'bar'.

   ...

Note that we erroneously say "while rebasing branch 'bar'" when we
should be referring to "foo".

This must have gone unnoticed for so long because it must be unusual to
start a bisection while another operation is in progress.  And even less
usual to involve different branches.

It caught my attention reviewing a leak introduced in 8b87cfd000
(wt-status: move strbuf into read_and_strip_branch(), 2013-03-16).

A simple change to deal with this situation can be to record in struct
wt_status_state, the branch where the bisect starts separately from the
branch related to other operations.

Let's do it and so we'll be able to display correct information and
we'll avoid the leak as well.

Signed-off-by: Rubén Justo <rjusto@gmail.com>

---

Let's try again.


 branch.c               |  4 ++--
 ref-filter.c           |  2 +-
 t/t7512-status-help.sh | 28 ++++++++++++++++++++++++++++
 worktree.c             |  4 ++--
 wt-status.c            |  7 ++++---
 wt-status.h            |  1 +
 6 files changed, 38 insertions(+), 8 deletions(-)

diff --git a/branch.c b/branch.c
index 06f7af9dd4..534594f7f8 100644
--- a/branch.c
+++ b/branch.c
@@ -420,9 +420,9 @@ static void prepare_checked_out_branches(void)
 		wt_status_state_free_buffers(&state);
 
 		if (wt_status_check_bisect(wt, &state) &&
-		    state.branch) {
+		    state.bisecting_from) {
 			struct strbuf ref = STRBUF_INIT;
-			strbuf_addf(&ref, "refs/heads/%s", state.branch);
+			strbuf_addf(&ref, "refs/heads/%s", state.bisecting_from);
 			old = strmap_put(&current_checked_out_branches,
 					 ref.buf,
 					 xstrdup(wt->path));
diff --git a/ref-filter.c b/ref-filter.c
index 1bfaf20fbf..7fd5548e93 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -2124,7 +2124,7 @@ char *get_head_description(void)
 				    state.detached_from);
 	} else if (state.bisect_in_progress)
 		strbuf_addf(&desc, _("(no branch, bisect started on %s)"),
-			    state.branch);
+			    state.bisecting_from);
 	else if (state.detached_from) {
 		if (state.detached_at)
 			strbuf_addf(&desc, _("(HEAD detached at %s)"),
diff --git a/t/t7512-status-help.sh b/t/t7512-status-help.sh
index c2ab8a444a..802f8f704c 100755
--- a/t/t7512-status-help.sh
+++ b/t/t7512-status-help.sh
@@ -692,6 +692,34 @@ EOF
 '
 
 
+test_expect_success 'status when bisecting while rebasing' '
+	git reset --hard main &&
+	test_when_finished "git rebase --abort" &&
+	ONTO=$(git rev-parse --short HEAD^) &&
+	FAKE_LINES="break" git rebase -i HEAD^ &&
+	test_when_finished "git checkout -" &&
+	git checkout -b bisect_while_rebasing &&
+	test_when_finished "git bisect reset" &&
+	git bisect start &&
+	cat >expected <<EOF &&
+On branch bisect_while_rebasing
+Last command done (1 command done):
+   break
+No commands remaining.
+You are currently editing a commit while rebasing branch '\''bisect'\'' on '\''$ONTO'\''.
+  (use "git commit --amend" to amend the current commit)
+  (use "git rebase --continue" once you are satisfied with your changes)
+
+You are currently bisecting, started from branch '\''bisect_while_rebasing'\''.
+  (use "git bisect reset" to get back to the original branch)
+
+nothing to commit (use -u to show untracked files)
+EOF
+	git status --untracked-files=no >actual &&
+	test_cmp expected actual
+'
+
+
 test_expect_success 'status when rebase --apply conflicts with statushints disabled' '
 	git reset --hard main &&
 	git checkout -b statushints_disabled &&
diff --git a/worktree.c b/worktree.c
index b8cf29e6a1..360e2b1866 100644
--- a/worktree.c
+++ b/worktree.c
@@ -395,9 +395,9 @@ int is_worktree_being_bisected(const struct worktree *wt,
 
 	memset(&state, 0, sizeof(state));
 	found_bisect = wt_status_check_bisect(wt, &state) &&
-		       state.branch &&
+		       state.bisecting_from &&
 		       skip_prefix(target, "refs/heads/", &target) &&
-		       !strcmp(state.branch, target);
+		       !strcmp(state.bisecting_from, target);
 	wt_status_state_free_buffers(&state);
 	return found_bisect;
 }
diff --git a/wt-status.c b/wt-status.c
index d03dfab9e4..dec28e8124 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -861,6 +861,7 @@ void wt_status_state_free_buffers(struct wt_status_state *state)
 	FREE_AND_NULL(state->branch);
 	FREE_AND_NULL(state->onto);
 	FREE_AND_NULL(state->detached_from);
+	FREE_AND_NULL(state->bisecting_from);
 }
 
 static void wt_longstatus_print_unmerged(struct wt_status *s)
@@ -1569,10 +1570,10 @@ static void show_revert_in_progress(struct wt_status *s,
 static void show_bisect_in_progress(struct wt_status *s,
 				    const char *color)
 {
-	if (s->state.branch)
+	if (s->state.bisecting_from)
 		status_printf_ln(s, color,
 				 _("You are currently bisecting, started from branch '%s'."),
-				 s->state.branch);
+				 s->state.bisecting_from);
 	else
 		status_printf_ln(s, color,
 				 _("You are currently bisecting."));
@@ -1733,7 +1734,7 @@ int wt_status_check_bisect(const struct worktree *wt,
 
 	if (!stat(worktree_git_path(wt, "BISECT_LOG"), &st)) {
 		state->bisect_in_progress = 1;
-		state->branch = get_branch(wt, "BISECT_START");
+		state->bisecting_from = get_branch(wt, "BISECT_START");
 		return 1;
 	}
 	return 0;
diff --git a/wt-status.h b/wt-status.h
index ab9cc9d8f0..819dcad723 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -94,6 +94,7 @@ struct wt_status_state {
 	char *branch;
 	char *onto;
 	char *detached_from;
+	char *bisecting_from;
 	struct object_id detached_oid;
 	struct object_id revert_head_oid;
 	struct object_id cherry_pick_head_oid;
-- 
2.40.1

Re: [PATCH v2] status: fix branch shown when not only bisecting

[Junio C Hamano]

Rubén Justo <rjusto@gmail.com> writes:

> In 83c750acde (wt-status.*: better advice for git status added,
> 2012-06-05), git-status received new informative messages to describe
> the ongoing work in a worktree.
>
> These messages were enhanced in 0722c805d6 (status: show the branch name
> if possible in in-progress info, 2013-02-03), to show, if possible, the
> branch where the operation was initiated.
>
> Since then, we show incorrect information when several operations are in
> progress and one of them is bisect:
>
>    $ git checkout -b foo
>    $ GIT_SEQUENCE_EDITOR='echo break >' git rebase -i HEAD~
>    $ git checkout -b bar
>    $ git bisect start
>    $ git status
>    ...
>
>    You are currently editing a commit while rebasing branch 'bar' on '...'.
>
>    You are currently bisecting, started from branch 'bar'.
>
>    ...
>
> Note that we erroneously say "while rebasing branch 'bar'" when we
> should be referring to "foo".
>
> This must have gone unnoticed for so long because it must be unusual to
> start a bisection while another operation is in progress.  And even less
> usual to involve different branches.
>
> It caught my attention reviewing a leak introduced in 8b87cfd000
> (wt-status: move strbuf into read_and_strip_branch(), 2013-03-16).
>
> A simple change to deal with this situation can be to record in struct
> wt_status_state, the branch where the bisect starts separately from the
> branch related to other operations.
>
> Let's do it and so we'll be able to display correct information and
> we'll avoid the leak as well.
>
> Signed-off-by: Rubén Justo <rjusto@gmail.com>
>
> ---
>
> Let's try again.

Sigh... nobody seems to be interested in making sure this correctly
improves the system X-<.

After a quick re-read, I didn't find anything glaringly wrong, so
let's see if anybody complains after the patch gets merged to
'next'.

Thanks.