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

From: Jeff King <peff@peff.net>
Date: Dec 18, 2020, 06:16 UTC
Message-ID: <X9xJLWdFJfNJTn0p@coredump.intra.peff.net>
In-Reply-To: <xmqqzh2dz071.fsf@gitster.c.googlers.com>

On Wed, Dec 16, 2020 at 05:32:50PM -0800, Junio C Hamano wrote:

Show 15 quoted lines

> Jonathan Tan <jonathantanmy@google.com> writes:
> 
> > Firstly, this allows a staged rollout in load-balancing situations
> > wherein we turn on "allow" for all servers, then "advertise", so that we
> > don't end up with a client that sees the advertisement but then sends
> > the follow-up request to a server that has not received the latest
> > configuration yet.
> 
> If this were the _first_ capability we are adding to the system, the
> above makes quite a lot of sense, but I do not recall any existing
> capability that can be configured this way.  How would one deploy a
> set of servers that gradually start allowing fetching unadvertised
> but reachable commits, for example?  I am not saying that the "I'll
> accept if asked, but I won't actively advertise" is a bad feature; I
> just find it disturbing that only this knob has that feature.

Yeah, that was my thought. It is a real problem, but not one we've dealt with before in this way (or at all, really). Two recent examples:

  - for fetching, the object-format=sha1 capability is always parroted
    back by v2.28 and higher clients. But there's no config to disable
    the advertisement, so during a partial deployment to a load-balanced
    cluster, a client may say "object-format=sha1" to a server that
    doesn't understand it.

  - for pushing, we added report-status-v2, which is likewise always
    repeated back by any v2.29 and higher client.

At GitHub we recently merged in v2.29, and just hacked report-status-v2 out of the advertisement within the code (leaving it in the "we can read this but won't advertise it state"). Temporarily modifying the code is definitely ugly, and I don't mind a cleaner solution, but:

  - it would be nice if this were done in a more consistent way for all
    new capabilities

  - one nice thing about the code change is that after the rollout is
    done, it's safe to make the code unconditional again, which makes
    it simpler to read/reason about.

    This may be oversimplifying it a bit, of course. On one platform, we
    know when the rollout is happening. But if it's something we ship
    upstream, then "rollout" may be on the jump from v2.28 to v2.29, or
    to v2.30, or v2.31, etc. You can never say "rollouts are done, and
    existing server versions know about this feature". So any upstream
    support like config has to stay forever.

So I dunno. My biggest complaint is that the config option defaults to _off_. So it's helping load-balanced rollouts, but creating complexity for everyone else who might want to enable the feature.

(I know there was also an indication that some people might want it off because they somehow want to have no HEAD at all. I don't find this particularly compelling, but even if it were, I think we could leave it the config as an escape hatch for such folks, but still default it to "on").

-Peff