[PATCH 0/3] use constants for sideband communication channels

[PATCH 0/3] use constants for sideband communication channels

[iheffner]

In order to make more clear how the different channels in sidechannel.c
are to be used, I'm proposing some macros/constants which can be used in
place of the "magic numbers" that mean little or nothing to someone not
familiar with the protocol.

In working on a script to handle archiving from a bare repository a
project which contained submodules, I had a difficult time determining
how to talk between my git-upload-archive replacement and git-archive.
It did not occur to me to look at documentation for git-send-pack or
git-receive-pack when trying to understand the communication protocol.
But looking through the code and poking at an implementation, I finally
understood that I needed to send a channel identifier to say what type
of communication I was sending. I determined that there were three
possible channels. I could easily tell that channel three (3) was for
catastrophic errors on the server side.  But it was not clear what the
difference between channel 1 and channel 2 were.  Because channel 2 was
the one that appeared to be read and parsed in some manner, I assumed
that this was the "important" data channel and tried sending my data on
that channel.

I was confused and frustrated when it appeared that git-archive treated
data coming from my --exec'd git-upload-archive replacement differently
than it did when receiving data from the real git-upload-archive.
Eventually I figured out that channel 1 was for data, channel 2 was for
non-fatal messages, and channel 3 was for fatal error communications.
Having comments in sidechannel.h would have helped. But constants or
macros would have helped as well and makes the code that uses them more
clear.

Ivan


 [PATCH 1/3] add constants for sideband communication channels
 [PATCH 2/3] switch sideband communication to use constants
 [PATCH 3/3] use SIDEBAND_*_ERROR constants in pack protocol

 builtin/fetch-pack.c     |    2 +-
 builtin/receive-pack.c   |    6 +++---
 builtin/send-pack.c      |    4 ++--
 builtin/upload-archive.c |    6 +++---
 sideband.c               |    6 +++---
 sideband.h               |    6 +++++-
 upload-pack.c            |   22 +++++++++++-----------
 7 files changed, 28 insertions(+), 24 deletions(-)

[PATCH 1/3] add constants for sideband communication channels

[iheffner]

From: Ivan Heffner <iheffner@gmail.com>

Add constants for sideband channel identifiers.

* SIDEBAND_CHAN_PACK     => 1
* SIDEBAND_CHAN_PROGRESS => 2
* SIDEBAND_CHAN_ERROR    => 3

Signed-off-by: Ivan Heffner <iheffner@gmail.com>
---
 sideband.h |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/sideband.h b/sideband.h
index d72db35..2954979 100644
--- a/sideband.h
+++ b/sideband.h
@@ -2,7 +2,11 @@
 #define SIDEBAND_H
 
 #define SIDEBAND_PROTOCOL_ERROR -2
-#define SIDEBAND_REMOTE_ERROR -1
+#define SIDEBAND_REMOTE_ERROR   -1
+#define SIDEBAND_CHAN_PACK       1
+#define SIDEBAND_CHAN_PROGRESS   2
+#define SIDEBAND_CHAN_ERROR      3
+
 
 #define DEFAULT_PACKET_MAX 1000
 #define LARGE_PACKET_MAX 65520
-- 
1.7.6.553.g917d7.dirty

[PATCH 2/3] switch sideband communication to use constants

[iheffner]

From: Ivan Heffner <iheffner@gmail.com>

Found all uses of magic numbers for sideband channel indicators and
changed them to use the new SIDEBAND_CHAN_* constants.

Signed-off-by: Ivan Heffner <iheffner@gmail.com>
---
 builtin/receive-pack.c   |    6 +++---
 builtin/upload-archive.c |    6 +++---
 sideband.c               |    6 +++---
 upload-pack.c            |   22 +++++++++++-----------
 4 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 7ec68a1..43f7a55 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -179,7 +179,7 @@ static void report_message(const char *prefix, const char *err, va_list params)
 	msg[sz++] = '\n';
 
 	if (use_sideband)
-		send_sideband(1, 2, msg, sz, use_sideband);
+		send_sideband(1, SIDEBAND_CHAN_PROGRESS, msg, sz, use_sideband);
 	else
 		xwrite(2, msg, sz);
 }
@@ -207,7 +207,7 @@ static int copy_to_sideband(int in, int out, void *arg)
 		ssize_t sz = xread(in, data, sizeof(data));
 		if (sz <= 0)
 			break;
