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

From: Jonathan Tan <jonathantanmy@google.com>
Date: Dec 16, 2020, 23:54 UTC
Message-ID: <20201216235443.1674103-1-jonathantanmy@google.com>
In-Reply-To: <X9pQjHrRQQqB6AFI@coredump.intra.peff.net>

Show 16 quoted lines

> On Tue, Dec 15, 2020 at 06:07:56PM -0800, Jonathan Tan wrote:
> 
> > +static int ls_refs_config(const char *var, const char *value, void *cb_data)
> > +{
> > +	struct ls_refs_data *data = cb_data;
> > +
> > +	if (!strcmp("lsrefs.unborn", var))
> > +		data->allow_unborn = !strcmp(value, "allow") ||
> > +			!strcmp(value, "advertise");
> 
> What's the reason we would want this to be configurable? I would think
> we would just want it always on for the server, and then clients can
> choose to make us of it or not (and probably not by omitting the
> capability; the question is what they want to do with the information
> about HEAD, but that is true whether it is unborn or not, and is
> controlled by options like "clone -b").

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.

Secondly, I wonder if some people purposely set HEAD to an unborn branch just so that the repository would be presented as not having a HEAD. Now, the name of the unborn branch would be revealed. I don't know if that's a problem, though - but if it is, at least this configuration variable is a way to solve that.

I'll include this in the documentation in a next version of this patch set.