diff mbox series

[v2,2/4] sideband: reverse its dependency on pkt-line

Message ID 5aa5d047c828a0db10544c706dd595f09db3215d.1547581039.git.jonathantanmy@google.com (mailing list archive)
State New, archived
Headers show
Series [v2,1/4] pkt-line: introduce struct packet_writer | expand

Commit Message

Jonathan Tan Jan. 15, 2019, 7:40 p.m. UTC
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 demultiplex_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 |  14 ++++-
 4 files changed, 124 insertions(+), 84 deletions(-)

Comments

SZEDER Gábor Jan. 16, 2019, 10:34 a.m. UTC | #1
On Tue, Jan 15, 2019 at 11:40:28AM -0800, Jonathan Tan wrote:
> 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 demultiplex_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.

In the last couple of days I noticed occasional but frequent failures
in the test 'leading space' in 't5409-colorize-remote-messas.sh' on
'pu' and on this topic.  Bisect suggests that this patch is the
culprit, but of course bisecting an occasional test failure can't be
completely trusted.

The trace output of the failing test looks like this:

  + git --git-dir child/.git -c color.remote=always push -f origin HEAD:refs/heads/leading-space
  + cat output
  remote: error: error        
  remote: ERROR: also highlighted        
  remote: hint: hint        
  remote: hinting: not highlighted        
  remote: success: success        
  remote: warning: warning        
  remote: prefixerror: error        
  remote:   
  remote: error: leading space        
  remote:             
  remote: Err        
  To /<...>/trash directory.t5409-colorize-remote-messages.stress-4/.
   * [new branch]      HEAD -> leading-space
  + test_decode_color
  + awk 
  < ... snip enormous awk script ... >
  + grep   <BOLD;RED>error<RESET>: leading space decoded
  error: last command exited with $?=1
  not ok 6 - leading space

Notice how that "error: leading space" lines up with the other
messages.  On 'master' that line looks like this:

  remote:   error: leading space
Jonathan Tan Jan. 16, 2019, 5:43 p.m. UTC | #2
> On Tue, Jan 15, 2019 at 11:40:28AM -0800, Jonathan Tan wrote:
> > 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 demultiplex_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.
> 
> In the last couple of days I noticed occasional but frequent failures
> in the test 'leading space' in 't5409-colorize-remote-messas.sh' on
> 'pu' and on this topic.  Bisect suggests that this patch is the
> culprit, but of course bisecting an occasional test failure can't be
> completely trusted.
> 
> The trace output of the failing test looks like this:

Thanks for checking. Junio noticed a similar thing and I have replied to
it here:

https://public-inbox.org/git/20190116003828.64889-1-jonathantanmy@google.com/

I'll try to ensure that this issue is fixed in the subsequent version.
SZEDER Gábor Jan. 16, 2019, 7:17 p.m. UTC | #3
On Wed, Jan 16, 2019 at 09:43:19AM -0800, Jonathan Tan wrote:
> > On Tue, Jan 15, 2019 at 11:40:28AM -0800, Jonathan Tan wrote:
> > In the last couple of days I noticed occasional but frequent failures
> > in the test 'leading space' in 't5409-colorize-remote-messas.sh' on
 
> Thanks for checking. Junio noticed a similar thing and I have replied to
> it here:
> 
> https://public-inbox.org/git/20190116003828.64889-1-jonathantanmy@google.com/
> 
> I'll try to ensure that this issue is fixed in the subsequent version.

Oh, indeed, that must be the same issue.  After I bisected the issue I
only paid attention to the discussion of patch 2/4 in v1, and
completely ignored the replies to the cover letter.

But I'm glad that --stress makes itself useful :)
diff mbox series

Patch

diff --git a/pkt-line.c b/pkt-line.c
index 9d3e402d41..5feb73ebec 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 = demultiplex_sideband(me, buf, len);
+		switch (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;
+		}
+	}
+}
+
 /* 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..dce22d20b1 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 demultiplex_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..7244971231 100644
--- a/sideband.h
+++ b/sideband.h
@@ -3,8 +3,20 @@ 
 
 #define SIDEBAND_PROTOCOL_ERROR -2
 #define SIDEBAND_REMOTE_ERROR -1
+#define SIDEBAND_FLUSH 0
+#define SIDEBAND_PRIMARY 1
+#define SIDEBAND_PROGRESS 2
+
+/*
+ * Inspects a multiplexed packet read from the remote and returns which
+ * sideband it is for.
+ *
+ * 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 demultiplex_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