Re: [PATCH 2/2] fetch: add branch.*.merge to default ref-prefix extension

[Junio C Hamano <gitster@pobox.com>]

Junio C Hamano <gitster@pobox.com> writes:

> So, is strvec_push() a correct thing to use here?  ref_prefixes will
> receive something like 'master' here, without 'refs/heads/master'
> getting pushed, when "branch.*.merge = master"?  Given that the
> advertisement restriction is merely an optimization, I wouldn't be
> surprised if 'master' in .ref_prefixes strvec is further expanded
> by an unnecessary extra call to expand_ref_prefix() later to cause
> the server side to advertise refs/heads/master and refs/tags/master
> etc., but it smells, eh, bad.
>
>>  	if (tags == TAGS_SET || tags == TAGS_DEFAULT) {
>>  		must_list_refs = 1;
>> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
>> index 081808009b..0b72112fb1 100755
>> --- a/t/t5520-pull.sh
>> +++ b/t/t5520-pull.sh
>> @@ -218,6 +218,23 @@ test_expect_success 'fail if upstream branch does not exist' '
>>  	test_cmp expect file
>>  '
>>  
>> +test_expect_success 'fetch upstream branch even if refspec excludes it' '
>> +	# the branch names are not important here except that
>> +	# the first one must not be a prefix of the second,
>> +	# since otherwise the ref-prefix protocol extension
>> +	# would match both
>> +	git branch in-refspec HEAD^ &&
>> +	git branch not-in-refspec HEAD &&
>> +	git init -b in-refspec downstream &&
>> +	git -C downstream remote add -t in-refspec origin "file://$(pwd)/.git" &&
>> +	git -C downstream config branch.in-refspec.remote origin &&
>> +	git -C downstream config branch.in-refspec.merge refs/heads/not-in-refspec &&

Ah, OK, so the breakage may be the other way around.

The new code assumes that branch.<name>.merge is a full refname, and
strvec_push() is the right thing to do, when we add the knowledge
that the current branch we are on by default merges with their
refs/heads/frotz.  We just ask them to advertise refs/heads/frotz
and they do not need to advertise refs/tags/frotz etc. let alone
refs/tags/refs/heads/frotz so using expand_ref_prefix() here is
wrong.

It means that the patch claims that remote.c::branch_merge_matches()
assume that branch->merge[i]->src may not be a full refname by
calling refname_match() on it, which is incorrect and may need to be
corrected.  But that is totally outside the scope of this fix.

Thanks.