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

Re: [PATCH 3/4] {fetch,upload}-pack: sideband v2 fetch response

From
Junio C Hamano <gitster@pobox.com>
Date
Jan 14, 2019, 22:27 UTC
Message-ID
<xmqqk1j6ub7t.fsf@gitster-ct.c.googlers.com>
In-Reply-To
<673b928b4158b0aecca843302483b48da5ed8d35.1547244620.git.jonathantanmy@google.com>
Jonathan Tan <jonathantanmy@google.com> writes:
Show 12 quoted lines
> Currently, a response to a fetch request has sideband support only while
> the packfile is being sent, meaning that the server cannot send notices
> until the start of the packfile.
>
> Extend sideband support in protocol v2 fetch responses to the whole
> response. upload-pack will advertise it if the
> uploadpack.allowsidebandall configuration variable is set, and
> fetch-pack will automatically request it if advertised.
>
> If the sideband is to be used throughout the whole response, upload-pack
> will use it to send errors instead of prefixing a PKT-LINE payload with
> "ERR ".
Makes sense.
Show 44 quoted lines
> diff --git a/fetch-pack.c b/fetch-pack.c
> index 3f9626dbf7..b89199891d 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -1125,7 +1125,8 @@ static int add_haves(struct fetch_negotiator *negotiator,
>  static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,
>  			      const struct fetch_pack_args *args,
>  			      const struct ref *wants, struct oidset *common,
> -			      int *haves_to_send, int *in_vain)
> +			      int *haves_to_send, int *in_vain,
> +			      int sideband_all)
>  {
>  	int ret = 0;
>  	struct strbuf req_buf = STRBUF_INIT;
> @@ -1151,6 +1152,8 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,
>  		packet_buf_write(&req_buf, "include-tag");
>  	if (prefer_ofs_delta)
>  		packet_buf_write(&req_buf, "ofs-delta");
> +	if (sideband_all)
> +		packet_buf_write(&req_buf, "sideband-all");
>  
>  	/* Add shallow-info and deepen request */
>  	if (server_supports_feature("fetch", "shallow", 0))
> @@ -1359,6 +1362,10 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
>  	packet_reader_init(&reader, fd[0], NULL, 0,
>  			   PACKET_READ_CHOMP_NEWLINE |
>  			   PACKET_READ_DIE_ON_ERR_PACKET);
> +	if (server_supports_feature("fetch", "sideband-all", 0)) {
> +		reader.use_sideband = 1;
> +		reader.me = "fetch-pack";
> +	}
>  
>  	while (state != FETCH_DONE) {
>  		switch (state) {
> @@ -1392,7 +1399,8 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
>  		case FETCH_SEND_REQUEST:
>  			if (send_fetch_request(&negotiator, fd[1], args, ref,
>  					       &common,
> -					       &haves_to_send, &in_vain))
> +					       &haves_to_send, &in_vain,
> +					       reader.use_sideband))
>  				state = FETCH_GET_PACK;
>  			else
>  				state = FETCH_PROCESS_ACKS;

OK, so the "fetch" side enables sideband-all any time it knows that the other side supports it.

It feels a bit strange at the logical level that reader.me is set only when we are going to talk over "sideband-all". I know that at the implementation level, reader.me is only looked at when sideband is use, but it still feels somewhat funny. Future developers may want to use reader.me to identify the entity that found some error during a read, even when the sideband is not yet in use, and at that point, uninitialized .me would be a source of an error. IOW, that assignment smells like a timebomb waiting to go off.

Show 26 quoted lines
> @@ -483,16 +483,42 @@ enum packet_read_status packet_reader_read(struct packet_reader *reader)
>  		return reader->status;
>  	}
>  
> -	reader->status = packet_read_with_status(reader->fd,
> -						 &reader->src_buffer,
> -						 &reader->src_len,
> -						 reader->buffer,
> -						 reader->buffer_size,
> -						 &reader->pktlen,
> -						 reader->options);
> +	while (1) {
> +		int retval;
> +		reader->status = packet_read_with_status(reader->fd,
> +							 &reader->src_buffer,
> +							 &reader->src_len,
> +							 reader->buffer,
> +							 reader->buffer_size,
> +							 &reader->pktlen,
> +							 reader->options);
> +		if (!reader->use_sideband)
> +			break;
> +		retval = diagnose_sideband(reader->me, reader->buffer,
> +					   reader->pktlen, 1);
> +		switch (retval) {
> +			case SIDEBAND_PROTOCOL_ERROR:
Dedent (see previous step).
Show 11 quoted lines
> +			case SIDEBAND_REMOTE_ERROR:
> +				BUG("should have died in diagnose_sideband");
> +			case SIDEBAND_FLUSH:
> +				goto nonprogress;
> +			case SIDEBAND_PRIMARY:
> +				if (reader->pktlen != 1)
> +					goto nonprogress;
> +				/*
> +				 * Since pktlen is 1, this is a keepalive
> +				 * packet. Wait for the next one.
> +				 */
What guarantees that a normal payload packet is never of length==1?
Show 7 quoted lines
> +				break;
> +			default: /* SIDEBAND_PROGRESS */
> +				;
> +		}
> +	}
>  
> +nonprogress:

It is totally unclear why this label is called nonprogress. Is it that the primary purpose of the while loop above is to spin and eat the keep-alive packets from the other side? If so, then it sort-of makes sense (i.e. "we are looking for progress-meter related packets, but now we found something else, so let's leave the loop").

