[PATCH v3 0/3] git-remote-fd & git-remote-ext

[PATCH v3 0/3] git-remote-fd & git-remote-ext

[Ilari Liusvaara]

This adds two new remote helpers.

* git-remote-fd, which connects to git service on given file descriptor(s),
useful for graphical user interfaces that want to use internal ssh client.

* git-remote-ext, which connect to git service using external program. Useful
for connecting using odd one-off ssh options, to services in abstract
namespace, using unix domain sockets, using TLS, etc...

Changes from last time:
* Actually include all needed files (oops)
* Lots of documentation wording changes and some minor code changes.

Ilari Liusvaara (3):
  Add bidirectional_transfer_loop()
  git-remote-fd
  git-remote-ext

 .gitignore                       |    2 +
 Documentation/git-remote-ext.txt |  112 ++++++++++++++++
 Documentation/git-remote-fd.txt  |   57 ++++++++
 Makefile                         |    2 +
 builtin.h                        |    2 +
 builtin/remote-ext.c             |  261 ++++++++++++++++++++++++++++++++++++++
 builtin/remote-fd.c              |   76 +++++++++++
 compat/mingw.h                   |    5 +
 git.c                            |    2 +
 transport-helper.c               |  254 +++++++++++++++++++++++++++++++++++++
 transport.h                      |    1 +
 11 files changed, 774 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/git-remote-ext.txt
 create mode 100644 Documentation/git-remote-fd.txt
 create mode 100644 builtin/remote-ext.c
 create mode 100644 builtin/remote-fd.c

-- 
1.7.3.1.48.g4fe83

[PATCH v3 1/3] Add bidirectional_transfer_loop()

[Ilari Liusvaara]

This helper function copies bidirectional stream of data between
stdin/stdout and specified file descriptors.

Signed-off-by: Ilari Liusvaara <ilari.liusvaara@elisanet.fi>
---
 compat/mingw.h     |    5 +
 transport-helper.c |  254 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 transport.h        |    1 +
 3 files changed, 260 insertions(+), 0 deletions(-)

diff --git a/compat/mingw.h b/compat/mingw.h
index 3b2477b..f27a7b6 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -23,6 +23,9 @@ typedef int pid_t;
 #define WEXITSTATUS(x) ((x) & 0xff)
 #define WTERMSIG(x) SIGTERM
 
+#define EWOULDBLOCK EAGAIN
+#define SHUT_WR SD_SEND
+
 #define SIGHUP 1
 #define SIGQUIT 3
 #define SIGKILL 9
@@ -50,6 +53,8 @@ struct pollfd {
 };
 #define POLLIN 1
 #define POLLHUP 2
+#define POLLOUT 4
+#define POLLNVAL 8
 #endif
 
 typedef void (__cdecl *sig_handler_t)(int);
diff --git a/transport-helper.c b/transport-helper.c
index acfc88e..1ebcebc 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -862,3 +862,257 @@ int transport_helper_init(struct transport *transport, const char *name)
 	transport->smart_options = &(data->transport_options);
 	return 0;
 }
