[PATCH 0/2] branch: support for at-refs like @{-1} in --edit-description, --set-upstream-to and --unset-upstream

[PATCH 0/2] branch: support for at-refs like @{-1} in --edit-description, --set-upstream-to and --unset-upstream

[Rubén Justo via GitGitGadget]

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

[PATCH 1/2] branch: refactor edit_description command switch case

[Rubén Justo via GitGitGadget]

From: =?UTF-8?q?Rub=C3=A9n=20Justo?= <rjusto@gmail.com>

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.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 builtin/branch.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 55cd9a6e998..5229cb796f8 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -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

[PATCH 2/2] branch: support for at-refs like @{-1}

[Rubén Justo via GitGitGadget]

From: =?UTF-8?q?Rub=C3=A9n=20Justo?= <rjusto@gmail.com>

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}

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 builtin/branch.c                      | 32 +++++++++++++++++++-----
 t/t3204-branch-name-interpretation.sh | 36 +++++++++++++++++++++++++++
 2 files changed, 62 insertions(+), 6 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 5229cb796f8..df620bee748 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -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);
diff --git a/t/t3204-branch-name-interpretation.sh b/t/t3204-branch-name-interpretation.sh
index 993a6b5eff7..853315ec20a 100755
--- a/t/t3204-branch-name-interpretation.sh
+++ b/t/t3204-branch-name-interpretation.sh
@@ -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

[PATCH v2 0/2] branch: support for shortcuts like @{-1}, completed

[Rubén Justo]

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 <rjusto@gmail.com>
     
      ## 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.
     
         Signed-off-by: Rubén Justo <rjusto@gmail.com>
     
2:  321854e118 ! 2:  3a591cab8a branch: support for at-refs like @{-1}
    @@ Metadata
     Author: Rubén Justo <rjusto@gmail.com>
     
      ## 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

[PATCH v2 1/2] branch: refactor "edit_description" code path

[Rubén Justo]

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.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 builtin/branch.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 55cd9a6e99..5229cb796f 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -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

[PATCH v2 2/2] branch: support for shortcuts like @{-1} completed

[Rubén Justo]

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}

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 builtin/branch.c                      | 29 +++++++++++++++++++++------
 t/t3204-branch-name-interpretation.sh | 25 +++++++++++++++++++++++
 2 files changed, 48 insertions(+), 6 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 5229cb796f..6ee95b4179 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -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);
diff --git a/t/t3204-branch-name-interpretation.sh b/t/t3204-branch-name-interpretation.sh
index 993a6b5eff..0a423298d6 100755
--- a/t/t3204-branch-name-interpretation.sh
+++ b/t/t3204-branch-name-interpretation.sh
@@ -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

Re: [PATCH v2 1/2] branch: refactor "edit_description" code path

[Junio C Hamano]

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

> 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.
>
> Signed-off-by: Rubén Justo <rjusto@gmail.com>
> ---
>  builtin/branch.c | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/builtin/branch.c b/builtin/branch.c
> index 55cd9a6e99..5229cb796f 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -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);


Our API convention is to signal a failure with negative return
value.  Granted that this is not a general API but is merely a
helper function in the implementation of a single command, it would
be less confusing if you sticked to the convention.

Unless there is a compelling reason not to, that is.

> @@ -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);

OK.  These are good uses of a new variable 'ret'.  Note that error()
returns negative one.

> -		}
> -		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;

When editor failed, we leaked branch_ref strbuf, but we no longer
do.

Which is good.

This makes cmd_branch() return -1 (when we see error() call) or 1
(when edit_branch_description() fails and returns 1).  I would
suggest to

 * Fix the return value of edit_branch_description() so that it
   signals a failure by returning -1

 * cmd_branch() to return (or call exit() with) -ret, as ret has 0
   when everything is peachy, and negative in any error code paths.

Re: [PATCH v2 1/2] branch: refactor "edit_description" code path

[Rubén Justo]

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?

>> -		}
>> -		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;

Thanks for you review.

Re: [PATCH v2 1/2] branch: refactor "edit_description" code path

[Rubén Justo]

On 9/7/22 11:24 PM, Rubén Justo wrote:
> 
> 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?
> 
>>> -		}
>>> -		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;
> 
> Thanks for you review.
> 

Nevermind. 1 is the return for exit. Sorry.

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

[PATCH v3 0/2] branch: support for shortcuts like @{-1}, completed

