[PATCH 0/2] branch: fix some malfunctions in -m/-c

[PATCH 0/2] branch: fix some malfunctions in -m/-c

[Rubén Justo]

Fix some malfunctions found in git branch, copying and renaming                                                                                               
branches.                                                                                                                                                     
                                                                                                                                                              
                                                                                                                                                              
Rubén Justo (2):                                                                                                                                              
  branch: force-copy a branch to itself via @{-1} is a no-op                                                                                                  
  branch: clear target branch configuration before copying or renaming                                                                                        
                                                                                                                                                              
 builtin/branch.c                      | 17 +++++++++++------                                                                                                 
 t/t3200-branch.sh                     | 19 +++++++++++++++++++                                                                                               
 t/t3204-branch-name-interpretation.sh | 10 ++++++++++                                                                                                        
 3 files changed, 40 insertions(+), 6 deletions(-)                                                                                                            
                                                                                                                                                              
--                                                                                                                                                            
2.36.1

[PATCH 1/2] branch: force-copy a branch to itself via @{-1} is a no-op

[Rubén Justo]

Since 52d59cc645 (branch: add a --copy (-c) option to go with --move
(-m), 2017-06-18) we can copy a branch to make a new branch with the
'-c' (copy) option or to overwrite an existing branch using the '-C'
(force copy) option.  A no-op possibility is considered when we are
asked to copy a branch to itself, to follow the same no-op introduced
for the rename (-M) operation in 3f59481e33 (branch: allow a no-op
"branch -M <current-branch> HEAD", 2011-11-25).  To check for this, in
52d59cc645 we compared the branch names provided by the user, source
(HEAD if omitted) and destination, and a match is considered as this
no-op.

Since ae5a6c3684 (checkout: implement "@{-N}" shortcut name for N-th
last branch, 2009-01-17) a branch can be specified using shortcuts like
@{-1}.  This allows this usage:

	$ git checkout -b test
	$ git checkout -
	$ git branch -C test test  # no-op
	$ git branch -C test @{-1} # oops
	$ git branch -C @{-1} test # oops

As we are using the branch name provided by the user to do the
comparison, if one of the branches is provided using a shortcut we are
not going to have a match and a call to git_config_copy_section() will
happen.  This will make a duplicate of the configuration for that
branch, and with this progression the second call will produce four
copies of the configuration, and so on.

Let's use the interpreted branch name instead for this comparison.

The rename operation is not affected.

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

diff --git a/builtin/branch.c b/builtin/branch.c
index 15be0c03ef..a35e174aae 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -584,13 +584,13 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
 	strbuf_release(&logmsg);
 
 	strbuf_addf(&oldsection, "branch.%s", interpreted_oldname);
-	strbuf_release(&oldref);
 	strbuf_addf(&newsection, "branch.%s", interpreted_newname);
-	strbuf_release(&newref);
 	if (!copy && git_config_rename_section(oldsection.buf, newsection.buf) < 0)
 		die(_("Branch is renamed, but update of config-file failed"));
-	if (copy && strcmp(oldname, newname) && git_config_copy_section(oldsection.buf, newsection.buf) < 0)
+	if (copy && strcmp(interpreted_oldname, interpreted_newname) && git_config_copy_section(oldsection.buf, newsection.buf) < 0)
 		die(_("Branch is copied, but update of config-file failed"));
+	strbuf_release(&oldref);
+	strbuf_release(&newref);
 	strbuf_release(&oldsection);
 	strbuf_release(&newsection);
 }
diff --git a/t/t3204-branch-name-interpretation.sh b/t/t3204-branch-name-interpretation.sh
index 793bf4d269..3399344f25 100755
--- a/t/t3204-branch-name-interpretation.sh
+++ b/t/t3204-branch-name-interpretation.sh
@@ -57,6 +57,16 @@ test_expect_success 'create branch with pseudo-qualified name' '
 	expect_branch refs/heads/refs/heads/qualified two
 '
 
+test_expect_success 'force-copy a branch to itself via @{-1} is no-op' '
+	git branch -t copiable main &&
+	git checkout copiable &&
+	git checkout - &&
+	git branch -C @{-1} copiable &&
+	git config --get-all branch.copiable.merge >actual &&
+	echo refs/heads/main >expect &&
+	test_cmp expect actual
+'
+
 test_expect_success 'delete branch via @{-1}' '
 	git branch previous-del &&
 
-- 
2.36.1

[PATCH 2/2] branch: clear target branch configuration before copying or renaming

[Rubén Justo]

There are two problems with -m (rename) and -c (copy) branch operations.

 1. If we force-rename or force-copy a branch to overwrite another
 branch that already has configuration, the resultant branch ends up
 with the source configuration (if any) mixed with the configuration for
 the overwritten branch.

	$ git branch upstream
	$ git branch -t foo upstream  # foo has tracking configuration
	$ git branch bar              # bar has not
	$ git branch -M bar foo       # force-rename bar to foo
	$ git config branch.foo.merge # must return clear
	refs/heads/upstream

 2. If we repeatedly force-copy a branch to the same name, the branch
 configuration is repeatedly copied each time.

	$ git branch upstream
	$ git branch -t foo upstream  # foo has tracking configuration
	$ git branch -c foo bar       # bar is a copy of foo
	$ git branch -C foo bar       # again
	$ git branch -C foo bar       # ..
	$ git config --get-all branch.bar.merge # must return one value
	refs/heads/upstream
	refs/heads/upstream
	refs/heads/upstream

Whenever we copy or move (forced or not) we must make sure that there is
no residual configuration that will be, probably erroneously, inherited
by the new branch.  To avoid confusions, clear any branch configuration
before setting the configuration from the copied or moved branch.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 builtin/branch.c  | 17 +++++++++++------
 t/t3200-branch.sh | 19 +++++++++++++++++++
 2 files changed, 30 insertions(+), 6 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index a35e174aae..14664f0278 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -583,12 +583,17 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
 
 	strbuf_release(&logmsg);
 
-	strbuf_addf(&oldsection, "branch.%s", interpreted_oldname);
-	strbuf_addf(&newsection, "branch.%s", interpreted_newname);
-	if (!copy && git_config_rename_section(oldsection.buf, newsection.buf) < 0)
-		die(_("Branch is renamed, but update of config-file failed"));
-	if (copy && strcmp(interpreted_oldname, interpreted_newname) && git_config_copy_section(oldsection.buf, newsection.buf) < 0)
-		die(_("Branch is copied, but update of config-file failed"));
+	if (strcmp(interpreted_oldname, interpreted_newname)) {
+		strbuf_addf(&oldsection, "branch.%s", interpreted_oldname);
+		strbuf_addf(&newsection, "branch.%s", interpreted_newname);
+
+		delete_branch_config(interpreted_newname);
+
+		if (!copy && git_config_rename_section(oldsection.buf, newsection.buf) < 0)
+			die(_("Branch is renamed, but update of config-file failed"));
+		if (copy && git_config_copy_section(oldsection.buf, newsection.buf) < 0)
+			die(_("Branch is copied, but update of config-file failed"));
+	}
 	strbuf_release(&oldref);
 	strbuf_release(&newref);
 	strbuf_release(&oldsection);
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 7f605f865b..c3b3d11c28 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -218,6 +218,25 @@ test_expect_success 'git branch -M should leave orphaned HEAD alone' '
 	)
 '
 
+test_expect_success 'git branch -M inherites clean tracking setup' '
+	test_when_finished git branch -D moved &&
+	git branch -t main-tracked main &&
+	git branch non-tracked &&
+	git branch -M main-tracked moved &&
+	git branch --unset-upstream moved &&
+	git branch -M non-tracked moved &&
+	test_must_fail git branch --unset-upstream moved
+'
+
+test_expect_success 'git branch -C inherites clean tracking setup' '
+	test_when_finished git branch -D copiable copied &&
+	git branch -t copiable main &&
+	git branch -C copiable copied &&
+	git branch --unset-upstream copied &&
+	git branch -C copied copiable &&
+	test_must_fail git branch --unset-upstream copiable
+'
+
 test_expect_success 'resulting reflog can be shown by log -g' '
 	oid=$(git rev-parse HEAD) &&
 	cat >expect <<-EOF &&
-- 
2.36.1

Re: [PATCH 1/2] branch: force-copy a branch to itself via @{-1} is a no-op

[Victoria Dye]

