git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / Atom feed
* [PATCH v4] branch: let '--edit-description' default to rebased/bisected branch
@ 2020-01-12  6:47 marcandre.lureau
  2020-01-12 10:17 ` Eric Sunshine
  0 siblings, 1 reply; 3+ messages in thread
From: marcandre.lureau @ 2020-01-12  6:47 UTC (permalink / raw)
  To: git; +Cc: szeder.dev, sunshine, Marc-André Lureau

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Defaulting to editing the description of the rebased or bisected branch
without an explicit branchname argument would be useful.  Even the git
bash prompt shows the name of the rebased branch, and then

  ~/src/git (mybranch|REBASE-i 1/2)$ git branch --edit-description
  fatal: Cannot give description to detached HEAD

looks quite unhelpful.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
Changed in v4:
 - use wt_status_get_state() that handles bisect state
 - add a bisecting test

builtin/branch.c  | 41 ++++++++++++++++++++++++++++++-----------
 t/t3200-branch.sh | 35 +++++++++++++++++++++++++++++++++++
 2 files changed, 65 insertions(+), 11 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index d8297f80ff..cda9fd53e6 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -745,33 +745,52 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		string_list_clear(&output, 0);
 		return 0;
 	} else if (edit_description) {
-		const char *branch_name;
+		char *branch_name = NULL;
 		struct strbuf branch_ref = STRBUF_INIT;
 
 		if (!argc) {
-			if (filter.detached)
-				die(_("Cannot give description to detached HEAD"));
-			branch_name = head;
+			if (!filter.detached)
+				branch_name = xstrdup(head);
+			else {
+				struct wt_status_state state;
+
+				memset(&state, 0, sizeof(state));
+				wt_status_get_state(the_repository, &state, 0);
+				branch_name = state.branch;
+				if (!branch_name)
+					die(_("Cannot give description to detached HEAD"));
+				free(state.onto);
+				free(state.detached_from);
+			}
 		} else if (argc == 1)
-			branch_name = argv[0];
+			branch_name = xstrdup(argv[0]);
 		else
 			die(_("cannot edit description of more than one branch"));
 
 		strbuf_addf(&branch_ref, "refs/heads/%s", branch_name);
 		if (!ref_exists(branch_ref.buf)) {
-			strbuf_release(&branch_ref);
+			int ret;
 
 			if (!argc)
-				return error(_("No commit on branch '%s' yet."),
-					     branch_name);
+				ret = error(_("No commit on branch '%s' yet."),
+					    branch_name);
 			else
-				return error(_("No branch named '%s'."),
-					     branch_name);
+				ret = error(_("No branch named '%s'."),
+					    branch_name);
+
+			strbuf_release(&branch_ref);
+			free(branch_name);
+			return ret;
+
 		}
 		strbuf_release(&branch_ref);
 
-		if (edit_branch_description(branch_name))
+		if (edit_branch_description(branch_name)) {
+			free(branch_name);
 			return 1;
+		}
+
+		free(branch_name);
 	} else if (copy) {
 		if (!argc)
 			die(_("branch name required"));
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 411a70b0ce..7ea6876fe7 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -1260,6 +1260,41 @@ test_expect_success 'use --edit-description' '
 	test_cmp expect EDITOR_OUTPUT
 '
 
+test_expect_success 'use --edit-description during rebase' '
+	write_script editor <<-\EOF &&
+		echo "Rebase contents" >"$1"
+	EOF
+	(
+		set_fake_editor &&
+		FAKE_LINES="break 1" git rebase -i HEAD^ &&
+		EDITOR=./editor git branch --edit-description &&
+		git rebase --continue
+	) &&
+	write_script editor <<-\EOF &&
+		git stripspace -s <"$1" >"EDITOR_OUTPUT"
+	EOF
+	EDITOR=./editor git branch --edit-description &&
+	echo "Rebase contents" >expect &&
+	test_cmp expect EDITOR_OUTPUT
+'
+
+test_expect_success 'use --edit-description during bisect' '
+	write_script editor <<-\EOF &&
+		echo "Bisect contents" >"$1"
+	EOF
+	git bisect start &&
+	git bisect bad &&
+	git bisect good HEAD~2 &&
+	EDITOR=./editor git branch --edit-description &&
+	git bisect reset &&
+	write_script editor <<-\EOF &&
+		git stripspace -s <"$1" >"EDITOR_OUTPUT"
+	EOF
+	EDITOR=./editor git branch --edit-description &&
+	echo "Bisect contents" >expect &&
+	test_cmp expect EDITOR_OUTPUT
+'
+
 test_expect_success 'detect typo in branch name when using --edit-description' '
 	write_script editor <<-\EOF &&
 		echo "New contents" >"$1"

base-commit: 7a6a90c6ec48fc78c83d7090d6c1b95d8f3739c0
-- 
2.25.0.rc2.1.ga00adf396b.dirty


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v4] branch: let '--edit-description' default to rebased/bisected branch
  2020-01-12  6:47 [PATCH v4] branch: let '--edit-description' default to rebased/bisected branch marcandre.lureau
@ 2020-01-12 10:17 ` Eric Sunshine
  2020-01-12 10:55   ` Eric Sunshine
  0 siblings, 1 reply; 3+ messages in thread
From: Eric Sunshine @ 2020-01-12 10:17 UTC (permalink / raw)
  To: marcandre.lureau; +Cc: git, szeder.dev

On Sun, Jan 12, 2020 at 10:47:06AM +0400, marcandre.lureau@redhat.com wrote:
> diff --git a/builtin/branch.c b/builtin/branch.c
> @@ -745,33 +745,52 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
>  		string_list_clear(&output, 0);
>  		return 0;
>  	} else if (edit_description) {
> -		const char *branch_name;
> +		char *branch_name = NULL;

Do you need to assign NULL here? Doesn't 'branch_name' get assigned in
all cases in which the code doesn't otherwise die()?

>  		if (!argc) {
> -			if (filter.detached)
> -				die(_("Cannot give description to detached HEAD"));
> -			branch_name = head;
> +			if (!filter.detached)
> +				branch_name = xstrdup(head);
> +			else {
> +				struct wt_status_state state;
> +
> +				memset(&state, 0, sizeof(state));
> +				wt_status_get_state(the_repository, &state, 0);
> +				branch_name = state.branch;
> +				if (!branch_name)
> +					die(_("Cannot give description to detached HEAD"));
> +				free(state.onto);
> +				free(state.detached_from);

I was wondering if it would make sense to attempt this branch name
lookup much earlier in the function when it assigns 'head' (if 'head'
is detached), with the idea that perhaps other git-branch modes might
benefit from it rather than doing it only for this one special-case.
However, it looks like other code (such as branch copy and branch
rename) would actively be hurt by such a change.

At any rate, it might make the 'edit_description' case easier to read
if this special-case branch lookup code was factored out into its own
function. Not itself worth a re-roll, but something to consider if you
do re-roll.

> +			}
>  		} else if (argc == 1)
> -			branch_name = argv[0];
> +			branch_name = xstrdup(argv[0]);
>  		else
>  			die(_("cannot edit description of more than one branch"));
>  
>  		strbuf_addf(&branch_ref, "refs/heads/%s", branch_name);
>  		if (!ref_exists(branch_ref.buf)) {
> -			strbuf_release(&branch_ref);
> +			int ret;
>  
>  			if (!argc)
> -				return error(_("No commit on branch '%s' yet."),
> -					     branch_name);
> +				ret = error(_("No commit on branch '%s' yet."),
> +					    branch_name);
>  			else
> -				return error(_("No branch named '%s'."),
> -					     branch_name);
> +				ret = error(_("No branch named '%s'."),
> +					    branch_name);
> +
> +			strbuf_release(&branch_ref);
> +			free(branch_name);
> +			return ret;
> +
>  		}

Unnecessary blank line after 'return'.

A couple observations...

The extra cleanup needed to handle 'branch_name' makes this code quite
a bit more verbose. I was wondering if it would be possible to
consolidate the cleanup in a "failure path" as the target of a 'goto'
(which is a common way to perform cleanup in the Git code-base).
However, doing it that way doesn't really make the code much nicer,
which leads to the next observation...

Those `return error(...)` invocations are anomalies in this function.
Every other case of error in cmd_branch() simply die()s -- without
bothering to clean up. There is no apparent reason why this code
instead uses error(). Changing these two cases to die() would simplify
cleanup since you wouldn't have to do any, which would make the code
clearer, shorter, and more consistent with the rest of cmd_branch().
(Such a change probably ought to be done first in a preparatory patch,
making this a two-patch series.)

>  		strbuf_release(&branch_ref);
>  
> -		if (edit_branch_description(branch_name))
> +		if (edit_branch_description(branch_name)) {
> +			free(branch_name);
>  			return 1;
> +		}
> +
> +		free(branch_name);

Taking the above comments and observations into account, perhaps
something like this would be cleaner:

--- >8 ---
diff --git a/builtin/branch.c b/builtin/branch.c
index d8297f80ff..0eb519561e 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -601,6 +601,22 @@ static int edit_branch_description(const char *branch_name)
 	return 0;
 }
 
+/*
+ * Return branch name of current worktree -- even if HEAD is detached -- or
+ * NULL if no branch is associated with worktree. Caller is responsible for
+ * freeing result.
+ */
+static char *get_worktree_branch()
+{
+	struct wt_status_state state;
+
+	memset(&state, 0, sizeof(state));
+	wt_status_get_state(the_repository, &state, 0);
+	free(state.onto);
+	free(state.detached_from);
+	return state.branch;
+}
+
 int cmd_branch(int argc, const char **argv, const char *prefix)
 {
 	int delete = 0, rename = 0, copy = 0, force = 0, list = 0;
@@ -745,13 +761,16 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		string_list_clear(&output, 0);
 		return 0;
 	} else if (edit_description) {
+		int ret;
 		const char *branch_name;
+		char *to_free = NULL;
 		struct strbuf branch_ref = STRBUF_INIT;
 
 		if (!argc) {
-			if (filter.detached)
+			if (!filter.detached)
+				branch_name = head;
+			else if (!(branch_name = to_free = get_worktree_branch()))
 				die(_("Cannot give description to detached HEAD"));
-			branch_name = head;
 		} else if (argc == 1)
 			branch_name = argv[0];
 		else
@@ -759,19 +778,16 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 
 		strbuf_addf(&branch_ref, "refs/heads/%s", branch_name);
 		if (!ref_exists(branch_ref.buf)) {
-			strbuf_release(&branch_ref);
-
 			if (!argc)
-				return error(_("No commit on branch '%s' yet."),
-					     branch_name);
+				die(_("No commit on branch '%s' yet."), branch_name);
 			else
-				return error(_("No branch named '%s'."),
-					     branch_name);
+				die(_("No branch named '%s'."), branch_name);
 		}
 		strbuf_release(&branch_ref);
 
-		if (edit_branch_description(branch_name))
-			return 1;
+		ret = edit_branch_description(branch_name);
+		free(to_free);
+		return ret;
 	} else if (copy) {
 		if (!argc)
 			die(_("branch name required"));
--- >8 ---

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v4] branch: let '--edit-description' default to rebased/bisected branch
  2020-01-12 10:17 ` Eric Sunshine
@ 2020-01-12 10:55   ` Eric Sunshine
  0 siblings, 0 replies; 3+ messages in thread
From: Eric Sunshine @ 2020-01-12 10:55 UTC (permalink / raw)
  To: marcandre.lureau; +Cc: Git List, SZEDER Gábor

On Sun, Jan 12, 2020 at 5:17 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
> +/*
> + * Return branch name of current worktree -- even if HEAD is detached -- or
> + * NULL if no branch is associated with worktree. Caller is responsible for
> + * freeing result.
> + */
> +static char *get_worktree_branch()

This would of course be:

    static char *get_worktree_branch(void)

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-12  6:47 [PATCH v4] branch: let '--edit-description' default to rebased/bisected branch marcandre.lureau
2020-01-12 10:17 ` Eric Sunshine
2020-01-12 10:55   ` Eric Sunshine

git@vger.kernel.org list mirror (unofficial, one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Example config snippet for mirrors

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git