[Rubén Justo]

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 <xmqq7d2fszhk.fsf@gitster.g>.
 
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 <rjusto@gmail.com>
     
      ## 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

[PATCH v3 1/2] branch: refactor "edit_description" code path

[Rubén Justo]

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.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 builtin/branch.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 55cd9a6e99..b1f6519cd9 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -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

[PATCH v3 2/2] branch: support for shortcuts like @{-1} completed

[Rubén Justo]

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}

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 builtin/branch.c                      | 29 +++++++++++++++++++++------
 t/t3204-branch-name-interpretation.sh | 25 +++++++++++++++++++++++
 2 files changed, 48 insertions(+), 6 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index b1f6519cd9..a4069d27e0 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -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);
diff --git a/t/t3204-branch-name-interpretation.sh b/t/t3204-branch-name-interpretation.sh
index 993a6b5eff..0a423298d6 100755
--- a/t/t3204-branch-name-interpretation.sh
+++ b/t/t3204-branch-name-interpretation.sh
@@ -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

[PATCH] branch: error codes for "edit_description"

[Rubén Justo]

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/xmqq7d2fszhk.fsf@gitster.g/

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

diff --git a/builtin/branch.c b/builtin/branch.c
index b1f6519cd9..01b6364f58 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -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"));
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 9723c2827c..d09db3fe3d 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -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

Re: [PATCH] branch: error codes for "edit_description"

[Junio C Hamano]

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

> 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/xmqq7d2fszhk.fsf@gitster.g/
>
>  builtin/branch.c  | 6 +++---
>  t/t3200-branch.sh | 8 ++++++++
>  2 files changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/branch.c b/builtin/branch.c
> index b1f6519cd9..01b6364f58 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -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;

If edit_branch_description() returns -1 on failure, this would
consistently exit with 1 on failure, so it does make sense.  

It is bad to exit with a value outside 0..255 for portability
reasons, but we wrap exit() to chomp off the higher bits, so in
practice it may be fine to exit(-1) but still the above is a good
clean-up, I would think.

[PATCH v4] branch: support for shortcuts like @{-1}, completed

[Rubén Justo]

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}

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

---

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(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 55cd9a6e99..197603241d 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -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);
diff --git a/t/t3204-branch-name-interpretation.sh b/t/t3204-branch-name-interpretation.sh
index 993a6b5eff..71afceac71 100755
--- a/t/t3204-branch-name-interpretation.sh
+++ b/t/t3204-branch-name-interpretation.sh
@@ -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

Re: [PATCH v4] branch: support for shortcuts like @{-1}, completed

[Eric Sunshine]

On Fri, Oct 7, 2022 at 9:36 PM Rubén Justo <rjusto@gmail.com> wrote:
> 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.
> [...]
> Signed-off-by: Rubén Justo <rjusto@gmail.com>
> ---
> diff --git a/t/t3204-branch-name-interpretation.sh b/t/t3204-branch-name-interpretation.sh
> @@ -133,4 +133,28 @@ test_expect_success 'checkout does not treat remote @{upstream} as a branch' '
> +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 &&

Is the intention here with the embedded "\n" that `echo` should emit
two newlines? If so, interpreting "\n" specially is not POSIX behavior
for `echo`, thus we probably don't want to rely upon it.

> +       test_cmp expect actual

Re: [PATCH v4] branch: support for shortcuts like @{-1}, completed

[Junio C Hamano]

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

>  	} 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"));

The above two are a bit repetitious.  A helper like

	static struct branch *interpret_local(int ac, const char **av)
	{
		struct branch *branch;
                if (!ac) {
                	branch = branch_get(NULL);
		} else if (ac == 1) {
			struct strbuf buf = STRBUF_INIT;
			strbuf_branchname(&buf, av[0], INTERPRET_BRANCH_LOCAL);
			branch = branch_get(buf.buf);
			strbuf_release(&buf);
		} else {
			die(_("too many arguments"));
		}
		return branch;
	}

might be useful, and it frees the callers from worrying about
temporary allocations.

But the code written is OK as-is.