Rubén Justo wrote:
> Since 52d59cc645 (branch: add a --copy (-c) option to go with --move
> (-m), 2017-06-18) we can copy a branch to make a new branch with the
> '-c' (copy) option or to overwrite an existing branch using the '-C'
> (force copy) option.  A no-op possibility is considered when we are
> asked to copy a branch to itself, to follow the same no-op introduced
> for the rename (-M) operation in 3f59481e33 (branch: allow a no-op
> "branch -M <current-branch> HEAD", 2011-11-25).  To check for this, in
> 52d59cc645 we compared the branch names provided by the user, source
> (HEAD if omitted) and destination, and a match is considered as this
> no-op.
> 
> Since ae5a6c3684 (checkout: implement "@{-N}" shortcut name for N-th
> last branch, 2009-01-17) a branch can be specified using shortcuts like
> @{-1}.  This allows this usage:
> 
> 	$ git checkout -b test
> 	$ git checkout -
> 	$ git branch -C test test  # no-op
> 	$ git branch -C test @{-1} # oops
> 	$ git branch -C @{-1} test # oops
> 
> As we are using the branch name provided by the user to do the
> comparison, if one of the branches is provided using a shortcut we are
> not going to have a match and a call to git_config_copy_section() will
> happen.  This will make a duplicate of the configuration for that
> branch, and with this progression the second call will produce four
> copies of the configuration, and so on.

This is a clear explanation of what the issue is and why it's happening.

> 
> Let's use the interpreted branch name instead for this comparison.
> 
> The rename operation is not affected.
> 
> Signed-off-by: Rubén Justo <rjusto@gmail.com>
> ---
>  builtin/branch.c                      |  6 +++---
>  t/t3204-branch-name-interpretation.sh | 10 ++++++++++
>  2 files changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/builtin/branch.c b/builtin/branch.c
> index 15be0c03ef..a35e174aae 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -584,13 +584,13 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
>  	strbuf_release(&logmsg);
>  
>  	strbuf_addf(&oldsection, "branch.%s", interpreted_oldname);
> -	strbuf_release(&oldref);
>  	strbuf_addf(&newsection, "branch.%s", interpreted_newname);
> -	strbuf_release(&newref);
>  	if (!copy && git_config_rename_section(oldsection.buf, newsection.buf) < 0)
>  		die(_("Branch is renamed, but update of config-file failed"));
> -	if (copy && strcmp(oldname, newname) && git_config_copy_section(oldsection.buf, newsection.buf) < 0)
> +	if (copy && strcmp(interpreted_oldname, interpreted_newname) && git_config_copy_section(oldsection.buf, newsection.buf) < 0)

I double-checked that 'interpreted_oldname' and 'interpreted_newname' are
always set (and not only when a shortcut name is used), and they are. So,
this does exactly what you intend.

>  		die(_("Branch is copied, but update of config-file failed"));
> +	strbuf_release(&oldref);
> +	strbuf_release(&newref);
>  	strbuf_release(&oldsection);
>  	strbuf_release(&newsection);
>  }
> diff --git a/t/t3204-branch-name-interpretation.sh b/t/t3204-branch-name-interpretation.sh
> index 793bf4d269..3399344f25 100755
> --- a/t/t3204-branch-name-interpretation.sh
> +++ b/t/t3204-branch-name-interpretation.sh
> @@ -57,6 +57,16 @@ test_expect_success 'create branch with pseudo-qualified name' '
>  	expect_branch refs/heads/refs/heads/qualified two
>  '
>  
> +test_expect_success 'force-copy a branch to itself via @{-1} is no-op' '
> +	git branch -t copiable main &&
> +	git checkout copiable &&
> +	git checkout - &&
> +	git branch -C @{-1} copiable &&
> +	git config --get-all branch.copiable.merge >actual &&
> +	echo refs/heads/main >expect &&
> +	test_cmp expect actual
> +'
> +

And the test is straightforward and demonstrates the fix. Thanks for the
well-written patch, this looks good to me! 

Re: [PATCH 2/2] branch: clear target branch configuration before copying or renaming

[Victoria Dye]

Rubén Justo wrote:
> There are two problems with -m (rename) and -c (copy) branch operations.
> 
>  1. If we force-rename or force-copy a branch to overwrite another
>  branch that already has configuration, the resultant branch ends up
>  with the source configuration (if any) mixed with the configuration for
>  the overwritten branch.
> 
> 	$ git branch upstream
> 	$ git branch -t foo upstream  # foo has tracking configuration
> 	$ git branch bar              # bar has not
> 	$ git branch -M bar foo       # force-rename bar to foo
> 	$ git config branch.foo.merge # must return clear
> 	refs/heads/upstream

What happens if 'bar' has tracking info? You mentioned that the
configuration is "mixed" - does that mean 'foo' would have both the original
'foo's tracking info *and* 'bar's? 
> 
>  2. If we repeatedly force-copy a branch to the same name, the branch
>  configuration is repeatedly copied each time.
> 
> 	$ git branch upstream
> 	$ git branch -t foo upstream  # foo has tracking configuration
> 	$ git branch -c foo bar       # bar is a copy of foo
> 	$ git branch -C foo bar       # again
> 	$ git branch -C foo bar       # ..
> 	$ git config --get-all branch.bar.merge # must return one value
> 	refs/heads/upstream
> 	refs/heads/upstream
> 	refs/heads/upstream
> 
> Whenever we copy or move (forced or not) we must make sure that there is
> no residual configuration that will be, probably erroneously, inherited
> by the new branch.  To avoid confusions, clear any branch configuration
> before setting the configuration from the copied or moved branch.

I wasn't sure whether "transfer the source's tracking information to the
destination" was the desired behavior, but it looks like it is (per
52d59cc6452 (branch: add a --copy (-c) option to go with --move (-m),
2017-06-18)). Given that, I agree this is the right fix for the issue you've
identified.

> 
> Signed-off-by: Rubén Justo <rjusto@gmail.com>
> ---
>  builtin/branch.c  | 17 +++++++++++------
>  t/t3200-branch.sh | 19 +++++++++++++++++++
>  2 files changed, 30 insertions(+), 6 deletions(-)
> 
> diff --git a/builtin/branch.c b/builtin/branch.c
> index a35e174aae..14664f0278 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -583,12 +583,17 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
>  
>  	strbuf_release(&logmsg);
>  
> -	strbuf_addf(&oldsection, "branch.%s", interpreted_oldname);
> -	strbuf_addf(&newsection, "branch.%s", interpreted_newname);
> -	if (!copy && git_config_rename_section(oldsection.buf, newsection.buf) < 0)
> -		die(_("Branch is renamed, but update of config-file failed"));
> -	if (copy && strcmp(interpreted_oldname, interpreted_newname) && git_config_copy_section(oldsection.buf, newsection.buf) < 0)
> -		die(_("Branch is copied, but update of config-file failed"));
> +	if (strcmp(interpreted_oldname, interpreted_newname)) {
> +		strbuf_addf(&oldsection, "branch.%s", interpreted_oldname);
> +		strbuf_addf(&newsection, "branch.%s", interpreted_newname);
> +
> +		delete_branch_config(interpreted_newname);
> +
> +		if (!copy && git_config_rename_section(oldsection.buf, newsection.buf) < 0)
> +			die(_("Branch is renamed, but update of config-file failed"));
> +		if (copy && git_config_copy_section(oldsection.buf, newsection.buf) < 0)
> +			die(_("Branch is copied, but update of config-file failed"));
> +	}

Makes sense. 

>  	strbuf_release(&oldref);
>  	strbuf_release(&newref);
>  	strbuf_release(&oldsection);
> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
> index 7f605f865b..c3b3d11c28 100755
> --- a/t/t3200-branch.sh
> +++ b/t/t3200-branch.sh
> @@ -218,6 +218,25 @@ test_expect_success 'git branch -M should leave orphaned HEAD alone' '
>  	)
>  '
>  
> +test_expect_success 'git branch -M inherites clean tracking setup' '

s/inherites/inherits

> +	test_when_finished git branch -D moved &&
> +	git branch -t main-tracked main &&
> +	git branch non-tracked &&
> +	git branch -M main-tracked moved &&
> +	git branch --unset-upstream moved &&
> +	git branch -M non-tracked moved &&
> +	test_must_fail git branch --unset-upstream moved

If I'm reading this correctly, the test doesn't actually demonstrate that
'git branch -M' cleans up the tracking info, since 'moved' never has any
tracking info immediately before 'git branch -M'. The test could also be
more precise by verifying the upstream name matches. What about something
like this?

	test_when_finished git branch -D moved &&
	git branch -t main-tracked main &&
	git branch non-tracked &&

	# `moved` doesn't exist, so it starts with no tracking info
	echo main >expect &&
	git branch -M main-tracked moved &&
	git rev-parse --abbrev-ref moved@{upstream} >actual &&
	test_cmp expect actual &&

	# At this point, `moved` is tracking `main`
	git branch -M non-tracked moved &&
	test_must_fail git rev-parse --abbrev-ref moved@{upstream}

