From: "Rubén Justo" <rjusto@gmail.com>
To: Victoria Dye <vdye@github.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: Sun, 20 Nov 2022 10:20:31 +0100 [thread overview]
Message-ID: <042c18df-deb6-6214-2d49-c214a872e1c1@gmail.com> (raw)
In-Reply-To: <8457ad4c-51c7-4c2d-8dbf-02a60045d288@github.com>
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 &&
next prev parent reply other threads:[~2022-11-20 9:20 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
2022-11-20 9:20 ` Rubén Justo [this message]
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=042c18df-deb6-6214-2d49-c214a872e1c1@gmail.com \
--to=rjusto@gmail.com \
--cc=avarab@gmail.com \
--cc=git@vger.kernel.org \
--cc=me@ttaylorr.com \
--cc=vdye@github.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).