From: Victoria Dye <vdye@github.com>
To: "Rubén Justo" <rjusto@gmail.com>, "Git List" <git@vger.kernel.org>
Cc: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
"Taylor Blau" <me@ttaylorr.com>
Subject: Re: [PATCH 2/2] branch: clear target branch configuration before copying or renaming
Date: Thu, 17 Nov 2022 18:10:52 -0800 [thread overview]
Message-ID: <8457ad4c-51c7-4c2d-8dbf-02a60045d288@github.com> (raw)
In-Reply-To: <762c1e8f-fd0c-3b4b-94a0-709d8c9431e4@gmail.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 &&
next prev parent reply other threads:[~2022-11-18 2:11 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-11-17 1:33 [PATCH 0/2] branch: fix some malfunctions in -m/-c Rubén Justo
2022-11-17 1:36 ` [PATCH 1/2] branch: force-copy a branch to itself via @{-1} is a no-op Rubén Justo
2022-11-17 22:18 ` Victoria Dye
2022-11-20 8:10 ` Rubén Justo
2022-11-18 3:58 ` Ævar Arnfjörð Bjarmason
2022-11-20 9:34 ` Rubén Justo
2022-11-17 1:44 ` [PATCH 2/2] branch: clear target branch configuration before copying or renaming Rubén Justo
2022-11-18 2:10 ` Victoria Dye [this message]
2022-11-20 9:20 ` Rubén Justo
2022-11-20 22:10 ` Victoria Dye
2022-11-21 23:13 ` Rubén Justo
2022-11-18 4:51 ` Ævar Arnfjörð Bjarmason
2022-11-18 16:36 ` Victoria Dye
2022-11-18 18:12 ` Ævar Arnfjörð Bjarmason
2022-11-20 14:55 ` Rubén Justo
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=8457ad4c-51c7-4c2d-8dbf-02a60045d288@github.com \
--to=vdye@github.com \
--cc=avarab@gmail.com \
--cc=git@vger.kernel.org \
--cc=me@ttaylorr.com \
--cc=rjusto@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).