| Message ID | 4079a1f110086bd412fb4f56bc8b7f8a6a7a8f9f.1547244620.git.jonathantanmy@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Sideband the whole fetch v2 response | expand |
Jonathan Tan <jonathantanmy@google.com> writes: > +int recv_sideband(const char *me, int in_stream, int out) > +{ > + char buf[LARGE_PACKET_MAX + 1]; > + int retval = 0; > + int len; > + > + while (1) { > + len = packet_read(in_stream, NULL, NULL, buf, LARGE_PACKET_MAX, 0); > + retval = diagnose_sideband(me, buf, len); This name of the helper does not convey much useful information. Let's think about it more later. > + switch (retval) { > + case SIDEBAND_PRIMARY: Dedent; case/default arms align with 's' in switch in our codebase. > + write_or_die(out, buf + 1, len - 1); > + break; OK, so PRIMARY return (band#1) gives us the payload unwrapped back in "out" and we are responsible for writing it out here. > + case SIDEBAND_PROGRESS: > + /* already written by diagnose_sideband() */ True, that happens in "switch(band) { case 2: }" in the other helper. > + break; > + default: /* flush or error */ > + return retval; Lack of corresponding comment bothers readers. In all of REMOTE_ERROR, PROGRESS and PROTOCOL_ERROR cases, the other helper stuffs the message in outbuf in "switch (band) { ... }" and writes it out with xwrite(2, outbuf.buf, outbuf.len) [*1*], so I can see there is no need for us to write anything out here. Perhaps case SIDEBAND_FLUSH: default: /* errors: message already written */ return retval; or something to clarify? > +/* > + * Receive multiplexed output stream over git native protocol. > + * in_stream is the input stream from the remote, which carries data > + * in pkt_line format with band designator. Demultiplex it into out > + * and err and return error appropriately. Band #1 carries the > + * primary payload. Things coming over band #2 is not necessarily > + * error; they are usually informative message on the standard error > + * stream, aka "verbose"). A message over band #3 is a signal that > + * the remote died unexpectedly. A flush() concludes the stream. > + * > + * Returns SIDEBAND_FLUSH upon a normal conclusion, and SIDEBAND_PROTOCOL_ERROR > + * or SIDEBAND_REMOTE_ERROR if an error occurred. > + */ > +int recv_sideband(const char *me, int in_stream, int out); This is well described. > diff --git a/sideband.c b/sideband.c > index 368647acf8..842a92e975 100644 > --- a/sideband.c > +++ b/sideband.c > ... > +int diagnose_sideband(const char *me, char *buf, int len) > { > + static const char *suffix; > struct strbuf outbuf = STRBUF_INIT; > int retval = 0; > + const char *b, *brk; > + int band; > + > + if (!suffix) { > + if (isatty(2) && !is_terminal_dumb()) > + suffix = ANSI_SUFFIX; > + else > + suffix = DUMB_SUFFIX; > + } It may be worth considering a further clean-up for the progress code, that consumes lines in the "switch(band)" below that are disproportionate to what it does in this function by introducing another helper function that is called from here. When it happens, the above "suffix" thing should move to the helper function, too. > + if (len == 0) { > + retval = SIDEBAND_FLUSH; > + goto cleanup; > + } > + if (len < 1) { > + strbuf_addf(&outbuf, > + "%s%s: protocol error: no band designator", > + outbuf.len ? "\n" : "", me); > + retval = SIDEBAND_PROTOCOL_ERROR; > + goto cleanup; > + } > + band = buf[0] & 0xff; > + buf[len] = '\0'; > + len--; > + switch (band) { > + case 3: > +... > + case 2: > +... > + case 1: > + retval = SIDEBAND_PRIMARY; > + break; > + default: > + strbuf_addf(&outbuf, "%s%s: protocol error: bad band #%d", > + outbuf.len ? "\n" : "", me, band); > + retval = SIDEBAND_PROTOCOL_ERROR; > + break; So, the point of this helper is to inspect a single packet-line data and dispatch the payload of the packet based on which band it is sent to. Perhaps call it with a name with demultiplex or dispatch in it? "diagnose" is a bit unclear in that what trait you are diagnosing and for what purpose. > diff --git a/sideband.h b/sideband.h > index 7a8146f161..a56cd86287 100644 > --- a/sideband.h > +++ b/sideband.h > @@ -3,8 +3,21 @@ > > #define SIDEBAND_PROTOCOL_ERROR -2 > #define SIDEBAND_REMOTE_ERROR -1 > +#define SIDEBAND_FLUSH 0 > +#define SIDEBAND_PRIMARY 1 > +#define SIDEBAND_PROGRESS 2 > + > +/* > + * buf and len should be the result of reading a line from a remote sending > + * multiplexed data. > + * > + * Determines the nature of the result and returns it. If "the result" may be from the point of view of the implementor or the "recv_sideband()" function who called packet_read(), but a casual reader would perceive it more natural if you referred it as "a packet read from a remote". "Inspect a packet read from the remote and returns which sideband it is for", perhaps? > + * 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 recv_sideband(const char *me, int in_stream, int out); > void send_sideband(int fd, int band, const char *data, ssize_t sz, int packet_max); > > #endif
Junio C Hamano <gitster@pobox.com> writes: > Lack of corresponding comment bothers readers. In all of > REMOTE_ERROR, PROGRESS and PROTOCOL_ERROR cases, the other helper > stuffs the message in outbuf in "switch (band) { ... }" and writes > it out with xwrite(2, outbuf.buf, outbuf.len) [*1*], so I can see > there is no need for us to write anything out here. Perhaps > > case SIDEBAND_FLUSH: > default: /* errors: message already written */ > return retval; > > or something to clarify? Forgot a footnote for *1* above. I was wondering if xwrite is what we really want, rather than write_in_full().
> Jonathan Tan <jonathantanmy@google.com> writes: > > > +int recv_sideband(const char *me, int in_stream, int out) > > +{ > > + char buf[LARGE_PACKET_MAX + 1]; > > + int retval = 0; > > + int len; > > + > > + while (1) { > > + len = packet_read(in_stream, NULL, NULL, buf, LARGE_PACKET_MAX, 0); > > + retval = diagnose_sideband(me, buf, len); > > This name of the helper does not convey much useful information. > Let's think about it more later. OK - after reading the later comments, switched to demultiplex_sideband. > > + switch (retval) { > > + case SIDEBAND_PRIMARY: > > Dedent; case/default arms align with 's' in switch in our codebase. Done. > > + break; > > + default: /* flush or error */ > > + return retval; > > Lack of corresponding comment bothers readers. In all of > REMOTE_ERROR, PROGRESS and PROTOCOL_ERROR cases, the other helper > stuffs the message in outbuf in "switch (band) { ... }" and writes > it out with xwrite(2, outbuf.buf, outbuf.len) [*1*], so I can see > there is no need for us to write anything out here. Perhaps > > case SIDEBAND_FLUSH: > default: /* errors: message already written */ > return retval; > > or something to clarify? Done. > > +/* > > + * Receive multiplexed output stream over git native protocol. > > + * in_stream is the input stream from the remote, which carries data > > + * in pkt_line format with band designator. Demultiplex it into out > > + * and err and return error appropriately. Band #1 carries the > > + * primary payload. Things coming over band #2 is not necessarily > > + * error; they are usually informative message on the standard error > > + * stream, aka "verbose"). A message over band #3 is a signal that > > + * the remote died unexpectedly. A flush() concludes the stream. > > + * > > + * Returns SIDEBAND_FLUSH upon a normal conclusion, and SIDEBAND_PROTOCOL_ERROR > > + * or SIDEBAND_REMOTE_ERROR if an error occurred. > > + */ > > +int recv_sideband(const char *me, int in_stream, int out); > > This is well described. Thanks, although the credit should be given to the original author - most of this was moved. > > diff --git a/sideband.c b/sideband.c > > index 368647acf8..842a92e975 100644 > > --- a/sideband.c > > +++ b/sideband.c > > ... > > +int diagnose_sideband(const char *me, char *buf, int len) > > { > > + static const char *suffix; > > struct strbuf outbuf = STRBUF_INIT; > > int retval = 0; > > + const char *b, *brk; > > + int band; > > + > > + if (!suffix) { > > + if (isatty(2) && !is_terminal_dumb()) > > + suffix = ANSI_SUFFIX; > > + else > > + suffix = DUMB_SUFFIX; > > + } > > It may be worth considering a further clean-up for the progress > code, that consumes lines in the "switch(band)" below that are > disproportionate to what it does in this function by introducing > another helper function that is called from here. When it happens, > the above "suffix" thing should move to the helper function, too. That's reasonable. If it's OK, I would like to limit the scope of this patch to splitting the existing recv_sideband() into recv_sideband() and demultiplex_sideband() (the latter is quite close to the old recv_sideband(), and the diff looks larger only because most of it is a dedent). > So, the point of this helper is to inspect a single packet-line data > and dispatch the payload of the packet based on which band it is > sent to. Perhaps call it with a name with demultiplex or dispatch > in it? "diagnose" is a bit unclear in that what trait you are > diagnosing and for what purpose. Changed to demultiplex_sideband. > > +/* > > + * buf and len should be the result of reading a line from a remote sending > > + * multiplexed data. > > + * > > + * Determines the nature of the result and returns it. If > > "the result" may be from the point of view of the implementor or the > "recv_sideband()" function who called packet_read(), but a casual > reader would perceive it more natural if you referred it as "a > packet read from a remote". "Inspect a packet read from the remote > and returns which sideband it is for", perhaps? Done.
> Junio C Hamano <gitster@pobox.com> writes: > > > Lack of corresponding comment bothers readers. In all of > > REMOTE_ERROR, PROGRESS and PROTOCOL_ERROR cases, the other helper > > stuffs the message in outbuf in "switch (band) { ... }" and writes > > it out with xwrite(2, outbuf.buf, outbuf.len) [*1*], so I can see > > there is no need for us to write anything out here. Perhaps > > > > case SIDEBAND_FLUSH: > > default: /* errors: message already written */ > > return retval; > > > > or something to clarify? > > Forgot a footnote for *1* above. I was wondering if xwrite is what > we really want, rather than write_in_full(). There is an existing comment explaining this: * The output is accumulated in a buffer and * each line is printed to stderr using * write(2) to ensure inter-process atomicity. which seems reasonable to me (I know that write(2) does not guarantee to write everything, but write_in_full() does not guarantee atomicity, as far as I know).
diff --git a/pkt-line.c b/pkt-line.c index 9d3e402d41..ebdc6c2530 100644 --- a/pkt-line.c +++ b/pkt-line.c @@ -439,6 +439,28 @@ ssize_t read_packetized_to_strbuf(int fd_in, struct strbuf *sb_out) return sb_out->len - orig_len; } +int recv_sideband(const char *me, int in_stream, int out) +{ + char buf[LARGE_PACKET_MAX + 1]; + int retval = 0; + int len; + + while (1) { + len = packet_read(in_stream, NULL, NULL, buf, LARGE_PACKET_MAX, 0); + retval = diagnose_sideband(me, buf, len); + 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; + } + } +} + /* Packet Reader Functions */ void packet_reader_init(struct packet_reader *reader, int fd, char *src_buffer, size_t src_len, diff --git a/pkt-line.h b/pkt-line.h index 023ad2951d..a8e92a4b63 100644 --- a/pkt-line.h +++ b/pkt-line.h @@ -3,6 +3,7 @@ #include "git-compat-util.h" #include "strbuf.h" +#include "sideband.h" /* * Write a packetized stream, where each line is preceded by @@ -120,6 +121,21 @@ char *packet_read_line_buf(char **src_buf, size_t *src_len, int *size); */ ssize_t read_packetized_to_strbuf(int fd_in, struct strbuf *sb_out); +/* + * Receive multiplexed output stream over git native protocol. + * in_stream is the input stream from the remote, which carries data + * in pkt_line format with band designator. Demultiplex it into out + * and err and return error appropriately. Band #1 carries the + * primary payload. Things coming over band #2 is not necessarily + * error; they are usually informative message on the standard error + * stream, aka "verbose"). A message over band #3 is a signal that + * the remote died unexpectedly. A flush() concludes the stream. + * + * Returns SIDEBAND_FLUSH upon a normal conclusion, and SIDEBAND_PROTOCOL_ERROR + * or SIDEBAND_REMOTE_ERROR if an error occurred. + */ +int recv_sideband(const char *me, int in_stream, int out); + struct packet_reader { /* source file descriptor */ int fd; diff --git a/sideband.c b/sideband.c index 368647acf8..842a92e975 100644 --- a/sideband.c +++ b/sideband.c @@ -1,7 +1,6 @@ #include "cache.h" #include "color.h" #include "config.h" -#include "pkt-line.h" #include "sideband.h" #include "help.h" @@ -109,103 +108,94 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n) } -/* - * Receive multiplexed output stream over git native protocol. - * in_stream is the input stream from the remote, which carries data - * in pkt_line format with band designator. Demultiplex it into out - * and err and return error appropriately. Band #1 carries the - * primary payload. Things coming over band #2 is not necessarily - * error; they are usually informative message on the standard error - * stream, aka "verbose"). A message over band #3 is a signal that - * the remote died unexpectedly. A flush() concludes the stream. - */ - #define DISPLAY_PREFIX "remote: " #define ANSI_SUFFIX "\033[K" #define DUMB_SUFFIX " " -int recv_sideband(const char *me, int in_stream, int out) +int diagnose_sideband(const char *me, char *buf, int len) { - const char *suffix; - char buf[LARGE_PACKET_MAX + 1]; + static const char *suffix; struct strbuf outbuf = STRBUF_INIT; int retval = 0; + const char *b, *brk; + int band; + + if (!suffix) { + if (isatty(2) && !is_terminal_dumb()) + suffix = ANSI_SUFFIX; + else + suffix = DUMB_SUFFIX; + } - if (isatty(2) && !is_terminal_dumb()) - suffix = ANSI_SUFFIX; - else - suffix = DUMB_SUFFIX; + if (len == 0) { + retval = SIDEBAND_FLUSH; + goto cleanup; + } + if (len < 1) { + strbuf_addf(&outbuf, + "%s%s: protocol error: no band designator", + outbuf.len ? "\n" : "", me); + retval = SIDEBAND_PROTOCOL_ERROR; + goto cleanup; + } + band = buf[0] & 0xff; + buf[len] = '\0'; + len--; + switch (band) { + case 3: + strbuf_addf(&outbuf, "%s%s", outbuf.len ? "\n" : "", + DISPLAY_PREFIX); + maybe_colorize_sideband(&outbuf, buf + 1, len); + + retval = SIDEBAND_REMOTE_ERROR; + break; + case 2: + b = buf + 1; - while (!retval) { - const char *b, *brk; - int band, len; - len = packet_read(in_stream, NULL, NULL, buf, LARGE_PACKET_MAX, 0); - if (len == 0) - break; - if (len < 1) { - strbuf_addf(&outbuf, - "%s%s: protocol error: no band designator", - outbuf.len ? "\n" : "", me); - retval = SIDEBAND_PROTOCOL_ERROR; - break; - } - band = buf[0] & 0xff; - buf[len] = '\0'; - len--; - switch (band) { - case 3: - strbuf_addf(&outbuf, "%s%s", outbuf.len ? "\n" : "", - DISPLAY_PREFIX); - maybe_colorize_sideband(&outbuf, buf + 1, len); - - retval = SIDEBAND_REMOTE_ERROR; - break; - case 2: - b = buf + 1; - - /* - * Append a suffix to each nonempty line to clear the - * end of the screen line. - * - * The output is accumulated in a buffer and - * each line is printed to stderr using - * write(2) to ensure inter-process atomicity. - */ - while ((brk = strpbrk(b, "\n\r"))) { - int linelen = brk - b; - - if (!outbuf.len) - strbuf_addstr(&outbuf, DISPLAY_PREFIX); - if (linelen > 0) { - maybe_colorize_sideband(&outbuf, b, linelen); - strbuf_addstr(&outbuf, suffix); - } - - strbuf_addch(&outbuf, *brk); - xwrite(2, outbuf.buf, outbuf.len); - strbuf_reset(&outbuf); - - b = brk + 1; + /* + * Append a suffix to each nonempty line to clear the + * end of the screen line. + * + * The output is accumulated in a buffer and + * each line is printed to stderr using + * write(2) to ensure inter-process atomicity. + */ + while ((brk = strpbrk(b, "\n\r"))) { + int linelen = brk - b; + + if (!outbuf.len) + strbuf_addstr(&outbuf, DISPLAY_PREFIX); + if (linelen > 0) { + maybe_colorize_sideband(&outbuf, b, linelen); + strbuf_addstr(&outbuf, suffix); } - if (*b) { - strbuf_addstr(&outbuf, outbuf.len ? - "" : DISPLAY_PREFIX); - maybe_colorize_sideband(&outbuf, b, strlen(b)); - } - break; - case 1: - write_or_die(out, buf + 1, len); - break; - default: - strbuf_addf(&outbuf, "%s%s: protocol error: bad band #%d", - outbuf.len ? "\n" : "", me, band); - retval = SIDEBAND_PROTOCOL_ERROR; - break; + strbuf_addch(&outbuf, *brk); + xwrite(2, outbuf.buf, outbuf.len); + strbuf_reset(&outbuf); + + b = brk + 1; + } + + if (*b) { + strbuf_addstr(&outbuf, outbuf.len ? + "" : DISPLAY_PREFIX); + maybe_colorize_sideband(&outbuf, b, strlen(b)); } + retval = SIDEBAND_PROGRESS; + break; + case 1: + retval = SIDEBAND_PRIMARY; + break; + default: + strbuf_addf(&outbuf, "%s%s: protocol error: bad band #%d", + outbuf.len ? "\n" : "", me, band); + retval = SIDEBAND_PROTOCOL_ERROR; + break; } +cleanup: if (outbuf.len) { strbuf_addch(&outbuf, '\n'); xwrite(2, outbuf.buf, outbuf.len); diff --git a/sideband.h b/sideband.h index 7a8146f161..a56cd86287 100644 --- a/sideband.h +++ b/sideband.h @@ -3,8 +3,21 @@ #define SIDEBAND_PROTOCOL_ERROR -2 #define SIDEBAND_REMOTE_ERROR -1 +#define SIDEBAND_FLUSH 0 +#define SIDEBAND_PRIMARY 1 +#define SIDEBAND_PROGRESS 2 + +/* + * buf and len should be the result of reading a line from a remote sending + * multiplexed data. + * + * 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. + */ +int diagnose_sideband(const char *me, char *buf, int len); -int recv_sideband(const char *me, int in_stream, int out); void send_sideband(int fd, int band, const char *data, ssize_t sz, int packet_max); #endif
A subsequent patch will teach struct packet_reader a new field that, if set, instructs it to interpret read data as multiplexed. This will create a dependency from pkt-line to sideband. To avoid a circular dependency, split recv_sideband() into 2 parts: the reading loop (left in recv_sideband()) and the processing of the contents (in diagnose_sideband()), and move the former into pkt-line. This reverses the direction of dependency: sideband no longer depends on pkt-line, and pkt-line now depends on sideband. Signed-off-by: Jonathan Tan <jonathantanmy@google.com> --- pkt-line.c | 22 ++++++++ pkt-line.h | 16 ++++++ sideband.c | 156 +++++++++++++++++++++++++---------------------------- sideband.h | 15 +++++- 4 files changed, 125 insertions(+), 84 deletions(-)