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

From: Jeff King <peff@peff.net>
Date: Sep 9, 2022, 02:17 UTC
Message-ID: <YxqiJW4cSp4cBR22@coredump.intra.peff.net>
In-Reply-To: <xmqqtu5hoamx.fsf@gitster.g>

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

Show 7 quoted lines

> 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