[PATCH 1/2] archive: allow remote to have more formats than we understand.

[PATCH 1/2] archive: allow remote to have more formats than we understand.

[Junio C Hamano]

This fixes git-archive --remote not to parse archiver arguments;
otherwise if the remote end implements formats other than the
one known locally we will not be able to access that format.

Signed-off-by: Junio C Hamano <junkio@cox.net>
---
 * At first sight, this should not matter that much, but (1) we
   do not really parse and validate the arguments when dealing
   with remote site, and (2) we have no way validating them if
   we wanted to, given that the remote end might be running
   different version of git.

   Having said that, you would realize that once we start
   refactoring things this way, "git archive --remote=foo" is
   not archive driver anymore.  There is nothing that prevents
   us saying "git archive --remote=foo --command=rev-list HEAD",
   other than that the remote archive protocol insists the
   command invoked at the remote end to be "git archive" itself.

 archive.h         |    1 -
 builtin-archive.c |   79 ++++++++++++++++++++++++++++++++---------------------
 2 files changed, 47 insertions(+), 33 deletions(-)

diff --git a/archive.h b/archive.h
index d8cca73..e0782b9 100644
--- a/archive.h
+++ b/archive.h
@@ -19,7 +19,6 @@ typedef void *(*parse_extra_args_fn_t)(i
 
 struct archiver {
 	const char *name;
-	const char *remote;
 	struct archiver_args args;
 	write_archive_fn_t write_archive;
 	parse_extra_args_fn_t parse_extra;
diff --git a/builtin-archive.c b/builtin-archive.c
index b944737..c70488c 100644
--- a/builtin-archive.c
+++ b/builtin-archive.c
@@ -26,7 +26,7 @@ struct archiver archivers[] = {
 	},
 };
 
-static int run_remote_archiver(struct archiver *ar, int argc,
+static int run_remote_archiver(const char *remote, int argc,
 			       const char **argv)
 {
 	char *url, buf[1024];
@@ -35,16 +35,13 @@ static int run_remote_archiver(struct ar
 
 	sprintf(buf, "git-upload-archive");
 
-	url = xstrdup(ar->remote);
+	url = xstrdup(remote);
 	pid = git_connect(fd, url, buf);
 	if (pid < 0)
 		return pid;
 
-	for (i = 1; i < argc; i++) {
-		if (!strncmp(argv[i], "--remote=", 9))
-			continue;
+	for (i = 1; i < argc; i++)
 		packet_write(fd[1], "argument %s\n", argv[i]);
-	}
 	packet_flush(fd[1]);
 
 	len = packet_read_line(fd[0], buf, sizeof(buf));
@@ -150,17 +147,16 @@ int parse_archive_args(int argc, const c
 	const char *extra_argv[MAX_EXTRA_ARGS];
 	int extra_argc = 0;
 	const char *format = NULL; /* might want to default to "tar" */
-	const char *remote = NULL;
 	const char *base = "";
-	int list = 0;
 	int i;
 
 	for (i = 1; i < argc; i++) {
 		const char *arg = argv[i];
 
 		if (!strcmp(arg, "--list") || !strcmp(arg, "-l")) {
-			list = 1;
-			continue;
+			for (i = 0; i < ARRAY_SIZE(archivers); i++)
+				printf("%s\n", archivers[i].name);
+			exit(0);
 		}
 		if (!strncmp(arg, "--format=", 9)) {
 			format = arg + 9;
@@ -170,10 +166,6 @@ int parse_archive_args(int argc, const c
 			base = arg + 9;
 			continue;
 		}
-		if (!strncmp(arg, "--remote=", 9)) {
-			remote = arg + 9;
-			continue;
-		}
 		if (!strcmp(arg, "--")) {
 			i++;
 			break;
@@ -187,44 +179,67 @@ int parse_archive_args(int argc, const c
 		break;
 	}
 
-	if (list) {
-		if (!remote) {
-			for (i = 0; i < ARRAY_SIZE(archivers); i++)
-				printf("%s\n", archivers[i].name);
-			exit(0);
-		}
-		die("--list and --remote are mutually exclusive");
-	}
-
-	if (argc - i < 1)
+	/* We need at least one parameter -- tree-ish */
+	if (argc - 1 < i)
 		usage(archive_usage);
 	if (!format)
 		die("You must specify an archive format");
 	if (init_archiver(format, ar) < 0)
 		die("Unknown archive format '%s'", format);
 
-	if (extra_argc && !remote) {
-		if (!ar->parse_extra) {
+	if (extra_argc) {
+		if (!ar->parse_extra)
 			die("%s", default_parse_extra(ar, extra_argv));
-		}
 		ar->args.extra = ar->parse_extra(extra_argc, extra_argv);
 	}
-	ar->remote = remote;
 	ar->args.base = base;
 
 	return i;
 }
 
+static const char *remote_request(int *ac, const char **av)
+{
+	int ix, iy, cnt = *ac;
+	int no_more_options = 0;
+	const char *remote = NULL;
+
+	for (ix = iy = 1; ix < cnt; ix++) {
+		const char *arg = av[ix];
+		if (!strcmp(arg, "--"))
+			no_more_options = 1;
+		if (!no_more_options) {
+			if (!strncmp(arg, "--remote=", 9)) {
+				if (remote)
+					die("Multiple --remote specified");
+				remote = arg + 9;
+				continue;
+			}
+			if (arg[0] != '-')
+				no_more_options = 1;
+		}
+		if (ix != iy)
+			av[iy] = arg;
+		iy++;
+	}
+	if (remote) {
+		av[--cnt] = NULL;
+		*ac = cnt;
+	}
+	return remote;
+}
+
 int cmd_archive(int argc, const char **argv, const char *prefix)
 {
 	struct archiver ar;
 	int tree_idx;
+	const char *remote = NULL;
 
-	tree_idx = parse_archive_args(argc, argv, &ar);
-
-	if (ar.remote)
-		return run_remote_archiver(&ar, argc, argv);
+	remote = remote_request(&argc, argv);
+	if (remote)
+		return run_remote_archiver(remote, argc, argv);
 
+	memset(&ar, 0, sizeof(ar));
+	tree_idx = parse_archive_args(argc, argv, &ar);
 	if (prefix == NULL)
 		prefix = setup_git_directory();
 
-- 
1.4.2.gc52f

[PATCH 2/2] Add --verbose to git-archive

[Junio C Hamano]

And teach backends about it.

Signed-off-by: Junio C Hamano <junkio@cox.net>
---
 * This is not interesting yet, and the output is discarded if
   you are running the remote archiver against git-daemon, but
   would be a useful progress indicator when we implement the
   side-band in git-archive protocol, which is probably a
   requirement before this "git-archive" series can graduate to
   "master" branch.

 archive.h          |    1 +
 builtin-archive.c  |    8 +++++++-
 builtin-tar-tree.c |    4 ++++
 builtin-zip-tree.c |    4 ++++
 4 files changed, 16 insertions(+), 1 deletions(-)

diff --git a/archive.h b/archive.h
index e0782b9..16dcdb8 100644
--- a/archive.h
+++ b/archive.h
@@ -10,6 +10,7 @@ struct archiver_args {
 	const unsigned char *commit_sha1;
 	time_t time;
 	const char **pathspec;
+	unsigned int verbose : 1;
 	void *extra;
 };
 
diff --git a/builtin-archive.c b/builtin-archive.c
index c70488c..b78d6d8 100644
--- a/builtin-archive.c
+++ b/builtin-archive.c
@@ -12,7 +12,7 @@ #include "exec_cmd.h"
 #include "pkt-line.h"
 
 static const char archive_usage[] = \
-"git-archive --format=<fmt> [--prefix=<prefix>/] [<extra>] <tree-ish> [path...]";
+"git-archive --format=<fmt> [--prefix=<prefix>/] [--verbose] [<extra>] <tree-ish> [path...]";
 
 struct archiver archivers[] = {
 	{
@@ -148,6 +148,7 @@ int parse_archive_args(int argc, const c
 	int extra_argc = 0;
 	const char *format = NULL; /* might want to default to "tar" */
 	const char *base = "";
+	int verbose = 0;
 	int i;
 
 	for (i = 1; i < argc; i++) {
@@ -158,6 +159,10 @@ int parse_archive_args(int argc, const c
 				printf("%s\n", archivers[i].name);
 			exit(0);
 		}
+		if (!strcmp(arg, "--verbose") || !strcmp(arg, "-v")) {
+			verbose = 1;
+			continue;
+		}
 		if (!strncmp(arg, "--format=", 9)) {
 			format = arg + 9;
 			continue;
@@ -192,6 +197,7 @@ int parse_archive_args(int argc, const c
 			die("%s", default_parse_extra(ar, extra_argv));
 		ar->args.extra = ar->parse_extra(extra_argc, extra_argv);
 	}
+	ar->args.verbose = verbose;
 	ar->args.base = base;
 
 	return i;
diff --git a/builtin-tar-tree.c b/builtin-tar-tree.c
index c20eb0e..fae2c0b 100644
--- a/builtin-tar-tree.c
+++ b/builtin-tar-tree.c
@@ -22,6 +22,7 @@ static unsigned long offset;
 
 static time_t archive_time;
 static int tar_umask;
+static int verbose;
 
 /* writes out the whole block, but only if it is full */
 static void write_if_needed(void)
@@ -169,6 +170,8 @@ static void write_entry(const unsigned c
 		mode = 0100666;
 		sprintf(header.name, "%s.paxheader", sha1_to_hex(sha1));
 	} else {
+		if (verbose)
+			fprintf(stderr, "%.*s\n", path->len, path->buf);
 		if (S_ISDIR(mode)) {
 			*header.typeflag = TYPEFLAG_DIR;
 			mode = (mode | 0777) & ~tar_umask;
@@ -385,6 +388,7 @@ int write_tar_archive(struct archiver_ar
 	git_config(git_tar_config);
 
 	archive_time = args->time;
+	verbose = args->verbose;
 
 	if (args->commit_sha1)
 		write_global_extended_header(args->commit_sha1);
diff --git a/builtin-zip-tree.c b/builtin-zip-tree.c
index 4e79633..0ebd547 100644
--- a/builtin-zip-tree.c
+++ b/builtin-zip-tree.c
@@ -13,6 +13,7 @@ #include "archive.h"
 static const char zip_tree_usage[] =
 "git-zip-tree [-0|...|-9] <tree-ish> [ <base> ]";
 
+static int verbose;
 static int zip_date;
 static int zip_time;
 
@@ -164,6 +165,8 @@ static int write_zip_entry(const unsigne
 	crc = crc32(0, Z_NULL, 0);
 
 	path = construct_path(base, baselen, filename, S_ISDIR(mode), &pathlen);
+	if (verbose)
+		fprintf(stderr, "%s\n", path);
 	if (pathlen > 0xffff) {
 		error("path too long (%d chars, SHA1: %s): %s", pathlen,
 		      sha1_to_hex(sha1), path);
@@ -361,6 +364,7 @@ int write_zip_archive(struct archiver_ar
 
 	zip_dir = xmalloc(ZIP_DIRECTORY_MIN_SIZE);
 	zip_dir_size = ZIP_DIRECTORY_MIN_SIZE;
+	verbose = args->verbose;
 
 	if (args->base && plen > 0 && args->base[plen - 1] == '/') {
 		char *base = strdup(args->base);
-- 
1.4.2.gc52f

[PATCH 1/3] Move sideband client side support into reusable form.

[Junio C Hamano]

This moves the receiver side of the sideband support from
fetch-clone.c to sideband.c and its header file, so that
archiver protocol can use it.

Signed-off-by: Junio C Hamano <junkio@cox.net>
---
 * The next one refactors the upload side.

 Makefile      |    4 ++--
 fetch-clone.c |   32 +++++---------------------------
 sideband.c    |   48 ++++++++++++++++++++++++++++++++++++++++++++++++
 sideband.h    |   11 +++++++++++
 4 files changed, 66 insertions(+), 29 deletions(-)

diff --git a/Makefile b/Makefile
index 4ac85fd..c724b48 100644
--- a/Makefile
+++ b/Makefile
@@ -233,7 +233,7 @@ XDIFF_LIB=xdiff/lib.a
 
 LIB_H = \
 	archive.h blob.h cache.h commit.h csum-file.h delta.h \
-	diff.h object.h pack.h pkt-line.h quote.h refs.h \
+	diff.h object.h pack.h pkt-line.h quote.h refs.h sideband.h \
 	run-command.h strbuf.h tag.h tree.h git-compat-util.h revision.h \
 	tree-walk.h log-tree.h dir.h path-list.h unpack-trees.h builtin.h
 
@@ -245,7 +245,7 @@ DIFF_OBJS = \
 LIB_OBJS = \
 	blob.o commit.o connect.o csum-file.o cache-tree.o base85.o \
 	date.o diff-delta.o entry.o exec_cmd.o ident.o lockfile.o \
-	object.o pack-check.o patch-delta.o path.o pkt-line.o \
+	object.o pack-check.o patch-delta.o path.o pkt-line.o sideband.o \
 	quote.o read-cache.o refs.o run-command.o dir.o object-refs.o \
 	server-info.o setup.o sha1_file.o sha1_name.o strbuf.o \
 	tag.o tree.o usage.o config.o environment.o ctype.o copy.o \
diff --git a/fetch-clone.c b/fetch-clone.c
index c5cf477..b62feac 100644
--- a/fetch-clone.c
+++ b/fetch-clone.c
@@ -1,6 +1,7 @@
 #include "cache.h"
 #include "exec_cmd.h"
 #include "pkt-line.h"
+#include "sideband.h"
 #include <sys/wait.h>
 #include <sys/time.h>
 
@@ -114,36 +115,13 @@ static pid_t setup_sideband(int sideband
 		die("%s: unable to fork off sideband demultiplexer", me);
 	if (!side_pid) {
 		/* subprocess */
+		char buf[DEFAULT_PACKET_MAX];
+
 		close(fd[0]);
 		if (xd[0] != xd[1])
 			close(xd[1]);
-		while (1) {
-			char buf[1024];
-			int len = packet_read_line(xd[0], buf, sizeof(buf));
-			if (len == 0)
-				break;
-			if (len < 1)
-				die("%s: protocol error: no band designator",
-				    me);
-			len--;
-			switch (buf[0] & 0xFF) {
-			case 3:
-				safe_write(2, "remote: ", 8);
-				safe_write(2, buf+1, len);
-				safe_write(2, "\n", 1);
-				exit(1);
-			case 2:
-				safe_write(2, "remote: ", 8);
-				safe_write(2, buf+1, len);
-				continue;
-			case 1:
-				safe_write(fd[1], buf+1, len);
-				continue;
-			default:
-				die("%s: protocol error: bad band #%d",
-				    me, (buf[0] & 0xFF));
-			}
-		}
+		if (recv_sideband(me, xd[0], fd[1], 2, buf, sizeof(buf)))
+			exit(1);
 		exit(0);
 	}
 	close(xd[0]);
diff --git a/sideband.c b/sideband.c
new file mode 100644
index 0000000..861f621
--- /dev/null
+++ b/sideband.c
@@ -0,0 +1,48 @@
+#include "pkt-line.h"
+#include "sideband.h"
+
+/*
+ * 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.
+ */
+int recv_sideband(const char *me, int in_stream, int out, int err, char *buf, int bufsz)
+{
+	while (1) {
+		int len = packet_read_line(in_stream, buf, bufsz);
+		if (len == 0)
+			break;
+		if (len < 1) {
+			len = sprintf(buf, "%s: protocol error: no band designator\n", me);
+			safe_write(err, buf, len);
+			return SIDEBAND_PROTOCOL_ERROR;
+		}
+		len--;
+		switch (buf[0] & 0xFF) {
+		case 3:
+			safe_write(err, "remote: ", 8);
+			safe_write(err, buf+1, len);
+			safe_write(err, "\n", 1);
+			return SIDEBAND_REMOTE_ERROR;
+		case 2:
+			safe_write(err, "remote: ", 8);
+			safe_write(err, buf+1, len);
+			continue;
+		case 1:
+			safe_write(out, buf+1, len);
+			continue;
+		default:
+			len = sprintf(buf + 1,
+				      "%s: protocol error: bad band #%d\n",
+				      me, buf[0] & 0xFF);
+			safe_write(err, buf+1, len);
+			return SIDEBAND_PROTOCOL_ERROR;
+		}
+	}
+	return 0;
+}
diff --git a/sideband.h b/sideband.h
new file mode 100644
index 0000000..90b3855
--- /dev/null
+++ b/sideband.h
@@ -0,0 +1,11 @@
+#ifndef SIDEBAND_H
+#define SIDEBAND_H
+
+#define SIDEBAND_PROTOCOL_ERROR -2
+#define SIDEBAND_REMOTE_ERROR -1
+
+#define DEFAULT_PACKET_MAX 1000
+
+int recv_sideband(const char *me, int in_stream, int out, int err, char *, int);
+
+#endif
-- 
1.4.2.gc52f

Re: [PATCH 1/3] Move sideband client side support into reusable form.

[Franck Bui-Huu]

2006/9/10, Junio C Hamano <junkio@cox.net>:
> This moves the receiver side of the sideband support from
> fetch-clone.c to sideband.c and its header file, so that
> archiver protocol can use it.
>

looks good, that's what I was doing but you beat me.

-- 
               Franck

[PATCH] Move sideband server side support into reusable form.

[Junio C Hamano]

The server side support; this is just the very low level, and the
caller needs to know which band it wants to send things out.

Signed-off-by: Junio C Hamano <junkio@cox.net>
---
 * With the previous downloader-side support, this should make
   it easier to add the status notification from git-archive
   --remote over git native transport (aka git-daemon).

 sideband.c    |   26 ++++++++++++++++++++++++++
 sideband.h    |    1 +
 upload-pack.c |   50 +++++++++++++-------------------------------------
 3 files changed, 40 insertions(+), 37 deletions(-)

diff --git a/sideband.c b/sideband.c
index 861f621..1b14ff8 100644
--- a/sideband.c
+++ b/sideband.c
@@ -46,3 +46,29 @@ int recv_sideband(const char *me, int in
 	}
 	return 0;
 }
+
+/*
+ * fd is connected to the remote side; send the sideband data
+ * over multiplexed packet stream.
+ */
+ssize_t send_sideband(int fd, int band, const char *data, ssize_t sz, int packet_max)
+{
+	ssize_t ssz = sz;
+	const char *p = data;
+
+	while (sz) {
+		unsigned n;
+		char hdr[5];
+
+		n = sz;
+		if (packet_max - 5 < n)
+			n = packet_max - 5;
+		sprintf(hdr, "%04x", n + 5);
+		hdr[4] = band;
+		safe_write(fd, hdr, 5);
+		safe_write(fd, p, n);
+		p += n;
+		sz -= n;
+	}
+	return ssz;
+}
diff --git a/sideband.h b/sideband.h
index 90b3855..c645cf2 100644
--- a/sideband.h
+++ b/sideband.h
@@ -7,5 +7,6 @@ #define SIDEBAND_REMOTE_ERROR -1
 #define DEFAULT_PACKET_MAX 1000
 
 int recv_sideband(const char *me, int in_stream, int out, int err, char *, int);
+ssize_t send_sideband(int fd, int band, const char *data, ssize_t sz, int packet_max);
 
 #endif
diff --git a/upload-pack.c b/upload-pack.c
index 51ce936..1f2f7f7 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -4,6 +4,7 @@ #include <sys/poll.h>
 #include "cache.h"
 #include "refs.h"
 #include "pkt-line.h"
+#include "sideband.h"
 #include "tag.h"
 #include "object.h"
 #include "commit.h"
@@ -33,45 +34,19 @@ static int strip(char *line, int len)
 	return len;
 }
 
-#define PACKET_MAX 1000
 static ssize_t send_client_data(int fd, const char *data, ssize_t sz)
 {
-	ssize_t ssz;
-	const char *p;
-
-	if (!data) {
-		if (!use_sideband)
-			return 0;
-		packet_flush(1);
-	}
-
-	if (!use_sideband) {
-		if (fd == 3)
-			/* emergency quit */
-			fd = 2;
-		if (fd == 2) {
-			xwrite(fd, data, sz);
-			return sz;
-		}
-		return safe_write(fd, data, sz);
-	}
-	p = data;
-	ssz = sz;
-	while (sz) {
-		unsigned n;
-		char hdr[5];
-
-		n = sz;
-		if (PACKET_MAX - 5 < n)
-			n = PACKET_MAX - 5;
-		sprintf(hdr, "%04x", n + 5);
-		hdr[4] = fd;
-		safe_write(1, hdr, 5);
-		safe_write(1, p, n);
-		p += n;
-		sz -= n;
+	if (use_sideband)
+		return send_sideband(1, fd, data, sz, DEFAULT_PACKET_MAX);
+
+	if (fd == 3)
+		/* emergency quit */
+		fd = 2;
+	if (fd == 2) {
+		xwrite(fd, data, sz);
+		return sz;
 	}
-	return ssz;
+	return safe_write(fd, data, sz);
 }
 
 static void create_pack_file(void)
@@ -308,7 +283,8 @@ static void create_pack_file(void)
 				goto fail;
 			fprintf(stderr, "flushed.\n");
 		}
-		send_client_data(1, NULL, 0);
+		if (use_sideband)
+			packet_flush(1);
 		return;
 	}
  fail:
-- 
1.4.2.gc52f

[PATCH 3/3] Add sideband status report to git-archive protocol

[Junio C Hamano]

Using the refactored sideband code from existing upload-pack protocol,
this lets the error condition and status output sent from the remote
process to be shown locally.

Signed-off-by: Junio C Hamano <junkio@cox.net>
---

 * This does it in a "stupid" way.  Ideally, write(2) to fd 1
   and 2 in each archiver backend could be wrapped with
   something like upload-pack.c::send_client_data() that does
   straight write(2) to the original destination when it is not
   driven by upload-pack, or use send_sideband() when it is, and
   that way we can lose two pipes and the multiplexer process.

   The sending side of the upload-pack protocol also does this
   in the stupid way, but it has an excuse.  It needs to set
   up a complex pipe and spawn a subprocess (pack-objects) that
   does not even want to know about send_client_data().

   What is driven by upload-archive() does not have to be a
   subprocess (it is just a single ar.write_archive() function
   call away), so having to do an extra fork for the multiplexer
   is a bit sad...

 builtin-archive.c        |   24 ++++++++++--
 builtin-upload-archive.c |   92 +++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 108 insertions(+), 8 deletions(-)

diff --git a/builtin-archive.c b/builtin-archive.c
index b78d6d8..0c2ad49 100644
--- a/builtin-archive.c
+++ b/builtin-archive.c
@@ -10,6 +10,7 @@ #include "commit.h"
 #include "tree-walk.h"
 #include "exec_cmd.h"
 #include "pkt-line.h"
+#include "sideband.h"
 
 static const char archive_usage[] = \
 "git-archive --format=<fmt> [--prefix=<prefix>/] [--verbose] [<extra>] <tree-ish> [path...]";
@@ -32,16 +33,30 @@ static int run_remote_archiver(const cha
 	char *url, buf[1024];
 	int fd[2], i, len, rv;
 	pid_t pid;
+	const char *exec = "git-upload-archive";
+	int exec_at = 0;
 
-	sprintf(buf, "git-upload-archive");
+	for (i = 1; i < argc; i++) {
+		const char *arg = argv[i];
+		if (!strncmp("--exec=", arg, 7)) {
+			if (exec_at)
+				die("multiple --exec specified");
+			exec = arg + 7;
+			exec_at = i;
+			break;
+		}
+	}
 
 	url = xstrdup(remote);
-	pid = git_connect(fd, url, buf);
+	pid = git_connect(fd, url, exec);
 	if (pid < 0)
 		return pid;
 
-	for (i = 1; i < argc; i++)
+	for (i = 1; i < argc; i++) {
+		if (i == exec_at)
+			continue;
 		packet_write(fd[1], "argument %s\n", argv[i]);
+	}
 	packet_flush(fd[1]);
 
 	len = packet_read_line(fd[0], buf, sizeof(buf));
@@ -60,8 +75,7 @@ static int run_remote_archiver(const cha
 		die("git-archive: expected a flush");
 
 	/* Now, start reading from fd[0] and spit it out to stdout */
-	rv = copy_fd(fd[0], 1);
-
+	rv = recv_sideband("archive", fd[0], 1, 2, buf, sizeof(buf));
 	close(fd[0]);
 	rv |= finish_connect(pid);
 
diff --git a/builtin-upload-archive.c b/builtin-upload-archive.c
index 3bdb607..96f96bd 100644
--- a/builtin-upload-archive.c
+++ b/builtin-upload-archive.c
@@ -6,12 +6,18 @@ #include "cache.h"
 #include "builtin.h"
 #include "archive.h"
 #include "pkt-line.h"
+#include "sideband.h"
+#include <sys/wait.h>
+#include <sys/poll.h>
 
 static const char upload_archive_usage[] =
 	"git-upload-archive <repo>";
 
+static const char deadchild[] =
+"git-upload-archive: archiver died with error";
 
-int cmd_upload_archive(int argc, const char **argv, const char *prefix)
+
+static int run_upload_archive(int argc, const char **argv, const char *prefix)
 {
 	struct archiver ar;
 	const char *sent_argv[MAX_ARGS];
@@ -64,9 +70,89 @@ int cmd_upload_archive(int argc, const c
 	parse_treeish_arg(sent_argv + treeish_idx, &ar.args, prefix);
 	parse_pathspec_arg(sent_argv + treeish_idx + 1, &ar.args);
 
+	return ar.write_archive(&ar.args);
+}
+
+int cmd_upload_archive(int argc, const char **argv, const char *prefix)
+{
+	pid_t writer;
+	int fd1[2], fd2[2];
+	/*
+	 * Set up sideband subprocess.
+	 *
+	 * We (parent) monitor and read from child, sending its fd#1 and fd#2
+	 * multiplexed out to our fd#1.  If the child dies, we tell the other
+	 * end over channel #3.
+	 */
+	if (pipe(fd1) < 0 || pipe(fd2) < 0) {
+		int err = errno;
+		packet_write(1, "NACK pipe failed on the remote side\n");
+		die("upload-archive: %s", strerror(err));
+	}
+	writer = fork();
+	if (writer < 0) {
+		int err = errno;
+		packet_write(1, "NACK fork failed on the remote side\n");
+		die("upload-archive: %s", strerror(err));
+	}
+	if (!writer) {
+		/* child - connect fd#1 and fd#2 to the pipe */
+		dup2(fd1[1], 1);
+		dup2(fd2[1], 2);
+		close(fd1[1]); close(fd2[1]);
+		close(fd1[0]); close(fd2[0]); /* we do not read from pipe */
+
+		exit(run_upload_archive(argc, argv, prefix));
+	}
+
+	/* parent - read from child, multiplex and send out to fd#1 */
+	close(fd1[1]); close(fd2[1]); /* we do not write to pipe */
 	packet_write(1, "ACK\n");
 	packet_flush(1);
 
-	return ar.write_archive(&ar.args);
-}
+	while (1) {
+		struct pollfd pfd[2];
+		char buf[16384];
+		ssize_t sz;
+		pid_t pid;
+		int status;
+
+		pfd[0].fd = fd1[0];
+		pfd[0].events = POLLIN;
+		pfd[1].fd = fd2[0];
+		pfd[1].events = POLLIN;
+		if (poll(pfd, 2, -1) < 0) {
+			if (errno != EINTR) {
+				error("poll failed resuming: %s",
+				      strerror(errno));
+				sleep(1);
+			}
+			continue;
+		}
+		if (pfd[0].revents & (POLLIN|POLLHUP)) {
+			/* Data stream ready */
+			sz = read(pfd[0].fd, buf, sizeof(buf));
+			send_sideband(1, 1, buf, sz, DEFAULT_PACKET_MAX);
+		}
+		if (pfd[1].revents & (POLLIN|POLLHUP)) {
+			/* Status stream ready */
+			sz = read(pfd[1].fd, buf, sizeof(buf));
+			send_sideband(1, 2, buf, sz, DEFAULT_PACKET_MAX);
+		}
 
+		if (((pfd[0].revents | pfd[1].revents) & POLLHUP) == 0)
+			continue;
+		/* did it die? */
+		pid = waitpid(writer, &status, WNOHANG);
+		if (!pid) {
+			fprintf(stderr, "Hmph, HUP?\n");
+			continue;
+		}
+		if (!WIFEXITED(status) || WEXITSTATUS(status) > 0)
+			send_sideband(1, 3, deadchild, strlen(deadchild),
+				      DEFAULT_PACKET_MAX);
+		packet_flush(1);
+		break;
+	}
+	return 0;
+}
-- 
1.4.2.gc52f

[PATCH] git-upload-archive: add config option to allow only specified formats

[Rene Scharfe]

 Documentation/config.txt |    5 +++++
 builtin-upload-archive.c |   39 +++++++++++++++++++++++++++++++++++++++
 daemon.c                 |    2 ++
 3 files changed, 46 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index ce722a2..5c3c6c7 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -236,6 +236,11 @@ tar.umask::
 	the same permissions as gitlink:git-checkout[1] would use. The default
 	value remains 0, which means world read-write.
 
+uploadarchive.daemonformats::
+	A comma-separated list of the git-archive formats allowed for upload
+	via git-daemon.  If this parameter is missing all formats are allowed
+	for upload.
+
 user.email::
 	Your email address to be recorded in any newly created commits.
 	Can be overridden by the 'GIT_AUTHOR_EMAIL' and 'GIT_COMMITTER_EMAIL'
diff --git a/builtin-upload-archive.c b/builtin-upload-archive.c
index 96f96bd..6a5245a 100644
--- a/builtin-upload-archive.c
+++ b/builtin-upload-archive.c
@@ -16,6 +16,37 @@ static const char upload_archive_usage[]
 static const char deadchild[] =
 "git-upload-archive: archiver died with error";
 
+static char *daemon_formats;
+
+static int upload_format_config(const char *var, const char *value)
+{
+	if (!strcmp(var, "uploadarchive.daemonformats"))
+		daemon_formats = xstrdup(value);
+	return 0;
+}
+
+static int is_in(const char *needle, const char *haystack, const char *delim)
+{
+	int len = strlen(needle);
+	const char *search = haystack;
+
+	for (;;) {
+		char *pos = strstr(search, needle);
+		if (!pos)
+			return 0;
+		search++;
+		if ((pos == haystack || strchr(delim, pos[-1])) &&
+		    (pos[len] == '\0' || strchr(delim, pos[len])))
+			return 1;
+	}
+}
+
+static int upload_format_allowed(const char *fmt)
+{
+	if (getenv("GIT_DAEMON"))
+		return daemon_formats ? is_in(fmt, daemon_formats, " \t,") : 1;
+	return 1;
+}
 
 static int run_upload_archive(int argc, const char **argv, const char *prefix)
 {
@@ -38,6 +69,8 @@ static int run_upload_archive(int argc, 
 	if (!enter_repo(buf, 0))
 		die("not a git archive");
 
+	git_config(upload_format_config);
+
 	/* put received options in sent_argv[] */
 	sent_argc = 1;
 	sent_argv[0] = "git-upload-archive";
@@ -67,6 +100,12 @@ static int run_upload_archive(int argc, 
 	/* parse all options sent by the client */
 	treeish_idx = parse_archive_args(sent_argc, sent_argv, &ar);
 
+	if (!upload_format_allowed(ar.name)) {
+		free(daemon_formats);
+		die("upload of %s format forbidden\n", ar.name);
+	}
+	free(daemon_formats);
+
 	parse_treeish_arg(sent_argv + treeish_idx, &ar.args, prefix);
 	parse_pathspec_arg(sent_argv + treeish_idx + 1, &ar.args);
 
diff --git a/daemon.c b/daemon.c
index a2954a0..2d58abe 100644
--- a/daemon.c
+++ b/daemon.c
@@ -304,6 +304,8 @@ static int run_service(char *dir, struct
 		return -1;
 	}
 
+	setenv("GIT_DAEMON", "I am your father.", 1);
+
 	/*
 	 * We'll ignore SIGTERM from now on, we have a
 	 * good client.

Re: [PATCH] git-upload-archive: add config option to allow only specified formats

[Rene Scharfe]

*sigh* The patch contained in the parent message is:

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>

Re: [PATCH] git-upload-archive: add config option to allow only specified formats

[Junio C Hamano]

Rene Scharfe <rene.scharfe@lsrfire.ath.cx> writes:

>  Documentation/config.txt |    5 +++++
>  builtin-upload-archive.c |   39 +++++++++++++++++++++++++++++++++++++++
>  daemon.c                 |    2 ++
>  3 files changed, 46 insertions(+)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index ce722a2..5c3c6c7 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -236,6 +236,11 @@ tar.umask::
>  	the same permissions as gitlink:git-checkout[1] would use. The default
>  	value remains 0, which means world read-write.
>  
> +uploadarchive.daemonformats::
> +	A comma-separated list of the git-archive formats allowed for upload
> +	via git-daemon.  If this parameter is missing all formats are allowed
> +	for upload.
> +

Fine -- do we have any other "list-ish" configuration variable,
by the way?  I am just wondering if we earlier established a
convention to use some delimiter to list out things and if we do
have such a convention if delimiter is a comma or not.

> diff --git a/builtin-upload-archive.c b/builtin-upload-archive.c
> index 96f96bd..6a5245a 100644
> --- a/builtin-upload-archive.c
> +++ b/builtin-upload-archive.c
> @@ -16,6 +16,37 @@ static const char upload_archive_usage[]
>  static const char deadchild[] =
>  "git-upload-archive: archiver died with error";
>  
> +static char *daemon_formats;
> +
> +static int upload_format_config(const char *var, const char *value)
> +{
> +	if (!strcmp(var, "uploadarchive.daemonformats"))
> +		daemon_formats = xstrdup(value);
> +	return 0;
> +}

This let's the repository owner to decide what can be used.

> +static int upload_format_allowed(const char *fmt)
> +{
> +	if (getenv("GIT_DAEMON"))
> +		return daemon_formats ? is_in(fmt, daemon_formats, " \t,") : 1;
> +	return 1;
> +}

And limits the allowed format when the environment set to the
value the repository owner decided.

>  static int run_upload_archive(int argc, const char **argv, const char *prefix)
>  {
> @@ -67,6 +100,12 @@ static int run_upload_archive(int argc, 
>  	/* parse all options sent by the client */
>  	treeish_idx = parse_archive_args(sent_argc, sent_argv, &ar);
>  
> +	if (!upload_format_allowed(ar.name)) {
> +		free(daemon_formats);
> +		die("upload of %s format forbidden\n", ar.name);
> +	}
> +	free(daemon_formats);
> +

So we could enhance "--remote --list" to show what are supported
(both codewise and policywise) on the remote end, with a bit of
code restructuring?

> diff --git a/daemon.c b/daemon.c
> index a2954a0..2d58abe 100644
> --- a/daemon.c
> +++ b/daemon.c
> @@ -304,6 +304,8 @@ static int run_service(char *dir, struct
>  		return -1;
>  	}
>  
> +	setenv("GIT_DAEMON", "I am your father.", 1);

I suspect "upload_format_allowed()" can be taught to see what is
in this environment variable and sometimes take that as
daemon_format without letting the repository to override it, so
that the site administrator can limit the formats that can be
used further, just like daemon service mechanism lets them be in
control.

Re: [PATCH] git-upload-archive: add config option to allow only specified formats

[Rene Scharfe]

Add a config options, uploadarchive.daemonformats, which can be used to
specify the archive formats that are OK to be uploaded using the
upload-archive service of git-daemon.  The list can be restricted
further using the environment variable GIT_DAEMON_ARCHIVE_FORMATS.

These settings are properly taken into account for --list output, so a
user can run git-archive --remote=<url> --list to inquire the allowed
and supported archive formats on the remote end.

git-daemon is changed to always set the environment variable GIT_DAEMON
before it spawns children.  This variable can thus be used to detect
the presence of the (more restrictive) daemon environment.

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---

Junio C Hamano schrieb:
> Rene Scharfe <rene.scharfe@lsrfire.ath.cx> writes:
>> +uploadarchive.daemonformats::
>> +	A comma-separated list of the git-archive formats allowed for upload
>> +	via git-daemon.  If this parameter is missing all formats are allowed
>> +	for upload.
>> +
> 
> Fine -- do we have any other "list-ish" configuration variable,
> by the way?  I am just wondering if we earlier established a
> convention to use some delimiter to list out things and if we do
> have such a convention if delimiter is a comma or not.

I didn't find any.  The code would accept whitespace as a separator, too
(in order to recognize lists like "one, two").  I found myself abusing
this during my testing: I always left out the comma.  So I dropped the
comma from the list of delimiters in this patch, going with shell-like
lists now.

> So we could enhance "--remote --list" to show what are supported
> (both codewise and policywise) on the remote end, with a bit of
> code restructuring?

Yes, of course.  D'oh!

>> diff --git a/daemon.c b/daemon.c
>> index a2954a0..2d58abe 100644
>> --- a/daemon.c
>> +++ b/daemon.c
>> @@ -304,6 +304,8 @@ static int run_service(char *dir, struct
>>  		return -1;
>>  	}
>>  
>> +	setenv("GIT_DAEMON", "I am your father.", 1);
> 
> I suspect "upload_format_allowed()" can be taught to see what is
> in this environment variable and sometimes take that as
> daemon_format without letting the repository to override it, so
> that the site administrator can limit the formats that can be
> used further, just like daemon service mechanism lets them be in
> control.

GIT_DAEMON is meant to indicate one thing, only: that git-daemon started
the process and that it thus has to obey stricter rules.  We can add
another variable which can override the archive config, though.

This patch applies to the 'next' branch (61af0aa).

 Documentation/config.txt |    5 +++++
 archive.h                |    2 ++
 builtin-archive.c        |   27 +++++++++++++++++++++++++--
 builtin-upload-archive.c |    6 ++++++
 cache.h                  |    4 ++++
 config.c                 |   22 ++++++++++++++++++++++
 daemon.c                 |    2 ++
 environment.c            |    1 +
 8 files changed, 67 insertions(+), 2 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index ce722a2..e4123f1 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -236,6 +236,11 @@ tar.umask::
 	the same permissions as gitlink:git-checkout[1] would use. The default
 	value remains 0, which means world read-write.
 
+uploadarchive.daemonformats::
+	A whitespace-separated list of the git-archive formats allowed for
+	upload via git-daemon.  If this parameter is missing or is equal to
+	"*" then all formats are allowed for upload.
+
 user.email::
 	Your email address to be recorded in any newly created commits.
 	Can be overridden by the 'GIT_AUTHOR_EMAIL' and 'GIT_COMMITTER_EMAIL'
diff --git a/archive.h b/archive.h
index 16dcdb8..9ffd300 100644
--- a/archive.h
+++ b/archive.h
@@ -27,6 +27,8 @@ struct archiver {
 
 extern struct archiver archivers[];
 
+extern int is_allowed_archive_format(const char *format);
+
 extern int parse_archive_args(int argc,
 			      const char **argv,
 			      struct archiver *ar);
diff --git a/builtin-archive.c b/builtin-archive.c
index cb883df..5060811 100644
--- a/builtin-archive.c
+++ b/builtin-archive.c
@@ -169,8 +169,10 @@ int parse_archive_args(int argc, const c
 		const char *arg = argv[i];
 
 		if (!strcmp(arg, "--list") || !strcmp(arg, "-l")) {
-			for (i = 0; i < ARRAY_SIZE(archivers); i++)
-				printf("%s\n", archivers[i].name);
+			for (i = 0; i < ARRAY_SIZE(archivers); i++) {
+				if (is_allowed_archive_format(archivers[i].name))
+					printf("%s\n", archivers[i].name);
+			}
 			exit(0);
 		}
 		if (!strcmp(arg, "--verbose") || !strcmp(arg, "-v")) {
@@ -217,6 +219,25 @@ int parse_archive_args(int argc, const c
 	return i;
 }
 
+int is_allowed_archive_format(const char *format)
+{
+	char *formats;
+
+	if (!getenv("GIT_DAEMON"))
+		return 1;
+
+	formats = getenv("GIT_DAEMON_ARCHIVE_FORMATS");
+	if (formats && !git_config_list_contains(formats, format))
+		return 0;
+
+	if (git_config_list_contains(daemon_archive_formats, "*"))
+		return 1;
+	if (!git_config_list_contains(daemon_archive_formats, format))
+		return 0;
+
+	return 1;
+}
+
 static const char *remote_request(int *ac, const char **av)
 {
 	int ix, iy, cnt = *ac;
@@ -260,6 +281,8 @@ int cmd_archive(int argc, const char **a
 
 	setlinebuf(stderr);
 
+	git_config(git_default_config);
+
 	memset(&ar, 0, sizeof(ar));
 	tree_idx = parse_archive_args(argc, argv, &ar);
 	if (prefix == NULL)
diff --git a/builtin-upload-archive.c b/builtin-upload-archive.c
index 42cb9f8..ff31c04 100644
--- a/builtin-upload-archive.c
+++ b/builtin-upload-archive.c
@@ -67,6 +67,9 @@ static int run_upload_archive(int argc, 
 	/* parse all options sent by the client */
 	treeish_idx = parse_archive_args(sent_argc, sent_argv, &ar);
 
+	if (!is_allowed_archive_format(ar.name))
+		die("upload of %s format forbidden\n", ar.name);
+
 	parse_treeish_arg(sent_argv + treeish_idx, &ar.args, prefix);
 	parse_pathspec_arg(sent_argv + treeish_idx + 1, &ar.args);
 
@@ -77,6 +80,9 @@ int cmd_upload_archive(int argc, const c
 {
 	pid_t writer;
 	int fd1[2], fd2[2];
+
+	git_config(git_default_config);
+
 	/*
 	 * Set up sideband subprocess.
 	 *
diff --git a/cache.h b/cache.h
index ac51ed1..9f8d58b 100644
--- a/cache.h
+++ b/cache.h
@@ -399,6 +399,7 @@ extern int git_config_int(const char *, 
 extern int git_config_bool(const char *, const char *);
 extern int git_config_set(const char *, const char *);
 extern int git_config_set_multivar(const char *, const char *, const char *, int);
+extern int git_config_list_contains(const char *haystack, const char *needle);
 extern int check_repository_format_version(const char *var, const char *value);
 
 #define MAX_GITNAME (1000)
@@ -408,6 +409,9 @@ extern char git_default_name[MAX_GITNAME
 #define MAX_ENCODING_LENGTH 64
 extern char git_commit_encoding[MAX_ENCODING_LENGTH];
 
+#define MAX_FORMATS_LENGTH 256
+extern char daemon_archive_formats[MAX_FORMATS_LENGTH];
+
 extern int copy_fd(int ifd, int ofd);
 extern void write_or_die(int fd, const void *buf, size_t count);
 extern int write_or_whine(int fd, const void *buf, size_t count, const char *msg);
diff --git a/config.c b/config.c
index e8f0caf..0f111ab 100644
--- a/config.c
+++ b/config.c
@@ -251,6 +251,23 @@ int git_config_bool(const char *name, co
 	return git_config_int(name, value) != 0;
 }
 
+int git_config_list_contains(const char *haystack, const char *needle)
+{
+	const char delim[] = " \t";
+	int len = strlen(needle);
+	const char *begin = haystack;
+
+	for (;;) {
+		char *pos = strstr(haystack, needle);
+		if (!pos)
+			return 0;
+		haystack++;
+		if ((pos == begin || strchr(delim, pos[-1])) &&
+		    (pos[len] == '\0' || strchr(delim, pos[len])))
+			return 1;
+	}
+}
+
 int git_default_config(const char *var, const char *value)
 {
 	/* This needs a better name */
@@ -314,6 +331,11 @@ int git_default_config(const char *var, 
 		return 0;
 	}
 
+	if (!strcmp(var, "uploadarchive.daemonformats")) {
+		strlcpy(daemon_archive_formats, value, sizeof(daemon_archive_formats));
+		return 0;
+	}
+
 	/* Add other config variables here and to Documentation/config.txt. */
 	return 0;
 }
diff --git a/daemon.c b/daemon.c
index a2954a0..2d58abe 100644
--- a/daemon.c
+++ b/daemon.c
@@ -304,6 +304,8 @@ static int run_service(char *dir, struct
 		return -1;
 	}
 
+	setenv("GIT_DAEMON", "I am your father.", 1);
+
 	/*
 	 * We'll ignore SIGTERM from now on, we have a
 	 * good client.
diff --git a/environment.c b/environment.c
index 84d870c..1796937 100644
--- a/environment.c
+++ b/environment.c
@@ -24,6 +24,7 @@ const char *apply_default_whitespace;
 int zlib_compression_level = Z_DEFAULT_COMPRESSION;
 int pager_in_use;
 int pager_use_color = 1;
+char daemon_archive_formats[MAX_FORMATS_LENGTH] = "*";
 
 static const char *git_dir;
 static char *git_object_dir, *git_index_file, *git_refs_dir, *git_graft_file;

Re: [PATCH] git-upload-archive: add config option to allow only specified formats

[Jakub Narebski]

Rene Scharfe wrote:

>> Fine -- do we have any other "list-ish" configuration variable,
>> by the way?  I am just wondering if we earlier established a
>> convention to use some delimiter to list out things and if we do
>> have such a convention if delimiter is a comma or not.
> 
> I didn't find any.  The code would accept whitespace as a separator, too
> (in order to recognize lists like "one, two").  I found myself abusing
> this during my testing: I always left out the comma.  So I dropped the
> comma from the list of delimiters in this patch, going with shell-like
> lists now.

Alternative would be to use multiple valued variable (multiple values with
the same configuration variable).

-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

Re: [PATCH] git-upload-archive: add config option to allow only specified formats

[Franck Bui-Huu]

2006/9/10, Rene Scharfe <rene.scharfe@lsrfire.ath.cx>:
>  Documentation/config.txt |    5 +++++
>  builtin-upload-archive.c |   39 +++++++++++++++++++++++++++++++++++++++
>  daemon.c                 |    2 ++
>  3 files changed, 46 insertions(+)

[snip]

>
> +       if (!upload_format_allowed(ar.name)) {
> +               free(daemon_formats);
> +               die("upload of %s format forbidden\n", ar.name);
> +       }

just out of curiousity, why "free(daemon_formats)" right before a "die()" ?

-- 
               Franck

Re: [PATCH] git-upload-archive: add config option to allow only specified formats

[Rene Scharfe]

Franck Bui-Huu schrieb:
> just out of curiousity, why "free(daemon_formats)" right before a
> "die()" ?

Hmm, do I show signs of a cleaning fetish? ;-)

A bit more seriously, it helps reduce the number of false positives
emitted by automatic memory checkers.

René

Re: [PATCH 3/3] Add sideband status report to git-archive protocol

[Franck Bui-Huu]

2006/9/10, Junio C Hamano <junkio@cox.net>:
> Using the refactored sideband code from existing upload-pack protocol,
> this lets the error condition and status output sent from the remote
> process to be shown locally.
>
> Signed-off-by: Junio C Hamano <junkio@cox.net>
> ---
>
>  * This does it in a "stupid" way.  Ideally, write(2) to fd 1
>    and 2 in each archiver backend could be wrapped with
>    something like upload-pack.c::send_client_data() that does
>    straight write(2) to the original destination when it is not
>    driven by upload-pack, or use send_sideband() when it is, and
>    that way we can lose two pipes and the multiplexer process.
>

Well I started that part the same way you do because I liked the fact
that archiver backend implementation needn't to know how to send out
data. It's totaly transparent for it. It just need to write its
payload on stdout.

>
>  builtin-archive.c        |   24 ++++++++++--
>  builtin-upload-archive.c |   92 +++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 108 insertions(+), 8 deletions(-)
>

[snip]

> +int cmd_upload_archive(int argc, const char **argv, const char *prefix)
> +{
> +       pid_t writer;
> +       int fd1[2], fd2[2];
> +       /*
> +        * Set up sideband subprocess.
> +        *
> +        * We (parent) monitor and read from child, sending its fd#1 and fd#2
> +        * multiplexed out to our fd#1.  If the child dies, we tell the other
> +        * end over channel #3.
> +        */
> +       if (pipe(fd1) < 0 || pipe(fd2) < 0) {
> +               int err = errno;
> +               packet_write(1, "NACK pipe failed on the remote side\n");
> +               die("upload-archive: %s", strerror(err));
> +       }
> +       writer = fork();
> +       if (writer < 0) {
> +               int err = errno;
> +               packet_write(1, "NACK fork failed on the remote side\n");
> +               die("upload-archive: %s", strerror(err));
> +       }
> +       if (!writer) {
> +               /* child - connect fd#1 and fd#2 to the pipe */
> +               dup2(fd1[1], 1);
> +               dup2(fd2[1], 2);
> +               close(fd1[1]); close(fd2[1]);
> +               close(fd1[0]); close(fd2[0]); /* we do not read from pipe */
> +
> +               exit(run_upload_archive(argc, argv, prefix));
> +       }
> +
> +       /* parent - read from child, multiplex and send out to fd#1 */
> +       close(fd1[1]); close(fd2[1]); /* we do not write to pipe */
>         packet_write(1, "ACK\n");
>         packet_flush(1);
>
> -       return ar.write_archive(&ar.args);
> -}
> +       while (1) {
> +               struct pollfd pfd[2];
> +               char buf[16384];
> +               ssize_t sz;
> +               pid_t pid;
> +               int status;
> +
> +               pfd[0].fd = fd1[0];
> +               pfd[0].events = POLLIN;
> +               pfd[1].fd = fd2[0];
> +               pfd[1].events = POLLIN;
> +               if (poll(pfd, 2, -1) < 0) {
> +                       if (errno != EINTR) {
> +                               error("poll failed resuming: %s",
> +                                     strerror(errno));
> +                               sleep(1);
> +                       }
> +                       continue;
> +               }
> +               if (pfd[0].revents & (POLLIN|POLLHUP)) {
> +                       /* Data stream ready */
> +                       sz = read(pfd[0].fd, buf, sizeof(buf));
> +                       send_sideband(1, 1, buf, sz, DEFAULT_PACKET_MAX);
> +               }
> +               if (pfd[1].revents & (POLLIN|POLLHUP)) {
> +                       /* Status stream ready */
> +                       sz = read(pfd[1].fd, buf, sizeof(buf));
> +                       send_sideband(1, 2, buf, sz, DEFAULT_PACKET_MAX);
> +               }
>
> +               if (((pfd[0].revents | pfd[1].revents) & POLLHUP) == 0)
> +                       continue;
> +               /* did it die? */
> +               pid = waitpid(writer, &status, WNOHANG);
> +               if (!pid) {
> +                       fprintf(stderr, "Hmph, HUP?\n");
> +                       continue;
> +               }
> +               if (!WIFEXITED(status) || WEXITSTATUS(status) > 0)
> +                       send_sideband(1, 3, deadchild, strlen(deadchild),
> +                                     DEFAULT_PACKET_MAX);
> +               packet_flush(1);
> +               break;
> +       }
> +       return 0;
> +}

Why all of this is part of upload-archive ? Shouldn't we put that code
in daemon.c ?
We could use a new service flag to ask daemon.c to start the service
with the sideband multiplexer process already setup and plugged with
the sercive is going to start ? Hence others future services could use
it.

-- 
               Franck

Re: [PATCH 3/3] Add sideband status report to git-archive protocol

[Junio C Hamano]

"Franck Bui-Huu" <vagabon.xyz@gmail.com> writes:

> We could use a new service flag to ask daemon.c to start the service
> with the sideband multiplexer process already setup and plugged with
> the sercive is going to start ? Hence others future services could use
> it.

We do not know how "future services" would look like, so I do
not think it is worth doing so at this moment.

Refactoring existing code after the requirements of the second
and subsequent users are identified would result in much nicer
outcome with less effort, just like I showed here with the two
patches, than overengineering things in advance without knowing
what those requirements are to be generic enough.

For example, moving that code to reusable piece (to daemon.c or
elsewhere -- it does not make a difference) does not even help a
similar code that already exists in upload-pack.c, which needs
to monitor multiple processes involved in a pipe and report
failure exit of any of them.  We would definitely want to have
something that can cover both of these existing cases when the
next user comes along, and if that next user fits in one of the
patterns (either have a pipeline with multiple processes like
upload-pack.c does and has to monitor all of them, or just one
process which is essentially an internal subroutine call and has
to monitor only that process) then refactoring this part right
now to cover both cases would make sense but we do not know what
that next user will look like.

Re: [PATCH 3/3] Add sideband status report to git-archive protocol

[Franck Bui-Huu]

Junio C Hamano wrote:
> Using the refactored sideband code from existing upload-pack protocol,
> this lets the error condition and status output sent from the remote
> process to be shown locally.
> 
> Signed-off-by: Junio C Hamano <junkio@cox.net>
> ---

[snip]

> -}
> +	while (1) {
> +		struct pollfd pfd[2];
> +		char buf[16384];
> +		ssize_t sz;
> +		pid_t pid;
> +		int status;
> +
> +		pfd[0].fd = fd1[0];
> +		pfd[0].events = POLLIN;
> +		pfd[1].fd = fd2[0];
> +		pfd[1].events = POLLIN;
> +		if (poll(pfd, 2, -1) < 0) {
> +			if (errno != EINTR) {
> +				error("poll failed resuming: %s",
> +				      strerror(errno));
> +				sleep(1);
> +			}
> +			continue;
> +		}
> +		if (pfd[0].revents & (POLLIN|POLLHUP)) {
> +			/* Data stream ready */
> +			sz = read(pfd[0].fd, buf, sizeof(buf));
> +			send_sideband(1, 1, buf, sz, DEFAULT_PACKET_MAX);
> +		}
> +		if (pfd[1].revents & (POLLIN|POLLHUP)) {
> +			/* Status stream ready */
> +			sz = read(pfd[1].fd, buf, sizeof(buf));
> +			send_sideband(1, 2, buf, sz, DEFAULT_PACKET_MAX);
> +		}
>  
> +		if (((pfd[0].revents | pfd[1].revents) & POLLHUP) == 0)
> +			continue;
> +		/* did it die? */
> +		pid = waitpid(writer, &status, WNOHANG);
> +		if (!pid) {
> +			fprintf(stderr, "Hmph, HUP?\n");
> +			continue;
> +		}

I get a lot of "Hmph, HUP?" messages when testing "git-archive
--remote" command. One guess: this can be due to the fact that when
the writer process exits, it first closes its fd but do not send a
SIGCHLD signal right after to its parent.

Therefore poll() can return POLLHUP flag to the parent process but
waitpid still returns 0 because the writer process has still not sent
SIGCHLD signal to its parent.

How about this patch ? If poll() doesn't only return the single POLLIN
flag, then either the pipe has been closed because the write process
died or something wrong happened. In all these cases we can wait for
the writer process to exit then die.

-- >8 --

diff --git a/builtin-upload-archive.c b/builtin-upload-archive.c
index 42cb9f8..2ebe9a0 100644
--- a/builtin-upload-archive.c
+++ b/builtin-upload-archive.c
@@ -114,7 +114,6 @@ int cmd_upload_archive(int argc, const c
 		struct pollfd pfd[2];
 		char buf[16384];
 		ssize_t sz;
-		pid_t pid;
 		int status;
 
 		pfd[0].fd = fd1[0];
@@ -140,13 +139,11 @@ int cmd_upload_archive(int argc, const c
 			send_sideband(1, 2, buf, sz, LARGE_PACKET_MAX);
 		}
 
-		if (((pfd[0].revents | pfd[1].revents) & POLLHUP) == 0)
-			continue;
-		/* did it die? */
-		pid = waitpid(writer, &status, WNOHANG);
-		if (!pid) {
-			fprintf(stderr, "Hmph, HUP?\n");
+		if ((pfd[0].revents | pfd[1].revents) == POLLIN)
 			continue;
+
+		if (waitpid(writer, &status, 0) < 0) {
+			die("waitpid failed: %s", strerror(errno));
 		}
 		if (!WIFEXITED(status) || WEXITSTATUS(status) > 0)
 			send_sideband(1, 3, deadchild, strlen(deadchild),

Re: [PATCH 3/3] Add sideband status report to git-archive protocol

[Junio C Hamano]

Franck Bui-Huu <vagabon.xyz@gmail.com> writes:

> I get a lot of "Hmph, HUP?" messages when testing "git-archive
> --remote" command. One guess: this can be due to the fact that when
> the writer process exits, it first closes its fd but do not send a
> SIGCHLD signal right after to its parent.

It does not reproduce for me, but the code I have is obviously
bogus in a few places.

 - When POLLHUP is set, it goes ahead and reads the file
   descriptor.  Worse yet, it does not check the return value of
   read() for errors when it does.

 - When we processed one POLLIN, we should just go back and see
   if any more data is available.  We can check if the child is
   still there when poll gave control back at us but without any
   actual input as you said.

I was uncomfortable letting waitpid() there to wait forever.
When does poll() return?  (1) we have data ready in which case
we process; (2) the child somehow closed the pipe but without
dying, which is an error in the child.  In the latter case even
not hanging in waitpid() and retrying the poll would not give
any useful input so that would not help either.

So I think your patch is a correct fix, except that I think we
should let the remote side know why we stopped talking to them
instead of calling die() there.

We should also check when read() returns an error, so how about
this on top of your patch?

diff --git a/builtin-upload-archive.c b/builtin-upload-archive.c
index 2ebe9a0..a53cfee 100644
--- a/builtin-upload-archive.c
+++ b/builtin-upload-archive.c
@@ -16,6 +16,9 @@ static const char upload_archive_usage[]
 static const char deadchild[] =
 "git-upload-archive: archiver died with error";
 
+static const char lostchild[] =
+"git-upload-archive: archiver process was lost";
+
 
 static int run_upload_archive(int argc, const char **argv, const char *prefix)
 {
@@ -73,6 +76,31 @@ static int run_upload_archive(int argc, 
 	return ar.write_archive(&ar.args);
 }
 
+static void error_clnt(const char *fmt, ...)
+{
+	char buf[1024];
+	va_list params;
+	int len;
+
+	va_start(params, fmt);
+	len = vsprintf(buf, fmt, params);
+	va_end(params);
+	send_sideband(1, 3, buf, len, LARGE_PACKET_MAX);
+	die("sent error to the client: %s", buf);
+}
+
+static void process_input(int child_fd, int band)
+{
+	char buf[16384];
+	ssize_t sz = read(child_fd, buf, sizeof(buf));
+	if (sz < 0) {
+		if (errno != EINTR)
+			error_clnt("read error: %s\n", strerror(errno));
+	}
+	else if (sz)
+		send_sideband(1, band, buf, sz, LARGE_PACKET_MAX);
+}
+
 int cmd_upload_archive(int argc, const char **argv, const char *prefix)
 {
 	pid_t writer;
@@ -112,8 +140,6 @@ int cmd_upload_archive(int argc, const c
 
 	while (1) {
 		struct pollfd pfd[2];
-		char buf[16384];
-		ssize_t sz;
 		int status;
 
 		pfd[0].fd = fd1[0];
@@ -128,26 +154,19 @@ int cmd_upload_archive(int argc, const c
 			}
 			continue;
 		}
-		if (pfd[0].revents & (POLLIN|POLLHUP)) {
+		if (pfd[0].revents & POLLIN)
 			/* Data stream ready */
-			sz = read(pfd[0].fd, buf, sizeof(buf));
-			send_sideband(1, 1, buf, sz, LARGE_PACKET_MAX);
-		}
-		if (pfd[1].revents & (POLLIN|POLLHUP)) {
+			process_input(pfd[0].fd, 1);
+		if (pfd[1].revents & POLLIN)
 			/* Status stream ready */
-			sz = read(pfd[1].fd, buf, sizeof(buf));
-			send_sideband(1, 2, buf, sz, LARGE_PACKET_MAX);
-		}
-
+			process_input(pfd[1].fd, 2);
 		if ((pfd[0].revents | pfd[1].revents) == POLLIN)
 			continue;
 
-		if (waitpid(writer, &status, 0) < 0) {
-			die("waitpid failed: %s", strerror(errno));
-		}
-		if (!WIFEXITED(status) || WEXITSTATUS(status) > 0)
-			send_sideband(1, 3, deadchild, strlen(deadchild),
-				      LARGE_PACKET_MAX);
+		if (waitpid(writer, &status, 0) < 0)
+			error_clnt("%s", lostchild);
+		else if (!WIFEXITED(status) || WEXITSTATUS(status) > 0)
+			error_clnt("%s", deadchild);
 		packet_flush(1);
 		break;
 	}

Re: [PATCH 3/3] Add sideband status report to git-archive protocol

[Franck Bui-Huu]

Junio C Hamano wrote:
> Franck Bui-Huu <vagabon.xyz@gmail.com> writes:
> 
>> I get a lot of "Hmph, HUP?" messages when testing "git-archive
>> --remote" command. One guess: this can be due to the fact that when
>> the writer process exits, it first closes its fd but do not send a
>> SIGCHLD signal right after to its parent.
> 
> It does not reproduce for me, but the code I have is obviously
> bogus in a few places.
> 
>  - When POLLHUP is set, it goes ahead and reads the file
>    descriptor.  Worse yet, it does not check the return value of
>    read() for errors when it does.
> 

The thing is that read() doesn't actually return an error in that
case instead it returns 0 meaning it saw EOF when reading on the
pipe. But I agree we should check for errors. FYI, here's an output
of strace on git-daemon. This trace is realized with the buggy code.

3070 read(5, "[core]\n\trepositoryformatversion "..., 4096) = 53
23070 read(5, "", 4096)                 = 0
23070 close(5)                          = 0
23070 munmap(0xb7f92000, 4096)          = 0
23070 write(1, "pax_global_header\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 10240) = 10240
23069 <... poll resumed> [{fd=5, events=POLLIN, revents=POLLIN}, {fd=7, events=POLLIN}], 2, -1) = 1
23069 read(5, "pax_global_header\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 16384) = 10240
23069 write(1, "2805\1", 5)             = 5
23069 write(1, "pax_global_header\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 10240) = 10240
23069 poll( <unfinished ...>
23070 write(1, "type f -iname \"$c-*\" | sed -e 1q"..., 10240) = 10240
23069 <... poll resumed> [{fd=5, events=POLLIN, revents=POLLIN}, {fd=7, events=POLLIN}], 2, -1) = 1
23069 read(5, "type f -iname \"$c-*\" | sed -e 1q"..., 16384) = 10240
23069 write(1, "2805\1", 5)             = 5
23069 write(1, "type f -iname \"$c-*\" | sed -e 1q"..., 10240) = 10240
23069 poll( <unfinished ...>
23070 exit_group(0)                     = ?
23069 <... poll resumed> [{fd=5, events=POLLIN, revents=POLLHUP}, {fd=7, events=POLLIN}], 2, -1) = 1
23069 read(5, "", 16384)                = 0
23069 waitpid(23070, 0xbfef3188, WNOHANG) = 0
23069 write(2, "Hmph, HUP?\n", 11)      = 11
23069 poll([{fd=5, events=POLLIN, revents=POLLHUP}, {fd=7, events=POLLIN}], 2, -1) = 1
23069 read(5, "", 16384)                = 0
23069 waitpid(23070, 0xbfef3188, WNOHANG) = 0
23069 write(2, "Hmph, HUP?\n", 11)      = 11

[...]

23077 poll([{fd=5, events=POLLIN, revents=POLLHUP}, {fd=7, events=POLLIN}], 2, -1) = 1
23077 read(5, "", 16384)                = 0
23077 waitpid(23078, 0xbf9bb448, WNOHANG) = 0
23077 --- SIGCHLD (Child exited) @ 0 (0) ---
23077 write(2, "Hmph, HUP?\n", 11)      = 11
23077 poll([{fd=5, events=POLLIN, revents=POLLHUP}, {fd=7, events=POLLIN, revents=POLLHUP}], 2, -1) = 2
23077 read(5, "", 16384)                = 0
23077 read(7, "", 16384)                = 0
23077 waitpid(23078, [{WIFEXITED(s) && WEXITSTATUS(s) == 0}], WNOHANG) = 23078
23077 write(1, "0000", 4)               = 4
23077 exit_group(0)                     = ?
23065 <... poll resumed> [{fd=3, events=POLLIN}, {fd=4, events=POLLIN}], 2, -1) = -1 EINTR (Interrupted system call)
23065 --- SIGCHLD (Child exited) @ 0 (0) ---
23065 waitpid(-1, [{WIFEXITED(s) && WEXITSTATUS(s) == 0}], WNOHANG) = 23077
23065 time(NULL)                        = 1158047440

[...]

>  - When we processed one POLLIN, we should just go back and see
>    if any more data is available.  We can check if the child is
>    still there when poll gave control back at us but without any
>    actual input as you said.
> 
> I was uncomfortable letting waitpid() there to wait forever.
> When does poll() return?  (1) we have data ready in which case
> we process; (2) the child somehow closed the pipe but without
> dying, which is an error in the child.  In the latter case even
> not hanging in waitpid() and retrying the poll would not give
> any useful input so that would not help either.
> 

your case (2) is not totaly right. If you look a the trace above,
for the normal case, you can see that the child close the pipe then 
_after_ a while die. So there's a time when the child is not died
but the pipe is closed.

I think it's safe to assume that if the child closes the pipe, either
because it has finished to write or something wrong going on, then
it's going to die pretty soon. 

> So I think your patch is a correct fix, except that I think we
> should let the remote side know why we stopped talking to them
> instead of calling die() there.
> 
> We should also check when read() returns an error, so how about
> this on top of your patch?
> 
> diff --git a/builtin-upload-archive.c b/builtin-upload-archive.c
> index 2ebe9a0..a53cfee 100644
> --- a/builtin-upload-archive.c
> +++ b/builtin-upload-archive.c
> @@ -16,6 +16,9 @@ static const char upload_archive_usage[]
>  static const char deadchild[] =
>  "git-upload-archive: archiver died with error";
>  
> +static const char lostchild[] =
> +"git-upload-archive: archiver process was lost";
> +
>  
>  static int run_upload_archive(int argc, const char **argv, const char *prefix)
>  {
> @@ -73,6 +76,31 @@ static int run_upload_archive(int argc, 
>  	return ar.write_archive(&ar.args);
>  }
>  
> +static void error_clnt(const char *fmt, ...)
> +{
> +	char buf[1024];
> +	va_list params;
> +	int len;
> +
> +	va_start(params, fmt);
> +	len = vsprintf(buf, fmt, params);
> +	va_end(params);
> +	send_sideband(1, 3, buf, len, LARGE_PACKET_MAX);
> +	die("sent error to the client: %s", buf);
> +}
> +
> +static void process_input(int child_fd, int band)
> +{
> +	char buf[16384];
> +	ssize_t sz = read(child_fd, buf, sizeof(buf));
> +	if (sz < 0) {
> +		if (errno != EINTR)
> +			error_clnt("read error: %s\n", strerror(errno));
> +	}
> +	else if (sz)
> +		send_sideband(1, band, buf, sz, LARGE_PACKET_MAX);
> +}
> +


I think calling send_sideband() with sz = 0 should be fine, otherwise
this function is buggy, no ? So you can simply write process_input() like

static void process_input(int child_fd, int band)
{
	char buf[16384];
	ssize_t sz = read(child_fd, buf, sizeof(buf));
	if (sz < 0) {
		if (errno != EINTR)
			error_clnt("read error: %s\n", strerror(errno));
		return;
	}
	send_sideband(1, band, buf, sz, LARGE_PACKET_MAX);
}

		Franck

Re: [PATCH 3/3] Add sideband status report to git-archive protocol

[Franck Bui-Huu]

Franck Bui-Huu wrote:
> Junio C Hamano wrote:
>>
>> We should also check when read() returns an error, so how about
>> this on top of your patch?
>>

argh, there's still something broken...doing:

$ while true; do
> git archive --format=tar --remote=git://localhost/repo.git HEAD | tar tf -
> done

stucks somehow. I won't have time to look at it today :(

		Franck

[PATCH] connect.c: finish_connect(): allow null pid parameter

[Franck Bui-Huu]

git_connect() can return 0 if we use git protocol for example.
Users of this function don't know and don't care if a process
had been created or not, and to avoid them to check it before
calling finish_connect() this patch allows finish_connect() to
take a null pid. And in that case return 0.

Signed-off-by: Franck Bui-Huu <vagabon.xyz@gmail.com>
---

 Found it when debugging 'git archive --remote=git://...'
 command. I noticed that this command always exited with 1 as
 status.

 connect.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/connect.c b/connect.c
index 1c6429b..e6efff9 100644
--- a/connect.c
+++ b/connect.c
@@ -737,6 +737,9 @@ int git_connect(int fd[2], char *url, co
 
 int finish_connect(pid_t pid)
 {
+	if (pid == 0)
+		return 0;
+
 	while (waitpid(pid, NULL, 0) < 0) {
 		if (errno != EINTR)
 			return -1;
-- 
1.4.2

Re: [PATCH] connect.c: finish_connect(): allow null pid parameter

[Junio C Hamano]

Franck Bui-Huu <vagabon.xyz@gmail.com> writes:

> git_connect() can return 0 if we use git protocol for example.
> Users of this function don't know and don't care if a process
> had been created or not, and to avoid them to check it before
> calling finish_connect() this patch allows finish_connect() to
> take a null pid. And in that case return 0.
>
> Signed-off-by: Franck Bui-Huu <vagabon.xyz@gmail.com>
> ---
>
>  Found it when debugging 'git archive --remote=git://...'
>  command. I noticed that this command always exited with 1 as
>  status.

True.  This should affect existing users of finish_connect(), but
existing callers do not check its return value X-<.

I think the return type of git_connect() should be changed to
pid_t with a warning that says it returns negative on error, pid
of a process finish_connect() should wait for if the underlying
protocol driver forks, and 0 if we do not have to wait in
finish_connect().  Making finish_connect() to accept 0 as its
input is probably a good change.

[PATCH] Test return value of finish_connect()

[Franck Bui-Huu]

Signed-off-by: Franck Bui-Huu <vagabon.xyz@gmail.com>
---
 fetch-pack.c  |    4 ++--
 peek-remote.c |    4 ++--
 send-pack.c   |    4 ++--
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/fetch-pack.c b/fetch-pack.c
index 1b2d6ee..e8708aa 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -525,7 +525,7 @@ int main(int argc, char **argv)
 	ret = fetch_pack(fd, nr_heads, heads);
 	close(fd[0]);
 	close(fd[1]);
-	finish_connect(pid);
+	ret |= finish_connect(pid);
 
 	if (!ret && nr_heads) {
 		/* If the heads to pull were given, we should have
@@ -540,5 +540,5 @@ int main(int argc, char **argv)
 			}
 	}
 
-	return ret;
+	return !!ret;
 }
diff --git a/peek-remote.c b/peek-remote.c
index 87f1543..353da00 100644
--- a/peek-remote.c
+++ b/peek-remote.c
@@ -66,6 +66,6 @@ int main(int argc, char **argv)
 	ret = peek_remote(fd, flags);
 	close(fd[0]);
 	close(fd[1]);
-	finish_connect(pid);
-	return ret;
+	ret |= finish_connect(pid);
+	return !!ret;
 }
diff --git a/send-pack.c b/send-pack.c
index 49be764..5bb123a 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -443,6 +443,6 @@ int main(int argc, char **argv)
 	ret = send_pack(fd[0], fd[1], nr_heads, heads);
 	close(fd[0]);
 	close(fd[1]);
-	finish_connect(pid);
-	return ret;
+	ret |= finish_connect(pid);
+	return !!ret;
 }
-- 
1.4.2

[PATCH] git_connect: change return type to pid_t

[Franck Bui-Huu]

Signed-off-by: Franck Bui-Huu <vagabon.xyz@gmail.com>
---

 For now I let this function die if an error has occured.
 Current users wouldn't do anything usefull with a negative
 value except exiting.

 cache.h   |    2 +-
 connect.c |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/cache.h b/cache.h
index ac51ed1..57db7c9 100644
--- a/cache.h
+++ b/cache.h
@@ -359,7 +359,7 @@ #define REF_NORMAL	(1u << 0)
 #define REF_HEADS	(1u << 1)
 #define REF_TAGS	(1u << 2)
 
-extern int git_connect(int fd[2], char *url, const char *prog);
+extern pid_t git_connect(int fd[2], char *url, const char *prog);
 extern int finish_connect(pid_t pid);
 extern int path_match(const char *path, int nr, char **match);
 extern int match_refs(struct ref *src, struct ref *dst, struct ref ***dst_tail,
diff --git a/connect.c b/connect.c
index e6efff9..4bf7914 100644
--- a/connect.c
+++ b/connect.c
@@ -602,7 +602,7 @@ static void git_proxy_connect(int fd[2],
 /*
  * Yeah, yeah, fixme. Need to pass in the heads etc.
  */
-int git_connect(int fd[2], char *url, const char *prog)
+pid_t git_connect(int fd[2], char *url, const char *prog)
 {
 	char command[1024];
 	char *host, *path = url;
-- 
1.4.2

Re: [PATCH 3/3] Add sideband status report to git-archive protocol

[Junio C Hamano]

Franck Bui-Huu <vagabon.xyz@gmail.com> writes:

>> I was uncomfortable letting waitpid() there to wait forever.
>> When does poll() return?  (1) we have data ready in which case
>> we process; (2) the child somehow closed the pipe but without
>> dying, which is an error in the child.  In the latter case even
>> not hanging in waitpid() and retrying the poll would not give
>> any useful input so that would not help either.
>
> your case (2) is not totaly right. If you look a the trace above,
> for the normal case, you can see that the child close the pipe then 
> _after_ a while die. So there's a time when the child is not died
> but the pipe is closed.
>
> I think it's safe to assume that if the child closes the pipe, either
> because it has finished to write or something wrong going on, then
> it's going to die pretty soon. 

I am essentially saying the same thing (and perhaps one more).
Something is wrong with the child, and either it's going to die
pretty soon in which case waitpid() to wait forever is fine, or
even if it is not going to die soon, going back to poll() would
not give us any useful information anyway, so WNOHANG was
pointless.

So we are in agreement.

> I think calling send_sideband() with sz = 0 should be fine,

I just coded it defensively -- no point calling send() when you
already know there is nothing to be sent.

Re: [PATCH 1/2] archive: allow remote to have more formats than we understand.

[Rene Scharfe]

Junio C Hamano schrieb:
> This fixes git-archive --remote not to parse archiver arguments;
> otherwise if the remote end implements formats other than the
> one known locally we will not be able to access that format.

Yes!  That's the right separation of work between the two parties.
And git-archive --remote=somewhere --list starts to magically work. =)

Thanks,
René

Re: [PATCH 1/2] archive: allow remote to have more formats than we understand.

[Franck Bui-Huu]

2006/9/10, Junio C Hamano <junkio@cox.net>:
> This fixes git-archive --remote not to parse archiver arguments;
> otherwise if the remote end implements formats other than the
> one known locally we will not be able to access that format.
>
> Signed-off-by: Junio C Hamano <junkio@cox.net>
> ---
>  * At first sight, this should not matter that much, but (1) we
>    do not really parse and validate the arguments when dealing
>    with remote site, and (2) we have no way validating them if
>    we wanted to, given that the remote end might be running
>    different version of git.
>
>    Having said that, you would realize that once we start
>    refactoring things this way, "git archive --remote=foo" is
>    not archive driver anymore.  There is nothing that prevents
>    us saying "git archive --remote=foo --command=rev-list HEAD",
>    other than that the remote archive protocol insists the
>    command invoked at the remote end to be "git archive" itself.
>

good change.

>  archive.h         |    1 -
>  builtin-archive.c |   79 ++++++++++++++++++++++++++++++++---------------------
>  2 files changed, 47 insertions(+), 33 deletions(-)
>

[snip]

>         return i;
>  }
>
> +static const char *remote_request(int *ac, const char **av)
> +{

just to be consistent with the rest of the file, I would have called
this function
"parse_remote_arg" or "extract_remote_arg"

-- 
               Franck

Re: [PATCH 1/2] archive: allow remote to have more formats than we understand.

[Junio C Hamano]

"Franck Bui-Huu" <vagabon.xyz@gmail.com> writes:

>> +static const char *remote_request(int *ac, const char **av)
>> +{
>
> just to be consistent with the rest of the file, I would have called
> this function
> "parse_remote_arg" or "extract_remote_arg"

I was thinking about calling this is_remote_request() actually.

Re: [PATCH 1/2] archive: allow remote to have more formats than we understand.

[Franck Bui-Huu]

2006/9/10, Junio C Hamano <junkio@cox.net>:
> "Franck Bui-Huu" <vagabon.xyz@gmail.com> writes:
>
> >> +static const char *remote_request(int *ac, const char **av)
> >> +{
> >
> > just to be consistent with the rest of the file, I would have called
> > this function
> > "parse_remote_arg" or "extract_remote_arg"
>
> I was thinking about calling this is_remote_request() actually.
>

that sounds like to return a boolean. You would need to pass remote as
a parameter, no ?
I think extract_remote_arg is nice because it tells you that it
returns remote option valu _and_ remove it from argv.

-- 
               Franck