Re: [PATCH v4 1/3] ls-refs: report unborn targets of symrefs

On Tue, Jan 26, 2021 at 10:13:58AM -0800, Jonathan Tan wrote:

(It was a bit confusing that refs_resolve_ref_unsafe() returns one of
its input arguments if it succeeds and NULL if it fails, but that's
outside the scope of this patch, I think.)

Yep. It would probably be much nicer for it to return a numeric success
code, and to take an optional strbuf into which to write the resolved
symref name (if the caller even cares about it). But definitely out of
scope for your patch.

quoted

This straight-boolean version works as long as you can atomically update
the _config_ on each version. But that seems like roughly the same
problem (having dealt with this on GitHub servers, they are not
equivalent, and depending on your infrastructure, it definitely _can_ be
easier to do one versus the other. But it seems like a funny place to
leave this upstream feature).

Well, I was just agreeing with what you said [1]. :-)

[1] https://lore.kernel.org/git/X9xJLWdFJfNJTn0p@coredump.intra.peff.net/ (local)

Oh, I just need to you to agree harder then. ;)

If we are not going to support config that helps you do an atomic
deploy, then I don't really see the point of having config at all.
Here are three plausible implementations I can conceive of:

  - allowUnborn is a tri-state for "accept-but-do-not-advertise",
    "accept-and-advertise", and "disallow". This helps with rollout in a
    cluster by setting it to the accept-but-do-not-advertise.  The
    default would be accept-and-advertise, which is what most servers
    would want. I don't really know why anyone would want "disallow".

  - allowUnborn is a bool for "accept-and-advertise" or "disallow". This
    doesn't help cluster rollout. I don't know why anyone would want to
    switch away from the default of accept-and-advertise.

  - allowUnborn is always on.

The first one helps the cluster case, at the cost of introducing an
extra config knob. The third one doesn't help that case, but is one less
knob for server admins to think about. But the second one has a knob
that I don't understand why anybody would tweak. It seems like the worst
of both.

Perhaps there's a reason for setting "disallow" that I don't know. Or
perhaps you're happy to help the cluster case using a simple bool with
atomic config rollouts (which are outside the scope of Git itself).

quoted

Or is the intent that an unconfigured reader would silently ignore the
unborn flag in that case? That would at least not cause it to bail on
the client in a mixed-version environment. But it does feel like a
confusing result.

Right now, an old server would ignore "unborn", yes. I'm not sure of
what the intent should be - tightening ls-refs and fetch to forbid
unknown arguments seems like a good idea to me.

If we had a just a bool (case 2 from above), and there was an
always-implied "accept unborn even if not advertised", then that _does_
let the config help out the cluster case (it just turns off
advertisements, basically making the bool "accept-but-do-not-advertise"
versus "disallow").

I don't love it. The protocol spec does say "don't ask for capability
foo if the server didn't say it knows about foo". We'd be loosening the
enforcement of that (if only for capabilities we _do_ in fact know
about), even though we don't know if it was due to a race, or if the
client is just misbehaving. But I wondered if that was the direction you
were going to try to solve your cluster-rollout problem.

-Peff