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

From: Junio C Hamano <gitster@pobox.com>
To: Jonathan Tan <jonathantanmy@google.com>
Cc: git@vger.kernel.org, peff@peff.net, felipe.contreras@gmail.com,
	avarab@gmail.com
Subject: Re: [PATCH v2 1/3] ls-refs: report unborn targets of symrefs
Date: Tue, 15 Dec 2020 22:16:14 -0800	[thread overview]
Message-ID: <xmqq1rfq5l8x.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <7f5b50c7b284f2630fe5be6455cc5d3e03b1eb70.1608084282.git.jonathantanmy@google.com> (Jonathan Tan's message of "Tue, 15 Dec 2020 18:07:56 -0800")

Jonathan Tan <jonathantanmy@google.com> writes:

> diff --git a/ls-refs.c b/ls-refs.c
> index a1e0b473e4..fdb644b482 100644
> --- a/ls-refs.c
> +++ b/ls-refs.c
> @@ -32,6 +32,8 @@ struct ls_refs_data {
>  	unsigned peel;
>  	unsigned symrefs;
>  	struct strvec prefixes;
> +	unsigned allow_unborn : 1;
> +	unsigned unborn : 1;
>  };

OK, so the idea is

 - lsrefs.unborn controls on the serving side if this new feature is
   advertised to the incoming clients;

 - the client side can ask "unborn" and that is recorded in
   data.unborn bit at the beginning of ls_refs();

 - the response may show an unborn symbolic ref when "unborn" was asked.

which looks internally consistent.

> @@ -47,7 +49,10 @@ static int send_ref(const char *refname, const struct object_id *oid,
>  	if (!ref_match(&data->prefixes, refname_nons))
>  		return 0;
>  
> -	strbuf_addf(&refline, "%s %s", oid_to_hex(oid), refname_nons);
> +	if (oid)
> +		strbuf_addf(&refline, "%s %s", oid_to_hex(oid), refname_nons);
> +	else
> +		strbuf_addf(&refline, "unborn %s", refname_nons);

A line that has token "unborn" instead of a full object name in hex
is something existing clients are not prepared to receive and grok.
It is not immediately clear how this new code makes sure that
clients that did not ask for "unborn" in what was parsed at the
beginning of ls_refs() will not see such a line.  Presumably, no
existing caller of send_ref passes NULL in oid and the only one
that does so is the one in send_possibly_unborn_head() when the HEAD
points at an unborn branch.

OK.

> @@ -74,8 +79,28 @@ static int send_ref(const char *refname, const struct object_id *oid,
>  	return 0;
>  }
>  
> -static int ls_refs_config(const char *var, const char *value, void *data)
> +static void send_possibly_unborn_head(struct ls_refs_data *data)
>  {
> +	struct strbuf namespaced = STRBUF_INIT;
> +	struct object_id oid;
> +	int flag;
> +	int null_oid;

I'd suggest renaming this one, which masks the global null_oid of
"const struct object_id" type.  This code does not break only
because is_null_oid() happens to be implemented as a static inline,
and not as a C-preprocessor macro, right?

> +	strbuf_addf(&namespaced, "%sHEAD", get_git_namespace());
> +	resolve_ref_unsafe(namespaced.buf, 0, &oid, &flag);
> +	null_oid = is_null_oid(&oid);
> +	if (!null_oid || (data->symrefs && (flag & REF_ISSYMREF)))
> +		send_ref(namespaced.buf, null_oid ? NULL : &oid, flag, data);
> +	strbuf_release(&namespaced);
> +}
> +
> +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");

Are there differences between allow and advertise?  Would
lsrefs.allowUnborn that is a boolean, thus allowing the value to be
parsed by git_config_bool(), make more sense here, I wonder.  Or is
this meant as some future enhancement, e.g. you plan to have some
servers that allow "unborn" request even though they do not actively
advertise the support of the feature?  Without documentation update
or an in-code comment, it is rather hard to guess the intention
here.

> @@ -91,7 +116,7 @@ int ls_refs(struct repository *r, struct strvec *keys,
>  
>  	memset(&data, 0, sizeof(data));
>  
> -	git_config(ls_refs_config, NULL);
> +	git_config(ls_refs_config, &data);
>  
>  	while (packet_reader_read(request) == PACKET_READ_NORMAL) {
>  		const char *arg = request->line;
> @@ -103,14 +128,35 @@ int ls_refs(struct repository *r, struct strvec *keys,
>  			data.symrefs = 1;
>  		else if (skip_prefix(arg, "ref-prefix ", &out))
>  			strvec_push(&data.prefixes, out);
> +		else if (data.allow_unborn && !strcmp("unborn", arg))
> +			data.unborn = 1;

Somehow, it appears to me that writing it in a way along with this
line ...

		else if (!strcmp("unborn", arg))
			data.unborn = data.allow_unborn;

... would make more sense.  Whether we allowed "unborn" request or
not, when the other side says "unborn", we are handling the request
for the unborn feature, and the condition with strcmp() alone
signals that better (in other words, when we acquire more request
types, we do not want to pass the control to "else if" clauses that
may come after this part when we see "unborn" request and when we
are configured not to accept "unborn" requests.

It does not make any difference in the current code, of course, and
it is more about future-proofing the cleanness of the code.

> -	head_ref_namespaced(send_ref, &data);
> +	if (data.unborn)
> +		send_possibly_unborn_head(&data);
> +	else
> +		head_ref_namespaced(send_ref, &data);

I found the "send_possibly 70% duplicates what the more generic
head_ref_namespaced() does" a bit disturbing.

>  	for_each_namespaced_ref(send_ref, &data);
>  	packet_flush(1);
>  	strvec_clear(&data.prefixes);
>  	return 0;
>  }
> +
> +int ls_refs_advertise(struct repository *r, struct strbuf *value)
> +{
> +	if (value) {
> +		char *str = NULL;
> +
> +		if (!repo_config_get_string(the_repository, "lsrefs.unborn",
> +					    &str) &&
> +		    !strcmp("advertise", str)) {
> +			strbuf_addstr(value, "unborn");
> +			free(str);
> +		}
> +	}
> +
> +	return 1;
> +}
> diff --git a/ls-refs.h b/ls-refs.h
> index 7b33a7c6b8..a99e4be0bd 100644
> --- a/ls-refs.h
> +++ b/ls-refs.h
> @@ -6,5 +6,6 @@ struct strvec;
>  struct packet_reader;
>  int ls_refs(struct repository *r, struct strvec *keys,
>  	    struct packet_reader *request);
> +int ls_refs_advertise(struct repository *r, struct strbuf *value);
>  
>  #endif /* LS_REFS_H */
> diff --git a/serve.c b/serve.c
> index f6341206c4..30cb56d507 100644
> --- a/serve.c
> +++ b/serve.c
> @@ -62,7 +62,7 @@ struct protocol_capability {
>  
>  static struct protocol_capability capabilities[] = {
>  	{ "agent", agent_advertise, NULL },
> -	{ "ls-refs", always_advertise, ls_refs },
> +	{ "ls-refs", ls_refs_advertise, ls_refs },
>  	{ "fetch", upload_pack_advertise, upload_pack_v2 },
>  	{ "server-option", always_advertise, NULL },
>  	{ "object-format", object_format_advertise, NULL },

Thanks.