branch: support for at-refs like @{-1} in --edit-description, --set-upstream-to and --unset-upstream

branch command with --edit-description, --set-upstream-to and --unset-upstream options expects a branch name. A branch can be specified using at-refs like @{-1}, so those references need to be resolved.

We can modify the description of the previously checked out branch with:

$ git branch --edit--description @{-1}

We can modify the upstream of the previously checked out branch with:

$ git branch --set-upstream-to upstream @{-1} $ git branch --unset-upstream
@{-1}

Rubén Justo (2):
branch: refactor edit_description command switch case
branch: support for at-refs like @{-1}

 builtin/branch.c                      | 49 +++++++++++++++++++--------
 t/t3204-branch-name-interpretation.sh | 36 ++++++++++++++++++++
 2 files changed, 70 insertions(+), 15 deletions(-)

base-commit: 795ea8776befc95ea2becd8020c7a284677b4161
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1346%2Frjusto%2Fbranch-support-at-refs-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1346/rjusto/branch-support-at-refs-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1346

gitgitgadget

From: =?UTF-8?q?Rub=C3=A9n=20Justo?= <[email protected]>

Minor refactoring to reduce the return exits in the switch case handling the edit_description command, so the calls to strbuf_release can also be reduced.

---
 builtin/branch.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

index 55cd9a6e998..5229cb796f8 100644
@@ -614,7 +614,7 @@ static int edit_branch_description(const char *branch_name)
 	strbuf_reset(&buf);
 	if (launch_editor(edit_description(), &buf, NULL)) {
 		strbuf_release(&buf);
-		return -1;
+		return 1;
 	}
 	strbuf_stripspace(&buf, 1);
 

@@ -791,6 +791,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 	} else if (edit_description) {
 		const char *branch_name;
 		struct strbuf branch_ref = STRBUF_INIT;
+		int ret = 0;
 
 		if (!argc) {
 			if (filter.detached)

@@ -803,19 +804,17 @@ 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."),
+				ret = error(_("No commit on branch '%s' yet."),
 					     branch_name);
 			else
-				return error(_("No branch named '%s'."),
+				ret = error(_("No branch named '%s'."),
 					     branch_name);
-		}
-		strbuf_release(&branch_ref);
+		} else
+			ret = edit_branch_description(branch_name);
 
-		if (edit_branch_description(branch_name))
-			return 1;
+		strbuf_release(&branch_ref);
+		return ret;
 	} else if (copy) {
 		if (!argc)
 			die(_("branch name required"));

gitgitgadget

From: =?UTF-8?q?Rub=C3=A9n=20Justo?= <[email protected]>

branch command with --edit-description, --set-upstream-to and --unset-upstream options expects a branch name. A branch can be specified using at-refs like @{-1}, so those references need to be resolved.

We can modify the description of the previously checked out branch with:

$ git branch --edit--description @{-1}

We can modify the upstream of the previously checked out branch with:

$ git branch --set-upstream-to upstream @{-1}
$ git branch --unset-upstream @{-1}

---
 builtin/branch.c                      | 32 +++++++++++++++++++-----
 t/t3204-branch-name-interpretation.sh | 36 +++++++++++++++++++++++++++
 2 files changed, 62 insertions(+), 6 deletions(-)

index 5229cb796f8..df620bee748 100644
@@ -791,14 +791,17 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 	} else if (edit_description) {
 		const char *branch_name;
 		struct strbuf branch_ref = STRBUF_INIT;
+		struct strbuf buf = STRBUF_INIT;
 		int ret = 0;
 
 		if (!argc) {
 			if (filter.detached)
 				die(_("Cannot give description to detached HEAD"));
 			branch_name = head;
-		} else if (argc == 1)
-			branch_name = argv[0];
+		} else if (argc == 1) {
+			strbuf_branchname(&buf, argv[0], INTERPRET_BRANCH_LOCAL);
+			branch_name = buf.buf;
+		}
 		else
 			die(_("cannot edit description of more than one branch"));
 

@@ -814,6 +817,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 			ret = edit_branch_description(branch_name);
 
 		strbuf_release(&branch_ref);
