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

[Jeff King <peff@peff.net>]

On Thu, Sep 08, 2022 at 01:48:38PM -0700, Junio C Hamano wrote:

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

Right. When I was writing the patch I had no inkling that branch.*.merge
could ever be anything but a fully qualified ref. I don't think I've
ever seen one that isn't, and the documentation is vague. It says:

  [...]The value is handled like the remote part of a refspec, and must match
  a ref which is fetched from the remote[...]

I took "match" to mean a full string match. That text comes from
b888d61c83 (Make fetch a builtin, 2007-09-10); before that it said "the
value has exactly to match a remote part of one of the refspecs...".

But documentation aside, if we've been allowing:

  git config branch.master.merge master

to work forever, then perhaps we need to continue to support it. I
dunno.

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

I make no claims. ;) I just didn't even consider a non-qualified ref to
be a possibility.

The code in fetch's add_merge_config() that does branch_merge_matches()
comes from 85682c1903 (Correct handling of branch.$name.merge in
builtin-fetch, 2007-09-18), but I don't see any indication there that
non-qualified refs were intended.

So I could either way: non-qualified refs in branch.*.merge has always
worked, and we should continue to support it. Or it was never intended
to work, and we are not obligated to continue supporting random things.

I do think "continue supporting" would probably just mean using
expand_ref_prefix() here as you suggest. It does increase the size of
our request, and the work the server has to do when it matches the
prefixes (which is inherently linear on the number of prefixes we give
it).

One thing we could do, but my patch doesn't, is skip sending this prefix
when it is a subset of the default refspec (i.e., in the default config
refs/heads/foo is already part of refs/heads/, so there is no need to
specify it separately). That doesn't really help if we expand the
prefix, though. The default refspec doesn't include:

  refs/remotes/refs/heads/foo/HEAD

which is exactly the kind of thing we'd ask for. Maybe that is all
premature optimization, though.

-Peff