Re: [RFCv2 04/16] upload-pack-2: Implement the version 2 of upload-pack

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

Stefan Beller <sbeller@google.com> writes:

> Subject: [RFCv2 04/16] upload-pack-2: Implement the version 2 of upload-pack

Nit; s/I/i/, to match others in the series, I think.

> In upload-pack-2 we send each capability in its own packet buffer.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>
> Notes:
>     Moved the capabilities into a struct containing all the capabilities,
>     and then we selectively cancel out unwanted capabilities.

> diff --git a/upload-pack-2.c b/upload-pack-2.c
> new file mode 120000
> index 0000000..e30a871
> --- /dev/null
> +++ b/upload-pack-2.c
> @@ -0,0 +1 @@
> +upload-pack.c
> \ No newline at end of file

Yuck.

Can't we do an equivalent without this symbolic link, i.e. a new
Makefile rule to compile upload-pack.c in two different ways to two
different object files?

The way this patch is organized makes it unclear which part is what
was added for v2 and which part is shared with v1 (and changes can
be possible breakage to the existing code), leading to a patch that
is hard to review.

> diff --git a/upload-pack.c b/upload-pack.c
> index 2493964..7477ca3 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -716,33 +716,69 @@ 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",
> +	"no-done",
> +};
> +
> +static void send_capabilities(void)

This looks like send-capabilities-v2.  I am OK to share code between
v1 and v2 by having two implementations in the same file and some
(or major part of) helper functions or overall structure code shared
between the two, but if you are taking that route, a version specific
helper like this needs to be made clear which one is which.

> +{
> +	int i;
> +	for (i = 0; i < ARRAY_SIZE(all_capabilities); i++) {
> +		const char *cap = all_capabilities[i];
> +		if (!strcmp(cap, "allow-tip-sha1-in-want") && !allow_tip_sha1_in_want)
> +			continue;
> +		if (!strcmp(cap, "no-done") && !stateless_rpc)
> +			continue;
> +		packet_write(1,"%s", cap);

s/1,/1, /;

>  static int send_ref(const char *refname, const unsigned char *sha1, 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";
> +

And is this one only for v1?

>  	const char *refname_nons = strip_namespace(refname);
>  	unsigned char peeled[20];
>  
>  	if (mark_our_ref(refname, sha1))
>  		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 agent=%s\n",
> -			     sha1_to_hex(sha1), refname_nons,
> -			     0, capabilities,
> -			     allow_tip_sha1_in_want ? " allow-tip-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_tip_sha1_in_want)
> +				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", sha1_to_hex(sha1), refname_nons,
> +			     0, capabilities.buf);
> +		strbuf_release(&capabilities);
> +		advertise_capabilities = 0;

The change itself may be going in the right direction, but I'd
suggest doing this in two steps:

 * refactor existing v1 without adding anything v2 specific
   (e.g. send-capabilities above).  A new file-scope global
   all_capabilities[] array, use of it in this new implementation of
   send_ref(), and introduction of the separate
   advertise_capabilities bool, are good examples of refactoring of
   v1.

 * then on top of that solid foundation, add v2 specific stuff.