[RFC PATCH 0/4] git_connect: add some flexibility

As you may be aware, I'm working on a git remote helper to access
mercurial repositories (https://github.com/glandium/git-cinnabar/).

At the moment, a small part is written in C, relying on the git code
base, but eventually, there would be more C.

As I want to get rid of the dependency on Mercurial itself, I'm planning
to implement the wire protocol parts in git-cinnabar. And while at it, I
figured I'd evaluate if I can't just rely on some git internals, from C
code. So I've turned to the git_connect function, that implements the
niceties around GIT_SSH and GIT_SSH_COMMAND, and also handles ssh client
specificities. (I'd rather not have to copy the code or reimplement it).
It also turns out to be a convenient wrapper around start_command() for
local urls.

The git commands that git_connect is invoked for all take the repository
path as their last argument. In mercurial's case, the command is:
  hg -R $path serve --stdio

which doesn't match that pattern. So one hack I was thinking about was
scan the url on my end, extract the path, replace it with "--stdio",
and pass "hg -R $path serve" as command. Unfortunately, parse_connect_url
is static, which means I'd either have to change connect.c to expose it,
or copy it. Since I'd rather avoid copying code, I figured that since I
was going to have to change connect.c, I might as well go with something
less hacky, assuming it's accepted mainline.

So following here are four patches that allow me to connect, via ssh, to
hg.mozilla.org, and access mercurial repositories there using:

  git_connect(fd, url, "hg -R %s serve --stdio",
              CONNECT_RELATIVE_SSH | CONNECT_WANT_STDERR)

And this works for local urls too, invoking `hg serve` locally.

Note that what the second patch does could be done in sq_quote_buf
instead, arguably.

I'm certainly open to any better ideas as long as they can make it to
mainline :).

Mike Hommey (4):
  git_connect: extend to take a pseudo format string for the program to
    run
  git_connect: avoid quoting the path on the command line when it's not
    necessary
  git_connect: allow a file descriptor to be allocated for stderr
  git_connect: add a flag to consider the path part of ssh urls relative

 connect.c | 52 ++++++++++++++++++++++++++++++++++++++++++++--------
 connect.h |  2 ++
 2 files changed, 46 insertions(+), 8 deletions(-)

-- 
2.8.1.5.g18c8a48

Currently, the path extracted from the url is passed as last argument to
the program/command passed to git_connect(). In every case the function
is used in the git code base, it's enough, but in order to allow the
reuse of e.g. the GIT_SSH/GIT_SSH_COMMAND logic, additional flexibility
is welcome.

With this change, when the program/command passed to git_connect()
contains a "%s", that "%s" is replaced with the path from the url,
allowing the path to be at a different position than last on the
executed command line.

Signed-off-by: Mike Hommey <redacted>
---
 connect.c | 29 ++++++++++++++++++++++++-----
 1 file changed, 24 insertions(+), 5 deletions(-)

diff --git a/connect.c b/connect.c
index dccf673..96c8c1d 100644
--- a/connect.c
+++ b/connect.c

@@ -658,6 +658,25 @@ static enum protocol parse_connect_url(const char *url_orig, char **ret_host,
 
 static struct child_process no_fork = CHILD_PROCESS_INIT;
 
+static void prepare_connect_command(struct strbuf *cmd, const char *prog,
+                                    const char *path, int quote)
+{
+	const char *found = strstr(prog, "%s");
+	if (found)
+		strbuf_add(cmd, prog, found - prog);
+	else {
+		strbuf_addstr(cmd, prog);
+		strbuf_addch(cmd, ' ');
+	}
+	if (quote)
+		sq_quote_buf(cmd, path);
+	else
+		strbuf_addstr(cmd, path);
+
+	if (found)
+		strbuf_addstr(cmd, found + 2);
+}
+
 /*
  * This returns a dummy child_process if the transport protocol does not
  * need fork(2), or a struct child_process object if it does.  Once done,

@@ -717,18 +736,18 @@ struct child_process *git_connect(int fd[2], const char *url,
 		 * Note: Do not add any other headers here!  Doing so
 		 * will cause older git-daemon servers to crash.
 		 */
+		prepare_connect_command(&cmd, prog, path, 0);
 		packet_write(fd[1],
-			     "%s %s%chost=%s%c",
-			     prog, path, 0,
+			     "%s%chost=%s%c",
+			     cmd.buf, 0,
 			     target_host, 0);
+		strbuf_release(&cmd);
 		free(target_host);
 	} else {
 		conn = xmalloc(sizeof(*conn));
 		child_process_init(conn);
 
-		strbuf_addstr(&cmd, prog);
-		strbuf_addch(&cmd, ' ');
-		sq_quote_buf(&cmd, path);
+		prepare_connect_command(&cmd, prog, path, 1);
 
 		/* remove repo-local variables from the environment */
 		conn->env = local_repo_env;

-- 
2.8.1.5.g18c8a48

In Mercurial ssh urls, the path part of the url is relative to the home
directory of the account being logged to instead of being absolute.

Add a flag allowing git_connect() to handle this kind of usecase.

Signed-off-by: Mike Hommey <redacted>
---
 connect.c | 10 +++++++---
 connect.h |  1 +
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/connect.c b/connect.c
index 9feedd8..0df6297 100644
--- a/connect.c
+++ b/connect.c

@@ -592,7 +592,7 @@ static char *get_port(char *host)
  * The caller must free() the returned strings.
  */
 static enum protocol parse_connect_url(const char *url_orig, char **ret_host,
-				       char **ret_path)
+				       char **ret_path, int relative_ssh)
 {
 	char *url;
 	char *host, *path;

@@ -642,7 +642,10 @@ static enum protocol parse_connect_url(const char *url_orig, char **ret_host,
 	end = path; /* Need to \0 terminate host here */
 	if (separator == ':')
 		path++; /* path starts after ':' */
-	if (protocol == PROTO_GIT || protocol == PROTO_SSH) {
+	if (protocol == PROTO_SSH && relative_ssh) {
+		if (path[0] == separator)
+			path++;
+	} else if (protocol == PROTO_GIT || protocol == PROTO_SSH) {
 		if (path[1] == '~')
 			path++;
 	}

@@ -712,7 +715,8 @@ struct child_process *git_connect(int fd[2], const char *url,
 	 */
 	signal(SIGCHLD, SIG_DFL);
 
-	protocol = parse_connect_url(url, &hostandport, &path);
+	protocol = parse_connect_url(url, &hostandport, &path,
+	                             flags & CONNECT_RELATIVE_SSH);
 	if ((flags & CONNECT_DIAG_URL) && (protocol != PROTO_SSH)) {
 		printf("Diag: url=%s\n", url ? url : "NULL");
 		printf("Diag: protocol=%s\n", prot_name(protocol));

diff --git a/connect.h b/connect.h
index fb3331b..1377028 100644
--- a/connect.h
+++ b/connect.h

@@ -6,6 +6,7 @@
 #define CONNECT_IPV4          (1u << 2)
 #define CONNECT_IPV6          (1u << 3)
 #define CONNECT_WANT_STDERR   (1u << 4)
+#define CONNECT_RELATIVE_SSH  (1u << 5)
 extern struct child_process *git_connect(int fd[2], const char *url, const char *prog, int flags);
 extern int finish_connect(struct child_process *conn);
 extern int git_connection_is_socket(struct child_process *conn);

-- 
2.8.1.5.g18c8a48

Some remote systems can employ restricted shells that aren't very smart
with quotes, so avoid quoting when it's not strictly necessary.

The list of "safe" characters comes from Mercurial's shell quoting
function used for its ssh client side. There likely are more that could
be added to the list.

Signed-off-by: Mike Hommey <redacted>
---
 connect.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/connect.c b/connect.c
index 96c8c1d..919bf9e 100644
--- a/connect.c
+++ b/connect.c

@@ -668,6 +668,17 @@ static void prepare_connect_command(struct strbuf *cmd, const char *prog,
 		strbuf_addstr(cmd, prog);
 		strbuf_addch(cmd, ' ');
 	}
+	if (quote) {
+		const char *p;
+		for (p = path; *p; p++) {
+			if (!isalnum(*p) && *p != '@' && *p != '%' &&
+			    *p != '_' && *p != '+' && *p != '=' && *p != ':' &&
+			    *p != ',' && *p != '.' && *p != '/' && *p != '-')
+				break;
+		}
+		if (!*p)
+			quote = 0;
+	}
 	if (quote)
 		sq_quote_buf(cmd, path);
 	else

-- 
2.8.1.5.g18c8a48

It can be useful to the caller of git_connect() to get access to stderr,
so add a flag that makes start_command allocate a file descriptor for
it.

Signed-off-by: Mike Hommey <redacted>
---
 connect.c | 2 ++
 connect.h | 1 +
 2 files changed, 3 insertions(+)

diff --git a/connect.c b/connect.c
index 919bf9e..9feedd8 100644
--- a/connect.c
+++ b/connect.c

@@ -764,6 +764,8 @@ struct child_process *git_connect(int fd[2], const char *url,
 		conn->env = local_repo_env;
 		conn->use_shell = 1;
 		conn->in = conn->out = -1;
+		if (flags & CONNECT_WANT_STDERR)
+			conn->err = -1;
 		if (protocol == PROTO_SSH) {
 			const char *ssh;
 			int putty = 0, tortoiseplink = 0;

diff --git a/connect.h b/connect.h
index 01f14cd..fb3331b 100644
--- a/connect.h
+++ b/connect.h

@@ -5,6 +5,7 @@
 #define CONNECT_DIAG_URL      (1u << 1)
 #define CONNECT_IPV4          (1u << 2)
 #define CONNECT_IPV6          (1u << 3)
+#define CONNECT_WANT_STDERR   (1u << 4)
 extern struct child_process *git_connect(int fd[2], const char *url, const char *prog, int flags);
 extern int finish_connect(struct child_process *conn);
 extern int git_connection_is_socket(struct child_process *conn);

-- 
2.8.1.5.g18c8a48

On Thu, Apr 28, 2016 at 7:12 AM, Mike Hommey [off-list ref] wrote:

Some remote systems can employ restricted shells that aren't very smart
with quotes, so avoid quoting when it's not strictly necessary.

The list of "safe" characters comes from Mercurial's shell quoting
function used for its ssh client side. There likely are more that could
be added to the list.

Would it make sense to move the new code into its own function and
document it with this paragraph of the commit message, i.e. hinting
at Mercurial safe characters or others?