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