> +'
> +
> +test_expect_success 'git branch -C inherites clean tracking setup' '

s/inherites/inherits (same typo as before, just pointing it out so it's
easier to find)

> +	test_when_finished git branch -D copiable copied &&
> +	git branch -t copiable main &&
> +	git branch -C copiable copied &&
> +	git branch --unset-upstream copied &&
> +	git branch -C copied copiable &&
> +	test_must_fail git branch --unset-upstream copiable

This doesn't have the same issue as the other test (since 'copied' has no
tracking but 'copiable' does before both 'git branch -C' calls), but it
would still be nice to use the 'git rev-parse --abbrev-ref
<branch>@{upstream}' for more precision here. 

> +'
> +
>  test_expect_success 'resulting reflog can be shown by log -g' '
>  	oid=$(git rev-parse HEAD) &&
>  	cat >expect <<-EOF &&

Re: [PATCH 1/2] branch: force-copy a branch to itself via @{-1} is a no-op

[Ævar Arnfjörð Bjarmason]

On Thu, Nov 17 2022, Rubén Justo wrote:

> Since 52d59cc645 (branch: add a --copy (-c) option to go with --move
> (-m), 2017-06-18) we can copy a branch to make a new branch with the
> '-c' (copy) option or to overwrite an existing branch using the '-C'
> (force copy) option.  A no-op possibility is considered when we are
> asked to copy a branch to itself, to follow the same no-op introduced
> for the rename (-M) operation in 3f59481e33 (branch: allow a no-op
> "branch -M <current-branch> HEAD", 2011-11-25).  To check for this, in
> 52d59cc645 we compared the branch names provided by the user, source
> (HEAD if omitted) and destination, and a match is considered as this
> no-op.
>
> Since ae5a6c3684 (checkout: implement "@{-N}" shortcut name for N-th
> last branch, 2009-01-17) a branch can be specified using shortcuts like
> @{-1}.  This allows this usage:
>
> 	$ git checkout -b test
> 	$ git checkout -
> 	$ git branch -C test test  # no-op
> 	$ git branch -C test @{-1} # oops
> 	$ git branch -C @{-1} test # oops
>
> As we are using the branch name provided by the user to do the
> comparison, if one of the branches is provided using a shortcut we are
> not going to have a match and a call to git_config_copy_section() will
> happen.  This will make a duplicate of the configuration for that
> branch, and with this progression the second call will produce four
> copies of the configuration, and so on.
>
> Let's use the interpreted branch name instead for this comparison.
>
> The rename operation is not affected.

Good catch! Yes this definitely wasn't intended, and is just a failure
of the config name v.s. ref names drifting from what the previous logic
was assuming.

> @@ -584,13 +584,13 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
>  	strbuf_release(&logmsg);
>  
>  	strbuf_addf(&oldsection, "branch.%s", interpreted_oldname);
> -	strbuf_release(&oldref);
>  	strbuf_addf(&newsection, "branch.%s", interpreted_newname);
> -	strbuf_release(&newref);
>  	if (!copy && git_config_rename_section(oldsection.buf, newsection.buf) < 0)
>  		die(_("Branch is renamed, but update of config-file failed"));
> -	if (copy && strcmp(oldname, newname) && git_config_copy_section(oldsection.buf, newsection.buf) < 0)
> +	if (copy && strcmp(interpreted_oldname, interpreted_newname) && git_config_copy_section(oldsection.buf, newsection.buf) < 0)

We try to stay under 79 chars, see CodingGuidelines. The pre-image was
already violating this, but the new one is really long. I think it would
be good to just wrap this after the last && while at it.

>  		die(_("Branch is copied, but update of config-file failed"));
> +	strbuf_release(&oldref);
> +	strbuf_release(&newref);
>  	strbuf_release(&oldsection);
>  	strbuf_release(&newsection);

This moving around of destructors isn't needed, and is just some
unrelated cleanup. Your change here only needs to be:

	-       if (copy && strcmp(oldname, newname) && git_config_copy_section(oldsection.buf, newsection.buf) < 0)
	+       if (copy && strcmp(interpreted_oldname, interpreted_newname) &&
	+           git_config_copy_section(oldsection.buf, newsection.buf) < 0)

If you'd like to re-arrange some of this and e.g. free stuff at the end
instead of after it's last used (which is what the current code is
aiming for) that's arguably good, but let's do that in another commit
then.

Re: [PATCH 2/2] branch: clear target branch configuration before copying or renaming

[Ævar Arnfjörð Bjarmason]

On Thu, Nov 17 2022, Rubén Justo wrote:

> There are two problems with -m (rename) and -c (copy) branch operations.
>
>  1. If we force-rename or force-copy a branch to overwrite another
>  branch that already has configuration, the resultant branch ends up
>  with the source configuration (if any) mixed with the configuration for
>  the overwritten branch.
>
> 	$ git branch upstream
> 	$ git branch -t foo upstream  # foo has tracking configuration
> 	$ git branch bar              # bar has not
> 	$ git branch -M bar foo       # force-rename bar to foo
> 	$ git config branch.foo.merge # must return clear
> 	refs/heads/upstream

I'm fuzzy on whether Sahil and I discussed these edge cases at the time,
but my first reaction was surprise that you thought this was purely a
bug, I'd have thought it was a feature.

I.e. yes there's bugs & edge cases here, but fundimentally doesn't it
make sense to think about "branch -c" as being mostly equivalent to a
hypothetical:

	git branch --just-the-ref-operations -c <old> <new>
	git config --rename-section branch.<old> branch.<new>

And not:

	git config --remove-section branch.<new>
	git branch --just-the-ref-operations -c <old> <new>
	git config --rename-section branch.<old> branch.<new>

From reading the initial thread I see the "delete first" seems to have
been a TODO item of Sahil's[1], but the "copy branch" initally (I
mentored Sahil on it) from a shell one-liner I still have in my
.gitconfig history, which was a mostly-rename-section.
	
>  2. If we repeatedly force-copy a branch to the same name, the branch
>  configuration is repeatedly copied each time.
>
> 	$ git branch upstream
> 	$ git branch -t foo upstream  # foo has tracking configuration
> 	$ git branch -c foo bar       # bar is a copy of foo
> 	$ git branch -C foo bar       # again
> 	$ git branch -C foo bar       # ..
> 	$ git config --get-all branch.bar.merge # must return one value
> 	refs/heads/upstream
> 	refs/heads/upstream
> 	refs/heads/upstream

Yeah, you came about this conclusion because you're looking at the
tracking config, of which there should be only one.

Our config space is multi-value in general, although most (all?) of our
branch.* space is one-value.

But users can also stick things in there, so....

> Whenever we copy or move (forced or not) we must make sure that there is
> no residual configuration that will be, probably erroneously, inherited
> by the new branch.  To avoid confusions, clear any branch configuration
> before setting the configuration from the copied or moved branch.

So, whatever tea leaves we read into the history, or whether this was a
good or bad design in the first place, I think we should probably lean
towards not having this be a bug fix, but a new feature. Both modes are
clearly easy to support.

And then document it in terms of some new switch being the equivalent to
--remove-section followed by a rename, the existing thing a rename etc.