-		send_sideband(1, 2, data, sz, use_sideband);
+		send_sideband(1, SIDEBAND_CHAN_PROGRESS, data, sz, use_sideband);
 	}
 	close(in);
 	return 0;
@@ -851,7 +851,7 @@ static void report(struct command *commands, const char *unpack_status)
 	packet_buf_flush(&buf);
 
 	if (use_sideband)
-		send_sideband(1, 1, buf.buf, buf.len, use_sideband);
+		send_sideband(1, SIDEBAND_CHAN_PACK, buf.buf, buf.len, use_sideband);
 	else
 		safe_write(1, buf.buf, buf.len);
 	strbuf_release(&buf);
diff --git a/builtin/upload-archive.c b/builtin/upload-archive.c
index 2d0b383..4ac7984 100644
--- a/builtin/upload-archive.c
+++ b/builtin/upload-archive.c
@@ -77,7 +77,7 @@ static void error_clnt(const char *fmt, ...)
 	va_start(params, fmt);
 	len = vsprintf(buf, fmt, params);
 	va_end(params);
-	send_sideband(1, 3, buf, len, LARGE_PACKET_MAX);
+	send_sideband(1, SIDEBAND_CHAN_ERROR, buf, len, LARGE_PACKET_MAX);
 	die("sent error to the client: %s", buf);
 }
 
@@ -149,11 +149,11 @@ int cmd_upload_archive(int argc, const char **argv, const char *prefix)
 		}
 		if (pfd[1].revents & POLLIN)
 			/* Status stream ready */
-			if (process_input(pfd[1].fd, 2))
+			if (process_input(pfd[1].fd, SIDEBAND_CHAN_PROGRESS))
 				continue;
 		if (pfd[0].revents & POLLIN)
 			/* Data stream ready */
-			if (process_input(pfd[0].fd, 1))
+			if (process_input(pfd[0].fd, SIDEBAND_CHAN_PACK))
 				continue;
 
 		if (waitpid(writer, &status, 0) < 0)
diff --git a/sideband.c b/sideband.c
index d5ffa1c..4b4199a 100644
--- a/sideband.c
+++ b/sideband.c
@@ -47,12 +47,12 @@ int recv_sideband(const char *me, int in_stream, int out)
 		band = buf[pf] & 0xff;
 		len--;
 		switch (band) {
-		case 3:
+		case SIDEBAND_CHAN_ERROR:
 			buf[pf] = ' ';
 			buf[pf+1+len] = '\0';
 			fprintf(stderr, "%s\n", buf);
 			return SIDEBAND_REMOTE_ERROR;
-		case 2:
+		case SIDEBAND_CHAN_PROGRESS:
 			buf[pf] = ' ';
 			do {
 				char *b = buf;
@@ -107,7 +107,7 @@ int recv_sideband(const char *me, int in_stream, int out)
 				memmove(buf + pf+1, b + brk, len);
 			} while (len);
 			continue;
-		case 1:
+		case SIDEBAND_CHAN_PACK:
 			safe_write(out, buf + pf+1, len);
 			continue;
 		default:
diff --git a/upload-pack.c b/upload-pack.c
index 470cffd..98c2542 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -56,19 +56,19 @@ static int strip(char *line, int len)
 	return len;
 }
 
-static ssize_t send_client_data(int fd, const char *data, ssize_t sz)
+static ssize_t send_client_data(int chan, const char *data, ssize_t sz)
 {
 	if (use_sideband)
-		return send_sideband(1, fd, data, sz, use_sideband);
-	if (fd == 3)
+		return send_sideband(1, chan, data, sz, use_sideband);
+	if (chan == SIDEBAND_CHAN_ERROR)
 		/* emergency quit */
-		fd = 2;
-	if (fd == 2) {
+		chan = SIDEBAND_CHAN_PROGRESS;
+	if (chan == SIDEBAND_CHAN_PROGRESS) {
 		/* XXX: are we happy to lose stuff here? */
-		xwrite(fd, data, sz);
+		xwrite(chan, data, sz);
 		return sz;
 	}
-	return safe_write(fd, data, sz);
+	return safe_write(chan, data, sz);
 }
 
 static FILE *pack_pipe = NULL;
@@ -243,7 +243,7 @@ static void create_pack_file(void)
 			sz = xread(pack_objects.err, progress,
 				  sizeof(progress));
 			if (0 < sz)
-				send_client_data(2, progress, sz);
+				send_client_data(SIDEBAND_CHAN_PROGRESS, progress, sz);
 			else if (sz == 0) {
 				close(pack_objects.err);
 				pack_objects.err = -1;
@@ -286,7 +286,7 @@ static void create_pack_file(void)
 			}
 			else
 				buffered = -1;
-			sz = send_client_data(1, data, sz);
+			sz = send_client_data(SIDEBAND_CHAN_PACK, data, sz);
 			if (sz < 0)
 				goto fail;
 		}
