Re: [PATCH 2/2] branch: clear target branch configuration before copying or renaming

[Victoria Dye <vdye@github.com>]

Rubén Justo wrote:
> On 17-nov-2022 18:10:52, Victoria Dye wrote:
>> Rubén Justo wrote:
>> 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
> ;-)

It does allude the _expected_ result, though ("[-C] uses the same underlying
machinery as the --move (-m) option except the reflog and configuration is
copied instead of being moved").

> 
> The design decisions in branch.c and config.c have brought us to this
> unexpected result, which just need to be addressed. IMHO

It's helpful to reviewers and future readers to include relevant context in
a commit message; a commit doesn't need to be responsible for a bug to help
someone understand what you're trying to do and why. In this case, I needed
to search through the commit history myself to gather that information (that
is, how you decided clearing the destination first was the "correct"
approach rather than, say, preserving the destination branch's config and
not copying the source's), so I would consider the explanation in the
current commit message incomplete. 

In general, it's often not enough to "just fix a bug" without elaborating on
why something *is* a bug. This isn't an obvious thing like a 'BUG()' or
segfault, so context like 52d59cc6452 is needed to convey the nuance of the
issue.

>>> +	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.

But by always having the destination branch have no tracking info, this test
doesn't verify that the unexpected behavior (that is, "mixing" the source
and destination config) has been fixed. You still need a case where the
destination config is non-empty and the source is empty (or some other
non-empty value) to reproduce the issue.

As for the 'rev-parse' vs. '--unset-upstream': making the test more precise
here doesn't increase its scope *and* makes the overall test suite more
effective at detecting regressions. And, a read-only check like 'rev-parse'
is more readable for other developers (especially if they need to debug the
test in the future), rather than needing to understand that
'--unset-upstream' is doing two things: throwing an error depending on the
presence of an upstream *and* removing the upstream from the target branch). 

In other words, it helps to separate your assertions from your setup steps.
If you still need to '--unset-upstream' for the rest of the test, you can do
the 'rev-parse' then '--unset-upstream' as two separate steps. 

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

This change makes the test less comprehensive (by removing the
"tracked-overwrites-untracked" case) and does not solve the issue of 'moved'
always having no tracking info before the 'git branch -M' (therefore not
properly reproducing the error case fixed in this patch).

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