> @@ -583,12 +583,17 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
>  
>  	strbuf_release(&logmsg);
>  
> -	strbuf_addf(&oldsection, "branch.%s", interpreted_oldname);
> -	strbuf_addf(&newsection, "branch.%s", interpreted_newname);
> -	if (!copy && git_config_rename_section(oldsection.buf, newsection.buf) < 0)
> -		die(_("Branch is renamed, but update of config-file failed"));
> -	if (copy && strcmp(interpreted_oldname, interpreted_newname) && git_config_copy_section(oldsection.buf, newsection.buf) < 0)
> -		die(_("Branch is copied, but update of config-file failed"));
> +	if (strcmp(interpreted_oldname, interpreted_newname)) {
> +		strbuf_addf(&oldsection, "branch.%s", interpreted_oldname);
> +		strbuf_addf(&newsection, "branch.%s", interpreted_newname);
> +
> +		delete_branch_config(interpreted_newname);
> +
> +		if (!copy && git_config_rename_section(oldsection.buf, newsection.buf) < 0)
> +			die(_("Branch is renamed, but update of config-file failed"));
> +		if (copy && git_config_copy_section(oldsection.buf, newsection.buf) < 0)
> +			die(_("Branch is copied, but update of config-file failed"));

Aside from any question of a hypothetical "should", your implementation
is running head-first into a major caveat in our config API.

Which is that we don't have transactions or rollbacks, and we don't even
carry a lock forward for all of these.

So, there's crappy edge cases in the old implementation as you've found,
but at least it mostly failed-safe.

But here we'll delete_branch_config(), then release the lock, and then
try to rename the new branch to that location, which might fail.

So, we'll be left with no config for the thing we tried to clobber, nor
the new config.

Re: [PATCH 2/2] branch: clear target branch configuration before copying or renaming

[Victoria Dye]

Ævar Arnfjörð Bjarmason wrote:
> 
> On Thu, Nov 17 2022, Rubén Justo wrote:
>> Whenever we copy or move (forced or not) we must make sure that there is
>> no residual configuration that will be, probably erroneously, inherited
>> by the new branch.  To avoid confusions, clear any branch configuration
>> before setting the configuration from the copied or moved branch.
> 
> So, whatever tea leaves we read into the history, or whether this was a
> good or bad design in the first place, I think we should probably lean
> towards not having this be a bug fix, but a new feature. Both modes are
> clearly easy to support.
> 
> And then document it in terms of some new switch being the equivalent to
> --remove-section followed by a rename, the existing thing a rename etc.

I've noticed a bit of a pattern on the mailing list where, if the desired
user experience is unclear, the suggestion is "add an option! then users can
choose for themselves." But then, at the same time, we'll complain about
option bloat (as you [1] and Taylor [2] did on another recent series).

I don't see a compelling enough reason to introduce an option variant here.
Could I imagine a user wanting such a feature? Yes. But it also isn't clear
what users would practically use. I think a more conservative approach would
be appropriate: in this case, come to an agreement on a sane default (i.e.,
should we preserve the source's, or the destination's, tracking config?),
then wait for feedback to indicate whether there's a desire for an
alternative to that default.

[1] https://lore.kernel.org/git/221117.86k03tiudl.gmgdl@evledraar.gmail.com/
[2] https://lore.kernel.org/git/Y3ave2+kEwLTvtE6@nand.local/

> 
>> @@ -583,12 +583,17 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
>>  
>>  	strbuf_release(&logmsg);
>>  
>> -	strbuf_addf(&oldsection, "branch.%s", interpreted_oldname);
>> -	strbuf_addf(&newsection, "branch.%s", interpreted_newname);
>> -	if (!copy && git_config_rename_section(oldsection.buf, newsection.buf) < 0)
>> -		die(_("Branch is renamed, but update of config-file failed"));
>> -	if (copy && strcmp(interpreted_oldname, interpreted_newname) && git_config_copy_section(oldsection.buf, newsection.buf) < 0)
>> -		die(_("Branch is copied, but update of config-file failed"));
>> +	if (strcmp(interpreted_oldname, interpreted_newname)) {
>> +		strbuf_addf(&oldsection, "branch.%s", interpreted_oldname);
>> +		strbuf_addf(&newsection, "branch.%s", interpreted_newname);
>> +
>> +		delete_branch_config(interpreted_newname);
>> +
>> +		if (!copy && git_config_rename_section(oldsection.buf, newsection.buf) < 0)
>> +			die(_("Branch is renamed, but update of config-file failed"));
>> +		if (copy && git_config_copy_section(oldsection.buf, newsection.buf) < 0)
>> +			die(_("Branch is copied, but update of config-file failed"));
> 
> Aside from any question of a hypothetical "should", your implementation
> is running head-first into a major caveat in our config API.
> 
> Which is that we don't have transactions or rollbacks, and we don't even
> carry a lock forward for all of these.
> 
> So, there's crappy edge cases in the old implementation as you've found,
> but at least it mostly failed-safe.
> 
> But here we'll delete_branch_config(), then release the lock, and then
> try to rename the new branch to that location, which might fail.
> 
> So, we'll be left with no config for the thing we tried to clobber, nor
> the new config.

This is a good point, so to add to it: I think a fairly unobtrusive solution
would be to move the config deletion after the rename is 100% complete.

Re: [PATCH 2/2] branch: clear target branch configuration before copying or renaming

[Ævar Arnfjörð Bjarmason]

On Fri, Nov 18 2022, Victoria Dye wrote:

> Ævar Arnfjörð Bjarmason wrote:
>> 
>> On Thu, Nov 17 2022, Rubén Justo wrote:
>>> Whenever we copy or move (forced or not) we must make sure that there is
>>> no residual configuration that will be, probably erroneously, inherited
>>> by the new branch.  To avoid confusions, clear any branch configuration
>>> before setting the configuration from the copied or moved branch.
>> 
>> So, whatever tea leaves we read into the history, or whether this was a
>> good or bad design in the first place, I think we should probably lean
>> towards not having this be a bug fix, but a new feature. Both modes are
>> clearly easy to support.
>> 
>> And then document it in terms of some new switch being the equivalent to
>> --remove-section followed by a rename, the existing thing a rename etc.
>
> I've noticed a bit of a pattern on the mailing list where, if the desired
> user experience is unclear, the suggestion is "add an option! then users can
> choose for themselves." But then, at the same time, we'll complain about
> option bloat (as you [1] and Taylor [2] did on another recent series).

I think [1] and [2] aren't comparable in at least a couple of ways to
this case:

 a) [1] and [2] are discussions about something you can trivially do
    outside of git itself, which isn't the case with most behavior and
    options we implement, including this "branch move/copy".

    I mean you can of course script it with the refs primatives and "git
    config", but it's not "just use grep" or "just use head".

 b) [1] and [2] are a discussion about the trade-offs of adding new
    behavior, which by definition nobody relies on us for. So I think
    "a" is kind of moot to begin with.

    There's UX behavior in Git that I think sucks, but which I think
    would be irresponsible to simply change, much of it I think we'd be
    better off not having, if we had a convenient time machine.

    But that's not realistic, even if behavior is dumb or we'd like to
    change it in hindsight, we'll always need to keep the potential
    impact in mind if it's in released versions.

    Here we're talking about changing the semantics of a feature that's
    already been in released version for 5 years.

Now, I honestly don't know where I stand on "b" yet, maybe this is a
thing we can simply change.

But my spidey sense is a bit tickled by the proposed change discussing
the existing behavior as a pure bug, when it seems to me that it's
approximately what you'd get if it was implemented in terms of "config
--rename-section", v.s. the proposed new behavior of (as I understand
it) a "config --remove-section" followed by "config --rename-section".

> I don't see a compelling enough reason to introduce an option variant here.
> Could I imagine a user wanting such a feature? Yes. But it also isn't clear
> what users would practically use. I think a more conservative approach would
> be appropriate: in this case, come to an agreement on a sane default (i.e.,
> should we preserve the source's, or the destination's, tracking config?),
> then wait for feedback to indicate whether there's a desire for an
> alternative to that default.

I think a conservative approach would be to focus narrowly on the
tracking config, if that's the thing that's at issue. I.e. we read that
ourselves, know that it's broken if it's transmuted in certain ways etc.

But the proposed patch suggests that we extend that fix to anything we
might find in that config namespace.

It's also the nature of the fix, i.e. an existing option being changed
without a doc change to be destructive when it wasn't before.

For "git remote" we error out if "remote_is_configured()" before we call
the very same underlying function that we do for the branch
copy/move. Maybe that would make more sense? I don't know...

