Re: [PATCH 3/4] {fetch,upload}-pack: sideband v2 fetch response
- From
- Jonathan Tan <jonathantanmy@google.com>
- Date
- Jan 15, 2019, 19:18 UTC
- Message-ID
- <20190115191842.123188-1-jonathantanmy@google.com>
- In-Reply-To
- <xmqqk1j6ub7t.fsf@gitster-ct.c.googlers.com>
Show 11 quoted lines
> 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.
I think the best solution here is to initialize .me to "git", like how we do it for trace. I've done that for now.
> > + switch (retval) {
> > + case SIDEBAND_PROTOCOL_ERROR:
>
> Dedent (see previous step).Done.
Show 9 quoted lines
> > + 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?
This is indeed assuming that we currently don't send 0004 (the equivalent of 0005\1 without the sideband). I chose this because it is currently what we use in create_pack_file() in upload-pack.c, but I'm OK with alternatives (e.g. if we use 0005\2 instead).
Show 20 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 > */
[snip]
Yes, it's meant to spin and consume the progress and keepalive packets. I've added a comment at the top of the loop and renamed the jump label to "nonprogress_received".
Show 6 quoted lines
> > 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?
Yes. Added comment to clarify that.
Show 13 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".
Done.
Show 13 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?
Yes - the branch that this patch set is on (ms/packet-err-check) handles this. I have taken care in this patch to ensure that the error message printed matches what Masaya used, so that in a future patch, when we force sideband-all (using GIT_TEST_SIDEBAND_ALL), the same message is printed so that the existing tests still pass.