Re: [PATCH 2/2] branch: clear target branch configuration before copying or renaming
- From
Rubén Justo <rjusto@gmail.com>
- Date
- Nov 20, 2022, 09:20 UTC
- Message-ID
- <042c18df-deb6-6214-2d49-c214a872e1c1@gmail.com>
- In-Reply-To
- <8457ad4c-51c7-4c2d-8dbf-02a60045d288@github.com>
On 17-nov-2022 18:10:52, Victoria Dye wrote:
Show 18 quoted lines
> 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.
Show 5 quoted lines
> 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.
Show 27 quoted lines
> > + 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.
Show 11 quoted lines
> > + 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 &&