> [1] https://lore.kernel.org/git/221117.86k03tiudl.gmgdl@evledraar.gmail.com/
> [2] https://lore.kernel.org/git/Y3ave2+kEwLTvtE6@nand.local/
>
>> 
>>> @@ -583,12 +583,17 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
>>>  
>>>  	strbuf_release(&logmsg);
>>>  
>>> -	strbuf_addf(&oldsection, "branch.%s", interpreted_oldname);
>>> -	strbuf_addf(&newsection, "branch.%s", interpreted_newname);
>>> -	if (!copy && git_config_rename_section(oldsection.buf, newsection.buf) < 0)
>>> -		die(_("Branch is renamed, but update of config-file failed"));
>>> -	if (copy && strcmp(interpreted_oldname, interpreted_newname) && git_config_copy_section(oldsection.buf, newsection.buf) < 0)
>>> -		die(_("Branch is copied, but update of config-file failed"));
>>> +	if (strcmp(interpreted_oldname, interpreted_newname)) {
>>> +		strbuf_addf(&oldsection, "branch.%s", interpreted_oldname);
>>> +		strbuf_addf(&newsection, "branch.%s", interpreted_newname);
>>> +
>>> +		delete_branch_config(interpreted_newname);
>>> +
>>> +		if (!copy && git_config_rename_section(oldsection.buf, newsection.buf) < 0)
>>> +			die(_("Branch is renamed, but update of config-file failed"));
>>> +		if (copy && git_config_copy_section(oldsection.buf, newsection.buf) < 0)
>>> +			die(_("Branch is copied, but update of config-file failed"));
>> 
>> Aside from any question of a hypothetical "should", your implementation
>> is running head-first into a major caveat in our config API.
>> 
>> Which is that we don't have transactions or rollbacks, and we don't even
>> carry a lock forward for all of these.
>> 
>> So, there's crappy edge cases in the old implementation as you've found,
>> but at least it mostly failed-safe.
>> 
>> But here we'll delete_branch_config(), then release the lock, and then
>> try to rename the new branch to that location, which might fail.
>> 
>> So, we'll be left with no config for the thing we tried to clobber, nor
>> the new config.
>
> This is a good point, so to add to it: I think a fairly unobtrusive solution
> would be to move the config deletion after the rename is 100% complete.

Right now we do it in one go under one *.lock in
git_config_copy_section(), which has a stateful loop that mutates the
config.

So rather than "after", why not all at once? I don't see why we'd need
two calls & lock/unlock operations.

It's not like we're calling a generic function, it already has
special-cased code for "here's what I do when I'm doing the branch copy
thing", so we could implement any new behavior there.

Aside from anything else, I think one thing this proposed change
absolutely need is a documentation update. I.e. now we say:
	
	With a `-m` or `-M` option, <oldbranch> will be renamed to <newbranch>.
	If <oldbranch> had a corresponding reflog, it is renamed to match
	<newbranch>, and a reflog entry is created to remember the branch
	renaming. If <newbranch> exists, -M must be used to force the rename
	to happen.
	
	The `-c` and `-C` options have the exact same semantics as `-m` and
	`-M`, except instead of the branch being renamed, it will be copied to a
	new name, along with its config and reflog.

Which to my reading really leaves this ambiguous either way. I.e. we do
say that we'll refuse to operate if the *ref* already exists, but say
nothing about existing config.

Between that and using "renamed" (which I think a user might reasonably
assume means '"git config"'s idea of rename") it seems to me to suggest
that the pre-flight checking explicitly doesn't include the config.

But it's really quite ambiguous, and isn't spelled out explicitly. So if
we're going to wipe out the existing config, we really should update the
docs.

Also, whatever this is supposed to do, shouldn't we make it consistent
with other "git branch" modes. E.g.:

	git branch -D foo
	git -P config --show-origin --get-regexp '^branch\.foo\.'
	git config branch.foo.rebase true
	git branch foo -t origin/seen
	git -P config --show-origin --get-regexp '^branch\.foo\.'

Will emit:
	
	Deleted branch foo (was ...).
	branch 'foo' set up to track 'origin/seen'.
	file:.git/config        branch.foo.rebase true
	file:.git/config        branch.foo.remote origin
	file:.git/config        branch.foo.merge refs/heads/seen

I.e. we don't wipe away existing branch.foo.* config for new branch
creation, so why would copying or moving to the "foo" name act
differently?

If you set "git config branch.foo.merge refs/heads/next" beforehand
instead we'll wipe away *that config key*, but as noted above I think
that might be sensible.

I haven't explored other existing behavior fully, but maybe the proposed
change can look into that further ... :)

The upthread commit message doesn't discuss it, but even multiple
"merge" values are a valid state of affairs. E.g.:
	
	$ git branch seen -t origin/seen
	branch 'seen' set up to track 'origin/seen'.
	$ git branch next -t origin/next
	branch 'next' set up to track 'origin/next'.
	$ git branch -C seen next
	$ git config --get-regexp 'branch\.next'
	branch.next.remote origin
	branch.next.merge refs/heads/seen
	branch.next.remote origin
	branch.next.merge refs/heads/next

Now, if you do:

	git checkout next

and:
	git pull --no-rebase

You'll get e.g.:

       Merge branches 'next' and 'seen' of github.com:git/git into next

Now, personally I don't use it like that, but it works now. Whatever
anyone's stance on the sensibility of existing established behavior, I'd
think (sans perhaps obvious bugs) that we'd start out with testing how
the existing behavior acts before changing it.

And again, don't take any of that as an a-priori argument against
changing it, I'm still undecided. I'd just prefer that we know what it
is we're changing.

Re: [PATCH 1/2] branch: force-copy a branch to itself via @{-1} is a no-op

[Rubén Justo]

On 17/11/22 23:18, Victoria Dye wrote:
> Rubén Justo wrote:
>> Since 52d59cc645 (branch: add a --copy (-c) option to go with --move
>> (-m), 2017-06-18) we can copy a branch to make a new branch with the
>> '-c' (copy) option or to overwrite an existing branch using the '-C'
>> (force copy) option.  A no-op possibility is considered when we are
>> asked to copy a branch to itself, to follow the same no-op introduced
>> for the rename (-M) operation in 3f59481e33 (branch: allow a no-op
>> "branch -M <current-branch> HEAD", 2011-11-25).  To check for this, in
>> 52d59cc645 we compared the branch names provided by the user, source
>> (HEAD if omitted) and destination, and a match is considered as this
>> no-op.
>>
>> Since ae5a6c3684 (checkout: implement "@{-N}" shortcut name for N-th
>> last branch, 2009-01-17) a branch can be specified using shortcuts like
>> @{-1}.  This allows this usage:
>>
>> 	$ git checkout -b test
>> 	$ git checkout -
>> 	$ git branch -C test test  # no-op
>> 	$ git branch -C test @{-1} # oops
>> 	$ git branch -C @{-1} test # oops
>>
>> As we are using the branch name provided by the user to do the
>> comparison, if one of the branches is provided using a shortcut we are
>> not going to have a match and a call to git_config_copy_section() will
>> happen.  This will make a duplicate of the configuration for that
>> branch, and with this progression the second call will produce four
>> copies of the configuration, and so on.
> 
> This is a clear explanation of what the issue is and why it's happening.
> 
>>
>> Let's use the interpreted branch name instead for this comparison.
>>
>> The rename operation is not affected.
>>
>> Signed-off-by: Rubén Justo <rjusto@gmail.com>
>> ---
>>  builtin/branch.c                      |  6 +++---
>>  t/t3204-branch-name-interpretation.sh | 10 ++++++++++
>>  2 files changed, 13 insertions(+), 3 deletions(-)
>>
>> diff --git a/builtin/branch.c b/builtin/branch.c
>> index 15be0c03ef..a35e174aae 100644
>> --- a/builtin/branch.c
>> +++ b/builtin/branch.c
>> @@ -584,13 +584,13 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
>>  	strbuf_release(&logmsg);
>>  
>>  	strbuf_addf(&oldsection, "branch.%s", interpreted_oldname);
>> -	strbuf_release(&oldref);
>>  	strbuf_addf(&newsection, "branch.%s", interpreted_newname);
>> -	strbuf_release(&newref);
>>  	if (!copy && git_config_rename_section(oldsection.buf, newsection.buf) < 0)
>>  		die(_("Branch is renamed, but update of config-file failed"));
>> -	if (copy && strcmp(oldname, newname) && git_config_copy_section(oldsection.buf, newsection.buf) < 0)
>> +	if (copy && strcmp(interpreted_oldname, interpreted_newname) && git_config_copy_section(oldsection.buf, newsection.buf) < 0)
> 
> I double-checked that 'interpreted_oldname' and 'interpreted_newname' are
> always set (and not only when a shortcut name is used), and they are. So,
> this does exactly what you intend.
> 
>>  		die(_("Branch is copied, but update of config-file failed"));
>> +	strbuf_release(&oldref);
>> +	strbuf_release(&newref);
>>  	strbuf_release(&oldsection);
>>  	strbuf_release(&newsection);
>>  }
>> diff --git a/t/t3204-branch-name-interpretation.sh b/t/t3204-branch-name-interpretation.sh
>> index 793bf4d269..3399344f25 100755
>> --- a/t/t3204-branch-name-interpretation.sh
>> +++ b/t/t3204-branch-name-interpretation.sh
>> @@ -57,6 +57,16 @@ test_expect_success 'create branch with pseudo-qualified name' '
>>  	expect_branch refs/heads/refs/heads/qualified two
>>  '
>>  
>> +test_expect_success 'force-copy a branch to itself via @{-1} is no-op' '
>> +	git branch -t copiable main &&
>> +	git checkout copiable &&
>> +	git checkout - &&
>> +	git branch -C @{-1} copiable &&
>> +	git config --get-all branch.copiable.merge >actual &&
>> +	echo refs/heads/main >expect &&
>> +	test_cmp expect actual
>> +'
>> +
> 
> And the test is straightforward and demonstrates the fix. Thanks for the
> well-written patch, this looks good to me! 
> 