Re: [PATCH v4] branch: support for shortcuts like @{-1}, completed

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Fri, Oct 7, 2022 at 9:36 PM Rubén Justo <rjusto@gmail.com> wrote:
>> 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.
>> [...]
>> Signed-off-by: Rubén Justo <rjusto@gmail.com>
>> ---
>> diff --git a/t/t3204-branch-name-interpretation.sh b/t/t3204-branch-name-interpretation.sh
>> @@ -133,4 +133,28 @@ test_expect_success 'checkout does not treat remote @{upstream} as a branch' '
>> +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 &&
>
> Is the intention here with the embedded "\n" that `echo` should emit
> two newlines? If so, interpreting "\n" specially is not POSIX behavior
> for `echo`, thus we probably don't want to rely upon it.

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

Thanks.

Re: [PATCH v4] branch: support for shortcuts like @{-1}, completed

[Rubén Justo]

On 8/10/22 5:17, Eric Sunshine wrote:
> On Fri, Oct 7, 2022 at 9:36 PM Rubén Justo <rjusto@gmail.com> wrote:
>> 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.
>> [...]
>> Signed-off-by: Rubén Justo <rjusto@gmail.com>
>> ---
>> diff --git a/t/t3204-branch-name-interpretation.sh b/t/t3204-branch-name-interpretation.sh
>> @@ -133,4 +133,28 @@ test_expect_success 'checkout does not treat remote @{upstream} as a branch' '
>> +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 &&
> 
> Is the intention here with the embedded "\n" that `echo` should emit
> two newlines? If so, interpreting "\n" specially is not POSIX behavior
> for `echo`, thus we probably don't want to rely upon it.
> 

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

>> +       test_cmp expect actual

Re: [PATCH v4] branch: support for shortcuts like @{-1}, completed

[Eric Sunshine]

On Sat, Oct 8, 2022 at 3:07 AM Rubén Justo <rjusto@gmail.com> wrote:
> On 8/10/22 5:17, Eric Sunshine wrote:
> > On Fri, Oct 7, 2022 at 9:36 PM Rubén Justo <rjusto@gmail.com> wrote:
> >> +       echo "Branch description\n" >expect &&
> >
> > Is the intention here with the embedded "\n" that `echo` should emit
> > two newlines? If so, interpreting "\n" specially is not POSIX behavior
> > for `echo`, thus we probably don't want to rely upon it.
>
> 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

Re: [PATCH v4] branch: support for shortcuts like @{-1}, completed

[Rubén Justo]

On 8/10/22 6:17, Junio C Hamano wrote:
> Rubén Justo <rjusto@gmail.com> writes:
> 
>>  	} 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"));
> 
> The above two are a bit repetitious.  A helper like
> 
> 	static struct branch *interpret_local(int ac, const char **av)
> 	{
> 		struct branch *branch;
>                 if (!ac) {
>                 	branch = branch_get(NULL);
> 		} else if (ac == 1) {
> 			struct strbuf buf = STRBUF_INIT;
> 			strbuf_branchname(&buf, av[0], INTERPRET_BRANCH_LOCAL);
> 			branch = branch_get(buf.buf);
> 			strbuf_release(&buf);
> 		} else {
> 			die(_("too many arguments"));
> 		}
> 		return branch;
> 	}
> 
> might be useful, and it frees the callers from worrying about
> temporary allocations.

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

> 
> But the code written is OK as-is.
> 

Re: [PATCH v4] branch: support for shortcuts like @{-1}, completed

[Rubén Justo]

On 8/10/22 9:23, Eric Sunshine wrote:
> On Sat, Oct 8, 2022 at 3:07 AM Rubén Justo <rjusto@gmail.com> wrote:
>> On 8/10/22 5:17, Eric Sunshine wrote:
>>> On Fri, Oct 7, 2022 at 9:36 PM Rubén Justo <rjusto@gmail.com> wrote:
>>>> +       echo "Branch description\n" >expect &&
>>>
>>> Is the intention here with the embedded "\n" that `echo` should emit
>>> two newlines? If so, interpreting "\n" specially is not POSIX behavior
>>> for `echo`, thus we probably don't want to rely upon it.
>>
>> 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!

Re: [PATCH v4] branch: support for shortcuts like @{-1}, completed

[Eric Sunshine]

On Sat, Oct 8, 2022 at 5:12 AM Rubén Justo <rjusto@gmail.com> wrote:
> On 8/10/22 9:23, Eric Sunshine wrote:
> > On Sat, Oct 8, 2022 at 3:07 AM Rubén Justo <rjusto@gmail.com> wrote:
> >> 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 &&
> >     [...]
>
> 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 :-)

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.