+
+
+#define BUFFERSIZE 4096
+#define PBUFFERSIZE 8192
+
+/* Print bidirectional transfer loop debug message. */
+static void transfer_debug(const char *fmt, ...)
+{
+	va_list args;
+	char msgbuf[PBUFFERSIZE];
+	static int debug_enabled = -1;
+
+	if (debug_enabled < 0)
+		debug_enabled = getenv("GIT_TRANSLOOP_DEBUG") ? 1 : 0;
+	if (!debug_enabled)
+		return;
+
+	sprintf(msgbuf, "Transfer loop debugging: ");
+	va_start(args, fmt);
+	vsprintf(msgbuf + strlen(msgbuf), fmt, args);
+	va_end(args);
+	fprintf(stderr, "%s\n", msgbuf);
+}
+
+/* Load the parameters into poll structure. Return number of entries loaded */
+static int load_poll_params(struct pollfd *polls, size_t inbufuse,
+	size_t outbufuse, int in_hup, int out_hup, int in_closed,
+	int out_closed, int socket_mode, int input_fd, int output_fd)
+{
+	int stdin_index = -1;
+	int stdout_index = -1;
+	int input_index = -1;
+	int output_index = -1;
+	int nextindex = 0;
+	int i;
+
+	/*
+	 * Inputs can't be waited at all if buffer is full since we can't
+	 * do read on 0 bytes as it could do strange things.
+	 */
+	if (!in_hup && inbufuse < BUFFERSIZE) {
+		stdin_index = nextindex++;
+		polls[stdin_index].fd = 0;
+		transfer_debug("Adding stdin to fds to wait for");
+	}
+	if (!out_hup && outbufuse < BUFFERSIZE) {
+		input_index = nextindex++;
+		polls[input_index].fd = input_fd;
+		transfer_debug("Adding remote input to fds to wait for");
+	}
+	if (!out_closed && outbufuse > 0) {
+		stdout_index = nextindex++;
+		polls[stdout_index].fd = 1;
+		transfer_debug("Adding stdout to fds to wait for");
+	}
+	if (!in_closed && inbufuse > 0) {
+		if (socket_mode && input_index >= 0)
+			output_index = input_index;
+		else {
+			output_index = nextindex++;
+			polls[output_index].fd = output_fd;
+		}
+		transfer_debug("Adding remote output to fds to wait for");
+	}
+
+	for (i = 0; i < nextindex; i++)
+		polls[i].events = polls[i].revents = 0;
+
+	if (stdin_index >= 0) {
+		polls[stdin_index].events |= POLLIN;
+		transfer_debug("Waiting for stdin to become readable");
+	}
+	if (input_index >= 0) {
+		polls[input_index].events |= POLLIN;
+		transfer_debug("Waiting for remote input to become readable");
+	}
+	if (stdout_index >= 0) {
+		polls[stdout_index].events |= POLLOUT;
+		transfer_debug("Waiting for stdout to become writable");
+	}
+	if (output_index >= 0) {
+		polls[output_index].events |= POLLOUT;
+		transfer_debug("Waiting for remote output to become writable");
+	}
+
+	/* Return number of indexes assigned. */
+	return nextindex;
+}
+
+static int transfer_handle_events(struct pollfd* polls, char *in_buffer,
+	char *out_buffer, size_t *in_buffer_use, size_t *out_buffer_use,
+	int *in_hup, int *out_hup, int *in_closed, int *out_closed,
+	int socket_mode, int poll_count, int input, int output)
+{
+	int i, r;
+	for(i = 0; i < poll_count; i++) {
+		/* Handle stdin. */
+		if (polls[i].fd == 0 && polls[i].revents & (POLLIN | POLLHUP)) {
+			transfer_debug("stdin is readable");
+			r = read(0, in_buffer + *in_buffer_use, BUFFERSIZE -
+				*in_buffer_use);
+			if (r < 0 && errno != EWOULDBLOCK && errno != EAGAIN &&
+				errno != EINTR) {
+				perror("read(git) failed");
+				return 1;
+			} else if (r == 0) {
+				transfer_debug("stdin EOF");
+				*in_hup = 1;
+				if (!*in_buffer_use) {
+					if (socket_mode)
+						shutdown(output, SHUT_WR);
+					else
+						close(output);
+					*in_closed = 1;
+					transfer_debug("Closed remote output");
+				} else
+					transfer_debug("Delaying remote output close because input buffer has data");
+			} else if (r > 0) {
+				*in_buffer_use += r;
+				transfer_debug("Read %i bytes from stdin (buffer now at %i)", r, (int)*in_buffer_use);
+			}
+		}
+
+		/* Handle remote end input. */
+		if (polls[i].fd == input &&
+			polls[i].revents & (POLLIN | POLLHUP)) {
+			transfer_debug("remote input is readable");
+			r = read(input, out_buffer + *out_buffer_use,
+				BUFFERSIZE - *out_buffer_use);
+			if (r < 0 && errno != EWOULDBLOCK && errno != EAGAIN &&
+				errno != EINTR) {
+				perror("read(connection) failed");
+				return 1;
+			} else if (r == 0) {
+				transfer_debug("remote input EOF");
+				*out_hup = 1;
+				if (!*out_buffer_use) {
+					close(1);
+					*out_closed = 1;
+					transfer_debug("Closed stdout");
+				} else
+					transfer_debug("Delaying stdout close because output buffer has data");
+
+			} else if (r > 0) {
+				*out_buffer_use += r;
+				transfer_debug("Read %i bytes from remote input (buffer now at %i)", r, (int)*out_buffer_use);
+			}
+		}
+
+		/* Handle stdout. */
+		if (polls[i].fd == 1 && polls[i].revents & POLLNVAL) {
+			error("Write pipe to Git unexpectedly closed.");
+			return 1;
+		}
+		if (polls[i].fd == 1 && polls[i].revents & POLLOUT) {
+			transfer_debug("stdout is writable");
+			r = write(1, out_buffer, *out_buffer_use);
+			if (r < 0 && errno != EWOULDBLOCK && errno != EAGAIN &&
+				errno != EINTR) {
+				perror("write(git) failed");
+				return 1;
+			} else if (r > 0){
+				*out_buffer_use -= r;
+				transfer_debug("Wrote %i bytes to stdout (buffer now at %i)", r, (int)*out_buffer_use);
+				if (*out_buffer_use > 0)
+					memmove(out_buffer, out_buffer + r,
+						*out_buffer_use);
+				if (*out_hup && !*out_buffer_use) {
+					close(1);
+					*out_closed = 1;
+					transfer_debug("Closed stdout");
+				}
+			}
+		}
+
+		/* Handle remote end output. */
+		if (polls[i].fd == output && polls[i].revents & POLLNVAL) {
+			error("Write pipe to remote end unexpectedly closed.");
+			return 1;
+		}
+		if (polls[i].fd == output && polls[i].revents & POLLOUT) {
+			transfer_debug("remote output is writable");
+			r = write(output, in_buffer, *in_buffer_use);
+			if (r < 0 && errno != EWOULDBLOCK && errno != EAGAIN &&
+				errno != EINTR) {
+				perror("write(connection) failed");
+				return 1;
+			} else if (r > 0) {
+				*in_buffer_use -= r;
+				transfer_debug("Wrote %i bytes to remote output (buffer now at %i)", r, (int)*in_buffer_use);
+				if (*in_buffer_use > 0)
+					memmove(in_buffer, in_buffer + r,
+						*in_buffer_use);
+				if (*in_hup && !*in_buffer_use) {
+					if (socket_mode)
+						shutdown(output, SHUT_WR);
+					else
+						close(output);
+					*in_closed = 1;
+					transfer_debug("Closed remote output");
+				}
+			}
+		}
+	}
+	return 0;
+}
+
+/* Copy data from stdin to output and from input to stdout. */
+int bidirectional_transfer_loop(int input, int output)
+{
+	struct pollfd polls[4];
+	char in_buffer[BUFFERSIZE];
+	char out_buffer[BUFFERSIZE];
+	size_t in_buffer_use = 0;
+	size_t out_buffer_use = 0;
+	int in_hup = 0;
+	int out_hup = 0;
+	int in_closed = 0;
+	int out_closed = 0;
+	int socket_mode = 0;
+	int poll_count = 4;
+
+	if (input == output)
+		socket_mode = 1;
+
+	while (1) {
+		int r;
+		poll_count = load_poll_params(polls, in_buffer_use,
+			out_buffer_use, in_hup, out_hup, in_closed, out_closed,
+			socket_mode, input, output);
+		if (!poll_count) {
+			transfer_debug("Transfer done");
+			break;
+		}
+		transfer_debug("Waiting for %i file descriptors", poll_count);
+		r = poll(polls, poll_count, -1);
+		if (r < 0) {
+			if (errno == EWOULDBLOCK || errno == EAGAIN ||
+				errno == EINTR)
+				continue;
+			perror("poll failed");
+			return 1;
+		} else if (r == 0)
+			continue;
+
+		r = transfer_handle_events(polls, in_buffer, out_buffer,
+			&in_buffer_use, &out_buffer_use, &in_hup, &out_hup,
+			&in_closed, &out_closed, socket_mode, poll_count,
+			input, output);
+		if (r)
+			return r;
+	}
+	return 0;
+}
diff --git a/transport.h b/transport.h
index c59d973..e803c0e 100644
--- a/transport.h
+++ b/transport.h
@@ -154,6 +154,7 @@ int transport_connect(struct transport *transport, const char *name,
 
 /* Transport methods defined outside transport.c */
 int transport_helper_init(struct transport *transport, const char *name);
+int bidirectional_transfer_loop(int input, int output);
 
 /* common methods used by transport.c and builtin-send-pack.c */
 void transport_verify_remote_names(int nr_heads, const char **heads);
-- 
1.7.3.1.48.g4fe83

[PATCH v3 2/3] git-remote-fd

[Ilari Liusvaara]

This remote helper reflects raw smart remote transport stream back to the
calling program. This is useful for example if some UI wants to handle
ssh itself and not use hacks via GIT_SSH.

Signed-off-by: Ilari Liusvaara <ilari.liusvaara@elisanet.fi>
---
 .gitignore                      |    1 +
 Documentation/git-remote-fd.txt |   57 +++++++++++++++++++++++++++++
 Makefile                        |    1 +
 builtin.h                       |    1 +
 builtin/remote-fd.c             |   76 +++++++++++++++++++++++++++++++++++++++
 git.c                           |    1 +
 6 files changed, 137 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/git-remote-fd.txt
 create mode 100644 builtin/remote-fd.c

diff --git a/.gitignore b/.gitignore
index 20560b8..89f37f4 100644
--- a/.gitignore
+++ b/.gitignore
@@ -112,6 +112,7 @@
 /git-remote-https
 /git-remote-ftp
 /git-remote-ftps
+/git-remote-fd
 /git-remote-testgit
 /git-repack
 /git-replace
diff --git a/Documentation/git-remote-fd.txt b/Documentation/git-remote-fd.txt
new file mode 100644
index 0000000..c4c0da1
--- /dev/null
+++ b/Documentation/git-remote-fd.txt
@@ -0,0 +1,57 @@
+git-remote-fd(1)
+=================
+
+NAME
+----
+git-remote-fd - Reflect smart transport back to caller.
+
+
+SYNOPSIS
+--------
+"fd::<fd>[/<anything>]" or "fd::<infd>,<outfd>[/<anything>]" (as URL)
+
+DESCRIPTION
+-----------
+This helper uses specified file descriptors to connect to remote git server.
+
+Data written to <fd> or <outfd> is assumed to make it unmolested to
+git-upload-pack/git-receive-pack/git-upload-archive, and data from that
+program is assumed to be readable unmolested from <fd> or <infd>.
+
+If just <fd> is specified, <fd> is assumed to be socket. Otherwise both
+<infd> and <outfd> are assumed to be pipes.
+
+It is assumed that any handshaking procedures have already been completed
+(such as sending service request for git://) before this helper is started.
+
+<anything> can be any string. It is ignored. It is meant for provoding
+information to user in the "URL".
+
+ENVIRONMENT VARIABLES:
+----------------------
+
+$GIT_TRANSLOOP_DEBUG (passed to git)::
+	If set, prints debugging information about various reads/writes.
+
+EXAMPLES:
+---------
+"fd::17" (as URL)::
+	Connect using socket in file descriptor #17.
+
+"fd::17/foo" (as URL)::
+	Same as above.
+
+"fd::7,8" (as URL)::
+	Connect using pipes in file descriptors #7 and #8. The incoming
+	pipe is at fd #7 and the outgoing pipe at fd #8.
+
+"fd::7,8/bar" (as URL)::
+	Same as above.
+
+Documentation
+--------------
+Documentation by Ilari Liusvaara and the git list <git@vger.kernel.org>
+
+GIT
+---
+Part of the linkgit:git[1] suite
diff --git a/Makefile b/Makefile
index 8a56b9a..7da54d7 100644
--- a/Makefile
+++ b/Makefile
@@ -728,6 +728,7 @@ BUILTIN_OBJS += builtin/read-tree.o
 BUILTIN_OBJS += builtin/receive-pack.o
 BUILTIN_OBJS += builtin/reflog.o
 BUILTIN_OBJS += builtin/remote.o
+BUILTIN_OBJS += builtin/remote-fd.o
 BUILTIN_OBJS += builtin/replace.o
 BUILTIN_OBJS += builtin/rerere.o
 BUILTIN_OBJS += builtin/reset.o
diff --git a/builtin.h b/builtin.h
index f2a25a0..1a816e1 100644
--- a/builtin.h
+++ b/builtin.h
@@ -108,6 +108,7 @@ extern int cmd_read_tree(int argc, const char **argv, const char *prefix);
 extern int cmd_receive_pack(int argc, const char **argv, const char *prefix);
 extern int cmd_reflog(int argc, const char **argv, const char *prefix);
 extern int cmd_remote(int argc, const char **argv, const char *prefix);
+extern int cmd_remote_fd(int argc, const char **argv, const char *prefix);
 extern int cmd_config(int argc, const char **argv, const char *prefix);
 extern int cmd_rerere(int argc, const char **argv, const char *prefix);
 extern int cmd_reset(int argc, const char **argv, const char *prefix);
diff --git a/builtin/remote-fd.c b/builtin/remote-fd.c
new file mode 100644
index 0000000..44c174b
--- /dev/null
+++ b/builtin/remote-fd.c
@@ -0,0 +1,76 @@
+#include "git-compat-util.h"
+#include "transport.h"
+
+/*
+ * URL syntax:
+ *	'fd::<inoutfd>[/<anything>]'		Read/write socket pair
+ *						<inoutfd>.
+ *	'fd::<infd>,<outfd>[/<anything>]'	Read pipe <infd> and write
+ *						pipe <outfd>.
+ *	[foo] indicates 'foo' is optional. <anything> is any string.
+ *
+ * The data output to <outfd>/<inoutfd> should be passed unmolested to
+ * git-receive-pack/git-upload-pack/git-upload-archive and output of
+ * git-receive-pack/git-upload-pack/git-upload-archive should be passed
+ * unmolested to <infd>/<inoutfd>.
+ *
+ */
+
+int input_fd = -1;
+int output_fd = -1;
+
+#define MAXCOMMAND 4096
+
+static int command_loop()
+{
+	char buffer[MAXCOMMAND];
+
+	while (1) {
+		if (!fgets(buffer, MAXCOMMAND - 1, stdin))
+			exit(0);
+		/* Strip end of line characters. */
+		while (isspace((unsigned char)buffer[strlen(buffer) - 1]))
+			buffer[strlen(buffer) - 1] = 0;
+
+		if (!strcmp(buffer, "capabilities")) {
+			printf("*connect\n\n");
+			fflush(stdout);
+		} else if (!strncmp(buffer, "connect ", 8)) {
+			printf("\n");
+			fflush(stdout);
+			return bidirectional_transfer_loop(input_fd,
+				output_fd);
+		} else {
+			fprintf(stderr, "Bad command");
+			return 1;
+		}
+	}
+}
+
+int cmd_remote_fd(int argc, const char **argv, const char *prefix)
+{
+	char* end;
+	unsigned long r;
+
+	if (argc < 3)
+		die("URL missing");
+
+	r = strtoul(argv[2], &end, 10);
+	input_fd = (int)r;
+
+	if ((*end != ',' && *end !='/' && *end) || end == argv[2])
+		die("Bad URL syntax");
+
+	if (*end == '/' || !*end) {
+		output_fd = input_fd;
+	} else {
+		char* end2;
+		r = strtoul(end + 1, &end2, 10);
+		output_fd = (int)r;
+
+		if ((*end2 !='/' && *end2) || end2 == end + 1)
+			die("Bad URL syntax");
+	}
+
+	return command_loop();
+}
diff --git a/git.c b/git.c
index 50a1401..b7b96b0 100644
--- a/git.c
+++ b/git.c
@@ -374,6 +374,7 @@ static void handle_internal_command(int argc, const char **argv)
 		{ "receive-pack", cmd_receive_pack },
 		{ "reflog", cmd_reflog, RUN_SETUP },
 		{ "remote", cmd_remote, RUN_SETUP },
+		{ "remote-fd", cmd_remote_fd },
 		{ "replace", cmd_replace, RUN_SETUP },
 		{ "repo-config", cmd_config, RUN_SETUP_GENTLY },
 		{ "rerere", cmd_rerere, RUN_SETUP },
-- 
1.7.3.1.48.g4fe83

[PATCH v3 3/3] git-remote-ext

[Ilari Liusvaara]

This remote helper invokes external command and passes raw smart transport
stream through it. This is useful for instance for invoking ssh with
one-off odd options, connecting to git services in unix domain
sockets, in abstract namespace, using TLS or other secure protocols,
etc...

Signed-off-by: Ilari Liusvaara <ilari.liusvaara@elisanet.fi>
---
 .gitignore                       |    1 +
 Documentation/git-remote-ext.txt |  112 ++++++++++++++++
 Makefile                         |    1 +
 builtin.h                        |    1 +
 builtin/remote-ext.c             |  261 ++++++++++++++++++++++++++++++++++++++
 git.c                            |    1 +
 6 files changed, 377 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/git-remote-ext.txt
 create mode 100644 builtin/remote-ext.c

diff --git a/.gitignore b/.gitignore
index 89f37f4..87b833c 100644
--- a/.gitignore
+++ b/.gitignore
@@ -113,6 +113,7 @@
 /git-remote-ftp
 /git-remote-ftps
 /git-remote-fd
+/git-remote-ext
 /git-remote-testgit
 /git-repack
 /git-replace
diff --git a/Documentation/git-remote-ext.txt b/Documentation/git-remote-ext.txt
new file mode 100644
index 0000000..53e8b58
--- /dev/null
+++ b/Documentation/git-remote-ext.txt
@@ -0,0 +1,112 @@
+git-remote-ext(1)
+=================
+
+NAME
+----
+git-remote-ext - Bridge smart transport to external command.
+
+SYNOPSIS
+--------
+git remote add nick "ext::<command>[ <arguments>...]"
+
+DESCRIPTION
+-----------
+This remote helper uses the specified 'program' to connect
+to a remote git server.
+
+Command and arguments are separated by unescaped space.
+
+The following sequences have a special meaning:
+
+'\ '::
+	Literal space in command or argument.
+
+'\\'::
+	Literal backslash.
+
+'\s'::
+	Replaced with name (receive-pack, upload-pack, or
+	upload-archive) of the service git wants to invoke.
+
+'\S'::
+	Replaced with long name (git-receive-pack,
+	git-upload-pack, or git-upload-archive) of the service
+	git wants to invoke.
+
+'\G<repository>' (as argument)::
+	This argument will not be passed to 'program'. Instead, it
+	will cause helper to start by sending git:// service request to
+	remote side with service field set to approiate value and
+	repository field set to <repository>. Default is not to send
+	such request.
++
+This is useful if remote side is git:// server accessed over
+some tunnel.
+
+'\V<host>' (as argument)::
+	This argument will not be passed to 'program'. Instead it sets
+	the vhost field in git:// service request. Default is not to
+	send vhost in such request (if sent).
+
+ENVIRONMENT VARIABLES:
+----------------------
+
+GIT_TRANSLOOP_DEBUG::
+	If set, prints debugging information about various reads/writes.
+
+ENVIRONMENT VARIABLES PASSED TO COMMAND:
+----------------------------------------
+
+GIT_EXT_SERVICE::
+	Set to long name (git-upload-pack, etc...) of service helper needs
+	to invoke.
+
+GIT_EXT_SERVICE_NOPREFIX::
+	Set to long name (upload-pack, etc...) of service helper needs
+	to invoke.
+
+
+EXAMPLES:
+---------
+This remote helper is transparently used by git when
+you use commands such as "git fetch <URL>", "git clone <URL>",
+, "git push <URL>" or "git remote add nick <URL>", where <URL>
+begins with `ext::`.  Examples:
+
+"ext::ssh -i /home/foo/.ssh/somekey user&#64;host.example \S \'foo/repo'"::
+	Like host.example:foo/repo, but use /home/foo/.ssh/somekey as
+	keypair and user as user on remote side. This avoids needing to
+	edit .ssh/config.
+
+"ext::socat -t3600 - ABSTRACT-CONNECT:/git-server \G/somerepo"::
+	Represents repository with path /somerepo accessable over
+	git protocol at abstract namespace address /git-server.
+
+"ext::git-server-alias foo \G/repo"::
+	Represents a repository with path /repo accessed using the
+	helper program "git-server-alias foo".  The path to the
+	repository and type of request are not passed on the command
+	line but as part of the protocol stream, as usual with git://
+	protocol.
+
+"ext::git-server-alias foo \G/repo \Vfoo"::
+	Represents a repository with path /repo accessed using the
+	helper program "git-server-alias foo".  The hostname for the
+	remote server passed in the protocol stream will be "foo"
+	(this allows multiple virtual git servers to share a
+	link-level address).
+
+"ext::git-ssl foo.example /bar"::
+	Represents a repository accessed using the helper program
+	"git-ssl foo.example /bar".  The type of request can be
+	determined by the helper using environment variables (see
+	above).
+
+Documentation
+--------------
+Documentation by Ilari Liusvaara, Jonathan Nieder and the git list
+<git@vger.kernel.org>
+
+GIT
+---
+Part of the linkgit:git[1] suite
diff --git a/Makefile b/Makefile
index 7da54d7..9909ca1 100644
--- a/Makefile
+++ b/Makefile
@@ -728,6 +728,7 @@ BUILTIN_OBJS += builtin/read-tree.o
 BUILTIN_OBJS += builtin/receive-pack.o
 BUILTIN_OBJS += builtin/reflog.o
 BUILTIN_OBJS += builtin/remote.o
+BUILTIN_OBJS += builtin/remote-ext.o
 BUILTIN_OBJS += builtin/remote-fd.o
 BUILTIN_OBJS += builtin/replace.o
 BUILTIN_OBJS += builtin/rerere.o
diff --git a/builtin.h b/builtin.h
index 1a816e1..a4bba61 100644
--- a/builtin.h
+++ b/builtin.h
@@ -108,6 +108,7 @@ extern int cmd_read_tree(int argc, const char **argv, const char *prefix);
 extern int cmd_receive_pack(int argc, const char **argv, const char *prefix);
 extern int cmd_reflog(int argc, const char **argv, const char *prefix);
 extern int cmd_remote(int argc, const char **argv, const char *prefix);
+extern int cmd_remote_ext(int argc, const char **argv, const char *prefix);
 extern int cmd_remote_fd(int argc, const char **argv, const char *prefix);
 extern int cmd_config(int argc, const char **argv, const char *prefix);
 extern int cmd_rerere(int argc, const char **argv, const char *prefix);
diff --git a/builtin/remote-ext.c b/builtin/remote-ext.c
new file mode 100644
index 0000000..46f1781
--- /dev/null
+++ b/builtin/remote-ext.c
@@ -0,0 +1,261 @@
+#include "git-compat-util.h"
+#include "transport.h"
+#include "run-command.h"
+
+/*
+ * URL syntax:
+ *	'command [arg1 [arg2 [...]]]'	Invoke command with given arguments.
+ *	Special characters:
+ *	'\ ': Literal space in argument.
+ *	'\\': Literal backslash.
+ *	'\S': Name of service (git-upload-pack/git-upload-archive/
+ *		git-receive-pack.
+ *	'\s': Same as \s, but with possible git- prefix stripped.
+ *	'\G': Only allowed as first 'character' of argument. Do not pass this
+ *		Argument to command, instead send this as name of repository
+ *		in in-line git://-style request (also activates sending this
+ *		style of request).
+ *	'\V': Only allowed as first 'character' of argument. Used in
+ *		conjunction with '\G': Do not pass this argument to command,
+ *		instead send this as vhost in git://-style request (note: does
+ *		not activate sending git:// style request).
+ */
+
+char* git_req = NULL;
+char* git_req_vhost = NULL;
+
+static char *strip_escapes(const char *str, const char *service,
+	const char **next)
+{
+	char *ret;
+	size_t rpos = 0;
+	size_t wpos = 0;
+	size_t finallen = 0;
+	int escape = 0;
+	char special = 0;
+	size_t pslen = 0;
+	size_t pSlen = 0;
+	size_t psoff = 0;
+
+	/* Calculate prefix length for \s and lengths for \s and \S */
+	if (!strncmp(service, "git-", 4))
+		psoff = 4;
+	pSlen = strlen(service);
+	pslen = pSlen - psoff;
+
+	/* Pass the service to command. */
+	setenv("GIT_EXT_SERVICE", service, 1);
+	setenv("GIT_EXT_SERVICE_NOPREFIX", service + psoff, 1);
+
+	/* Calculate output length and start of next argument. */
+	while (str[rpos] && (escape || str[rpos] != ' ')) {
+		if (escape) {
+			switch (str[rpos]) {
+			case ' ':
+			case '\\':
+				finallen++;
+				break;
+			case 's':
+				finallen += pslen;
+				break;
+			case 'S':
+				finallen += pSlen;
+				break;
+			case 'G':
+			case 'V':
+				special = str[rpos];
+				if (rpos == 1)
+					break;
+				/* Fall-through to error. */
+			default:
+				die("Bad remote-ext placeholder '\\%c'.",
+					str[rpos]);
+			}
+			escape = 0;
+		} else
+			switch (str[rpos]) {
+			case '\\':
+				escape = 1;
+				break;
+			default:
+				finallen++;
+				break;
+			}
+		rpos++;
+	}
+	if (escape && !str[rpos])
+		die("remote-ext command has incomplete placeholder");
+	*next = str + rpos;
+	if (**next == ' ')
+		++*next;	/* Skip over space */
+
+	/*
+	 * Do the actual placeholder substitution. The string will be short
+	 * enough not to overflow integers.
+	 */
+	ret = xmalloc(finallen + 1);
+	rpos = special ? 2 : 0;		/* Skip first 2 bytes in specials. */
+	escape = 0;
+	while (str[rpos] && (escape || str[rpos] != ' ')) {
+		if (escape) {
+			switch(str[rpos]) {
+			case ' ':
+			case '\\':
+				ret[wpos++] = str[rpos];
+				break;
+			case 's':
+				strcpy(ret + wpos, service + psoff);
+				wpos += pslen;
+				break;
+			case 'S':
+				strcpy(ret + wpos, service);
+				wpos += pSlen;
+				break;
+			}
+			escape = 0;
+		} else
+			switch(str[rpos]) {
+			case '\\':
+				escape = 1;
+				break;
+			default:
+				ret[wpos++] = str[rpos];
+				break;
+			}
+		rpos++;
+	}
+	ret[wpos] = 0;
+	switch(special) {
+	case 'G':
+		git_req = ret;
+		return NULL;
+	case 'V':
+		git_req_vhost = ret;
+		return NULL;
+	default:
+		return ret;
+	}
+}
+
+/* Should be enough... */
+#define MAXARGUMENTS 256
+
+static const char **parse_argv(const char *arg, const char *service)
+{
+	int arguments = 0;
+	int i;
+	char** ret;
+	char *(temparray[MAXARGUMENTS + 1]);
+
+	while (*arg) {
+		char* ret;
+		if (arguments == MAXARGUMENTS)
+			die("remote-ext command has too many arguments");
+		ret = strip_escapes(arg, service, &arg);
+		if (ret)
+			temparray[arguments++] = ret;
+	}
+
+	ret = xcalloc(arguments + 1, sizeof(char*));
+	for (i = 0; i < arguments; i++)
+		ret[i] = temparray[i];
+
+	return (const char**)ret;
+}
+
+static void send_git_request(int stdin_fd, const char *serv, const char *repo,
+	const char *vhost)
+{
+	size_t bufferspace;
+	size_t wpos = 0;
+	char* buffer;
+
+	/*
+	 * Request needs 12 bytes extra if there is vhost (xxxx \0host=\0) and
+	 * 6 bytes extra (xxxx \0) if there is no vhost.
+	 */
+	if (vhost)
+		bufferspace = strlen(serv) + strlen(repo) + strlen(vhost) + 12;
+	else
+		bufferspace = strlen(serv) + strlen(repo) + 6;
+
+	if (bufferspace > 0xFFFF)
+		die("Request too large to send");
+	buffer = xmalloc(bufferspace);
+
+	/* Make the packet. */
+	wpos = sprintf(buffer, "%04x%s %s%c", (unsigned)bufferspace,
+		serv, repo, 0);
+
+	/* Add vhost if any. */
+	if (vhost)
+		sprintf(buffer + wpos, "host=%s%c", vhost, 0);
+
+	/* Send the request */
+	if (write_in_full(stdin_fd, buffer, bufferspace) < 0)
+		die_errno("Failed to send request");
+
+	free(buffer);
+}
+
+static int run_child(const char *arg, const char *service)
+{
+	int r;
+	struct child_process child;
+
+	memset(&child, 0, sizeof(child));
+	child.in = -1;
+	child.out = -1;
+	child.err = 0;
+	child.argv = parse_argv(arg, service);
+
+	if (start_command(&child) < 0)
+		die("Can't run specified command");
+
+	if (git_req)
+		send_git_request(child.in, service, git_req, git_req_vhost);
+
+	r = bidirectional_transfer_loop(child.out, child.in);
+	if (!r)
+		r = finish_command(&child);
+	else
+		finish_command(&child);
+	return r;
+}
+
+#define MAXCOMMAND 4096
+
+static int command_loop(const char *child)
+{
+	char buffer[MAXCOMMAND];
+
+	while (1) {
+		if (!fgets(buffer, MAXCOMMAND - 1, stdin))
+			exit(0);
+		/* Strip end of line characters. */
+		while (isspace((unsigned char)buffer[strlen(buffer) - 1]))
+			buffer[strlen(buffer) - 1] = 0;
+
+		if (!strcmp(buffer, "capabilities")) {
+			printf("*connect\n\n");
+			fflush(stdout);
+		} else if (!strncmp(buffer, "connect ", 8)) {
+			printf("\n");
+			fflush(stdout);
+			return run_child(child, buffer + 8);
+		} else {
+			fprintf(stderr, "Bad command");
+			return 1;
+		}
+	}
+}
+
+int cmd_remote_ext(int argc, const char **argv, const char *prefix)
+{
+	if (argc < 3) {
+		fprintf(stderr, "Error: URL missing");
+		exit(1);
+	}
+
+	return command_loop(argv[2]);
+}
diff --git a/git.c b/git.c
index b7b96b0..e95a1ba 100644
--- a/git.c
+++ b/git.c
@@ -374,6 +374,7 @@ static void handle_internal_command(int argc, const char **argv)
 		{ "receive-pack", cmd_receive_pack },
 		{ "reflog", cmd_reflog, RUN_SETUP },
 		{ "remote", cmd_remote, RUN_SETUP },
+		{ "remote-ext", cmd_remote_ext },
 		{ "remote-fd", cmd_remote_fd },
 		{ "replace", cmd_replace, RUN_SETUP },
 		{ "repo-config", cmd_config, RUN_SETUP_GENTLY },
-- 
1.7.3.1.48.g4fe83

Re: [PATCH v3 1/3] Add bidirectional_transfer_loop()

[Jonathan Nieder]

Hi,

Ilari Liusvaara wrote:

> This helper function copies bidirectional stream of data between
> stdin/stdout and specified file descriptors.

>From this description, I am expecting something to the effect of

 sendfile(output, input, NULL, SIZE_MAX);

but this is much longer than that.  Why?  Let's see...

> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -862,3 +862,257 @@ int transport_helper_init(struct transport *transport, const char *name)
>  	transport->smart_options = &(data->transport_options);
>  	return 0;
>  }
> +
> +
> +#define BUFFERSIZE 4096
> +#define PBUFFERSIZE 8192

Magic numbers.  Where do these come from?  Were they tuned or are they
off the top of the head?  (Either is fine; it just is nice to know.)

> +/* Print bidirectional transfer loop debug message. */
> +static void transfer_debug(const char *fmt, ...)
> +{
> +	va_list args;
> +	char msgbuf[PBUFFERSIZE];
> +	static int debug_enabled = -1;
> +
> +	if (debug_enabled < 0)
> +		debug_enabled = getenv("GIT_TRANSLOOP_DEBUG") ? 1 : 0;
> +	if (!debug_enabled)
> +		return;
> +
> +	sprintf(msgbuf, "Transfer loop debugging: ");
> +	va_start(args, fmt);
> +	vsprintf(msgbuf + strlen(msgbuf), fmt, args);
> +	va_end(args);
> +	fprintf(stderr, "%s\n", msgbuf);
> +}

Why this instead of just vfprintf?  (vreportf() in usage.c
does the same thing; maybe there is some portability reason?)

This can overflow if the caller is not careful.

Might be clearer to write

	va_start(args, fmt);
	vsnprintf(msgbuf, sizeof(msgbuf), fmt, args);
	va_end(args);
	vfprintf(stderr, "Transfer loop debugging: %s\n", msgbuf);

like vreportf() does.

> +/* Load the parameters into poll structure. Return number of entries loaded */

Not clear to me what this comment means.  What is this function used for?

> +static int load_poll_params(struct pollfd *polls, size_t inbufuse,
> +	size_t outbufuse, int in_hup, int out_hup, int in_closed,
> +	int out_closed, int socket_mode, int input_fd, int output_fd)

Scary.  Maybe a params struct would make callers easier to understand?

What the parameters mean (I'm guessing):

	polls - buffer for file descriptors to poll on.  There should
	        be room for 4.
	inbufuse - < BUFFERSIZE if we can handle more input on stdin
	outbufuse - < BUFFERSIZE if we can handle more input on input_fd
		--- why is this called "out"?
	in_hup - whether the other end of stdin has already hung up
	out_hup - whether the other end of input_fd has already hung up
	in_closed - ???
	out_closed - ???
	socket_mode - true if input_fd == output_fd
	input_fd, output_fd - file descriptors

[...]
> +	if (!in_hup && inbufuse < BUFFERSIZE) {
> +		stdin_index = nextindex++;
> +		polls[stdin_index].fd = 0;
> +		transfer_debug("Adding stdin to fds to wait for");
> +	}
> +	if (!out_hup && outbufuse < BUFFERSIZE) {
> +		input_index = nextindex++;
> +		polls[input_index].fd = input_fd;
> +		transfer_debug("Adding remote input to fds to wait for");
> +	}
> +	if (!out_closed && outbufuse > 0) {
> +		stdout_index = nextindex++;
> +		polls[stdout_index].fd = 1;
> +		transfer_debug("Adding stdout to fds to wait for");
> +	}

Repetitive.  Maybe this could be factored out as a mini-function:

	consider_waiting_for(in_hup, inbufuse, &stdin_index,
	                     nextindex, polls, 0, "stdin");

Hmm, never mind.

[...]
> +}
> +
> +static int transfer_handle_events(struct pollfd* polls, char *in_buffer,
> +	char *out_buffer, size_t *in_buffer_use, size_t *out_buffer_use,
> +	int *in_hup, int *out_hup, int *in_closed, int *out_closed,
> +	int socket_mode, int poll_count, int input, int output)

Scarier.

> +{
> +	int i, r;
> +	for(i = 0; i < poll_count; i++) {

Long loop.  What is it for?  (A comment might help.)  Maybe the body
could get its own function?

> +		/* Handle stdin. */
> +		if (polls[i].fd == 0 && polls[i].revents & (POLLIN | POLLHUP)) {

Or better, the code to handle each event might get its own function.

This one reads as much data as possible into in_buffer[], taking
care to handle EOF appropriately.

[...]
> +		}
> +
> +		/* Handle remote end input. */
> +		if (polls[i].fd == input &&
> +			polls[i].revents & (POLLIN | POLLHUP)) {

This one reads as much data as possible into out_buffer[], taking
care to handle EOF appropriately.  Presumably out_buffer[] means
data scheduled for output.

> +			transfer_debug("remote input is readable");
> +			r = read(input, out_buffer + *out_buffer_use,
> +				BUFFERSIZE - *out_buffer_use);
> +			if (r < 0 && errno != EWOULDBLOCK && errno != EAGAIN &&
> +				errno != EINTR) {
> +				perror("read(connection) failed");
> +				return 1;

Why use perror() instead of error() which can be overridden by
setting error_routine?  Why return 1 instead of the usual -1
for error?

> +			} else if (r == 0) {
> +				transfer_debug("remote input EOF");
> +				*out_hup = 1;
> +				if (!*out_buffer_use) {
> +					close(1);
> +					*out_closed = 1;
> +					transfer_debug("Closed stdout");
> +				} else
> +					transfer_debug("Delaying stdout close because output buffer has data");

Why is stdout closed here?  Could that be taken care of later
by checking *out_hup?

[...]
> +			r = write(1, out_buffer, *out_buffer_use);
[...]
> +				if (*out_buffer_use > 0)
> +					memmove(out_buffer, out_buffer + r,
> +						*out_buffer_use);

This only writes as much data to stdout as one write() allows,
to avoid deadlock, presumably.

> +				if (*out_hup && !*out_buffer_use) {
> +					close(1);

After each write() we check *out_hup, but only after a write.  Would
be simpler to unconditionally check *out_hup and avoid the duplicate
code before; would that slow this down?

> +		/* Handle remote end output. */

Dual to the above.

[...]
> +/* Copy data from stdin to output and from input to stdout. */

Ah.  I think this belongs in the commit message, too. :)

Still I wonder "why".  What is the motivational example?

> +int bidirectional_transfer_loop(int input, int output)
> +{
[...]
> +	while (1) {

A typical poll loop.  Nothing scary here.

> +		int r;
> +		poll_count = load_poll_params(polls, in_buffer_use,
> +			out_buffer_use, in_hup, out_hup, in_closed, out_closed,
> +			socket_mode, input, output);
> +		if (!poll_count) {
> +			transfer_debug("Transfer done");
> +			break;

			return 0;

would avoid the reader having to scroll down, I think.

Okay, so we actually have the effect of

 sendfile(output_fd, 0, NULL, SIZE_MAX);
 sendfile(1, input_fd, NULL, SIZE_MAX);

interleaved to avoid deadlock.  In other words, this interchanges
input for output file descriptors.  The main application is remote-fd,
which needs to do this to forward input to and output from a parent
process.

For remote-ext, for a moment one might imagine one could avoid the
trouble by letting the child inherit stdin and stdout.  Unfortunately,
remote-ext needs to be able to prepend some data to its child's
input stream.  So the effect of

 sendfile(output_fd, 0, NULL, SIZE_MAX);

is still necessary (output_fd is a pipe pointing to the child's
standard input), and to time this to avoid deadlock, it still needs
to be interleaved with

 sendfile(1, input_fd, NULL, SIZE_MAX);

Hope that helps.

Re: [PATCH v3 2/3] git-remote-fd

[Jonathan Nieder]

Hi again,

Ilari Liusvaara wrote:

> This remote helper reflects raw smart remote transport stream back to the
> calling program.

Fundamentally I think remote-fd (unlike remote-ext) suffers from the
"without-a-user" problem: without an example making use of this, it is
hard to know whether we have the interface right.

For example, might the caller want to use the usual in-stream
service identification as in git:// protocol in some cases?  Should
it would be possible to do

 some-complex-command fd::3

where some-complex-command makes multiple requests?

So personally I would be happier to see remote-fd in contrib/ until
we have at least some experience of what it's like to use.

Anyway, now the documentation is clearer (thanks!).  Some nitpicks
follow; patch at end.

> --- /dev/null
> +++ b/Documentation/git-remote-fd.txt
> @@ -0,0 +1,57 @@
> +git-remote-fd(1)
> +=================
> +
> +NAME
> +----
> +git-remote-fd - Reflect smart transport back to caller.

Has an extra period.  Might be worth including the word "stream":

	git-remote-fd - Reflect a smart transport stream back to caller

This describes what the helper does rather than the URL scheme...

> +SYNOPSIS
> +--------
> +"fd::<fd>[/<anything>]" or "fd::<infd>,<outfd>[/<anything>]" (as URL)

... so maybe the synopsis should describe the remote helper?

	'git remote-fd' <name> <infd>[,<outfd>][/<comment>]

> +
> +DESCRIPTION
> +-----------
> +This helper uses specified file descriptors to connect to remote git server.

Maybe it would make sense to clarify that this is not for end-users.
(see patch)

[...]
> +ENVIRONMENT VARIABLES:
> +----------------------
> +
> +$GIT_TRANSLOOP_DEBUG (passed to git)::
> +	If set, prints debugging information about various reads/writes.
> +

Nits: other manpages do not use the $ or trailing colon

> +EXAMPLES:
> +---------
> +"fd::17" (as URL)::
> +	Connect using socket in file descriptor #17.
[...]

Maybe full commands would be easier to read:

	`git fetch fd::17 master`::
		Fetch branch master, using a socket with file descriptor 17
		to communicate with 'git upload-pack'.

> --- /dev/null
> +++ b/builtin/remote-fd.c
> @@ -0,0 +1,76 @@
[...]
> +static int command_loop()

nit: static int command_loop(void)

Might even be simpler to return void.

> +{
> +	char buffer[MAXCOMMAND];
> +
> +	while (1) {
> +		if (!fgets(buffer, MAXCOMMAND - 1, stdin))
> +			exit(0);

This code path is used for errors, no?

		if (!fgets(buffer, MAXCOMMAND - 1, stdin)) {
			if (ferror(stdin))
				die_errno("input error");
			exit(0);
		}

> +		/* Strip end of line characters. */
> +		while (isspace((unsigned char)buffer[strlen(buffer) - 1]))
> +			buffer[strlen(buffer) - 1] = 0;

Git isspace does not require the cast.  Won't the repeated strlen()
be slow?

		{
			char *p = buffer + strlen(buffer);
			while (p > buffer && isspace(*--p))
				*p = 0;
		}

> +
> +		if (!strcmp(buffer, "capabilities")) {
> +			printf("*connect\n\n");
> +			fflush(stdout);
> +		} else if (!strncmp(buffer, "connect ", 8)) {
> +			printf("\n");
> +			fflush(stdout);
> +			return bidirectional_transfer_loop(input_fd,
> +				output_fd);

If this returns nonzero, that's a fatal error, right?  So

			if (bidir...)
				exit(128);
			exit(0);

> +		} else {
> +			fprintf(stderr, "Bad command");
> +			return 1;

			die("bad command %s", buffer);

might be more useful.

> +int cmd_remote_fd(int argc, const char **argv, const char *prefix)
> +{
> +	char* end;
> +	unsigned long r;

The * should stick to the variable name.

> +	r = strtoul(argv[2], &end, 10);
> +	input_fd = (int)r;

Why not just

	input_fd = (int) strtoul(...

?

> +	if ((*end != ',' && *end !='/' && *end) || end == argv[2])

I find

	if (end == argv[2] || (*end && ...

more idiomatic, but that is serious nitpicking now.

Anyway, perhaps some of the below can be useful.
---
diff --git a/Documentation/git-remote-fd.txt b/Documentation/git-remote-fd.txt
index c4c0da1..b305c26 100644
--- a/Documentation/git-remote-fd.txt
+++ b/Documentation/git-remote-fd.txt
@@ -3,53 +3,68 @@ git-remote-fd(1)
 
 NAME
 ----
-git-remote-fd - Reflect smart transport back to caller.
-
+git-remote-fd - Reflect a smart transport stream back to caller
 
 SYNOPSIS
 --------
-"fd::<fd>[/<anything>]" or "fd::<infd>,<outfd>[/<anything>]" (as URL)
+'git remote-fd' <remote> (<fd> | <in_fd>,<out_fd>)[/<comment>]
 
 DESCRIPTION
 -----------
-This helper uses specified file descriptors to connect to remote git server.
+This is not a command the end user would ever want to run.  This
+documentation is meant for people who are writing Porcelain-ish
+programs that need to intercept the data that 'git' transfers.
+
+The 'git remote-fd' helper is used by git to handle requests for a
+repository with a URL such as `fd::3/git.example.com` (see
+linkgit:git-remote-helpers[7]).  Such requests are handled by reading
+from and writing to file descriptors inherited from the parent process.
 
-Data written to <fd> or <outfd> is assumed to make it unmolested to
-git-upload-pack/git-receive-pack/git-upload-archive, and data from that
-program is assumed to be readable unmolested from <fd> or <infd>.
+Data written to '<fd>' or '<out_fd>' is assumed to make it unmolested
+to 'git upload-pack', 'git receive-pack', or 'git upload-archive',
+and data from that program is assumed to be readable unmolested
+from '<fd>' or '<in_fd>'.
 
-If just <fd> is specified, <fd> is assumed to be socket. Otherwise both
-<infd> and <outfd> are assumed to be pipes.
+If only '<fd>' is specified, '<fd>' is assumed to be a socket.
+Otherwise both '<in_fd>' and '<out_fd>' are assumed to be pipes.
 
-It is assumed that any handshaking procedures have already been completed
-(such as sending service request for git://) before this helper is started.
+It is assumed that any handshaking procedures (such as sending a
+service request in `git://` protocol) have already been completed
+before this helper is started.
 
-<anything> can be any string. It is ignored. It is meant for provoding
-information to user in the "URL".
+The '<comment>' following a trailing `/` can be any string.
+It is ignored. It is meant for providing information to the user
+in the URL, in case the URL gets included in an error message
+shown to the end user.
 
-ENVIRONMENT VARIABLES:
-----------------------
+ENVIRONMENT VARIABLES
+---------------------
 
-$GIT_TRANSLOOP_DEBUG (passed to git)::
-	If set, prints debugging information about various reads/writes.
+'GIT_TRANSLOOP_DEBUG' (passed to git)::
+	If this is set, 'git' will print copious debugging information
+	about the transport data it reads and writes.
 
-EXAMPLES:
----------
-"fd::17" (as URL)::
-	Connect using socket in file descriptor #17.
+EXAMPLES
+--------
+`git fetch fd::17 master`::
+	Fetch branch master, using a socket with file descriptor 17
+	to communicate with 'git upload-pack'.
 
-"fd::17/foo" (as URL)::
+`git fetch fd::17/foo master`::
 	Same as above.
 
-"fd::7,8" (as URL)::
-	Connect using pipes in file descriptors #7 and #8. The incoming
-	pipe is at fd #7 and the outgoing pipe at fd #8.
+`git push fd::7,8 master`::
+	Push branch master, using two pipes with file descriptors 7
+	and 8 to communicate with 'git receive-pack'.
+	'git receive-pack'{apostrophe}s output should be readable
+	via the pipe on fd 7 and its input connected to the outgoing
+	pipe on fd 8.
 
-"fd::7,8/bar" (as URL)::
+`git push fd::7,8/bar master`::
 	Same as above.
 
 Documentation
---------------
+-------------
 Documentation by Ilari Liusvaara and the git list <git@vger.kernel.org>
 
 GIT
diff --git a/builtin/remote-fd.c b/builtin/remote-fd.c
index 44c174b..7bc62be 100644
--- a/builtin/remote-fd.c
+++ b/builtin/remote-fd.c
@@ -16,21 +16,28 @@
  *
  */
 
+static const char builtin_remote_fd_usage[] =
+	"git remote-fd <repository> (<fd> | <in_fd>,<out_fd>)[/<comment>]";
+
 int input_fd = -1;
 int output_fd = -1;
 
-#define MAXCOMMAND 4096
-
-static int command_loop()
+static NORETURN void command_loop(void)
 {
-	char buffer[MAXCOMMAND];
+	char buffer[4096];
 
-	while (1) {
-		if (!fgets(buffer, MAXCOMMAND - 1, stdin))
+	for (;;) {
+		if (!fgets(buffer, sizeof(buffer) - 1, stdin)) {
+			if (ferror(stdin))
+				die_errno("input error");
 			exit(0);
+		}
 		/* Strip end of line characters. */
-		while (isspace((unsigned char)buffer[strlen(buffer) - 1]))
-			buffer[strlen(buffer) - 1] = 0;
+		{
+			char *p = buffer + strlen(buffer);
+			while (p > buffer && isspace(*--p))
+				*p = 0;
+		}
 
 		if (!strcmp(buffer, "capabilities")) {
 			printf("*connect\n\n");
@@ -38,39 +45,34 @@ static int command_loop()
 		} else if (!strncmp(buffer, "connect ", 8)) {
 			printf("\n");
 			fflush(stdout);
-			return bidirectional_transfer_loop(input_fd,
-				output_fd);
+			if (bidirectional_transfer_loop(input_fd, output_fd))
+				exit(128);
+			exit(0);
 		} else {
-			fprintf(stderr, "Bad command");
-			return 1;
+			die("bad command %s", buffer);
 		}
 	}
 }
 
 int cmd_remote_fd(int argc, const char **argv, const char *prefix)
 {
-	char* end;
-	unsigned long r;
+	char *end, *end2;
 
 	if (argc < 3)
-		die("URL missing");
-
-	r = strtoul(argv[2], &end, 10);
-	input_fd = (int)r;
+		usage(builtin_remote_fd_usage);
 
-	if ((*end != ',' && *end !='/' && *end) || end == argv[2])
+	input_fd = (int) strtoul(argv[2], &end, 10);
+	if (end == argv[2] || (*end && *end != ',' && *end != '/'))
 		die("Bad URL syntax");
 
-	if (*end == '/' || !*end) {
+	if (*end != ',') {	/* fd::fd[/comment] */
 		output_fd = input_fd;
-	} else {
-		char* end2;
-		r = strtoul(end + 1, &end2, 10);
-		output_fd = (int)r;
-
-		if ((*end2 !='/' && *end2) || end2 == end + 1)
-			die("Bad URL syntax");
+		command_loop();
 	}
 
-	return command_loop();
+	/* fd::in_fd,out_fd[/comment] */
+	output_fd = (int) strtoul(end + 1, &end2, 10);
+	if (end2 == end + 1 || (*end2 && *end2 != '/'))
+		die("Bad URL syntax");
+	command_loop();
 }
--