Thank you for reviewing this.

Re: [PATCH 2/2] branch: clear target branch configuration before copying or renaming

[Rubén Justo]

On 17-nov-2022 18:10:52, Victoria Dye wrote:
> Rubén Justo wrote:
> > There are two problems with -m (rename) and -c (copy) branch operations.
> > 
> >  1. If we force-rename or force-copy a branch to overwrite another
> >  branch that already has configuration, the resultant branch ends up
> >  with the source configuration (if any) mixed with the configuration for
> >  the overwritten branch.
> > 
> > 	$ git branch upstream
> > 	$ git branch -t foo upstream  # foo has tracking configuration
> > 	$ git branch bar              # bar has not
> > 	$ git branch -M bar foo       # force-rename bar to foo
> > 	$ git config branch.foo.merge # must return clear
> > 	refs/heads/upstream
> 
> What happens if 'bar' has tracking info? You mentioned that the
> configuration is "mixed" - does that mean 'foo' would have both the original
> 'foo's tracking info *and* 'bar's? 

Of course :-).  My reasoning here is considering 'no tracking' as
tracking information, hence the 'if any'.  I think that the unexpected
functioning this patch is trying to resolve is more obvious having an
unexpected tracking information if we rename a branch /with 'no
tracking'/, overwriting (-M) a branch that already has tracking
information.

> I wasn't sure whether "transfer the source's tracking information to the
> destination" was the desired behavior, but it looks like it is (per
> 52d59cc6452 (branch: add a --copy (-c) option to go with --move (-m),
> 2017-06-18)). Given that, I agree this is the right fix for the issue you've
> identified.

Yes, a reference to that commit is a good information to have in the
message.  But I prefer to not refer to it as I don't think that commit
is responsible or explains this unexpected result, though I cc'ed Ævar
;-)

The design decisions in branch.c and config.c have brought us to this
unexpected result, which just need to be addressed. IMHO

> >  
> > +test_expect_success 'git branch -M inherites clean tracking setup' '
> 
> s/inherites/inherits

Oops, thanks.

> > +	test_when_finished git branch -D moved &&
> > +	git branch -t main-tracked main &&
> > +	git branch non-tracked &&
> > +	git branch -M main-tracked moved &&
> > +	git branch --unset-upstream moved &&
> > +	git branch -M non-tracked moved &&
> > +	test_must_fail git branch --unset-upstream moved
> 
> If I'm reading this correctly, the test doesn't actually demonstrate that
> 'git branch -M' cleans up the tracking info, since 'moved' never has any
> tracking info immediately before 'git branch -M'. The test could also be
> more precise by verifying the upstream name matches. What about something
> like this?
> 
> 	test_when_finished git branch -D moved &&
> 	git branch -t main-tracked main &&
> 	git branch non-tracked &&
> 
> 	# `moved` doesn't exist, so it starts with no tracking info
> 	echo main >expect &&
> 	git branch -M main-tracked moved &&
> 	git rev-parse --abbrev-ref moved@{upstream} >actual &&
> 	test_cmp expect actual &&
> 
> 	# At this point, `moved` is tracking `main`
> 	git branch -M non-tracked moved &&
> 	test_must_fail git rev-parse --abbrev-ref moved@{upstream}

You are right, good eye.  Thanks.  That first '--unset-upstream'
eliminates the possible undesired inherited tracking info.  Removing it
is needed to make the test meaningful.  'rev-parse' is a good change,
but as the test is not testing that '-M' works as expected but doesn't
work in the unexpected way the message describes, I don't think we need
it here, imho.

> s/inherites/inherits (same typo as before, just pointing it out so it's
> easier to find)

Oops, thanks again.

> > +	test_when_finished git branch -D copiable copied &&
> > +	git branch -t copiable main &&
> > +	git branch -C copiable copied &&
> > +	git branch --unset-upstream copied &&
> > +	git branch -C copied copiable &&
> > +	test_must_fail git branch --unset-upstream copiable
> 
> This doesn't have the same issue as the other test (since 'copied' has no
> tracking but 'copiable' does before both 'git branch -C' calls), but it
> would still be nice to use the 'git rev-parse --abbrev-ref
> <branch>@{upstream}' for more precision here. 

I still prefer the test_must_fail for '--unset-upstream', for the same
reasons that in the previous hunk.  I also think it improves
t3200-branch.sh.

Something like...

--- >8 ---

Thank you!

 t/t3200-branch.sh | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index c3b3d11c28..ba959a82de 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -218,17 +218,16 @@ test_expect_success 'git branch -M should leave orphaned HEAD alone' '
 	)
 '
 
-test_expect_success 'git branch -M inherites clean tracking setup' '
+test_expect_success 'git branch -M inherits clean tracking setup' '
 	test_when_finished git branch -D moved &&
 	git branch -t main-tracked main &&
 	git branch non-tracked &&
-	git branch -M main-tracked moved &&
 	git branch --unset-upstream moved &&
 	git branch -M non-tracked moved &&
 	test_must_fail git branch --unset-upstream moved
 '
 
-test_expect_success 'git branch -C inherites clean tracking setup' '
+test_expect_success 'git branch -C inherits clean tracking setup' '
 	test_when_finished git branch -D copiable copied &&
 	git branch -t copiable main &&
 	git branch -C copiable copied &&

Re: [PATCH 1/2] branch: force-copy a branch to itself via @{-1} is a no-op

[Rubén Justo]

On 18-nov-2022 04:58:54, Ævar Arnfjörð Bjarmason wrote:
> > @@ -584,13 +584,13 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
> >  	strbuf_release(&logmsg);
> >  
> >  	strbuf_addf(&oldsection, "branch.%s", interpreted_oldname);
> > -	strbuf_release(&oldref);
> >  	strbuf_addf(&newsection, "branch.%s", interpreted_newname);
> > -	strbuf_release(&newref);
> >  	if (!copy && git_config_rename_section(oldsection.buf, newsection.buf) < 0)
> >  		die(_("Branch is renamed, but update of config-file failed"));
> > -	if (copy && strcmp(oldname, newname) && git_config_copy_section(oldsection.buf, newsection.buf) < 0)
> > +	if (copy && strcmp(interpreted_oldname, interpreted_newname) && git_config_copy_section(oldsection.buf, newsection.buf) < 0)
> 
> We try to stay under 79 chars, see CodingGuidelines. The pre-image was
> already violating this, but the new one is really long. I think it would
> be good to just wrap this after the last && while at it.

Yeah, thought about that.  I preferred to not doing it mainly because my
plan is to move out that strcmp() from there, but also wrapping that
line induces to wrapping the previous one too, and there are many lines
in that file above 79... I already have a series[1] to follow the
CodingGuideLines in branch.c, currently focused in error messages but,
maybe this change makes more sense there.  Dunno.

> 
> >  		die(_("Branch is copied, but update of config-file failed"));
> > +	strbuf_release(&oldref);
> > +	strbuf_release(&newref);
> >  	strbuf_release(&oldsection);
> >  	strbuf_release(&newsection);
> 
> This moving around of destructors isn't needed, and is just some
> unrelated cleanup. Your change here only needs to be:
>
> 	-       if (copy && strcmp(oldname, newname) && git_config_copy_section(oldsection.buf, newsection.buf) < 0)
> 	+       if (copy && strcmp(interpreted_oldname, interpreted_newname) &&
> 	+           git_config_copy_section(oldsection.buf, newsection.buf) < 0)

'interpreted_oldname' is a pointer to the 'oldref' buffer, and it is
used in the next comparison, so the release for 'oldref' needs to be
done later (same for interpreted_newname and newref).

Maybe you are thinking in another change... I also thought comparing
'{old,new}section.buf, the section names in the configuration, but I
preferred to use interpreted_{old,new}name.  It looks more clear what we
are doing and in future commits that section names might not be composed
at that point yet.

Re: [PATCH 2/2] branch: clear target branch configuration before copying or renaming

[Rubén Justo]

On 18-nov-2022 19:12:10, Ævar Arnfjörð Bjarmason wrote:
> 
> On Fri, Nov 18 2022, Victoria Dye wrote:
> 
> > Ævar Arnfjörð Bjarmason wrote:
> >> 
> >> On Thu, Nov 17 2022, Rubén Justo wrote:
> >>> Whenever we copy or move (forced or not) we must make sure that there is
> >>> no residual configuration that will be, probably erroneously, inherited
> >>> by the new branch.  To avoid confusions, clear any branch configuration
> >>> before setting the configuration from the copied or moved branch.
> >> 
> >> So, whatever tea leaves we read into the history, or whether this was a
> >> good or bad design in the first place, I think we should probably lean
> >> towards not having this be a bug fix, but a new feature. Both modes are
> >> clearly easy to support.
> >> 
> >> And then document it in terms of some new switch being the equivalent to
> >> --remove-section followed by a rename, the existing thing a rename etc.