> 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.

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).

> 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,..?

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`.

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

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

Re: [PATCH v4] branch: support for shortcuts like @{-1}, completed

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

>> 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 :-)
>
> 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.

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.

Re: [PATCH v4] branch: support for shortcuts like @{-1}, completed

[Rubén Justo]

On 8/10/22 19:46, Junio C Hamano wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
> 
>>> 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 :-)
>>
>> 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.
> 
> 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.

[PATCH v5] branch: support for shortcuts like @{-1}, completed

[Rubén Justo]

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}

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

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(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 55cd9a6e99..197603241d 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -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);
diff --git a/t/t3204-branch-name-interpretation.sh b/t/t3204-branch-name-interpretation.sh
index 993a6b5eff..793bf4d269 100755
--- a/t/t3204-branch-name-interpretation.sh
+++ b/t/t3204-branch-name-interpretation.sh
@@ -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

Re: [PATCH v4] branch: support for shortcuts like @{-1}, completed

[Rubén Justo]

On 8/10/22 19:10, Eric Sunshine wrote:

>> 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,..?
> 
> 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`.
>

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:

--- a/t/t3204-branch-name-interpretation.sh
+++ b/t/t3204-branch-name-interpretation.sh
@@ -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.

Re: [PATCH v5] branch: support for shortcuts like @{-1}, completed

[Junio C Hamano]

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

> ... I think this is a good stop point for this change if no new problems
> are detected.
>
> Un saludo.

Looks good.

Thanks.

Re: [PATCH v4] branch: support for shortcuts like @{-1}, completed

[Eric Sunshine]

