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

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 16, 2020, 06:16 UTC
Message-ID: <xmqq1rfq5l8x.fsf@gitster.c.googlers.com>
In-Reply-To: <7f5b50c7b284f2630fe5be6455cc5d3e03b1eb70.1608084282.git.jonathantanmy@google.com>

Jonathan Tan <jonathantanmy@google.com> writes:

Show 11 quoted lines

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

Show 9 quoted lines

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

Show 11 quoted lines

> @@ -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?

Show 15 quoted lines

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

Show 15 quoted lines

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

Show 5 quoted lines

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

Show 45 quoted lines

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