> 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.
> > + 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).
> > + 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".
> > 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.
> > 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.
> > @@ -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.
