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

Jonathan Tan [off-list ref] writes:

quoted

I'm not sure what the ideal endgame state is, but I could see how
sending all symlinks would be useful (e.g. if a client wanted to mirror
another repo with more fidelity). Right now I don't plan on adding
support for dangling symrefs other than HEAD, though. Maybe I'll update
it to something like:

  If HEAD is a symref pointing to an unborn branch, the server may send
  it in the form "unborn HEAD symref-target:<target>". In the future,
  this may be extended to other symrefs as well.

Unless you plan to add support for symbolic refs that are not HEAD
in immediate future, "In the future, ..." is not even necessary to
say.  The users cannot expect to exploit the missing feature anyway,
and they cannot even plan to use it in near future.

I've been disturbed by the phrase "the server may send it" quite a
lot, actually.  In the original before the rewrite above, it was a
good cop-out excuse "no, we do not send symbolic refs other than
HEAD because we only say 'the server may' and do not promise
anything beyond that".  But now we are tightening the description
to HEAD that we do intend to support well, it probably is a good
idea to give users a promise a bit firmer than that.

    unborn If HEAD is a symref pointing to an unborn branch <b>, the
    server reports it as "unborn HEAD symref-target:refs/heads/<b>"
    in its response.

It would make it clear that by sending 'unborn' in the request, the
client is not just allowing the server to include the unborn
information in the response.  It is asking the server, that has
advertised that it is capable to do so, to exercise the feature.

That makes sense. OK, I'll make the promise firmer.

quoted

I think that there is a discussion point to be decided
(advertise/allow/ignore vs allow/ignore), so I'll wait for that before
sending v7.

What is the downside of having three choices (which allows phased
deployment, where everybody starts as capable of responding without
advertising in the first phase, and once everybody becomes capable
of responding, they start advertising) and the reason we might
prefer just allow/ignore instead?  Too much complexity?  It does not
help the real deployment as much in practice as it seems on paper?

I am not advocating three-choice option; I am neutral, but do not
see a good reason to reject it (while I can easily see a reason to
reject the other one).

Here's a reason from Peff's email [1] against advertise/allow/ignore (the "code
change" is a temporary hack that teaches Git to accept but not advertise
report-status-v2). Granted, he does say that this may be an oversimplification,
and in the overall email, he was arguing more for having this feature on by
default (whether we have advertise/allow/ignore, allow/ignore, or no config at
all) rather than for any specific configuration scheme.

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

To balance that out, from the same email [1], a slight argument against no
config at all:

  (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").

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

And an argument against the allow/ignore [2]:

  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.

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