| Message ID | cover.1547581039.git.jonathantanmy@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Jonathan Tan <jonathantanmy@google.com> writes: > @@ -474,6 +474,7 @@ void packet_reader_init(struct packet_reader *reader, int fd, > reader->buffer = packet_buffer; > reader->buffer_size = sizeof(packet_buffer); > reader->options = options; > + reader->me = "git"; > } This was somewhat unexpected. I would have thought that an interdiff would be more like + reader.me = "fetch-pack"; if (using sideband-all) { reader.use_sideband = 1; - reader.me = "fetch-pack"; } > + case SIDEBAND_PRIMARY: > + if (reader->pktlen != 1) > + goto nonprogress_received; > + /* > + * Since the packet contains nothing but the sideband > + * designator, this is a keepalive packet. Wait for the > + * next one. > + */ > + break; > + default: /* SIDEBAND_PROGRESS */ > + ; OK. Will replace. Thanks.
Jonathan Tan <jonathantanmy@google.com> writes:
> Like v1, this is on origin/ms/packet-err-check.
By the way, when merged to 'pu' as one of the earlier topic, t5409
starts to fail under --stress.
$ git checkout 'origin/pu^{/^Merge branch .jt/fetch-v2-sideband}'
$ make
$ cd t && sh ./t5409-col*.sh --stress
This is not new to this round; v1 exhibited the same symptom.
Thanks.
> Jonathan Tan <jonathantanmy@google.com> writes: > > > Like v1, this is on origin/ms/packet-err-check. > > By the way, when merged to 'pu' as one of the earlier topic, t5409 > starts to fail under --stress. > > $ git checkout 'origin/pu^{/^Merge branch .jt/fetch-v2-sideband}' > $ make > $ cd t && sh ./t5409-col*.sh --stress > > This is not new to this round; v1 exhibited the same symptom. > > Thanks. Thanks for checking. I don't think this branch is the cause of this issue, though. I ran the same stress test on both: - f3035d003e ("Merge branch 'sg/stress-test' into jch", 2019-01-14) and - the result of merging sg/stress-test into master, and the test fails with the same result.
Jonathan Tan <jonathantanmy@google.com> writes: >> Jonathan Tan <jonathantanmy@google.com> writes: >> >> > Like v1, this is on origin/ms/packet-err-check. >> >> By the way, when merged to 'pu' as one of the earlier topic, t5409 >> starts to fail under --stress. >> >> $ git checkout 'origin/pu^{/^Merge branch .jt/fetch-v2-sideband}' >> $ make >> $ cd t && sh ./t5409-col*.sh --stress >> >> This is not new to this round; v1 exhibited the same symptom. >> >> Thanks. > > Thanks for checking. I don't think this branch is the cause of this > issue, though. I ran the same stress test on both: > > - f3035d003e ("Merge branch 'sg/stress-test' into jch", 2019-01-14) and > - the result of merging sg/stress-test into master, > > and the test fails with the same result. Interesting. That is not what I am seeing (as I manually bisected the first-parent chain between f3035d003e and the tip of pu).
> Jonathan Tan <jonathantanmy@google.com> writes: > > >> Jonathan Tan <jonathantanmy@google.com> writes: > >> > >> > Like v1, this is on origin/ms/packet-err-check. > >> > >> By the way, when merged to 'pu' as one of the earlier topic, t5409 > >> starts to fail under --stress. > >> > >> $ git checkout 'origin/pu^{/^Merge branch .jt/fetch-v2-sideband}' > >> $ make > >> $ cd t && sh ./t5409-col*.sh --stress > >> > >> This is not new to this round; v1 exhibited the same symptom. > >> > >> Thanks. > > > > Thanks for checking. I don't think this branch is the cause of this > > issue, though. I ran the same stress test on both: > > > > - f3035d003e ("Merge branch 'sg/stress-test' into jch", 2019-01-14) and > > - the result of merging sg/stress-test into master, > > > > and the test fails with the same result. > > Interesting. That is not what I am seeing (as I manually bisected > the first-parent chain between f3035d003e and the tip of pu). Ah...yes, you're right. I forgot to build before running the tests. I'll take a look.
Jonathan Tan <jonathantanmy@google.com> writes: >> Jonathan Tan <jonathantanmy@google.com> writes: >> >> >> Jonathan Tan <jonathantanmy@google.com> writes: >> >> >> >> > Like v1, this is on origin/ms/packet-err-check. >> >> >> >> By the way, when merged to 'pu' as one of the earlier topic, t5409 >> >> starts to fail under --stress. >> >> >> >> $ git checkout 'origin/pu^{/^Merge branch .jt/fetch-v2-sideband}' >> >> $ make >> >> $ cd t && sh ./t5409-col*.sh --stress >> >> >> >> This is not new to this round; v1 exhibited the same symptom. >> >> >> >> Thanks. >> > >> > Thanks for checking. I don't think this branch is the cause of this >> > issue, though. I ran the same stress test on both: >> > >> > - f3035d003e ("Merge branch 'sg/stress-test' into jch", 2019-01-14) and >> > - the result of merging sg/stress-test into master, >> > >> > and the test fails with the same result. >> >> Interesting. That is not what I am seeing (as I manually bisected >> the first-parent chain between f3035d003e and the tip of pu). > > Ah...yes, you're right. I forgot to build before running the tests. I'll > take a look. Thanks.
> > Ah...yes, you're right. I forgot to build before running the tests. I'll > > take a look. > > Thanks. Thanks once again for taking a look. It turns out that it's because progress messages are sometimes split across PKT-LINEs depending on your luck, and we need to retain the "leftover" on a \2 sideband in order to combine it with the next one if necessary. So, for example, the following fixup works: diff --git a/sideband.c b/sideband.c index c185c38637..d5da587d68 100644 --- a/sideband.c +++ b/sideband.c @@ -117,7 +117,7 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n) int demultiplex_sideband(const char *me, char *buf, int len, int die_on_error) { static const char *suffix; - struct strbuf outbuf = STRBUF_INIT; + static struct strbuf outbuf = STRBUF_INIT; int retval = 0; const char *b, *brk; int band; @@ -187,8 +187,7 @@ int demultiplex_sideband(const char *me, char *buf, int len, int die_on_error) "" : DISPLAY_PREFIX); maybe_colorize_sideband(&outbuf, b, strlen(b)); } - retval = SIDEBAND_PROGRESS; - break; + return SIDEBAND_PROGRESS; /* skip cleanup */ case 1: retval = SIDEBAND_PRIMARY; break; We could make the caller of demultiplex_sideband() store the outbuf, but at this point, it might be better to refactor packet_reader into its own file and have it depend on both pkt-line.h and sideband.h. If you (or anyone else) have any ideas, let me know what you think. I'll think further about this too.
Jonathan Tan <jonathantanmy@google.com> writes: > We could make the caller of demultiplex_sideband() store the outbuf, but > at this point, it might be better to refactor packet_reader into its own > file and have it depend on both pkt-line.h and sideband.h. I do not know or too deeply care about the pkt-line / sideband distinction, as they are logically one thing. With a packet-reader abstraction in place, doesn't this "here is the remainder of the packet from the previous round" belong to each reader instance?
diff --git a/pkt-line.c b/pkt-line.c index 71b17e6b93..69162f3990 100644 --- a/pkt-line.c +++ b/pkt-line.c @@ -447,16 +447,16 @@ int recv_sideband(const char *me, int in_stream, int out) while (1) { len = packet_read(in_stream, NULL, NULL, buf, LARGE_PACKET_MAX, 0); - retval = diagnose_sideband(me, buf, len, 0); + retval = demultiplex_sideband(me, buf, len, 0); switch (retval) { - case SIDEBAND_PRIMARY: - write_or_die(out, buf + 1, len - 1); - break; - case SIDEBAND_PROGRESS: - /* already written by diagnose_sideband() */ - break; - default: /* flush or error */ - return retval; + case SIDEBAND_PRIMARY: + write_or_die(out, buf + 1, len - 1); + break; + case SIDEBAND_PROGRESS: + /* already written by demultiplex_sideband() */ + break; + default: /* errors: message already written */ + return retval; } } } @@ -474,6 +474,7 @@ void packet_reader_init(struct packet_reader *reader, int fd, reader->buffer = packet_buffer; reader->buffer_size = sizeof(packet_buffer); reader->options = options; + reader->me = "git"; } enum packet_read_status packet_reader_read(struct packet_reader *reader) @@ -483,6 +484,10 @@ enum packet_read_status packet_reader_read(struct packet_reader *reader) return reader->status; } + /* + * Consume all progress and keepalive packets until a primary payload + * packet is received + */ while (1) { int retval; reader->status = packet_read_with_status(reader->fd, @@ -494,29 +499,31 @@ enum packet_read_status packet_reader_read(struct packet_reader *reader) reader->options); if (!reader->use_sideband) break; - retval = diagnose_sideband(reader->me, reader->buffer, - reader->pktlen, 1); + retval = demultiplex_sideband(reader->me, reader->buffer, + reader->pktlen, 1); switch (retval) { - case SIDEBAND_PROTOCOL_ERROR: - 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. - */ - break; - default: /* SIDEBAND_PROGRESS */ - ; + case SIDEBAND_PROTOCOL_ERROR: + case SIDEBAND_REMOTE_ERROR: + BUG("should have died in diagnose_sideband"); + case SIDEBAND_FLUSH: + goto nonprogress_received; + case SIDEBAND_PRIMARY: + if (reader->pktlen != 1) + goto nonprogress_received; + /* + * Since the packet contains nothing but the sideband + * designator, this is a keepalive packet. Wait for the + * next one. + */ + break; + default: /* SIDEBAND_PROGRESS */ + ; } } -nonprogress: +nonprogress_received: if (reader->status == PACKET_READ_NORMAL) + /* Skip the sideband designator if sideband is used */ reader->line = reader->use_sideband ? reader->buffer + 1 : reader->buffer; else @@ -549,7 +556,7 @@ void packet_writer_write(struct packet_writer *writer, const char *fmt, ...) va_start(args, fmt); packet_write_fmt_1(writer->dest_fd, 0, - writer->use_sideband ? "\1" : "", fmt, args); + writer->use_sideband ? "\001" : "", fmt, args); va_end(args); } @@ -559,7 +566,7 @@ void packet_writer_error(struct packet_writer *writer, const char *fmt, ...) va_start(args, fmt); packet_write_fmt_1(writer->dest_fd, 0, - writer->use_sideband ? "\3" : "ERR ", fmt, args); + writer->use_sideband ? "\003" : "ERR ", fmt, args); va_end(args); } diff --git a/sideband.c b/sideband.c index cbeab380ae..9d3051e3fe 100644 --- a/sideband.c +++ b/sideband.c @@ -113,7 +113,7 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n) #define ANSI_SUFFIX "\033[K" #define DUMB_SUFFIX " " -int diagnose_sideband(const char *me, char *buf, int len, int die_on_error) +int demultiplex_sideband(const char *me, char *buf, int len, int die_on_error) { static const char *suffix; struct strbuf outbuf = STRBUF_INIT; diff --git a/sideband.h b/sideband.h index 3786670694..f75c4fde2a 100644 --- a/sideband.h +++ b/sideband.h @@ -8,15 +8,14 @@ #define SIDEBAND_PROGRESS 2 /* - * buf and len should be the result of reading a line from a remote sending - * multiplexed data. + * Inspects a multiplexed packet read from the remote and returns which + * sideband it is for. * - * Determines the nature of the result and returns it. If - * SIDEBAND_PROTOCOL_ERROR, SIDEBAND_REMOTE_ERROR, or SIDEBAND_PROGRESS, also - * prints a message (or the formatted contents of the notice in the case of - * SIDEBAND_PROGRESS) to stderr. + * If SIDEBAND_PROTOCOL_ERROR, SIDEBAND_REMOTE_ERROR, or SIDEBAND_PROGRESS, + * also prints a message (or the formatted contents of the notice in the case + * of SIDEBAND_PROGRESS) to stderr. */ -int diagnose_sideband(const char *me, char *buf, int len, int die_on_error); +int demultiplex_sideband(const char *me, char *buf, int len, int die_on_error); void send_sideband(int fd, int band, const char *data, ssize_t sz, int packet_max);
Like v1, this is on origin/ms/packet-err-check. Thanks, Junio, for your reviews. Jonathan Tan (4): pkt-line: introduce struct packet_writer sideband: reverse its dependency on pkt-line {fetch,upload}-pack: sideband v2 fetch response tests: define GIT_TEST_SIDEBAND_ALL Documentation/technical/protocol-v2.txt | 10 ++ fetch-pack.c | 13 +- pkt-line.c | 121 +++++++++++++++--- pkt-line.h | 34 +++++ sideband.c | 161 ++++++++++++------------ sideband.h | 14 ++- t/README | 5 + t/lib-httpd/apache.conf | 1 + t/t5537-fetch-shallow.sh | 3 +- t/t5701-git-serve.sh | 2 +- t/t5702-protocol-v2.sh | 4 +- upload-pack.c | 131 +++++++++++-------- 12 files changed, 343 insertions(+), 156 deletions(-) Interdiff against v1: