Re: [PATCH 02/14] upload-pack.c: Refactor capability advertising

[Junio C Hamano <gitster@pobox.com>]

Stefan Beller <sbeller@google.com> writes:

> Instead of having the capabilities in a local string, keep them
> in a struct outside the function. This will allow us in a later patch
> to easily reuse the capabilities in version 2 of the protocol.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---

Between a flat string and a list, you may be able to enumerate the
elements in a list more easily, but my knee-jerk reaction is that
this does not go far enough, if you are to introduce a table.  It
for example does not allow us to lose the conditional writing of
some capabilties based on allow_*_request and replace that with a
more table-driven approach.

Perhaps that can come in a later step (in other words, the above is
not a basis for rejecting this change; just pointing it out that
this does not have to be the end of the story).

>  upload-pack.c | 59 ++++++++++++++++++++++++++++++++++++++++-------------------
>  1 file changed, 40 insertions(+), 19 deletions(-)
>
> diff --git a/upload-pack.c b/upload-pack.c
> index aaaf883..85381d5 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -719,37 +719,58 @@ static void format_symref_info(struct strbuf *buf, struct string_list *symref)
>  		strbuf_addf(buf, " symref=%s:%s", item->string, (char *)item->util);
>  }
>  
> +static int advertise_capabilities = 1;
> +const char *all_capabilities[] = {
> +	"multi_ack",
> +	"thin-pack",
> +	"side-band",
> +	"side-band-64k",
> +	"ofs-delta",
> +	"shallow",
> +	"no-progress",
> +	"include-tag",
> +	"multi_ack_detailed",
> +	"allow-tip-sha1-in-want",
> +	"allow-reachable-sha1-in-want",
> +	"no-done",
> +};
> +
>  static int send_ref(const char *refname, const struct object_id *oid,
>  		    int flag, void *cb_data)
>  {
> -	static const char *capabilities = "multi_ack thin-pack side-band"
> -		" side-band-64k ofs-delta shallow no-progress"
> -		" include-tag multi_ack_detailed";
>  	const char *refname_nons = strip_namespace(refname);
>  	struct object_id peeled;
>  
>  	if (mark_our_ref(refname_nons, refname, oid))
>  		return 0;
>  
> -	if (capabilities) {
> -		struct strbuf symref_info = STRBUF_INIT;
> -
> -		format_symref_info(&symref_info, cb_data);
> -		packet_write(1, "%s %s%c%s%s%s%s%s agent=%s\n",
> -			     oid_to_hex(oid), refname_nons,
> -			     0, capabilities,
> -			     (allow_unadvertised_object_request & ALLOW_TIP_SHA1) ?
> -				     " allow-tip-sha1-in-want" : "",
> -			     (allow_unadvertised_object_request & ALLOW_REACHABLE_SHA1) ?
> -				     " allow-reachable-sha1-in-want" : "",
> -			     stateless_rpc ? " no-done" : "",
> -			     symref_info.buf,
> -			     git_user_agent_sanitized());
> -		strbuf_release(&symref_info);
> +	if (advertise_capabilities) {
> +		int i;
> +		struct strbuf capabilities = STRBUF_INIT;
> +
> +		for (i = 0; i < ARRAY_SIZE(all_capabilities); i++) {
> +			const char *cap = all_capabilities[i];
> +			if (!strcmp(cap, "allow-tip-sha1-in-want")
> +			    && !(allow_unadvertised_object_request & ALLOW_TIP_SHA1))
> +				continue;
> +			if (!strcmp(cap, "allow-reachable-sha1-in-want")
> +			    && !(allow_unadvertised_object_request & ALLOW_REACHABLE_SHA1))
> +				continue;
> +			if (!strcmp(cap, "no-done") && !stateless_rpc)
> +				continue;
> +			strbuf_addf(&capabilities, " %s", cap);
> +		}
> +
> +		format_symref_info(&capabilities, cb_data);
> +		strbuf_addf(&capabilities, " agent=%s", git_user_agent_sanitized());
> +
> +		packet_write(1, "%s %s%c%s\n", oid_to_hex(oid), refname_nons,
> +			     0, capabilities.buf);
> +		strbuf_release(&capabilities);
> +		advertise_capabilities = 0;
>  	} else {
>  		packet_write(1, "%s %s\n", oid_to_hex(oid), refname_nons);
>  	}
> -	capabilities = NULL;
>  	if (!peel_ref(refname, peeled.hash))
>  		packet_write(1, "%s %s^{}\n", oid_to_hex(&peeled), refname_nons);
>  	return 0;