Mmm... the way I see this is as an unexpected _and unintentional_
result, not sure if I am happy describing this is as a bug.

Maybe we can see this as "completing the functionality of moving and
copying a branch", a missing "NEEDSWORK".  Though I think the message
describing this patch as a problem is sensible because of the
motivations and for future reference.

I might agree with a new option like "--mix-configuration" to add this
"feature" (and document it).  But I don't find sensible to consolidate
this unexpected and unintentional result, imho, and making the
expectable an option.

> But my spidey sense is a bit tickled by the proposed change discussing
> the existing behavior as a pure bug, when it seems to me that it's
> approximately what you'd get if it was implemented in terms of "config
> --rename-section", v.s. the proposed new behavior of (as I understand
> it) a "config --remove-section" followed by "config --rename-section".

Explaining this as something you'd get modifying directly the
configuration, might not be a good idea.  A user can break the config,
but "git branch" should not.

> I think a conservative approach would be to focus narrowly on the
> tracking config, if that's the thing that's at issue. I.e. we read that
> ourselves, know that it's broken if it's transmuted in certain ways etc.
> 
> But the proposed patch suggests that we extend that fix to anything we
> might find in that config namespace.

Yes, as the expectation (again, imho) is that the resultant branch gets
its original configuration not a mix of the original and the remanent.

> >>> @@ -583,12 +583,17 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
> >>>  
> >>>  	strbuf_release(&logmsg);
> >>>  
> >>> -	strbuf_addf(&oldsection, "branch.%s", interpreted_oldname);
> >>> -	strbuf_addf(&newsection, "branch.%s", interpreted_newname);
> >>> -	if (!copy && git_config_rename_section(oldsection.buf, newsection.buf) < 0)
> >>> -		die(_("Branch is renamed, but update of config-file failed"));
> >>> -	if (copy && strcmp(interpreted_oldname, interpreted_newname) && git_config_copy_section(oldsection.buf, newsection.buf) < 0)
> >>> -		die(_("Branch is copied, but update of config-file failed"));
> >>> +	if (strcmp(interpreted_oldname, interpreted_newname)) {
> >>> +		strbuf_addf(&oldsection, "branch.%s", interpreted_oldname);
> >>> +		strbuf_addf(&newsection, "branch.%s", interpreted_newname);
> >>> +
> >>> +		delete_branch_config(interpreted_newname);
> >>> +
> >>> +		if (!copy && git_config_rename_section(oldsection.buf, newsection.buf) < 0)
> >>> +			die(_("Branch is renamed, but update of config-file failed"));
> >>> +		if (copy && git_config_copy_section(oldsection.buf, newsection.buf) < 0)
> >>> +			die(_("Branch is copied, but update of config-file failed"));
> >> 
> >> Aside from any question of a hypothetical "should", your implementation
> >> is running head-first into a major caveat in our config API.
> >> 
> >> Which is that we don't have transactions or rollbacks, and we don't even
> >> carry a lock forward for all of these.
> >> 
> >> So, there's crappy edge cases in the old implementation as you've found,
> >> but at least it mostly failed-safe.
> >> 
> >> But here we'll delete_branch_config(), then release the lock, and then
> >> try to rename the new branch to that location, which might fail.
> >> 
> >> So, we'll be left with no config for the thing we tried to clobber, nor
> >> the new config.
> >
> > This is a good point, so to add to it: I think a fairly unobtrusive solution
> > would be to move the config deletion after the rename is 100% complete.

The update of the ref is already done when we operate on the
configuration.  I think that _even_ if the
git_config_{rename/copy}_section fails, the delete_branch_config() is
sensible.

> Also, whatever this is supposed to do, shouldn't we make it consistent
> with other "git branch" modes. E.g.:
> 
> 	git branch -D foo
> 	git -P config --show-origin --get-regexp '^branch\.foo\.'
> 	git config branch.foo.rebase true
> 	git branch foo -t origin/seen
> 	git -P config --show-origin --get-regexp '^branch\.foo\.'
> 
> Will emit:
> 	
> 	Deleted branch foo (was ...).
> 	branch 'foo' set up to track 'origin/seen'.
> 	file:.git/config        branch.foo.rebase true
> 	file:.git/config        branch.foo.remote origin
> 	file:.git/config        branch.foo.merge refs/heads/seen
 
I'm not sure.  I did notice that, and I can think of ill but legit uses
there: a user might be setting the configuration and then creating the
reference.  It is tangential here, but we can issue a warning like:

	$ git branch -t foo
	$ git branch -t bar
	$ git branch -C foo bar
	$ git branch --set-upstream-to foo bar
	warning: branch.bar.remote has multiple values
	warning: branch.bar.merge has multiple values
	branch 'bar' set up to track 'foo'.

I have different unsent patches to address this too, but I'm not sure
what we want there, and how.

> The upthread commit message doesn't discuss it, but even multiple
> "merge" values are a valid state of affairs. E.g.:
> 	
> 	$ git branch seen -t origin/seen
> 	branch 'seen' set up to track 'origin/seen'.
> 	$ git branch next -t origin/next
> 	branch 'next' set up to track 'origin/next'.
> 	$ git branch -C seen next
> 	$ git config --get-regexp 'branch\.next'
> 	branch.next.remote origin
> 	branch.next.merge refs/heads/seen
> 	branch.next.remote origin
> 	branch.next.merge refs/heads/next
> 
> Now, if you do:
> 
> 	git checkout next
> 
> and:
> 	git pull --no-rebase
> 
> You'll get e.g.:
> 
>        Merge branches 'next' and 'seen' of github.com:git/git into next

I think this is unexpected and unintentional.

> And again, don't take any of that as an a-priori argument against
> changing it, I'm still undecided. I'd just prefer that we know what it
> is we're changing.

Thank you for your analysis.

Re: [PATCH 2/2] branch: clear target branch configuration before copying or renaming

[Victoria Dye]

Rubén Justo wrote:
> On 17-nov-2022 18:10:52, Victoria Dye wrote:
>> Rubén Justo wrote:
>> I wasn't sure whether "transfer the source's tracking information to the
>> destination" was the desired behavior, but it looks like it is (per
>> 52d59cc6452 (branch: add a --copy (-c) option to go with --move (-m),
>> 2017-06-18)). Given that, I agree this is the right fix for the issue you've
>> identified.
> 
> Yes, a reference to that commit is a good information to have in the
> message.  But I prefer to not refer to it as I don't think that commit
> is responsible or explains this unexpected result, though I cc'ed Ævar
> ;-)

It does allude the _expected_ result, though ("[-C] uses the same underlying
machinery as the --move (-m) option except the reflog and configuration is
copied instead of being moved").

> 
> The design decisions in branch.c and config.c have brought us to this
> unexpected result, which just need to be addressed. IMHO

It's helpful to reviewers and future readers to include relevant context in
a commit message; a commit doesn't need to be responsible for a bug to help
someone understand what you're trying to do and why. In this case, I needed
to search through the commit history myself to gather that information (that
is, how you decided clearing the destination first was the "correct"
approach rather than, say, preserving the destination branch's config and
not copying the source's), so I would consider the explanation in the
current commit message incomplete. 

In general, it's often not enough to "just fix a bug" without elaborating on
why something *is* a bug. This isn't an obvious thing like a 'BUG()' or
segfault, so context like 52d59cc6452 is needed to convey the nuance of the
issue.

>>> +	test_when_finished git branch -D moved &&
>>> +	git branch -t main-tracked main &&
>>> +	git branch non-tracked &&
>>> +	git branch -M main-tracked moved &&
>>> +	git branch --unset-upstream moved &&
>>> +	git branch -M non-tracked moved &&
>>> +	test_must_fail git branch --unset-upstream moved
>>
>> If I'm reading this correctly, the test doesn't actually demonstrate that
>> 'git branch -M' cleans up the tracking info, since 'moved' never has any
>> tracking info immediately before 'git branch -M'. The test could also be
>> more precise by verifying the upstream name matches. What about something
>> like this?
>>
>> 	test_when_finished git branch -D moved &&
>> 	git branch -t main-tracked main &&
>> 	git branch non-tracked &&
>>
>> 	# `moved` doesn't exist, so it starts with no tracking info
>> 	echo main >expect &&
>> 	git branch -M main-tracked moved &&
>> 	git rev-parse --abbrev-ref moved@{upstream} >actual &&
>> 	test_cmp expect actual &&
>>
>> 	# At this point, `moved` is tracking `main`
>> 	git branch -M non-tracked moved &&
>> 	test_must_fail git rev-parse --abbrev-ref moved@{upstream}
> 
> You are right, good eye.  Thanks.  That first '--unset-upstream'
> eliminates the possible undesired inherited tracking info.  Removing it
> is needed to make the test meaningful.  'rev-parse' is a good change,
> but as the test is not testing that '-M' works as expected but doesn't
> work in the unexpected way the message describes, I don't think we need
> it here, imho.

But by always having the destination branch have no tracking info, this test
doesn't verify that the unexpected behavior (that is, "mixing" the source
and destination config) has been fixed. You still need a case where the
destination config is non-empty and the source is empty (or some other
non-empty value) to reproduce the issue.

As for the 'rev-parse' vs. '--unset-upstream': making the test more precise
here doesn't increase its scope *and* makes the overall test suite more
effective at detecting regressions. And, a read-only check like 'rev-parse'
is more readable for other developers (especially if they need to debug the
test in the future), rather than needing to understand that
'--unset-upstream' is doing two things: throwing an error depending on the
presence of an upstream *and* removing the upstream from the target branch). 

