git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Junio C Hamano <gitster@pobox.com>
Date
Jun 2, 2015, 18:59 UTC
Message-ID
<xmqqfv6a2ayx.fsf@gitster.dls.corp.google.com>
In-Reply-To
<1433203338-27493-5-git-send-email-sbeller@google.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.
Show 8 quoted lines
> 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.
Show 8 quoted lines
> 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.

Show 24 quoted lines
> 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.

Show 9 quoted lines
> +{
> +	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, /;
Show 6 quoted lines
>  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?
Show 38 quoted lines
>  	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.
Previous: Stefan BellerNext: Stefan Beller
Message 10 of 44 in “[RFCv2 00/16] Protocol version 2”
  1. Stefan BellerJun 2, 2015
  2. 01/16 stringlist: add from_space_separated_stringStefan Beller, Jun 2, 2015
  3. Duy NguyenJun 2, 2015
  4. Eric SunshineJun 2, 2015
  5. Stefan BellerJun 2, 2015
  6. 02/16 upload-pack: make client capability parsing code a separate functionStefan Beller, Jun 2, 2015
  7. 03/16 connect: rewrite feature parsing to work on string_listStefan Beller, Jun 2, 2015
  8. Junio C HamanoJun 2, 2015
  9. 04/16 upload-pack-2: Implement the version 2 of upload-packStefan Beller, Jun 2, 2015
  10. Junio C HamanoJun 2, 2015
  11. Stefan BellerJun 2, 2015
  12. 05/16 remote.h: Change get_remote_heads return to voidStefan Beller, Jun 2, 2015
  13. Junio C HamanoJun 2, 2015
  14. Stefan BellerJun 2, 2015
  15. Junio C HamanoJun 2, 2015
  16. 06/16 remote.h: add new struct for optionsStefan Beller, Jun 2, 2015
  17. Junio C HamanoJun 2, 2015
  18. Stefan BellerJun 2, 2015
  19. Junio C HamanoJun 2, 2015
  20. 07/16 transport: add infrastructure to support a protocol version numberStefan Beller, Jun 2, 2015
  21. 08/16 transport: select transport version via command line or configStefan Beller, Jun 2, 2015
  22. 09/16 remote.h: add get_remote_capabilities, request_capabilitiesStefan Beller, Jun 2, 2015
  23. Junio C HamanoJun 2, 2015
  24. 10/16 transport: connect_setup appends protocol version numberStefan Beller, Jun 2, 2015
  25. Duy NguyenJun 2, 2015
  26. Stefan BellerJun 2, 2015
  27. Junio C HamanoJun 2, 2015
  28. Stefan BellerJun 2, 2015
  29. Junio C HamanoJun 2, 2015
  30. 11/16 remote: have preselect_capabilitiesStefan Beller, Jun 2, 2015
  31. Junio C HamanoJun 2, 2015
  32. 12/16 transport: get_refs_via_connect exchanges capabilities before refs.Stefan Beller, Jun 2, 2015
  33. Junio C HamanoJun 2, 2015
  34. Stefan BellerJun 2, 2015
  35. 13/16 fetch-pack: use the configured transport protocolStefan Beller, Jun 2, 2015
  36. Duy NguyenJun 2, 2015
  37. Duy NguyenJun 2, 2015
  38. Ilari LiusvaaraJun 2, 2015
  39. 14/16 t5544: add a test case for the new protocolStefan Beller, Jun 2, 2015
  40. Eric SunshineJun 3, 2015
  41. 15/16 Documentation/technical/pack-protocol: Mention http as possible protocolStefan Beller, Jun 2, 2015
  42. Junio C HamanoJun 2, 2015
  43. Junio C HamanoJun 2, 2015
  44. 16/16 Document protocol version 2Stefan Beller, Jun 2, 2015

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.