It would have greatly helped reviewers to have a comment in front of that infinite loop, perhaps like

	/*
	 * Spin and consume the keep-alive packets
	 */
	while (1) {
		int keepalive = 0;
		get_packet();
		/* decide if we got a keepalive here */
		...
		keepalive = ...;
		if (keepalive) {
			/* do the keep-alive thing here */
			...
			continue;
		}
		/* do one-off thing for non-keepalive packet if any */
		...
		break;
	}
>  	if (reader->status == PACKET_READ_NORMAL)
> -		reader->line = reader->buffer;
> +		reader->line = reader->use_sideband ?
> +			reader->buffer + 1 : reader->buffer;
Is the one byte the sideband designator?
Show 10 quoted lines
>  void packet_writer_write(struct packet_writer *writer, const char *fmt, ...)
> @@ -521,7 +548,8 @@ void packet_writer_write(struct packet_writer *writer, const char *fmt, ...)
>  	va_list args;
>  
>  	va_start(args, fmt);
> -	packet_write_fmt_1(writer->dest_fd, 0, "", fmt, args);
> +	packet_write_fmt_1(writer->dest_fd, 0,
> +			   writer->use_sideband ? "\1" : "", fmt, args);
>  	va_end(args);
>  }

As I am superstitious, I'd prefer to see octal literals to be fully spelled out as 3-digit sequence, i.e. "\001". Likewise for "\003".

Show 7 quoted lines
> @@ -530,7 +558,8 @@ void packet_writer_error(struct packet_writer *writer, const char *fmt, ...)
>  	va_list args;
>  
>  	va_start(args, fmt);
> -	packet_write_fmt_1(writer->dest_fd, 0, "ERR ", fmt, args);
> +	packet_write_fmt_1(writer->dest_fd, 0,
> +			   writer->use_sideband ? "\3" : "ERR ", fmt, args);

OK, band#3 is an error message for emergency exit. It is a bit curious that this patch does not show any line from the existing code in the context that reacts to the ERR string, but that is because the errors are noticed at a lot lower level when the sideband is in use than the code that currently checks for errors?

Previous: Jonathan TanNext: Jonathan Tan
Message 11 of 41 in “Sideband the whole fetch v2 response”
  1. 0/4 Sideband the whole fetch v2 responseJonathan Tan, Jan 11, 2019
  2. 1/4 pkt-line: introduce struct packet_writerJonathan Tan, Jan 11, 2019
  3. Junio C HamanoJan 14, 2019
  4. Jonathan TanJan 15, 2019
  5. 2/4 sideband: reverse its dependency on pkt-lineJonathan Tan, Jan 11, 2019
  6. Junio C HamanoJan 14, 2019
  7. Junio C HamanoJan 14, 2019
  8. Jonathan TanJan 15, 2019
  9. Jonathan TanJan 15, 2019
  10. 3/4 {fetch,upload}-pack: sideband v2 fetch responseJonathan Tan, Jan 11, 2019
  11. Junio C HamanoJan 14, 2019
  12. Jonathan TanJan 15, 2019
  13. 4/4 tests: define GIT_TEST_SIDEBAND_ALLJonathan Tan, Jan 11, 2019
  14. 5/4 Documentation: order protocol v2 sectionsJonathan Tan, Jan 11, 2019
  15. 6/4 Documentation: add Packfile URIs design docJonathan Tan, Jan 11, 2019
  16. 7/4 upload-pack: refactor reading of pack-objects outJonathan Tan, Jan 11, 2019
  17. 8/4 upload-pack: send part of packfile response as uriJonathan Tan, Jan 11, 2019
  18. 0/4 Sideband the whole fetch v2 responseJonathan Tan, Jan 15, 2019
  19. 1/4 pkt-line: introduce struct packet_writerJonathan Tan, Jan 15, 2019
  20. 2/4 sideband: reverse its dependency on pkt-lineJonathan Tan, Jan 15, 2019
  21. SZEDER GáborJan 16, 2019
  22. Jonathan TanJan 16, 2019
  23. SZEDER GáborJan 16, 2019
  24. 3/4 {fetch,upload}-pack: sideband v2 fetch responseJonathan Tan, Jan 15, 2019
  25. 4/4 tests: define GIT_TEST_SIDEBAND_ALLJonathan Tan, Jan 15, 2019
  26. Junio C HamanoJan 15, 2019
  27. Junio C HamanoJan 15, 2019
  28. Jonathan TanJan 15, 2019
  29. Junio C HamanoJan 15, 2019
  30. Jonathan TanJan 15, 2019
  31. Junio C HamanoJan 15, 2019
  32. Jonathan TanJan 16, 2019
  33. Junio C HamanoJan 16, 2019
  34. 0/4 Sideband the whole fetch v2 responseJonathan Tan, Jan 16, 2019
  35. 1/4 pkt-line: introduce struct packet_writerJonathan Tan, Jan 16, 2019
  36. 2/4 sideband: reverse its dependency on pkt-lineJonathan Tan, Jan 16, 2019
  37. 3/4 {fetch,upload}-pack: sideband v2 fetch responseJonathan Tan, Jan 16, 2019
  38. 4/4 tests: define GIT_TEST_SIDEBAND_ALLJonathan Tan, Jan 16, 2019
  39. Jonathan NiederJan 29, 2019
  40. Jeff KingFeb 13, 2019
  41. Junio C HamanoJan 17, 2019

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.