threads / patch / 60042

patchstatus: fix branch shown when not only bisecting

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

## tl;dr

7 messages between Jul 30, 2023 and Oct 16, 2023. Diffs are folded; open one to read it.

replies: 6people: 2as markdown or json

Rubén Justo· Jul 30, 2023, 01:07 UTC · lore

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(-)
Show changes to 6 files +40 −8

branch.c, ref-filter.c, t/t7512-status-help.sh, worktree.c, wt-status.c, wt-status.h

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
Junio C Hamano· Jul 31, 2023, 23:45 UTC · re: Rubén Justo · lore

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

Rubén Justo <rjusto@gmail.com> writes:
Show 6 quoted lines
> 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?

Rubén Justo· Aug 1, 2023, 20:39 UTC · re: Junio C Hamano · lore

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

On 31-jul-2023 16:45:56, Junio C Hamano wrote:
Show 17 quoted lines
> 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.

Show 6 quoted lines
> 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.

Junio C Hamano· Aug 1, 2023, 20:45 UTC · re: Rubén Justo · lore

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

Rubén Justo <rjusto@gmail.com> writes:
Show 11 quoted lines
>> 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.
Junio C Hamano· Aug 16, 2023, 04:24 UTC · re: Junio C Hamano · lore

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

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.

Rubén Justo· Sep 9, 2023, 20:12 UTC · re: Rubén Justo · lore

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

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(-)
Show changes to 6 files +38 −8

branch.c, ref-filter.c, t/t7512-status-help.sh, worktree.c, wt-status.c, wt-status.h

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
Junio C Hamano· Oct 16, 2023, 22:08 UTC · re: Rubén Justo · lore

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

Rubén Justo <rjusto@gmail.com> writes:
Show 46 quoted lines
> 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.

← back to recent threads