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