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

[Victoria Dye <vdye@github.com>]

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