On Sat, Oct 8, 2022 at 7:28 PM Rubén Justo <rjusto@gmail.com> wrote:
> On 8/10/22 19:10, Eric Sunshine wrote:
> > 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`.
>
> 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:
>
> +       test_cmp_str actual "Branch description\n\n"
>
> +       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 "-",...

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.

Re: [PATCH v5] branch: support for shortcuts like @{-1}, completed

[Junio C Hamano]

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

> @@ -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"));

Here branch_name could be pointing at buf.buf (or head).

>  		strbuf_addf(&branch_ref, "refs/heads/%s", branch_name);
>  		if (!ref_exists(branch_ref.buf)) {
>  			strbuf_release(&branch_ref);
> +			strbuf_release(&buf);

But the contents of the buf becomes invalid at this point.

>  			if (!argc)
>  				return error(_("No commit on branch '%s' yet."),

In the post context of this hunk, we see that the '%s' above is
filled with branch_name, accessing the potentially invalid contents.

I'll put a squashable band-aid on top of the topic for now.

--- >8 ---

 * cmd_foo() should not return an negative value.

 * branch_name used in the calls to error() could point at buf.buf
   that holds the expansion of @{-1}, but buf was released way too
   early, leading to a use-after-free.

 * Style: if/else if/else cascade whose one arm has multiple
   statements and requires {braces} around it should have {braces}
   around all of its arms.

 * each arm in the top-level if/else if/else cascade for "git
   branch" subcommands were more or less independent, and there
   wasn't anything common that they need to execute after exiting
   the cascade.  Unconditionally returning from the arm for the
   edit-description subcommand seems to make the logic flow easier
   to read.

 builtin/branch.c | 31 +++++++++++++------------------
 1 file changed, 13 insertions(+), 18 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 197603241d..17853225fa 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -792,6 +792,7 @@ 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)
@@ -800,29 +801,23 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		} 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."),
-					     branch_name);
-			else
-				return error(_("No branch named '%s'."),
-					     branch_name);
-		}
-		strbuf_release(&branch_ref);
+		if (!ref_exists(branch_ref.buf))
+			error(!argc
+			      ? _("No commit on branch '%s' yet.")
+			      : _("No branch named '%s'."),
+			      branch_name);
+		else if (!edit_branch_description(branch_name))
+			ret = 0; /* happy */
 
-		if (edit_branch_description(branch_name)) {
-			strbuf_release(&buf);
-			return 1;
-		}
+		strbuf_release(&branch_ref);
 		strbuf_release(&buf);
+
+		return ret;
 	} else if (copy) {
 		if (!argc)
 			die(_("branch name required"));
-- 
2.38.0-167-g3030fd6006

Re: [PATCH v4] branch: support for shortcuts like @{-1}, completed

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> 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.

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

Re: [PATCH v5] branch: support for shortcuts like @{-1}, completed

[Rubén Justo]

On 9/10/22 21:11, Junio C Hamano wrote:
> Rubén Justo <rjusto@gmail.com> writes:
> 
>  * cmd_foo() should not return an negative value.

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

> 
>  * branch_name used in the calls to error() could point at buf.buf
>    that holds the expansion of @{-1}, but buf was released way too
>    early, leading to a use-after-free.

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

> 
>  * Style: if/else if/else cascade whose one arm has multiple
>    statements and requires {braces} around it should have {braces}
>    around all of its arms.

Ok.

> 
>  * each arm in the top-level if/else if/else cascade for "git
>    branch" subcommands were more or less independent, and there
>    wasn't anything common that they need to execute after exiting
>    the cascade.  Unconditionally returning from the arm for the
>    edit-description subcommand seems to make the logic flow easier
>    to read.

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

diff --git a/builtin/branch.c b/builtin/branch.c
index 17853225fa..307073cc47 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -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.

Re: [PATCH v4] branch: support for shortcuts like @{-1}, completed

[Rubén Justo]

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.


On 9/10/22 8:46, Eric Sunshine wrote:
> My knee-jerk reaction is that the form which takes a string as
> argument would hardly be used, thus an unnecessary complication. The

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.


> 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

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


> 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.

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...


On 9/10/22 21:33, Junio C Hamano wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
> 
>> 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.
> 
> I share the sentiment.  Leaving the result that was used in comparison
> in file(s) also helps debugging.
> 

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


Thank you.

Re: [PATCH v5] branch: support for shortcuts like @{-1}, completed

[Junio C Hamano]

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

> Mmm, I don't feel the same here, we already discussed about this. Maybe?:
>
> diff --git a/builtin/branch.c b/builtin/branch.c
> index 17853225fa..307073cc47 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -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"));

Before the above change, the body of the "else if" clause for the
option was self contained.  With the above change, the reader has to
follow to the end of the long top-level cascade to see the rest of
the function does not do anything funny.

If we have a big common clean-up after each operation, then, falling
through in the success case might be good, but that is not what I am
seeing here.  So...

Re: [PATCH v5] branch: support for shortcuts like @{-1}, completed

[Rubén Justo]

On 10/10/22 2:38, Junio C Hamano wrote:
> Rubén Justo <rjusto@gmail.com> writes:
> 
>> Mmm, I don't feel the same here, we already discussed about this. Maybe?:
>>
>> diff --git a/builtin/branch.c b/builtin/branch.c
>> index 17853225fa..307073cc47 100644
>> --- a/builtin/branch.c
>> +++ b/builtin/branch.c
>> @@ -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"));
> 
> Before the above change, the body of the "else if" clause for the
> option was self contained.  With the above change, the reader has to
> follow to the end of the long top-level cascade to see the rest of
> the function does not do anything funny.
> 
> If we have a big common clean-up after each operation, then, falling
> through in the success case might be good, but that is not what I am
> seeing here.  So...
> 

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.

Re: [PATCH v5] branch: support for shortcuts like @{-1}, completed

[Junio C Hamano]

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

>> If we have a big common clean-up after each operation, then, falling
>> through in the success case might be good, but that is not what I am
>> seeing here.  So...
>> 
>
> 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).

Re: [PATCH v5] branch: support for shortcuts like @{-1}, completed

[Rubén Justo]

On 10/10/22 18:55, Junio C Hamano wrote:
> Rubén Justo <rjusto@gmail.com> writes:
> 
>>> If we have a big common clean-up after each operation, then, falling
>>> through in the success case might be good, but that is not what I am
>>> seeing here.  So...
>>>
>>
>> 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.

[PATCH v6] branch: support for shortcuts like @{-1}, completed

[Rubén Justo]

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}

Signed-off-by: Rubén Justo <rjusto@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

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:
<xmqqlepp8smc.fsf@gitster.g>
<xmqqmtajosek.fsf@gitster.g>


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(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 55cd9a6e99..658c9f8741 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -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);
diff --git a/t/t3204-branch-name-interpretation.sh b/t/t3204-branch-name-interpretation.sh
index 993a6b5eff..793bf4d269 100755
--- a/t/t3204-branch-name-interpretation.sh
+++ b/t/t3204-branch-name-interpretation.sh
@@ -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