+		strbuf_release(&buf);
 		return ret;
 	} else if (copy) {
 		if (!argc)

@@ -834,11 +838,19 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		else
 			die(_("too many arguments for a rename operation"));
 	} else if (new_upstream) {
-		struct branch *branch = branch_get(argv[0]);
+		struct branch *branch;
+		struct strbuf buf = STRBUF_INIT;
 
-		if (argc > 1)
+		if (!argc)
+			branch = branch_get(NULL);
+		else if (argc == 1) {
+			strbuf_branchname(&buf, argv[0], INTERPRET_BRANCH_LOCAL);
+			branch = branch_get(buf.buf);
+		}
+		else
 			die(_("too many arguments to set new upstream"));
 
+
 		if (!branch) {
 			if (!argc || !strcmp(argv[0], "HEAD"))
 				die(_("could not set upstream of HEAD to %s when "

@@ -853,11 +865,18 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		dwim_and_setup_tracking(the_repository, branch->name,
 					new_upstream, BRANCH_TRACK_OVERRIDE,
 					quiet);
+		strbuf_release(&buf);
 	} else if (unset_upstream) {
-		struct branch *branch = branch_get(argv[0]);
+		struct branch *branch;
 		struct strbuf buf = STRBUF_INIT;
 
-		if (argc > 1)
+		if (!argc)
+			branch = branch_get(NULL);
+		else if (argc == 1) {
+			strbuf_branchname(&buf, argv[0], INTERPRET_BRANCH_LOCAL);
+			branch = branch_get(buf.buf);
+		}
+		else
 			die(_("too many arguments to unset upstream"));
 
 		if (!branch) {

@@ -870,6 +889,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		if (!branch_has_merge_config(branch))
 			die(_("Branch '%s' has no upstream information"), branch->name);
 
+		strbuf_reset(&buf);
 		strbuf_addf(&buf, "branch.%s.remote", branch->name);
 		git_config_set_multivar(buf.buf, NULL, NULL, CONFIG_FLAGS_MULTI_REPLACE);
 		strbuf_reset(&buf);

index 993a6b5eff7..853315ec20a 100755
@@ -133,4 +133,40 @@ test_expect_success 'checkout does not treat remote @{upstream} as a branch' '
 	expect_branch HEAD one
 '
 
+test_expect_success 'edit-description via @{-1}' '
+	git checkout main &&
+	git checkout - &&
+	write_script editor <<-\EOF &&
+		echo "Branch description" >"$1"
+	EOF
+	EDITOR=./editor git branch --edit-description @{-1} &&
+		write_script editor <<-\EOF &&
+		git stripspace -s <"$1" >"EDITOR_OUTPUT"
+	EOF
+	EDITOR=./editor git branch --edit-description @{-1} &&
+	echo "Branch description" >expect &&
+	test_cmp expect EDITOR_OUTPUT
+'
+
+test_expect_success 'modify branch upstream via "@{-1}@{upstream}"' "
+	cat >expect <<-\EOF &&
+	warning: not setting branch 'upstream-branch' as its own upstream
+	EOF
+	git branch upstream-branch &&
+	git checkout -b upstream-other -t upstream-branch &&
+	git checkout - &&
+	git branch --set-upstream-to upstream-branch @{-1}@{upstream} 2>actual &&
+	test_cmp expect actual
+"
+
+test_expect_success 'unset branch upstream via "@{-1}"' "
+	test "$(git config branch.upstream-other.remote)" = "." &&
+	test "$(git config branch.upstream-other.merge)" = "refs/heads/upstream-branch" &&
+	git checkout upstream-other &&
+	git checkout - &&
+	git branch --unset-upstream @{-1} &&
+	test_must_fail git config branch.upstream-other.remote &&
+	test_must_fail git config branch.upstream-other.merge
+"
+
 test_done

gitgitgadget

branch options: "edit-description", "set-upstream-to" and "unset-upstream" don't handle shortcuts like @{-1}.

This patch address this problem.

In v2 I've resolved some noise in the code, aggregated two tests in one with some refactoring to do the test in a more simple way while covering the functionality. Also changed the commit messages to better explain and document the change.

Thanks

Rubén Justo (2):
branch: refactor "edit_description" code path
branch: support for shortcuts like @{-1} completed

 builtin/branch.c                      | 46 ++++++++++++++++++---------
 t/t3204-branch-name-interpretation.sh | 25 +++++++++++++++
 2 files changed, 56 insertions(+), 15 deletions(-)

base-commit: 795ea8776befc95ea2becd8020c7a284677b4161

Range-diff vs v1:

1: aaa2f3ec31 ! 1: ce14194187 branch: refactor edit_description command switch case
@@ Metadata
Author: Rubén Justo <[email protected]>

## Commit message ##
- branch: refactor edit_description command switch case
+ branch: refactor "edit_description" code path

- Minor refactoring to reduce the return exits in the switch case
- handling the edit_description command, so the calls to strbuf_release
- can also be reduced.
+ Minor refactoring to reduce the number of returns in the switch case
+ handling the "edit_description" option, so the calls to strbuf_release
+ can also be reduced. New resources to be added also do not need to be
+ released in multiple places.

2: 321854e118 ! 2: 3a591cab8a branch: support for at-refs like @{-1}

    @@ Metadata
     Author: Rubén Justo <[email protected]>
     
      ## Commit message ##
    -    branch: support for at-refs like @{-1}
    +    branch: support for shortcuts like @{-1} completed
     
    -    branch command with --edit-description, --set-upstream-to and
    -    --unset-upstream options expects a branch name. A branch can be
    -    specified using at-refs like @{-1}, so those references need to
    -    be resolved.
    +    branch command with options "edit-description", "set-upstream-to"
    +    and "unset-upstream" expects a branch name.  Since ae5a6c3684
    +    (checkout: implement "@{-N}" shortcut name for N-th last branch,
    +    2009-01-17) a branch can be specified using shortcuts like @{-1}.
    +    Those shortcuts need to be resolved when considering the
    +    arguments.
    +
    +    Usage examples:
     
         We can modify the description of the previously checked out branch
         with:
    @@ builtin/branch.c: int cmd_branch(int argc, const char **argv, const char *prefix
     +		else if (argc == 1) {
     +			strbuf_branchname(&buf, argv[0], INTERPRET_BRANCH_LOCAL);
     +			branch = branch_get(buf.buf);
    -+		}
    -+		else
    ++		} else
      			die(_("too many arguments to set new upstream"));
      
    -+
      		if (!branch) {
    - 			if (!argc || !strcmp(argv[0], "HEAD"))
    - 				die(_("could not set upstream of HEAD to %s when "
     @@ builtin/branch.c: int cmd_branch(int argc, const char **argv, const char *prefix)
      		dwim_and_setup_tracking(the_repository, branch->name,
      					new_upstream, BRANCH_TRACK_OVERRIDE,
    @@ builtin/branch.c: int cmd_branch(int argc, const char **argv, const char *prefix
     +		else if (argc == 1) {
     +			strbuf_branchname(&buf, argv[0], INTERPRET_BRANCH_LOCAL);
     +			branch = branch_get(buf.buf);
    -+		}
    -+		else
    ++		} else
      			die(_("too many arguments to unset upstream"));
      
      		if (!branch) {
    @@ t/t3204-branch-name-interpretation.sh: test_expect_success 'checkout does not tr
     +	test_cmp expect EDITOR_OUTPUT
     +'
     +
    -+test_expect_success 'modify branch upstream via "@{-1}@{upstream}"' "
    -+	cat >expect <<-\EOF &&
    -+	warning: not setting branch 'upstream-branch' as its own upstream
    -+	EOF
    ++test_expect_success 'modify branch upstream via "@{-1}" and "@{-1}@{upstream}"' '
     +	git branch upstream-branch &&
     +	git checkout -b upstream-other -t upstream-branch &&
     +	git checkout - &&
    -+	git branch --set-upstream-to upstream-branch @{-1}@{upstream} 2>actual &&
    -+	test_cmp expect actual
    -+"
    -+
    -+test_expect_success 'unset branch upstream via "@{-1}"' "
    -+	test "$(git config branch.upstream-other.remote)" = "." &&
    -+	test "$(git config branch.upstream-other.merge)" = "refs/heads/upstream-branch" &&
    -+	git checkout upstream-other &&
    -+	git checkout - &&
    ++	git branch --set-upstream-to @{-1} @{-1}@{upstream} &&
    ++	test "$(git config branch.upstream-branch.merge)" = "refs/heads/upstream-other" &&
     +	git branch --unset-upstream @{-1} &&
    -+	test_must_fail git config branch.upstream-other.remote &&
     +	test_must_fail git config branch.upstream-other.merge
    -+"
    ++'
     +
      test_done

2.36.1

Minor refactoring to reduce the number of returns in the switch case handling the "edit_description" option, so the calls to strbuf_release can also be reduced. New resources to be added also do not need to be released in multiple places.

---
 builtin/branch.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

index 55cd9a6e99..5229cb796f 100644
@@ -614,7 +614,7 @@ static int edit_branch_description(const char *branch_name)
 	strbuf_reset(&buf);
 	if (launch_editor(edit_description(), &buf, NULL)) {
 		strbuf_release(&buf);
-		return -1;
+		return 1;
 	}
 	strbuf_stripspace(&buf, 1);
 

@@ -791,6 +791,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 	} else if (edit_description) {
 		const char *branch_name;
 		struct strbuf branch_ref = STRBUF_INIT;
+		int ret = 0;
 
 		if (!argc) {
 			if (filter.detached)

@@ -803,19 +804,17 @@ 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."),
+				ret = error(_("No commit on branch '%s' yet."),
 					     branch_name);
 			else
-				return error(_("No branch named '%s'."),
+				ret = error(_("No branch named '%s'."),
 					     branch_name);
-		}
-		strbuf_release(&branch_ref);
+		} else
+			ret = edit_branch_description(branch_name);
 
-		if (edit_branch_description(branch_name))
-			return 1;
+		strbuf_release(&branch_ref);
+		return ret;
 	} else if (copy) {
 		if (!argc)
 			die(_("branch name required"));

2.36.1

On 9/7/22 10:25 PM, Junio C Hamano wrote:

Maybe the return 1 when edit_branch_description(), was a typo I was maintaining?

Thanks for you review.

Nevermind. 1 is the return for exit. Sorry.

I'm sending a v3 with your suggestions. Thanks again.

branch command with options "edit-description", "set-upstream-to" and "unset-upstream" expects a branch name. Since ae5a6c3684 (checkout: implement "@{-N}" shortcut name for N-th last branch, 2009-01-17) a branch can be specified using shortcuts like @{-1}. Those shortcuts need to be resolved when considering the arguments.

Usage examples:

We can modify the description of the previously checked out branch with:

$ git branch --edit--description @{-1}

We can modify the upstream of the previously checked out branch with:

$ git branch --set-upstream-to upstream @{-1}
$ git branch --unset-upstream @{-1}

---
 builtin/branch.c                      | 29 +++++++++++++++++++++------
 t/t3204-branch-name-interpretation.sh | 25 +++++++++++++++++++++++
 2 files changed, 48 insertions(+), 6 deletions(-)

index 5229cb796f..6ee95b4179 100644
@@ -791,14 +791,17 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 	} else if (edit_description) {
 		const char *branch_name;
 		struct strbuf branch_ref = STRBUF_INIT;
+		struct strbuf buf = STRBUF_INIT;
 		int ret = 0;
 
 		if (!argc) {
 			if (filter.detached)
 				die(_("Cannot give description to detached HEAD"));
 			branch_name = head;
-		} else if (argc == 1)
-			branch_name = argv[0];
+		} else if (argc == 1) {
+			strbuf_branchname(&buf, argv[0], INTERPRET_BRANCH_LOCAL);
+			branch_name = buf.buf;
+		}
 		else
 			die(_("cannot edit description of more than one branch"));
 

@@ -814,6 +817,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 			ret = edit_branch_description(branch_name);
 
 		strbuf_release(&branch_ref);
+		strbuf_release(&buf);
 		return ret;
 	} else if (copy) {
 		if (!argc)

@@ -834,9 +838,15 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		else
 			die(_("too many arguments for a rename operation"));
 	} else if (new_upstream) {
-		struct branch *branch = branch_get(argv[0]);
+		struct branch *branch;
+		struct strbuf buf = STRBUF_INIT;
 
-		if (argc > 1)
+		if (!argc)
+			branch = branch_get(NULL);
+		else if (argc == 1) {
+			strbuf_branchname(&buf, argv[0], INTERPRET_BRANCH_LOCAL);
+			branch = branch_get(buf.buf);
+		} else
 			die(_("too many arguments to set new upstream"));
 
 		if (!branch) {

@@ -853,11 +863,17 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		dwim_and_setup_tracking(the_repository, branch->name,
 					new_upstream, BRANCH_TRACK_OVERRIDE,
 					quiet);
+		strbuf_release(&buf);
 	} else if (unset_upstream) {
-		struct branch *branch = branch_get(argv[0]);
+		struct branch *branch;
 		struct strbuf buf = STRBUF_INIT;
 
-		if (argc > 1)
+		if (!argc)
+			branch = branch_get(NULL);
+		else if (argc == 1) {
+			strbuf_branchname(&buf, argv[0], INTERPRET_BRANCH_LOCAL);
+			branch = branch_get(buf.buf);
+		} else
 			die(_("too many arguments to unset upstream"));
 
 		if (!branch) {

@@ -870,6 +886,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		if (!branch_has_merge_config(branch))
 			die(_("Branch '%s' has no upstream information"), branch->name);
 
+		strbuf_reset(&buf);
 		strbuf_addf(&buf, "branch.%s.remote", branch->name);
 		git_config_set_multivar(buf.buf, NULL, NULL, CONFIG_FLAGS_MULTI_REPLACE);
 		strbuf_reset(&buf);

index 993a6b5eff..0a423298d6 100755
@@ -133,4 +133,29 @@ test_expect_success 'checkout does not treat remote @{upstream} as a branch' '
 	expect_branch HEAD one
 '
 
+test_expect_success 'edit-description via @{-1}' '
+	git checkout main &&
+	git checkout - &&
+	write_script editor <<-\EOF &&
+		echo "Branch description" >"$1"
+	EOF
+	EDITOR=./editor git branch --edit-description @{-1} &&
+		write_script editor <<-\EOF &&
+		git stripspace -s <"$1" >"EDITOR_OUTPUT"
+	EOF
+	EDITOR=./editor git branch --edit-description @{-1} &&
+	echo "Branch description" >expect &&
+	test_cmp expect EDITOR_OUTPUT
+'
+
+test_expect_success 'modify branch upstream via "@{-1}" and "@{-1}@{upstream}"' '
+	git branch upstream-branch &&
+	git checkout -b upstream-other -t upstream-branch &&
+	git checkout - &&
+	git branch --set-upstream-to @{-1} @{-1}@{upstream} &&
+	test "$(git config branch.upstream-branch.merge)" = "refs/heads/upstream-other" &&
+	git branch --unset-upstream @{-1} &&
+	test_must_fail git config branch.upstream-other.merge
+'
+
 test_done

2.36.1

branch options: "edit-description", "set-upstream-to" and "unset-upstream" don't handle shortcuts like @{-1}.

This patch address this problem.

Changes in v3 are about following code conventions to avoid unnecessary confusions, pointed out by Junio in <[email protected]>.

Thanks.

Rubén Justo (2):
branch: refactor "edit_description" code path
branch: support for shortcuts like @{-1} completed

 builtin/branch.c                      | 44 ++++++++++++++++++---------
 t/t3204-branch-name-interpretation.sh | 25 +++++++++++++++
 2 files changed, 55 insertions(+), 14 deletions(-)

Range-diff:
1: ce14194187 ! 1: 0a69b6cf18 branch: refactor "edit_description" code path

    @@ Commit message
         Signed-off-by: Rubén Justo <[email protected]>
     
      ## builtin/branch.c ##
    -@@ builtin/branch.c: static int edit_branch_description(const char *branch_name)
    - 	strbuf_reset(&buf);
    - 	if (launch_editor(edit_description(), &buf, NULL)) {
    - 		strbuf_release(&buf);
    --		return -1;
    -+		return 1;
    - 	}
    - 	strbuf_stripspace(&buf, 1);
    - 
     @@ builtin/branch.c: int cmd_branch(int argc, const char **argv, const char *prefix)
      	} else if (edit_description) {
      		const char *branch_name;
    @@ builtin/branch.c: int cmd_branch(int argc, const char **argv, const char *prefix
      					     branch_name);
     -		}
     -		strbuf_release(&branch_ref);
    -+		} else
    -+			ret = edit_branch_description(branch_name);
    ++		} else if (edit_branch_description(branch_name))
    ++			ret = 1;
      
     -		if (edit_branch_description(branch_name))
     -			return 1;

2: 3a591cab8a ! 2: 91b308b52a branch: support for shortcuts like @{-1} completed
@@ builtin/branch.c: int cmd_branch(int argc, const char **argv, const char *prefix
die(_("cannot edit description of more than one branch"));

@@ builtin/branch.c: int cmd_branch(int argc, const char **argv, const char *prefix)
- ret = edit_branch_description(branch_name);
+ ret = 1;

strbuf_release(&branch_ref);
+ strbuf_release(&buf);

2.36.1

Minor refactoring to reduce the number of returns in the switch case handling the "edit_description" option, so the calls to strbuf_release can also be reduced. New resources to be added also do not need to be released in multiple places.

---
 builtin/branch.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

index 55cd9a6e99..b1f6519cd9 100644
@@ -791,6 +791,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 	} else if (edit_description) {
 		const char *branch_name;
 		struct strbuf branch_ref = STRBUF_INIT;
+		int ret = 0;
 
 		if (!argc) {
 			if (filter.detached)

@@ -803,19 +804,17 @@ 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."),
+				ret = error(_("No commit on branch '%s' yet."),
 					     branch_name);
 			else
-				return error(_("No branch named '%s'."),
+				ret = error(_("No branch named '%s'."),
 					     branch_name);
-		}
-		strbuf_release(&branch_ref);
+		} else if (edit_branch_description(branch_name))
+			ret = 1;
 
-		if (edit_branch_description(branch_name))
-			return 1;
+		strbuf_release(&branch_ref);
+		return ret;
 	} else if (copy) {
 		if (!argc)
 			die(_("branch name required"));

2.36.1

Stick to the convention of exit with 1 on error. May break some third party tool that expects -1 on "No branch named 'bla'" or "No commit on branch 'bla' yet." errors.

Test added to avoid regresion on this behaviour.
---

Junio, here is the patch for the change you suggested [1]. My concern is this might break some third party tool or script. If no one thinks this is an issue, I can squash it with the patch I'm responding to.

[1] https://lore.kernel.org/git/[email protected]/

 builtin/branch.c  | 6 +++---
 t/t3200-branch.sh | 8 ++++++++
 2 files changed, 11 insertions(+), 3 deletions(-)

index b1f6519cd9..01b6364f58 100644
@@ -810,11 +810,11 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 			else
 				ret = error(_("No branch named '%s'."),
 					     branch_name);
-		} else if (edit_branch_description(branch_name))
-			ret = 1;
+		} else
+			ret = edit_branch_description(branch_name);
 
 		strbuf_release(&branch_ref);
-		return ret;
+		return -ret;
 	} else if (copy) {
 		if (!argc)
 			die(_("branch name required"));

index 9723c2827c..d09db3fe3d 100755
@@ -1409,6 +1409,14 @@ test_expect_success 'refuse --edit-description on unborn branch for now' '
 	test_must_fail env EDITOR=./editor git branch --edit-description
 '
 
+test_expect_success 'test error codes using --edit-description' '
+	test_expect_code 1 env EDITOR=./invalid-editor git branch --edit-description main &&
+	write_script editor <<-\EOF &&
+		echo "New contents" >"$1"
+	EOF
+	test_expect_code 1 env EDITOR=./editor git branch --edit-description no-such-branch
+'
+
 test_expect_success '--merged catches invalid object names' '
 	test_must_fail git branch --merged 0000000000000000000000000000000000000000
 '

2.36.1

branch command with options "edit-description", "set-upstream-to" and "unset-upstream" expects a branch name. Since ae5a6c3684 (checkout: implement "@{-N}" shortcut name for N-th last branch, 2009-01-17) a branch can be specified using shortcuts like @{-1}. Those shortcuts need to be resolved when considering the arguments.

Usage examples:

We can modify the description of the previously checked out branch with:

$ git branch --edit--description @{-1}

We can modify the upstream of the previously checked out branch with:

$ git branch --set-upstream-to upstream @{-1}
$ git branch --unset-upstream @{-1}

---
 builtin/branch.c                      | 29 +++++++++++++++++++++------
 t/t3204-branch-name-interpretation.sh | 25 +++++++++++++++++++++++
 2 files changed, 48 insertions(+), 6 deletions(-)

index b1f6519cd9..a4069d27e0 100644
@@ -791,14 +791,17 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 	} else if (edit_description) {
 		const char *branch_name;
 		struct strbuf branch_ref = STRBUF_INIT;
+		struct strbuf buf = STRBUF_INIT;
 		int ret = 0;
 
 		if (!argc) {
 			if (filter.detached)
 				die(_("Cannot give description to detached HEAD"));
 			branch_name = head;
-		} else if (argc == 1)
-			branch_name = argv[0];
+		} else if (argc == 1) {
+			strbuf_branchname(&buf, argv[0], INTERPRET_BRANCH_LOCAL);
+			branch_name = buf.buf;
+		}
 		else
 			die(_("cannot edit description of more than one branch"));
 

@@ -814,6 +817,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 			ret = 1;
 
 		strbuf_release(&branch_ref);
+		strbuf_release(&buf);
 		return ret;
 	} else if (copy) {
 		if (!argc)

@@ -834,9 +838,15 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		else
 			die(_("too many arguments for a rename operation"));
 	} else if (new_upstream) {
-		struct branch *branch = branch_get(argv[0]);
+		struct branch *branch;
+		struct strbuf buf = STRBUF_INIT;
 
-		if (argc > 1)
+		if (!argc)
+			branch = branch_get(NULL);
+		else if (argc == 1) {
+			strbuf_branchname(&buf, argv[0], INTERPRET_BRANCH_LOCAL);
+			branch = branch_get(buf.buf);
+		} else
 			die(_("too many arguments to set new upstream"));
 
 		if (!branch) {

@@ -853,11 +863,17 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		dwim_and_setup_tracking(the_repository, branch->name,
 					new_upstream, BRANCH_TRACK_OVERRIDE,
 					quiet);
+		strbuf_release(&buf);
 	} else if (unset_upstream) {
-		struct branch *branch = branch_get(argv[0]);
+		struct branch *branch;
 		struct strbuf buf = STRBUF_INIT;
 
-		if (argc > 1)
+		if (!argc)
+			branch = branch_get(NULL);
+		else if (argc == 1) {
+			strbuf_branchname(&buf, argv[0], INTERPRET_BRANCH_LOCAL);
+			branch = branch_get(buf.buf);
+		} else
 			die(_("too many arguments to unset upstream"));
 
 		if (!branch) {

@@ -870,6 +886,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		if (!branch_has_merge_config(branch))
 			die(_("Branch '%s' has no upstream information"), branch->name);
 
+		strbuf_reset(&buf);
 		strbuf_addf(&buf, "branch.%s.remote", branch->name);
 		git_config_set_multivar(buf.buf, NULL, NULL, CONFIG_FLAGS_MULTI_REPLACE);
 		strbuf_reset(&buf);

index 993a6b5eff..0a423298d6 100755
@@ -133,4 +133,29 @@ test_expect_success 'checkout does not treat remote @{upstream} as a branch' '
 	expect_branch HEAD one
 '
 
+test_expect_success 'edit-description via @{-1}' '
+	git checkout main &&
+	git checkout - &&
+	write_script editor <<-\EOF &&
+		echo "Branch description" >"$1"
+	EOF
+	EDITOR=./editor git branch --edit-description @{-1} &&
+		write_script editor <<-\EOF &&
+		git stripspace -s <"$1" >"EDITOR_OUTPUT"
+	EOF
+	EDITOR=./editor git branch --edit-description @{-1} &&
+	echo "Branch description" >expect &&
+	test_cmp expect EDITOR_OUTPUT
+'
+
+test_expect_success 'modify branch upstream via "@{-1}" and "@{-1}@{upstream}"' '
+	git branch upstream-branch &&
+	git checkout -b upstream-other -t upstream-branch &&
+	git checkout - &&
+	git branch --set-upstream-to @{-1} @{-1}@{upstream} &&
+	test "$(git config branch.upstream-branch.merge)" = "refs/heads/upstream-other" &&
+	git branch --unset-upstream @{-1} &&
+	test_must_fail git config branch.upstream-other.merge
+'
+
 test_done

2.36.1

branch command with options "edit-description", "set-upstream-to" and "unset-upstream" expects a branch name. Since ae5a6c3684 (checkout:
implement "@{-N}" shortcut name for N-th last branch, 2009-01-17) a branch can be specified using shortcuts like @{-1}. Those shortcuts need to be resolved when considering the arguments.

We can modify the description of the previously checked out branch with:

$ git branch --edit--description @{-1}

We can modify the upstream of the previously checked out branch with:

$ git branch --set-upstream-to upstream @{-1}
$ git branch --unset-upstream @{-1}

---

To simplify the patch and make it easier to review, I removed all refactoring related to return codes and errors. Leaving that and everything else discussed in the thread, to next series.

This patch now only has one commit with the missing @{-N} handling completed and the tests for it. Some refactoring has been done in the test as well.

Un saludo.

 builtin/branch.c                      | 34 +++++++++++++++++++++------
 t/t3204-branch-name-interpretation.sh | 24 +++++++++++++++++++
 2 files changed, 51 insertions(+), 7 deletions(-)

index 55cd9a6e99..197603241d 100644
@@ -791,19 +791,23 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 	} else if (edit_description) {
 		const char *branch_name;
 		struct strbuf branch_ref = STRBUF_INIT;
+		struct strbuf buf = STRBUF_INIT;
 
 		if (!argc) {
 			if (filter.detached)
 				die(_("Cannot give description to detached HEAD"));
 			branch_name = head;
-		} else if (argc == 1)
-			branch_name = argv[0];
+		} else if (argc == 1) {
+			strbuf_branchname(&buf, argv[0], INTERPRET_BRANCH_LOCAL);
+			branch_name = buf.buf;
+		}
 		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);
+			strbuf_release(&buf);
 
 			if (!argc)
 				return error(_("No commit on branch '%s' yet."),

@@ -814,8 +818,11 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		}
 		strbuf_release(&branch_ref);
 
-		if (edit_branch_description(branch_name))
+		if (edit_branch_description(branch_name)) {
+			strbuf_release(&buf);
 			return 1;
+		}
+		strbuf_release(&buf);
 	} else if (copy) {
 		if (!argc)
 			die(_("branch name required"));

@@ -835,9 +842,15 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		else
 			die(_("too many arguments for a rename operation"));
 	} else if (new_upstream) {
-		struct branch *branch = branch_get(argv[0]);
+		struct branch *branch;
+		struct strbuf buf = STRBUF_INIT;
 
-		if (argc > 1)
+		if (!argc)
+			branch = branch_get(NULL);
+		else if (argc == 1) {
+			strbuf_branchname(&buf, argv[0], INTERPRET_BRANCH_LOCAL);
+			branch = branch_get(buf.buf);
+		} else
 			die(_("too many arguments to set new upstream"));
 
 		if (!branch) {

@@ -854,11 +867,17 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		dwim_and_setup_tracking(the_repository, branch->name,
 					new_upstream, BRANCH_TRACK_OVERRIDE,
 					quiet);
+		strbuf_release(&buf);
 	} else if (unset_upstream) {
-		struct branch *branch = branch_get(argv[0]);
+		struct branch *branch;
 		struct strbuf buf = STRBUF_INIT;
 
-		if (argc > 1)
+		if (!argc)
+			branch = branch_get(NULL);
+		else if (argc == 1) {
+			strbuf_branchname(&buf, argv[0], INTERPRET_BRANCH_LOCAL);
+			branch = branch_get(buf.buf);
+		} else
 			die(_("too many arguments to unset upstream"));
 
 		if (!branch) {

@@ -871,6 +890,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		if (!branch_has_merge_config(branch))
 			die(_("Branch '%s' has no upstream information"), branch->name);
 
+		strbuf_reset(&buf);
 		strbuf_addf(&buf, "branch.%s.remote", branch->name);
 		git_config_set_multivar(buf.buf, NULL, NULL, CONFIG_FLAGS_MULTI_REPLACE);
 		strbuf_reset(&buf);

index 993a6b5eff..71afceac71 100755
@@ -133,4 +133,28 @@ test_expect_success 'checkout does not treat remote @{upstream} as a branch' '
 	expect_branch HEAD one
 '
 
+test_expect_success 'edit-description via @{-1}' '
+	git checkout -b desc-branch &&
+	git checkout -b non-desc-branch &&
+	write_script editor <<-\EOF &&
+		echo "Branch description" >"$1"
+	EOF
+	EDITOR=./editor git branch --edit-description @{-1} &&
+	test_must_fail git config branch.non-desc-branch.description &&
+	git config branch.desc-branch.description >actual &&
+	echo "Branch description\n" >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'modify branch upstream via "@{-1}" and "@{-1}@{upstream}"' '
+	git checkout -b upstream-branch &&
+	git checkout -b upstream-other -t upstream-branch &&
+	git branch --set-upstream-to upstream-other @{-1} &&
+	git config branch.upstream-branch.merge >actual &&
+	echo "refs/heads/upstream-other" >expect &&
+	test_cmp expect actual &&
+	git branch --unset-upstream @{-1}@{upstream} &&
+	test_must_fail git config branch.upstream-other.merge
+'
+
 test_done

2.32.0

Good eyes. You are correct that "echo" and "\n" do not play well together.

Thanks.

Oops. Thank you! I'll reroll back to using "git stripspace".

git stripspace is perhaps unnecessarily heavyweight. Lightweight alternatives would include:

    printf "Branch description\n\n" >expect &&

    test_write_lines "Branch description" "" >expect &&

    { echo "Branch description" && echo; } >expect &&

    cat >expect <<-\EOF &&
    Branch description

    EOF

Yeah, I thought about that. What convinced me to use "git stripspace" was that maybe that '\n' tail could be removed sometime from the description setting and this will be fine with that. I haven't found any reason for that '\n' and it bugs me a little seeing it in the config :-)

But I agree with you about the unnecessarily heavyweight, though all involves a new process, probably echo, cat or printf are lightweight than another instance of git for that.

All of this involves two files and that is how it is done almost everywhere except in some places where it looks like an 'older way' (test_i18ngrep) of doing it. Is there any reason to do it this way and not using variables, process substitution,..?

Anyway I'll switch to one of your suggestions, as it is definitely easier to read, understand and therefore change if needed.

Thanks!

That reasoning occurred to me, as well, and I'd have no objection to git-stripspace if that's the motivation for using it. I don't feel strongly one way or the other, and my previous email was intended primarily to point out the lightweight alternatives in case you hadn't considered them. Feel free to use git-stripspace if you feel it is the more appropriate choice.

In most shells, printf, echo, cat are builtins, so no extra processes are involved (and test_write_lines is a shell function built atop printf). As a matter of personal preference, given the lightweight options, I find that printf "...\n\n" shows the intention of the code most plainly (but if you go with git-stripspace, then echo would be an idiomatic way to create the "expect" file).

An invocation such as:

    test $(git foo) = $(git bar) &&

throws away the exit-code from the two commands, which means we'd miss if one or the other (or both) crashed, especially if the crash was after the command produced the correct output. These days we try to avoid losing the exit command of Git commands. It's possible to avoid losing the exit-code by using variables:

    expect=$(git foo) &&
    actual=$(git bar) &&
    test "$expect" = "$actual" &&

but, if the expected and actual output don't match, you don't learn much (other than that they failed). You could address that by showing a message saying what failed:

    expect=... &&
    actual=... &&
    if test "$expect" != "$actual"
    then
        echo "expect not match actual"
        # maybe emit $expect and $actual too
    fi

However, test_cmp gives you that behavior for free, and it emits a helpful "diff" upon failure, so these days we usually go with test_cmp.

It's fine to use git-stripspace if you feel it's more appropriate for the situation.

I do not think I would agree with the line of reasoning.

It all depends on why we anticipate that the terminating LF may go away someday, but if it is because we may do so by mistake and without a good reason when making some unrelated changes to the implementation of "git branch --edit-desc", we would want to know about it, and such a loose check that would miss it is definitely unwelcome. It is very likely that not just "git merge" but people's external scripts depend on the presence of final LF especially when the description has only one line, so unless we are doing deliberately so, we should prepare to catch such a change.

If it is because we may gain a consensus that the description string (which by the way can well consist of multiple lines) is better without the LF on its final line, and we are "fixing" the code to do so very much on purpose, it would be good to have a test to protect such a change from future unintended breakages. Adding a loose test that won't break across such a change today may be OK, but whoever is making such a change in the future has to make sure there is a test that is not loose to protect the change. And it would very likely to be done by adding a new test, instead of noticing that this loosely written test can be tightened to serve the purpose.

So if we start with a tight test that expects the exact number of LFs at the end, we would be better off in that case, too.

Fair point. Thank you for being cautious.

This is already out of the subject, the patch, of this thread, so sorry, but the context is worth of it.

I understand the possibility of losing exit codes or segfaults in subcommands or pipes, but I'm more thinking in the element to compare to. Having something like:

	test_cmp_str () {
		test -f "$1" || BUG "first argument must be a file"
		if test "$#" -gt 1
		then
			local F=$1
			shift
			printf "$@" | eval "$GIT_TEST_CMP" '"$F"' -
		else
			eval "$GIT_TEST_CMP" '"$1"' -
		fi
	}

to allow writing simpler tests like:

@@ -142,8 +142,7 @@ test_expect_success 'edit-description via @{-1}' '
 	EDITOR=./editor git branch --edit-description @{-1} &&
 	test_must_fail git config branch.non-desc-branch.description &&
 	git config branch.desc-branch.description >actual &&
-	printf "Branch description\n\n" >expect &&
-	test_cmp expect actual
+	test_cmp_str actual "Branch description\n\n"
 '
 
--- a/t/t3204-branch-name-interpretation.sh
+++ b/t/t3204-branch-name-interpretation.sh

@@ -142,8 +142,10 @@ test_expect_success 'edit-description via @{-1}' '
        EDITOR=./editor git branch --edit-description @{-1} &&
        test_must_fail git config branch.non-desc-branch.description &&
        git config branch.desc-branch.description >actual &&
-       printf "Branch description\n\n" >expect &&
-       test_cmp expect actual
+       test_cmp_str actual <<-\EOF
+       Branch description
+
+       EOF
 '
 

My doubts are that maybe this can induce to some bad usages, is unusable in some systems, it has already been explored and discarded, using files gives the diff with nice names not "-",...

Maybe this needs a new RFC thread. I dunno.

Thanks for your comments and explanations.

I wouldn't be at all surprised if this sort of thing has been discussed already (it sounds familiar enough), or that such an improvement has already been submitted in the form of a patch (possibly by Ævar, Cc:'d).

My knee-jerk reaction is that the form which takes a string as argument would hardly be used, thus an unnecessary complication. The form which accepts stdin could even be retrofitted onto the existing test_cmp, thus you don't even need to invent a new function name. A different approach would be to introduce a function test_stdout which both accepts a command to run, as well as the "expected" text on stdin with which to compare the output of the command; i.e. a combination of the existing test_stdout_line_count in t/test-lib-functions.sh and your proposed function above.

That said, I'm not yet seeing all that much added value in such a function; at most, it seems to save only a single line of code, and it's not as if the code it's replacing was doing anything complicated or hard to grok. So, I'm pretty ambivalent about it. But, of course, I'm only one person expressing a knee-jerk reaction. Submitting the idea as an RFC patch might generate more feedback.

I share the sentiment. Leaving the result that was used in comparison in file(s) also helps debugging.

For me, the most value here is in the expressiveness, writing and reading a simple test.

git config branch.desc-branch.description >actual && test_cmp actual "Branch description\n\n"

vs

    git config branch.desc-branch.description >actual &&
    printf "Branch description\n\n" >expect &&
    test_cmp expect actual

vs

    printf "Branch description\n\n" >expect &&

...
git config branch.desc-branch.description >actual && test_cmp expect actual

vs (oops)

    printf "Branch description\n\n" >expect &&
...
    git config branch.desc-branch.description >actual &&
    printf "New branch description\n\n" >expect_ &&
    test_cmp expect actual

Less lines, less files and less error prone are a plus.

A quick overview over a not much elaborated search:

git grep -B 3 'test_cmp expect' | grep '\(echo\|cat\).*expect'

gives many results that imo should be beneficial of this expressiveness.

Nice. Maybe even the printf form with a "test ! -f", some risky though.

Sounds good to me but maybe this goes in the direction of "inducing bad usages", and the tests to cover starts to be not so simple...

And if this is fulfilled with a drop a file on failure? That resolves the '-' in the diff output result too.

Thank you.

Yes, it leaves a repetitive taste. I thought about joining the two cases, but the taste with multiple if/else was worse. Following what you suggest with the "too many arguments" error, I'll try to reduce that repetitive taste.

Thanks

branch command with options "edit-description", "set-upstream-to" and "unset-upstream" expects a branch name. Since ae5a6c3684 (checkout:
implement "@{-N}" shortcut name for N-th last branch, 2009-01-17) a branch can be specified using shortcuts like @{-1}. Those shortcuts need to be resolved when considering the arguments.

We can modify the description of the previously checked out branch with:

$ git branch --edit--description @{-1}

We can modify the upstream of the previously checked out branch with:

$ git branch --set-upstream-to upstream @{-1}
$ git branch --unset-upstream @{-1}

---

Changes from v4:
- Fix non POSIX compatible usage of echo.

Junio, I have taken two approaches to reduce the repetitive code, merging the two cases "new_upstream" and "unset_upstream". First keeping the different error messages to each case, which leads to multiple ugly if/elses. Second approach, merging the error messages with generics, which resolves the if/elses but leads to changes in t/t3200-branch.sh (at least). I haven't finished the tests yet, but I already have the same feeling as with the other patch for branch. I think this is a good stop point for this change if no new problems are detected.

Un saludo.

Range-diff:
1: 72a17039e0 ! 1: 393250bbac branch: support for shortcuts like @{-1}, completed

    @@ t/t3204-branch-name-interpretation.sh: test_expect_success 'checkout does not tr
     +	EDITOR=./editor git branch --edit-description @{-1} &&
     +	test_must_fail git config branch.non-desc-branch.description &&
     +	git config branch.desc-branch.description >actual &&
    -+	echo "Branch description\n" >expect &&
    ++	printf "Branch description\n\n" >expect &&
     +	test_cmp expect actual
     +'
     +

 builtin/branch.c                      | 34 +++++++++++++++++++++------
 t/t3204-branch-name-interpretation.sh | 24 +++++++++++++++++++
 2 files changed, 51 insertions(+), 7 deletions(-)

index 55cd9a6e99..197603241d 100644
@@ -791,19 +791,23 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 	} else if (edit_description) {
 		const char *branch_name;
 		struct strbuf branch_ref = STRBUF_INIT;
+		struct strbuf buf = STRBUF_INIT;
 
 		if (!argc) {
 			if (filter.detached)
 				die(_("Cannot give description to detached HEAD"));
 			branch_name = head;
-		} else if (argc == 1)
-			branch_name = argv[0];
+		} else if (argc == 1) {
+			strbuf_branchname(&buf, argv[0], INTERPRET_BRANCH_LOCAL);
+			branch_name = buf.buf;
+		}
 		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);
+			strbuf_release(&buf);
 
 			if (!argc)
 				return error(_("No commit on branch '%s' yet."),

@@ -814,8 +818,11 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		}
 		strbuf_release(&branch_ref);
 
-		if (edit_branch_description(branch_name))
+		if (edit_branch_description(branch_name)) {
+			strbuf_release(&buf);
 			return 1;
+		}
+		strbuf_release(&buf);
 	} else if (copy) {
 		if (!argc)
 			die(_("branch name required"));

@@ -835,9 +842,15 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		else
 			die(_("too many arguments for a rename operation"));
 	} else if (new_upstream) {
-		struct branch *branch = branch_get(argv[0]);
+		struct branch *branch;
+		struct strbuf buf = STRBUF_INIT;
 
-		if (argc > 1)
+		if (!argc)
+			branch = branch_get(NULL);
+		else if (argc == 1) {
+			strbuf_branchname(&buf, argv[0], INTERPRET_BRANCH_LOCAL);
+			branch = branch_get(buf.buf);
+		} else
 			die(_("too many arguments to set new upstream"));
 
 		if (!branch) {

@@ -854,11 +867,17 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		dwim_and_setup_tracking(the_repository, branch->name,
 					new_upstream, BRANCH_TRACK_OVERRIDE,
 					quiet);
+		strbuf_release(&buf);
 	} else if (unset_upstream) {
-		struct branch *branch = branch_get(argv[0]);
+		struct branch *branch;
 		struct strbuf buf = STRBUF_INIT;
 
-		if (argc > 1)
+		if (!argc)
+			branch = branch_get(NULL);
+		else if (argc == 1) {
+			strbuf_branchname(&buf, argv[0], INTERPRET_BRANCH_LOCAL);
+			branch = branch_get(buf.buf);
+		} else
 			die(_("too many arguments to unset upstream"));
 
 		if (!branch) {

@@ -871,6 +890,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		if (!branch_has_merge_config(branch))
 			die(_("Branch '%s' has no upstream information"), branch->name);
 
+		strbuf_reset(&buf);
 		strbuf_addf(&buf, "branch.%s.remote", branch->name);
 		git_config_set_multivar(buf.buf, NULL, NULL, CONFIG_FLAGS_MULTI_REPLACE);
 		strbuf_reset(&buf);

index 993a6b5eff..793bf4d269 100755
@@ -133,4 +133,28 @@ test_expect_success 'checkout does not treat remote @{upstream} as a branch' '
 	expect_branch HEAD one
 '
 
+test_expect_success 'edit-description via @{-1}' '
+	git checkout -b desc-branch &&
+	git checkout -b non-desc-branch &&
+	write_script editor <<-\EOF &&
+		echo "Branch description" >"$1"
+	EOF
+	EDITOR=./editor git branch --edit-description @{-1} &&
+	test_must_fail git config branch.non-desc-branch.description &&
+	git config branch.desc-branch.description >actual &&
+	printf "Branch description\n\n" >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'modify branch upstream via "@{-1}" and "@{-1}@{upstream}"' '
+	git checkout -b upstream-branch &&
+	git checkout -b upstream-other -t upstream-branch &&
+	git branch --set-upstream-to upstream-other @{-1} &&
+	git config branch.upstream-branch.merge >actual &&
+	echo "refs/heads/upstream-other" >expect &&
+	test_cmp expect actual &&
+	git branch --unset-upstream @{-1}@{upstream} &&
+	test_must_fail git config branch.upstream-other.merge
+'
+
 test_done

2.36.1

Looks good.

Thanks.

Yes, the refactoring we already discussed early in this thread.

:-( good catch, thanks. Removing the refactoring commit was not carefully done.

Ok.

Mmm, I don't feel the same here, we already discussed about this. Maybe?:

index 17853225fa..307073cc47 100644
@@ -817,7 +817,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
                strbuf_release(&branch_ref);
                strbuf_release(&buf);
 
-               return ret;
+               if (ret)
+                       return ret; /* some failure happened */
        } else if (copy) {
                if (!argc)
                        die(_("branch name required"));

not much important, though.

You can squash the changes in the commit or if you need me to send a v6, please let me know. Thank you for your careful review.

I would like to see some kind of free(head) in a clean-up to not get distracted with that. Not a proper leak though and the leak checkers does not refer to that as leak. So not important. We can go with the unconditional return and let the dust settle.

"head" is not leaking, as a pointer to it is head in a location that is still in scope (namely, a file-scope global variable) when the program exits.

In fact, the only thing the code before or after this patch does after leaving this top-level if/elseif/else cascade is to return 0 and doing nothing else. Inserting free(head) would be an unneeded distraction for human developers (doing such a patch, reviewing, and even worse, having to read the resulting code in the coming years) and waste of computer resources (exiting the process will reclaim such a piece of memory just fine).

Just to say that I truly agree.

Un saludo.

branch command with options "edit-description", "set-upstream-to" and "unset-upstream" expects a branch name. Since ae5a6c3684 (checkout:
implement "@{-N}" shortcut name for N-th last branch, 2009-01-17) a branch can be specified using shortcuts like @{-1}. Those shortcuts need to be resolved when considering the arguments.

We can modify the description of the previously checked out branch with:

$ git branch --edit--description @{-1}

We can modify the upstream of the previously checked out branch with:

$ git branch --set-upstream-to upstream @{-1}
$ git branch --unset-upstream @{-1}

---

This is the v5 with your fix squashed, but with one change to maintain the -1 on error.

    -			error(!argc
    +			ret = error(!argc
    			      ? _("No commit on branch '%s' yet.")
    			      : _("No branch named '%s'."),
    			      branch_name);

We discussed to keep this and other things like the capitalized error messages, to be sorted out in next series, when the dust settles after this and the other patch about errors on unborn branches.

ref:
<[email protected]>
<[email protected]>

Range-diff:
1: 00ca255874 ! 1: 11650c6340 branch: support for shortcuts like @{-1}, completed

    @@ builtin/branch.c: int cmd_branch(int argc, const char **argv, const char *prefix
      		const char *branch_name;
      		struct strbuf branch_ref = STRBUF_INIT;
     +		struct strbuf buf = STRBUF_INIT;
    ++		int ret = 1; /* assume failure */
      
      		if (!argc) {
      			if (filter.detached)
    @@ builtin/branch.c: int cmd_branch(int argc, const char **argv, const char *prefix
      			branch_name = head;
     -		} else if (argc == 1)
     -			branch_name = argv[0];
    +-		else
     +		} else if (argc == 1) {
     +			strbuf_branchname(&buf, argv[0], INTERPRET_BRANCH_LOCAL);
     +			branch_name = buf.buf;
    -+		}
    - 		else
    ++		} 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);
    -+			strbuf_release(&buf);
    - 
    - 			if (!argc)
    - 				return error(_("No commit on branch '%s' yet."),
    -@@ builtin/branch.c: int cmd_branch(int argc, const char **argv, const char *prefix)
    - 		}
    +-		if (!ref_exists(branch_ref.buf)) {
    +-			strbuf_release(&branch_ref);
    +-
    +-			if (!argc)
    +-				return error(_("No commit on branch '%s' yet."),
    +-					     branch_name);
    +-			else
    +-				return error(_("No branch named '%s'."),
    +-					     branch_name);
    +-		}
    ++		if (!ref_exists(branch_ref.buf))
    ++			ret = error(!argc
    ++			      ? _("No commit on branch '%s' yet.")
    ++			      : _("No branch named '%s'."),
    ++			      branch_name);
    ++		else if (!edit_branch_description(branch_name))
    ++			ret = 0; /* happy */
    ++
      		strbuf_release(&branch_ref);
    ++		strbuf_release(&buf);
      
     -		if (edit_branch_description(branch_name))
    -+		if (edit_branch_description(branch_name)) {
    -+			strbuf_release(&buf);
    - 			return 1;
    -+		}
    -+		strbuf_release(&buf);
    +-			return 1;
    ++		return ret;
      	} else if (copy) {
      		if (!argc)
      			die(_("branch name required"));

 builtin/branch.c                      | 53 +++++++++++++++++----------
 t/t3204-branch-name-interpretation.sh | 24 ++++++++++++
 2 files changed, 58 insertions(+), 19 deletions(-)

index 55cd9a6e99..658c9f8741 100644
@@ -791,31 +791,33 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 	} else if (edit_description) {
 		const char *branch_name;
 		struct strbuf branch_ref = STRBUF_INIT;
+		struct strbuf buf = STRBUF_INIT;
+		int ret = 1; /* assume failure */
 
 		if (!argc) {
 			if (filter.detached)
 				die(_("Cannot give description to detached HEAD"));
 			branch_name = head;
-		} else if (argc == 1)
-			branch_name = argv[0];
-		else
+		} else if (argc == 1) {
+			strbuf_branchname(&buf, argv[0], INTERPRET_BRANCH_LOCAL);
+			branch_name = buf.buf;
+		} 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);
-
-			if (!argc)
-				return error(_("No commit on branch '%s' yet."),
-					     branch_name);
-			else
-				return error(_("No branch named '%s'."),
-					     branch_name);
-		}
+		if (!ref_exists(branch_ref.buf))
+			ret = error(!argc
+			      ? _("No commit on branch '%s' yet.")
+			      : _("No branch named '%s'."),
+			      branch_name);
+		else if (!edit_branch_description(branch_name))
+			ret = 0; /* happy */
+
 		strbuf_release(&branch_ref);
+		strbuf_release(&buf);
 
-		if (edit_branch_description(branch_name))
-			return 1;
+		return ret;
 	} else if (copy) {
 		if (!argc)
 			die(_("branch name required"));

@@ -835,9 +837,15 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		else
 			die(_("too many arguments for a rename operation"));
 	} else if (new_upstream) {
-		struct branch *branch = branch_get(argv[0]);
+		struct branch *branch;
+		struct strbuf buf = STRBUF_INIT;
 
-		if (argc > 1)
+		if (!argc)
+			branch = branch_get(NULL);
+		else if (argc == 1) {
+			strbuf_branchname(&buf, argv[0], INTERPRET_BRANCH_LOCAL);
+			branch = branch_get(buf.buf);
+		} else
 			die(_("too many arguments to set new upstream"));
 
 		if (!branch) {

@@ -854,11 +862,17 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		dwim_and_setup_tracking(the_repository, branch->name,
 					new_upstream, BRANCH_TRACK_OVERRIDE,
 					quiet);
+		strbuf_release(&buf);
 	} else if (unset_upstream) {
-		struct branch *branch = branch_get(argv[0]);
+		struct branch *branch;
 		struct strbuf buf = STRBUF_INIT;
 
-		if (argc > 1)
+		if (!argc)
+			branch = branch_get(NULL);
+		else if (argc == 1) {
+			strbuf_branchname(&buf, argv[0], INTERPRET_BRANCH_LOCAL);
+			branch = branch_get(buf.buf);
+		} else
 			die(_("too many arguments to unset upstream"));
 
 		if (!branch) {

@@ -871,6 +885,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		if (!branch_has_merge_config(branch))
 			die(_("Branch '%s' has no upstream information"), branch->name);
 
+		strbuf_reset(&buf);
 		strbuf_addf(&buf, "branch.%s.remote", branch->name);
 		git_config_set_multivar(buf.buf, NULL, NULL, CONFIG_FLAGS_MULTI_REPLACE);
 		strbuf_reset(&buf);

index 993a6b5eff..793bf4d269 100755
@@ -133,4 +133,28 @@ test_expect_success 'checkout does not treat remote @{upstream} as a branch' '
 	expect_branch HEAD one
 '
 
+test_expect_success 'edit-description via @{-1}' '
+	git checkout -b desc-branch &&
+	git checkout -b non-desc-branch &&
+	write_script editor <<-\EOF &&
+		echo "Branch description" >"$1"
+	EOF
+	EDITOR=./editor git branch --edit-description @{-1} &&
+	test_must_fail git config branch.non-desc-branch.description &&
+	git config branch.desc-branch.description >actual &&
+	printf "Branch description\n\n" >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'modify branch upstream via "@{-1}" and "@{-1}@{upstream}"' '
+	git checkout -b upstream-branch &&
+	git checkout -b upstream-other -t upstream-branch &&
+	git branch --set-upstream-to upstream-other @{-1} &&
+	git config branch.upstream-branch.merge >actual &&
+	echo "refs/heads/upstream-other" >expect &&
+	test_cmp expect actual &&
+	git branch --unset-upstream @{-1}@{upstream} &&
+	test_must_fail git config branch.upstream-other.merge
+'
+
 test_done

2.36.1