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

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

Jeff King <peff@peff.net> writes:

> When running "git pull" with no arguments, we'll do a default "git
> fetch" and then try to merge the branch specified by the branch.*.merge
> config. There's code in get_ref_map() to treat that "merge" branch as
> something we want to fetch, even if it is not otherwise covered by the
> default refspec.
>
> This works fine with the v0 protocol, as the server tells us about all
> of the refs, and get_ref_map() is the ultimate decider of what we fetch.

Correct.

> But in the v2 protocol, we send the ref-prefix extension to the server,
> asking it to limit the ref advertisement. And we only tell it about the
> default refspec for the remote; we don't mention the branch.*.merge
> config at all.

Yikes.  But unfortunately it is not at all surprising that v2 is
still buggy like this.  It tries to do things in much fancier way to
"optimize" and this is an example of such.

> This usually doesn't matter, because the default refspec matches
> "refs/heads/*", which covers all branches. But if you explicitly use a
> narrow refspec, then "git pull" on some branches may fail. The server
> doesn't advertise the branch, so we don't fetch it, and "git pull"
> thinks that it went away upstream.

Nicely analysed.

> We can fix this by including any branch.*.merge entries for the current
> branch in the list of ref-prefixes we pass to the server. This only
> needs to happen when using the default configured refspec (since
> command-line refspecs are already added, and take precedence in deciding
> what we fetch). We don't otherwise need to replicate any of the "what to
> fetch" logic in get_ref_map(). These ref-prefixes are an optimization,
> so it's OK if we tell the server to advertise the branch.*.merge ref,
> even if we're not going to pull it. We'll just choose not to fetch it.

The solution does make sense.

> -	} else if (transport->remote->fetch.nr)
> -		refspec_ref_prefixes(&transport->remote->fetch,
> -				     &transport_ls_refs_options.ref_prefixes);
> +	} else {
> +		struct branch *branch = branch_get(NULL);
> +
> +		if (transport->remote->fetch.nr)
> +			refspec_ref_prefixes(&transport->remote->fetch,
> +					     &transport_ls_refs_options.ref_prefixes);

OK, this is what we add from remote.$there.fetch, just as before.

> +		if (branch_has_merge_config(branch) &&
> +		    !strcmp(branch->remote_name, transport->remote->name)) {
> +			int i;
> +			for (i = 0; i < branch->merge_nr; i++) {
> +				strvec_push(&transport_ls_refs_options.ref_prefixes,
> +					    branch->merge[i]->src);
> +			}
> +		}
> +	}

I am surprised strvec_push() is used here, not expand_ref_prefix().

refspec_ref_prefixes() takes refspec, inspects each item in it, and
munges the source side (i.e. the name the server side calls it) to
chomp at the first '*' and strvec_push the result for a pattern refspec,
or calls expand_ref_prefix(), to prefix all the possible rev-parse
dwim prefixes to given string.  So "remote.origin.fetch = a:something"
is turned into "a", which is not a pattern, and refs/a, refs/tags/a,
refs/heads/a, refs/remotes/a, and refs/remotes/a/HEAD are asked to
be advertised.

Here, branch->merge[i]->src is branch.<name>.merge for the branch
currently checked out, which is?  'master'?  'refs/heads/master'?

remote.c::branch_merge_matches() uses refname_match() on its value,
so it seems that we expect the branch name proper, without refs/heads
prefix, as its value.

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



> +	git -C downstream pull &&
> +	git rev-parse --verify not-in-refspec >expect &&
> +	git -C downstream rev-parse --verify HEAD >actual &&
> +	test_cmp expect actual
> +'
> +
>  test_expect_success 'fail if the index has unresolved entries' '
>  	git checkout -b third second^ &&
>  	test_when_finished "git checkout -f copy && git branch -D third" &&