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

[PATCH v2 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...


Differences from last time:

* The mingw build patch is now included.
* Include some documentation.
* Patches squashed into more logical chunks instead of base and discrete
fixes.

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

 Documentation/git-remote-ext.txt |   87 +++++++++++++
 Documentation/git-remote-fd.txt  |   57 +++++++++
 Makefile                         |    2 +
 builtin.h                        |    2 +
 builtin/remote-fd.c              |   88 +++++++++++++
 compat/mingw.h                   |    5 +
 git.c                            |    2 +
 transport-helper.c               |  254 ++++++++++++++++++++++++++++++++++++++
 transport.h                      |    1 +
 9 files changed, 498 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/git-remote-ext.txt
 create mode 100644 Documentation/git-remote-fd.txt
 create mode 100644 builtin/remote-fd.c

-- 
1.7.2.3.401.g919b6e

[PATCH v2 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.2.3.401.g919b6e

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

[Jonathan Nieder]

Ilari Liusvaara wrote:

> --- a/transport-helper.c
> +++ b/transport-helper.c
[...]
> +	if (!in_hup && inbufuse < BUFFERSIZE) {
> +		stdin_index = nextindex++;
> +		polls[stdin_index].fd = 0;
> +		transfer_debug("Adding stdin to fds to wait for");

Why not:

		trace_printf("trace: adding stdin to ...");

?  That would give the user control of where tracing output goes
(settings like GIT_TRACE=17).

If trace is too noisy (I don't think it is), maybe we should
make it more nuanced, like GIT_TRACE=17:transport?

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

[Ilari Liusvaara]

On Thu, Sep 30, 2010 at 08:55:02AM -0500, Jonathan Nieder wrote:
> Ilari Liusvaara wrote:
> 
> Why not:
> 
> 		trace_printf("trace: adding stdin to ...");
> 
> ?  That would give the user control of where tracing output goes
> (settings like GIT_TRACE=17).
> 
> If trace is too noisy (I don't think it is), maybe we should
> make it more nuanced, like GIT_TRACE=17:transport?

The debugging output is hellishly noisy. I just (successfully) tried to clone
git repo from local copy. The result was no less than 106 773 debugging
messages...

-Ilari

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

[Jonathan Nieder]

Ilari Liusvaara wrote:

> The debugging output is hellishly noisy. I just (successfully) tried to clone
> git repo from local copy. The result was no less than 106 773 debugging
> messages...

Ah, okay.  I suppose there is no need to teach this to share tracing
infrastructure (e.g., generalizing get_trace_fd()) now; that can
be done later if someone decides they need the flexibility.

[PATCH v2 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>
---
 Documentation/git-remote-fd.txt |   57 +++++++++++++++++++++++++
 Makefile                        |    1 +
 builtin.h                       |    1 +
 builtin/remote-fd.c             |   88 +++++++++++++++++++++++++++++++++++++++
 git.c                           |    1 +
 5 files changed, 148 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/git-remote-fd.txt
 create mode 100644 builtin/remote-fd.c

diff --git a/Documentation/git-remote-fd.txt b/Documentation/git-remote-fd.txt
new file mode 100644
index 0000000..12e588a
--- /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 command uses specified file descriptors to connect to remote git server.
+
+If just <fd> is specified, <fd> is assumed to be socket that is
+transparently connected to git server program.
+
+If <infd> and <outfd> are specified, <infd> is assumed to be pipe from
+git server program and <outfd> is assumed to be pipe to git server program.
+
+It is assumed that any handshaking procedures have already been completed
+(such as sending service request for git://).
+
+<anything> can be any string. It is ignored. It is meant for provoding
+information to user in form of "URL".
+
+ENVIRONMENT VARIABLES:
+----------------------
+
+$GIT_TRANSLOOP_DEBUG (passed to git)::
+	If set, prints debugging information about various reads/writes.
+
+
+EXAMPLES:
+---------
+"fd::17"::
+	Connect using socket in file descriptor #17.
+
+"fd::17/foo"::
+	Same as above.
+
+"fd::7,8"::
+	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"::
+	Same as above.
+
+Documentation
+--------------
+Documentation by Ilari Liusvaara.
+
+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..748cc13 100644
--- a/builtin.h
+++ b/builtin.h
@@ -140,5 +140,6 @@ extern int cmd_verify_pack(int argc, const char **argv, const char *prefix);
 extern int cmd_show_ref(int argc, const char **argv, const char *prefix);
 extern int cmd_pack_refs(int argc, const char **argv, const char *prefix);
 extern int cmd_replace(int argc, const char **argv, const char *prefix);
+extern int cmd_remote_fd(int argc, const char **argv, const char *prefix);
 
 #endif
diff --git a/builtin/remote-fd.c b/builtin/remote-fd.c
new file mode 100644
index 0000000..08ff522
--- /dev/null
+++ b/builtin/remote-fd.c
@@ -0,0 +1,88 @@
+#include "git-compat-util.h"
+#include "transport.h"
+#include <errno.h>
+#include <stdlib.h>
+#include <string.h>
+#include <stdio.h>
+#include <unistd.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) {
+		fprintf(stderr, "Error: URL missing");
+		exit(1);
+	}
+
+	r = strtoul(argv[2], &end, 10);
+	input_fd = (int)r;
+
+	if ((*end != ',' && *end !='/' && *end) || end == argv[2]) {
+		fprintf(stderr, "Error: Bad URL syntax");
+		exit(1);
+	}
+
+	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) {
+			fprintf(stderr, "Error: Bad URL syntax");
+			exit(1);
+		}
+	}
+
+	return command_loop();
+}
diff --git a/git.c b/git.c
index 50a1401..250ecc5 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, 0 },
 		{ "replace", cmd_replace, RUN_SETUP },
 		{ "repo-config", cmd_config, RUN_SETUP_GENTLY },
 		{ "rerere", cmd_rerere, RUN_SETUP },
-- 
1.7.2.3.401.g919b6e

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

[Ævar Arnfjörð Bjarmason]

On Thu, Sep 30, 2010 at 11:52, Ilari Liusvaara
<ilari.liusvaara@elisanet.fi> wrote:

> +               //Strip end of line characters.

Don't use a C++/C99 comment.

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

[Jonathan Nieder]

Hi Ilari,

Ilari Liusvaara wrote:

> 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.

Do you ever use this directly, or is it a "because we can" sort of thing?
Whenever I try to come up with an example, ext:: seems to be easier.

An example (ideally a test) would be nice.

> +++ b/Documentation/git-remote-fd.txt
> @@ -0,0 +1,57 @@
[...]
> +"fd::<fd>[/<anything>]" or "fd::<infd>,<outfd>[/<anything>]" (as URL)
[...]
> +<anything> can be any string. It is ignored. It is meant for provoding
> +information to user in form of "URL".

Is this meant for future expansion?  Why not just use a comment in
.git/config?

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

For the curious: introduced by patch 1.

> +EXAMPLES:
> +---------
> +"fd::17"::
> +	Connect using socket in file descriptor #17.

Maybe with more context?

	socat ... "git push fd::3 HEAD" ...

> +Documentation
> +--------------
> +Documentation by Ilari Liusvaara.

I think one tends to write "Documentation by <person> and the git list
<git@vger.kernel.org>" these days.

Why is this section needed at all?  Doesn't the changelog already
record the relevant information?

> --- /dev/null
> +++ b/builtin/remote-fd.c
> @@ -0,0 +1,88 @@
> +#include "git-compat-util.h"
> +#include "transport.h"
> +#include <errno.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <stdio.h>
> +#include <unistd.h>

Doesn't git-compat-util.h imply these?

> + * 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>.

Maybe worth mentioning in the manual?

> +static int command_loop()

Simple, I like it. :)

> +int cmd_remote_fd(int argc, const char **argv, const char *prefix)
> +{
> +	char* end;
> +	unsigned long r;
> +
> +	if (argc < 3) {
> +		fprintf(stderr, "Error: URL missing");
> +		exit(1);

Why not use usage() or die()?  (Just curious.)

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

[Ilari Liusvaara]

On Thu, Sep 30, 2010 at 08:14:40AM -0500, Jonathan Nieder wrote:
> Hi Ilari,
> 
> Ilari Liusvaara wrote:
> 
> > 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.
> 
> Do you ever use this directly, or is it a "because we can" sort of thing?
> Whenever I try to come up with an example, ext:: seems to be easier.

I don't have any UIs that would want to override SSH, git:// or something
like that, so no. ext:: sees lots of internal use.

> > +<anything> can be any string. It is ignored. It is meant for provoding
> > +information to user in form of "URL".
> 
> Is this meant for future expansion?  Why not just use a comment in
> .git/config?

These URLs are not meant to go into .git/config.
 
> > +EXAMPLES:
> > +---------
> > +"fd::17"::
> > +	Connect using socket in file descriptor #17.
> 
> Maybe with more context?
> 
> 	socat ... "git push fd::3 HEAD" ...

More like something execing git, having open connection to some git repo.

If you want stuff like that (using socat to establish connections), use
ext::

-Ilari

[PATCH v2 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>
---
 Documentation/git-remote-ext.txt |   87 ++++++++++++++++++++++++++++++++++++++
 Makefile                         |    1 +
 builtin.h                        |    1 +
 git.c                            |    1 +
 4 files changed, 90 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/git-remote-ext.txt

diff --git a/Documentation/git-remote-ext.txt b/Documentation/git-remote-ext.txt
new file mode 100644
index 0000000..e2d40d9
--- /dev/null
+++ b/Documentation/git-remote-ext.txt
@@ -0,0 +1,87 @@
+git-remote-ext(1)
+=================
+
+NAME
+----
+git-remote-ext - Bridge smart transport to external command.
+
+
+SYNOPSIS
+--------
+"ext::<command>[ <arguments>...]" (as URL)
+
+DESCRIPTION
+-----------
+This command uses specified command to connect to remote git server.
+
+Between <command> and <arguments> (if present) is space. Also space
+splits different arguments.
+
+The following sequences have special meaning:
+
+'\ '::
+	Don't interpret the space as command/argument separator.
+
+'\\'::
+	Literal backslash
+
+'\s' (as argument)::
+	Replaced by short name (receive-pack, upload-pack, upload-archive)
+	of service git wants to invoke.
+
+'\S' (as argument)::
+	Replaced by long name (git-receive-pack, git-upload-pack,
+	git-upload-archive) of service git wants to invoke.
+
+'\G<repository>' (as argument)::
+	This argument will not be passed to command. Instead, git will send
+	in-line git:// service request for <repository>. Default is not to
+	send in-line request.
+
+'\V<host>' (as argument)::
+	Set the vhost used in in-line git:// service request. Default is
+	to omit vhost.
+
+
+ENVIRONMENT VARIABLES:
+----------------------
+
+$GIT_EXT_SERVICE (passed to command)::
+	Initialzed to long name of service git wants to invoke.
+
+$GIT_EXT_SERVICE_NOPREFIX (passed to command)::
+	Initialzed to short name of service git wants to invoke.
+
+$GIT_TRANSLOOP_DEBUG (passed to git)::
+	If set, prints debugging information about various reads/writes.
+
+
+EXAMPLES:
+---------
+"ext::ssh -i /home/foo/.ssh/somekey user@host.example \S \'foo/repo'"::
+	Use  /home/foo/.ssh/somekey as key when connecting to host.example
+	and request repo foo/repo.
+
+"ext::socat -t3600 - ABSTRACT-CONNECT:/git-server \G/somerepo"::
+	Connect to git:// server named '/git-server' in abstract namespace
+	and request '/somerepo' from it.
+
+"ext::git-server-alias foo \G/repo"::
+	Connect to wherever 'git-server-alias foo' connects to and send
+	git:// request there for '/repo'.
+
+"ext::git-server-alias foo \G/repo \Vfoo"::
+	Connect to wherever 'git-server-alias foo' connects to and send
+	git:// request there for '/repo' using vhost 'foo'.
+
+"ext::git-ssl foo.example /bar"::
+	Connect to whatever repo 'git-ssl foo.example /bar' goes.
+
+
+Documentation
+--------------
+Documentation by Ilari Liusvaara.
+
+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 748cc13..b15b486 100644
--- a/builtin.h
+++ b/builtin.h
@@ -141,5 +141,6 @@ extern int cmd_show_ref(int argc, const char **argv, const char *prefix);
 extern int cmd_pack_refs(int argc, const char **argv, const char *prefix);
 extern int cmd_replace(int argc, const char **argv, const char *prefix);
 extern int cmd_remote_fd(int argc, const char **argv, const char *prefix);
+extern int cmd_remote_ext(int argc, const char **argv, const char *prefix);
 
 #endif
diff --git a/git.c b/git.c
index 250ecc5..867d8cf 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, 0 },
 		{ "remote-fd", cmd_remote_fd, 0 },
 		{ "replace", cmd_replace, RUN_SETUP },
 		{ "repo-config", cmd_config, RUN_SETUP_GENTLY },
-- 
1.7.2.3.401.g919b6e

Re: [PATCH v2 3/3] git-remote-ext

[Jonathan Nieder]

Ilari Liusvaara wrote:

> 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...

Tunneling, too (e.g., native git protocol passing through draconian
firewall), right?

>  Documentation/git-remote-ext.txt |   87 ++++++++++++++++++++++++++++++++++++++
>  Makefile                         |    1 +
>  builtin.h                        |    1 +
>  git.c                            |    1 +
>  4 files changed, 90 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/git-remote-ext.txt

Where is the implementation?

> +++ b/Documentation/git-remote-ext.txt
> @@ -0,0 +1,87 @@
> +git-remote-ext(1)
> +=================
> +
> +NAME
> +----
> +git-remote-ext - Bridge smart transport to external command.
> +
> +
> +SYNOPSIS
> +--------
> +"ext::<command>[ <arguments>...]" (as URL)

Maybe:

	git remote add nick "ext::<program>[ <arguments>...]"

as a concrete example.

> +
> +DESCRIPTION
> +-----------
> +This command uses specified command to connect to remote git server.

 - Most users won't invoke remote-ext directly, right?
 - Missing articles ('the' and 'a').
 - Missing formatting ('command' is passed on the command line).

So maybe:

	This remote helper uses the specified 'program' to connect
	to a remote git server.

> +
> +Between <command> and <arguments> (if present) is space. Also space
> +splits different arguments.

	Arguments should be separated by a single unescaped space.

Do I understand correctly?

> +
> +The following sequences have special meaning:

Missing article:

	... have a special meaning:

> +
> +'\ '::
> +	Don't interpret the space as command/argument separator.

'\ '::
	Literal space in 'program' or an argument.

> +
> +'\\'::
> +	Literal backslash

Missing period.

> +'\s' (as argument)::
> +	Replaced by short name (receive-pack, upload-pack, upload-archive)
> +	of service git wants to invoke.

'\s'::
	Name (receive-pack, upload-pack, or upload-archive) of the
	service git wants to invoke.  Can only be used as an entire
	argument (like "ext::foo \s", not "ext::foo BLAH\sBLAH"),

Is that right?

> +'\S' (as argument)::
> +	Replaced by long name (git-receive-pack, git-upload-pack,
> +	git-upload-archive) of service git wants to invoke.

'\S'::
	Long name (git-receive-pack, ...

Does this really mean "name + 'git-'", or does it respect the
fetch-pack --upload-pack et al options?

> +'\G<repository>' (as argument)::
> +	This argument will not be passed to command. Instead, git will send
> +	in-line git:// service request for <repository>. Default is not to
> +	send in-line request.

'\G<repository>'::
	This argument will not be passed to 'program'. Instead, ...

Huh?  What is an in-line git://service request?

> +'\V<host>' (as argument)::
> +	Set the vhost used in in-line git:// service request. Default is
> +	to omit vhost.

Likewise.

> +ENVIRONMENT VARIABLES:
> +----------------------
> +
> +$GIT_EXT_SERVICE (passed to command)::
> +	Initialzed to long name of service git wants to invoke.

The existing manual pages tend to use 'italics' and leave out the $
here.

Maybe the environment passed to the command deserves its own section?
Just nitpicking.

s/Initialzed/Initialized/?  s/long name/the long name/? etc.

> +EXAMPLES:
> +---------

Maybe some introductory text would help.  E.g:

	This remote helper is transparently used by git when
	you use commands such as "git fetch <URL>" where <URL>
	begins with `ext::`.  Examples:

> +"ext::ssh -i /home/foo/.ssh/somekey user@host.example \S \'foo/repo'"::
> +	Use  /home/foo/.ssh/somekey as key when connecting to host.example
> +	and request repo foo/repo.

Probably worth mentioning this avoids adding a nickname and stanza
for this remote in ~/.ssh/config?

An address doesn't really request anything on its own.  Maybe saying
what they point to would be clearer?

	Represents a repository accessible at host.example:foo/repo
	when connecting as user "user" with private key "~foo/.ssh/somekey".

> +"ext::socat -t3600 - ABSTRACT-CONNECT:/git-server \G/somerepo"::
> +	Connect to git:// server named '/git-server' in abstract namespace
> +	and request '/somerepo' from it.

	Represents a repository with path /somerepo accessible over
	git protocol at Unix-domain socket address "/git-server".

> +"ext::git-server-alias foo \G/repo"::
> +	Connect to wherever 'git-server-alias foo' connects to and send
> +	git:// request there for '/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"::
> +	Connect to wherever 'git-server-alias foo' connects to and send
> +	git:// request there for '/repo' using vhost 'foo'.

	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"::
> +	Connect to whatever repo 'git-ssl foo.example /bar' goes.

	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).

> --- a/Makefile
> +++ b/Makefile
[...]
> --- a/builtin.h
> +++ b/builtin.h
[...]
This boilerplate looks good, but where's the command?

> --- 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, 0 },
>  		{ "remote-fd", cmd_remote_fd, 0 },

The style of surrounding entries is to leave off the "0" where it can
be inferred like this.

Thanks for a pleasant read.

Re: [PATCH v2 3/3] git-remote-ext

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Ilari Liusvaara wrote:
>
>> 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...
>
> Tunneling, too (e.g., native git protocol passing through draconian
> firewall), right?
>
>>  Documentation/git-remote-ext.txt |   87 ++++++++++++++++++++++++++++++++++++++
>>  Makefile                         |    1 +
>>  builtin.h                        |    1 +
>>  git.c                            |    1 +
>>  4 files changed, 90 insertions(+), 0 deletions(-)
>>  create mode 100644 Documentation/git-remote-ext.txt
>
> Where is the implementation?
>
>> +++ b/Documentation/git-remote-ext.txt
>> @@ -0,0 +1,87 @@
>> +git-remote-ext(1)
>> +=================
>> +
>> +NAME
>> +----
>> +git-remote-ext - Bridge smart transport to external command.

Is "remote-ext" a git subcommand that "git help --all" would want to show?

>> +SYNOPSIS
>> +--------
>> +"ext::<command>[ <arguments>...]" (as URL)
>
> Maybe:
>
> 	git remote add nick "ext::<program>[ <arguments>...]"
>
> as a concrete example.

Ahh.  I wasted minutes and hears scratching my head to figure out what
that "(as URL)" was about.

>> +
>> +DESCRIPTION
>> +-----------
>> +This command uses specified command to connect to remote git server.
>
>  - Most users won't invoke remote-ext directly, right?
>  - Missing articles ('the' and 'a').
>  - Missing formatting ('command' is passed on the command line).
>
> So maybe:
>
> 	This remote helper uses the specified 'program' to connect
> 	to a remote git server.
>
>> +
>> +Between <command> and <arguments> (if present) is space. Also space
>> +splits different arguments.
>
> 	Arguments should be separated by a single unescaped space.
>
> Do I understand correctly?
>
>> +
>> +The following sequences have special meaning:
>
> Missing article:
>
> 	... have a special meaning:
>
>> +
>> +'\ '::
>> +	Don't interpret the space as command/argument separator.

Is it just me who finds the placeholders with backslashes somewhat out of
place where most other placeholders in git are per-cent prefixed?

Re: [PATCH v2 3/3] git-remote-ext

[Jonathan Nieder]

Junio C Hamano wrote:

> Is it just me who finds the placeholders with backslashes somewhat out of
> place where most other placeholders in git are per-cent prefixed?

"\ " and "\\" seemed sane to me but the other ones left me more
puzzled.

It would be nicer when reading the code if it used strbuf_expand(),
too.