In other words, it helps to separate your assertions from your setup steps.
If you still need to '--unset-upstream' for the rest of the test, you can do
the 'rev-parse' then '--unset-upstream' as two separate steps. 

> --- >8 ---
> 
> Thank you!
> 
>  t/t3200-branch.sh | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
> index c3b3d11c28..ba959a82de 100755
> --- a/t/t3200-branch.sh
> +++ b/t/t3200-branch.sh
> @@ -218,17 +218,16 @@ test_expect_success 'git branch -M should leave orphaned HEAD alone' '
>  	)
>  '
>  
> -test_expect_success 'git branch -M inherites clean tracking setup' '
> +test_expect_success 'git branch -M inherits clean tracking setup' '
>  	test_when_finished git branch -D moved &&
>  	git branch -t main-tracked main &&
>  	git branch non-tracked &&
> -	git branch -M main-tracked moved &&
>  	git branch --unset-upstream moved &&
>  	git branch -M non-tracked moved &&
>  	test_must_fail git branch --unset-upstream moved

This change makes the test less comprehensive (by removing the
"tracked-overwrites-untracked" case) and does not solve the issue of 'moved'
always having no tracking info before the 'git branch -M' (therefore not
properly reproducing the error case fixed in this patch).

>  '
>  
> -test_expect_success 'git branch -C inherites clean tracking setup' '
> +test_expect_success 'git branch -C inherits clean tracking setup' '
>  	test_when_finished git branch -D copiable copied &&
>  	git branch -t copiable main &&
>  	git branch -C copiable copied &&
> 

Re: [PATCH 2/2] branch: clear target branch configuration before copying or renaming

[Rubén Justo]

On 20/11/22 23:10, Victoria Dye wrote:
 
> It does allude the _expected_ result, though ("[-C] uses the same underlying
> machinery as the --move (-m) option except the reflog and configuration is
> copied instead of being moved").
> 
>>
>> The design decisions in branch.c and config.c have brought us to this
>> unexpected result, which just need to be addressed. IMHO
> 
> It's helpful to reviewers and future readers to include relevant context in
> a commit message; a commit doesn't need to be responsible for a bug to help
> someone understand what you're trying to do and why. In this case, I needed
> to search through the commit history myself to gather that information (that
> is, how you decided clearing the destination first was the "correct"
> approach rather than, say, preserving the destination branch's config and
> not copying the source's), so I would consider the explanation in the
> current commit message incomplete. 
> 
> In general, it's often not enough to "just fix a bug" without elaborating on
> why something *is* a bug. This isn't an obvious thing like a 'BUG()' or
> segfault, so context like 52d59cc6452 is needed to convey the nuance of the
> issue.

OK. Clearly the message it's wrong.
 
>>>> +	test_when_finished git branch -D moved &&
>>>> +	git branch -t main-tracked main &&
>>>> +	git branch non-tracked &&
>>>> +	git branch -M main-tracked moved &&
>>>> +	git branch --unset-upstream moved &&
>>>> +	git branch -M non-tracked moved &&
>>>> +	test_must_fail git branch --unset-upstream moved
>>>
>>> If I'm reading this correctly, the test doesn't actually demonstrate that
>>> 'git branch -M' cleans up the tracking info, since 'moved' never has any
>>> tracking info immediately before 'git branch -M'. The test could also be
>>> more precise by verifying the upstream name matches. What about something
>>> like this?
>>>
>>> 	test_when_finished git branch -D moved &&
>>> 	git branch -t main-tracked main &&
>>> 	git branch non-tracked &&
>>>
>>> 	# `moved` doesn't exist, so it starts with no tracking info
>>> 	echo main >expect &&
>>> 	git branch -M main-tracked moved &&
>>> 	git rev-parse --abbrev-ref moved@{upstream} >actual &&
>>> 	test_cmp expect actual &&
>>>
>>> 	# At this point, `moved` is tracking `main`
>>> 	git branch -M non-tracked moved &&
>>> 	test_must_fail git rev-parse --abbrev-ref moved@{upstream}
>>
>> You are right, good eye.  Thanks.  That first '--unset-upstream'
>> eliminates the possible undesired inherited tracking info.  Removing it
>> is needed to make the test meaningful.  'rev-parse' is a good change,
>> but as the test is not testing that '-M' works as expected but doesn't
>> work in the unexpected way the message describes, I don't think we need
>> it here, imho.
> 
> But by always having the destination branch have no tracking info, this test
> doesn't verify that the unexpected behavior (that is, "mixing" the source
> and destination config) has been fixed. You still need a case where the
> destination config is non-empty and the source is empty (or some other
> non-empty value) to reproduce the issue.

OK, understood, that needs a test.
 
> As for the 'rev-parse' vs. '--unset-upstream': making the test more precise
> here doesn't increase its scope *and* makes the overall test suite more
> effective at detecting regressions. And, a read-only check like 'rev-parse'
> is more readable for other developers (especially if they need to debug the
> test in the future), rather than needing to understand that

OK, that makes sense.

> '--unset-upstream' is doing two things: throwing an error depending on the
> presence of an upstream *and* removing the upstream from the target branch). 
> 
> In other words, it helps to separate your assertions from your setup steps.
> If you still need to '--unset-upstream' for the rest of the test, you can do
> the 'rev-parse' then '--unset-upstream' as two separate steps. 
> 
>> --- >8 ---
>>
>> Thank you!
>>
>>  t/t3200-branch.sh | 5 ++---
>>  1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
>> index c3b3d11c28..ba959a82de 100755
>> --- a/t/t3200-branch.sh
>> +++ b/t/t3200-branch.sh
>> @@ -218,17 +218,16 @@ test_expect_success 'git branch -M should leave orphaned HEAD alone' '
>>  	)
>>  '
>>  
>> -test_expect_success 'git branch -M inherites clean tracking setup' '
>> +test_expect_success 'git branch -M inherits clean tracking setup' '
>>  	test_when_finished git branch -D moved &&
>>  	git branch -t main-tracked main &&
>>  	git branch non-tracked &&
>> -	git branch -M main-tracked moved &&
>>  	git branch --unset-upstream moved &&
>>  	git branch -M non-tracked moved &&
>>  	test_must_fail git branch --unset-upstream moved
> 
> This change makes the test less comprehensive (by removing the
> "tracked-overwrites-untracked" case) and does not solve the issue of 'moved'
> always having no tracking info before the 'git branch -M' (therefore not
> properly reproducing the error case fixed in this patch).

Sorry, I sent the wrong patch.  Removing the line 'git branch
--unset-upstream moved' is what makes the test meaningful.

>>  '
>>  
>> -test_expect_success 'git branch -C inherites clean tracking setup' '
>> +test_expect_success 'git branch -C inherits clean tracking setup' '
>>  	test_when_finished git branch -D copiable copied &&
>>  	git branch -t copiable main &&
>>  	git branch -C copiable copied &&
>>
> 

---

This needs more work and consensus.  I think we can reject this patch,
2/2, and let the other, 1/2, settle.

I'll try to address this again considering what we've discussed here,
improving the message with context as you suggested and trying to
use more a sense of completion than a fix or bug.  Maybe also covering
other options in branch that might be affected for this configuration
thing.

Thank you.