@@ -302,7 +302,7 @@ static void create_pack_file(void)
 	/* flush the data */
 	if (0 <= buffered) {
 		data[0] = buffered;
-		sz = send_client_data(1, data, 1);
+		sz = send_client_data(SIDEBAND_CHAN_PACK, data, 1);
 		if (sz < 0)
 			goto fail;
 		fprintf(stderr, "flushed.\n");
@@ -312,7 +312,7 @@ static void create_pack_file(void)
 	return;
 
  fail:
-	send_client_data(3, abort_msg, sizeof(abort_msg));
+	send_client_data(SIDEBAND_CHAN_ERROR, abort_msg, sizeof(abort_msg));
 	die("git upload-pack: %s", abort_msg);
 }
 
-- 
1.7.6.553.g917d7.dirty

[PATCH 3/3] use SIDEBAND_*_ERROR constants in pack protocol

[iheffner]

From: Ivan Heffner <iheffner@gmail.com>

Switched calls to send_sideband() for pack protocol errors to use
SIDEBAND_PROTOCOL_ERROR and SIDEBAND_REMOTE_ERROR in place of the
sideband magic numbers of -2 and -1, respectively.

Signed-off-by: Ivan Heffner <iheffner@gmail.com>
---
 builtin/fetch-pack.c |    2 +-
 builtin/send-pack.c  |    4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index c6bc8eb..63e9ac4 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -245,7 +245,7 @@ static enum ack_type get_ack(int fd, unsigned char *result_sha1)
 static void send_request(int fd, struct strbuf *buf)
 {
 	if (args.stateless_rpc) {
-		send_sideband(fd, -1, buf->buf, buf->len, LARGE_PACKET_MAX);
+		send_sideband(fd, SIDEBAND_REMOTE_ERROR, buf->buf, buf->len, LARGE_PACKET_MAX);
 		packet_flush(fd);
 	} else
 		safe_write(fd, buf->buf, buf->len);
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index e0b8030..67c9fe5 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -96,7 +96,7 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *ext
 			ssize_t n = xread(po.out, buf, LARGE_PACKET_MAX);
 			if (n <= 0)
 				break;
-			send_sideband(fd, -1, buf, n, LARGE_PACKET_MAX);
+			send_sideband(fd, SIDEBAND_REMOTE_ERROR, buf, n, LARGE_PACKET_MAX);
 		}
 		free(buf);
 		close(po.out);
@@ -320,7 +320,7 @@ int send_pack(struct send_pack_args *args,
 	if (args->stateless_rpc) {
 		if (!args->dry_run && cmds_sent) {
 			packet_buf_flush(&req_buf);
-			send_sideband(out, -1, req_buf.buf, req_buf.len, LARGE_PACKET_MAX);
+			send_sideband(out, SIDEBAND_REMOTE_ERROR, req_buf.buf, req_buf.len, LARGE_PACKET_MAX);
 		}
 	} else {
 		safe_write(out, req_buf.buf, req_buf.len);
-- 
1.7.6.553.g917d7.dirty

Re: [PATCH 0/3] use constants for sideband communication channels

[Junio C Hamano]

iheffner@gmail.com writes:

> In order to make more clear how the different channels in sidechannel.c
> are to be used, I'm proposing some macros/constants which can be used in
> place of the "magic numbers" that mean little or nothing to someone not
> familiar with the protocol.

I am not fundamentally opposed to the stated goal, but the posted patches
make the resulting code way too wide for comfortable reading. Can we use a
bit shorter symbols?

Perhaps a good way to start would be to first refrain from using these
symbols, but give a prominent comment near the API functions that are used
to send and receive sideband data to explain which band is used for what
purpose, which should be enough for people who are writing the code to
link with these functions.