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

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

[Mike Hommey]

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

[PATCH 1/4] git_connect: extend to take a pseudo format string for the program to run

[Mike Hommey]

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 <mh@glandium.org>
---
 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

[PATCH 2/4] git_connect: avoid quoting the path on the command line when it's not necessary

[Mike Hommey]

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 <mh@glandium.org>
---
 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

[PATCH 3/4] git_connect: allow a file descriptor to be allocated for stderr

[Mike Hommey]

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 <mh@glandium.org>
---
 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

[PATCH 4/4] git_connect: add a flag to consider the path part of ssh urls relative

[Mike Hommey]

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 <mh@glandium.org>
---
 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

Re: [PATCH 2/4] git_connect: avoid quoting the path on the command line when it's not necessary

[Stefan Beller]

On Thu, Apr 28, 2016 at 7:12 AM, Mike Hommey <mh@glandium.org> 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?

> Signed-off-by: Mike Hommey <mh@glandium.org>
> ---
>  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
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Junio C Hamano]

Mike Hommey <mh@glandium.org> writes:

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

While all of the above sounds like a good thing to do, what I do not
understand is why you need to even touch git_connect() at all, and
we certainly do *not* want you to touch it to make its external
interface unnecessarily ugly and complex with features that are only
necessary if it needs to talk to non-git services.

In other words, why can't this cinnabar thing live on the other side
of transport API, just like Git transport itself does not know about
cURL and HTTP when talking with https:// repositories?

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

[Mike Hommey]

On Thu, Apr 28, 2016 at 10:41:12AM -0700, Junio C Hamano wrote:
> Mike Hommey <mh@glandium.org> writes:
> 
> > 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.
> 
> While all of the above sounds like a good thing to do, what I do not
> understand is why you need to even touch git_connect() at all, and
> we certainly do *not* want you to touch it to make its external
> interface unnecessarily ugly and complex with features that are only
> necessary if it needs to talk to non-git services.
> 
> In other words, why can't this cinnabar thing live on the other side
> of transport API, just like Git transport itself does not know about
> cURL and HTTP when talking with https:// repositories?

It does live on the other side of transport API. The changes are not
about git itself talking to mercurial servers. They are about a remote
helper (git-cinnabar), using native code based on git core code, to talk
to mercurial servers. Because I'd rather bend git_connect a little, if I
can, than copy it (as well as parse_connect_url). Because I want to
benefit from all tweaks it has (handling GIT_SSH/GIT_SSH_COMMAND
properly, handling tortoiseplink/plink properly, handling IPv6 forms of
the ssh url properly, ...)

Now, tweaking git_connect is one possible way to do what I want. Another
would be to make parse_connect_url non-static, and move the ssh command
line construction to a separate (non-static) function. Something with a
signature like:
  void prepare_ssh_command(struct argv_array **command,
                           const char *hostandport)

that would essentially do the part of git_connect that is in the if
(protocol == PROTO_SSH) block (not sure how CONNECT_DIAG_URL would fit
in this, though).

Thoughts?

Mike

[RFC PATCH 1/3] connect: make parse_connect_url public

[Mike Hommey]

Signed-off-by: Mike Hommey <mh@glandium.org>
---
 connect.c | 11 ++---------
 connect.h |  9 +++++++++
 2 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/connect.c b/connect.c
index c53f3f1..29569b3 100644
--- a/connect.c
+++ b/connect.c
@@ -231,13 +231,6 @@ int server_supports(const char *feature)
 	return !!server_feature_value(feature, NULL);
 }
 
-enum protocol {
-	PROTO_LOCAL = 1,
-	PROTO_FILE,
-	PROTO_SSH,
-	PROTO_GIT
-};
-
 int url_is_local_not_ssh(const char *url)
 {
 	const char *colon = strchr(url, ':');
@@ -591,8 +584,8 @@ static char *get_port(char *host)
  * Extract protocol and relevant parts from the specified connection URL.
  * The caller must free() the returned strings.
  */
-static enum protocol parse_connect_url(const char *url_orig, char **ret_host,
-				       char **ret_path)
+enum protocol parse_connect_url(const char *url_orig, char **ret_host,
+				char **ret_path)
 {
 	char *url;
 	char *host, *path;
diff --git a/connect.h b/connect.h
index 01f14cd..dede6e8 100644
--- a/connect.h
+++ b/connect.h
@@ -1,6 +1,15 @@
 #ifndef CONNECT_H
 #define CONNECT_H
 
+enum protocol {
+	PROTO_LOCAL = 1,
+	PROTO_FILE,
+	PROTO_SSH,
+	PROTO_GIT
+};
+enum protocol parse_connect_url(const char *url_orig, char **ret_host,
+				char **ret_path);
+
 #define CONNECT_VERBOSE       (1u << 0)
 #define CONNECT_DIAG_URL      (1u << 1)
 #define CONNECT_IPV4          (1u << 2)
-- 
2.8.1.8.gc23d642.dirty

[RFC PATCH 2/3] connect: group CONNECT_DIAG_URL handling code

[Mike Hommey]

Signed-off-by: Mike Hommey <mh@glandium.org>
---
 connect.c | 27 ++++++++++++---------------
 1 file changed, 12 insertions(+), 15 deletions(-)

Note this makes http://marc.info/?l=git&m=146183714532394 irrelevant.


diff --git a/connect.c b/connect.c
index 29569b3..ce216cb 100644
--- a/connect.c
+++ b/connect.c
@@ -676,10 +676,20 @@ struct child_process *git_connect(int fd[2], const char *url,
 	signal(SIGCHLD, SIG_DFL);
 
 	protocol = parse_connect_url(url, &hostandport, &path);
-	if ((flags & CONNECT_DIAG_URL) && (protocol != PROTO_SSH)) {
+	if (flags & CONNECT_DIAG_URL) {
 		printf("Diag: url=%s\n", url ? url : "NULL");
 		printf("Diag: protocol=%s\n", prot_name(protocol));
-		printf("Diag: hostandport=%s\n", hostandport ? hostandport : "NULL");
+		if (protocol == PROTO_SSH) {
+			char *ssh_host = hostandport;
+			const char *port = NULL;
+			get_host_and_port(&ssh_host, &port);
+			if (!port)
+				port = get_port(ssh_host);
+			printf("Diag: userandhost=%s\n", ssh_host ? ssh_host : "NULL");
+			printf("Diag: port=%s\n", port ? port : "NONE");
+		} else {
+			printf("Diag: hostandport=%s\n", hostandport ? hostandport : "NULL");
+		}
 		printf("Diag: path=%s\n", path ? path : "NULL");
 		conn = NULL;
 	} else if (protocol == PROTO_GIT) {
@@ -738,19 +748,6 @@ struct child_process *git_connect(int fd[2], const char *url,
 			if (!port)
 				port = get_port(ssh_host);
 
-			if (flags & CONNECT_DIAG_URL) {
-				printf("Diag: url=%s\n", url ? url : "NULL");
-				printf("Diag: protocol=%s\n", prot_name(protocol));
-				printf("Diag: userandhost=%s\n", ssh_host ? ssh_host : "NULL");
-				printf("Diag: port=%s\n", port ? port : "NONE");
-				printf("Diag: path=%s\n", path ? path : "NULL");
-
-				free(hostandport);
-				free(path);
-				free(conn);
-				return NULL;
-			}
-
 			ssh = getenv("GIT_SSH_COMMAND");
 			if (!ssh) {
 				const char *base;
-- 
2.8.1.8.gc23d642.dirty

[RFC PATCH 3/3] connect: move ssh command line preparation to a separate (public) function

[Mike Hommey]

Signed-off-by: Mike Hommey <mh@glandium.org>
---
 connect.c | 108 +++++++++++++++++++++++++++++++++-----------------------------
 connect.h |   2 ++
 2 files changed, 60 insertions(+), 50 deletions(-)

diff --git a/connect.c b/connect.c
index ce216cb..c71563f 100644
--- a/connect.c
+++ b/connect.c
@@ -649,6 +649,62 @@ enum protocol parse_connect_url(const char *url_orig, char **ret_host,
 	return protocol;
 }
 
+int prepare_ssh_command(struct argv_array *cmd, char *hostandport, int flags)
+{
+	const char *ssh;
+	int putty = 0, tortoiseplink = 0, use_shell = 1;
+	char *ssh_host = hostandport;
+	const char *port = NULL;
+	get_host_and_port(&ssh_host, &port);
+
+	if (!port)
+		port = get_port(ssh_host);
+
+	ssh = getenv("GIT_SSH_COMMAND");
+	if (!ssh) {
+		const char *base;
+		char *ssh_dup;
+
+		/*
+		 * GIT_SSH is the no-shell version of
+		 * GIT_SSH_COMMAND (and must remain so for
+		 * historical compatibility).
+		 */
+		use_shell = 0;
+
+		ssh = getenv("GIT_SSH");
+		if (!ssh)
+			ssh = "ssh";
+
+		ssh_dup = xstrdup(ssh);
+		base = basename(ssh_dup);
+
+		tortoiseplink = !strcasecmp(base, "tortoiseplink") ||
+			!strcasecmp(base, "tortoiseplink.exe");
+		putty = tortoiseplink ||
+			!strcasecmp(base, "plink") ||
+			!strcasecmp(base, "plink.exe");
+
+		free(ssh_dup);
+	}
+
+	argv_array_push(cmd, ssh);
+	if (flags & CONNECT_IPV4)
+		argv_array_push(cmd, "-4");
+	else if (flags & CONNECT_IPV6)
+		argv_array_push(cmd, "-6");
+	if (tortoiseplink)
+		argv_array_push(cmd, "-batch");
+	if (port) {
+		/* P is for PuTTY, p is for OpenSSH */
+		argv_array_push(cmd, putty ? "-P" : "-p");
+		argv_array_push(cmd, port);
+	}
+	argv_array_push(cmd, ssh_host);
+
+	return use_shell;
+}
+
 static struct child_process no_fork = CHILD_PROCESS_INIT;
 
 /*
@@ -738,57 +794,9 @@ struct child_process *git_connect(int fd[2], const char *url,
 		conn->use_shell = 1;
 		conn->in = conn->out = -1;
 		if (protocol == PROTO_SSH) {
-			const char *ssh;
-			int putty = 0, tortoiseplink = 0;
-			char *ssh_host = hostandport;
-			const char *port = NULL;
 			transport_check_allowed("ssh");
-			get_host_and_port(&ssh_host, &port);
-
-			if (!port)
-				port = get_port(ssh_host);
-
-			ssh = getenv("GIT_SSH_COMMAND");
-			if (!ssh) {
-				const char *base;
-				char *ssh_dup;
-
-				/*
-				 * GIT_SSH is the no-shell version of
-				 * GIT_SSH_COMMAND (and must remain so for
-				 * historical compatibility).
-				 */
-				conn->use_shell = 0;
-
-				ssh = getenv("GIT_SSH");
-				if (!ssh)
-					ssh = "ssh";
-
-				ssh_dup = xstrdup(ssh);
-				base = basename(ssh_dup);
-
-				tortoiseplink = !strcasecmp(base, "tortoiseplink") ||
-					!strcasecmp(base, "tortoiseplink.exe");
-				putty = tortoiseplink ||
-					!strcasecmp(base, "plink") ||
-					!strcasecmp(base, "plink.exe");
-
-				free(ssh_dup);
-			}
-
-			argv_array_push(&conn->args, ssh);
-			if (flags & CONNECT_IPV4)
-				argv_array_push(&conn->args, "-4");
-			else if (flags & CONNECT_IPV6)
-				argv_array_push(&conn->args, "-6");
-			if (tortoiseplink)
-				argv_array_push(&conn->args, "-batch");
-			if (port) {
-				/* P is for PuTTY, p is for OpenSSH */
-				argv_array_push(&conn->args, putty ? "-P" : "-p");
-				argv_array_push(&conn->args, port);
-			}
-			argv_array_push(&conn->args, ssh_host);
+			conn->use_shell = prepare_ssh_command(
+				&conn->args, hostandport, flags);
 		} else {
 			transport_check_allowed("file");
 		}
diff --git a/connect.h b/connect.h
index dede6e8..5392102 100644
--- a/connect.h
+++ b/connect.h
@@ -10,6 +10,8 @@ enum protocol {
 enum protocol parse_connect_url(const char *url_orig, char **ret_host,
 				char **ret_path);
 
+int prepare_ssh_command(struct argv_array *cmd, char *hostandport, int flags);
+
 #define CONNECT_VERBOSE       (1u << 0)
 #define CONNECT_DIAG_URL      (1u << 1)
 #define CONNECT_IPV4          (1u << 2)
-- 
2.8.1.8.gc23d642.dirty

Re: [RFC PATCH 2/3] connect: group CONNECT_DIAG_URL handling code

[Junio C Hamano]

Mike Hommey <mh@glandium.org> writes:

> Signed-off-by: Mike Hommey <mh@glandium.org>

I feel that this commit is under-explained.  I think you should feel
entitled to boast the goodness this brings to us louder in the log
message.

It bothers me somewhat that this ended up copying, not moving, a bit
of code to call get-host-and-port helper, but I do not think it is a
problem and it makes the codeflow easier to follow.  Attempt to
refactor it to reduce the duplication is likely to make it worse.

We used to allocate and prepare the child process structure 'conn',
then realized that we are not going to use it anyway and discarded,
only because the DIAG_URL check for SSH transport was done way too
late.  That wastage is removed by this change as well.

Another change I notice is that DIAG_URL code for PROTO_SSH did not
even kick in if transport_check_allowed("ssh") said no, but with
this new code Diag is always given, which makes it consistent with
PROTO_GIT codepath.

> ---
>  connect.c | 27 ++++++++++++---------------
>  1 file changed, 12 insertions(+), 15 deletions(-)
>
> Note this makes http://marc.info/?l=git&m=146183714532394 irrelevant.

Indeed.

>
> diff --git a/connect.c b/connect.c
> index 29569b3..ce216cb 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -676,10 +676,20 @@ struct child_process *git_connect(int fd[2], const char *url,
>  	signal(SIGCHLD, SIG_DFL);
>  
>  	protocol = parse_connect_url(url, &hostandport, &path);
> -	if ((flags & CONNECT_DIAG_URL) && (protocol != PROTO_SSH)) {
> +	if (flags & CONNECT_DIAG_URL) {
>  		printf("Diag: url=%s\n", url ? url : "NULL");
>  		printf("Diag: protocol=%s\n", prot_name(protocol));
> -		printf("Diag: hostandport=%s\n", hostandport ? hostandport : "NULL");
> +		if (protocol == PROTO_SSH) {
> +			char *ssh_host = hostandport;
> +			const char *port = NULL;
> +			get_host_and_port(&ssh_host, &port);
> +			if (!port)
> +				port = get_port(ssh_host);
> +			printf("Diag: userandhost=%s\n", ssh_host ? ssh_host : "NULL");
> +			printf("Diag: port=%s\n", port ? port : "NONE");
> +		} else {
> +			printf("Diag: hostandport=%s\n", hostandport ? hostandport : "NULL");
> +		}
>  		printf("Diag: path=%s\n", path ? path : "NULL");
>  		conn = NULL;
>  	} else if (protocol == PROTO_GIT) {
> @@ -738,19 +748,6 @@ struct child_process *git_connect(int fd[2], const char *url,
>  			if (!port)
>  				port = get_port(ssh_host);
>  
> -			if (flags & CONNECT_DIAG_URL) {
> -				printf("Diag: url=%s\n", url ? url : "NULL");
> -				printf("Diag: protocol=%s\n", prot_name(protocol));
> -				printf("Diag: userandhost=%s\n", ssh_host ? ssh_host : "NULL");
> -				printf("Diag: port=%s\n", port ? port : "NONE");
> -				printf("Diag: path=%s\n", path ? path : "NULL");
> -
> -				free(hostandport);
> -				free(path);
> -				free(conn);
> -				return NULL;
> -			}
> -
>  			ssh = getenv("GIT_SSH_COMMAND");
>  			if (!ssh) {
>  				const char *base;

Re: [RFC PATCH 3/3] connect: move ssh command line preparation to a separate (public) function

[Junio C Hamano]

Mike Hommey <mh@glandium.org> writes:

> Signed-off-by: Mike Hommey <mh@glandium.org>
> ---

I think moving it out into a separate helper function is a very good
change to make the end result easier to follow.

Turning the helper to extern and adding it to connect.h does not
benefit us (and [PATCH 1/3] doesn't, either).

And in the longer term, I doubt think it (and [PATCH 1/3]) would
benefit you, either.  We do not mean to make what goes into libgit.a
(which connect.o is a part of) be a stable "library interface", so
all sorts of future changes we make would affect your external code,
e.g.

 - some people periodically check for extern symbols that do not
   have to be extern, and this will be marked static again any time
   (or you somehow need to arrange it so that this function is
   marked as special to reduce false positives);

 - we may need to update the external interface to this function,
   changing parameters, or updating its return value.

even if we promise not to make such a change only for the purpose of
deliberately breaking you.  So you have to be prepared and willing
to follow the evolution of in-core code anyway.

So I would suggest restructuring this series to do

 * 2/3 (DIAG consolidation)
 * refactoring in 3/3 but not s/static/extern/

and in optional follow(s)-up, do

 * s/static/extern/ and update to *.h in 3/3

 * 1/3, but I do not think it is necessary for users of
   prepare_ssh_command()

The last patch could be kept in your fork of git.git if you want to
keep linking with libgit.a, but my feeling is that we'd prefer to do
without it at least for now, until we gain in-tree users that live
outside connect.c that want to call the helper.


Thanks.

Re: [RFC PATCH 2/3] connect: group CONNECT_DIAG_URL handling code

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Mike Hommey <mh@glandium.org> writes:
>
>> Signed-off-by: Mike Hommey <mh@glandium.org>
>
> I feel that this commit is under-explained.  I think you should feel
> entitled to boast the goodness this brings to us louder in the log
> message.
>
> It bothers me somewhat that this ended up copying, not moving, a bit
> of code to call get-host-and-port helper, but I do not think it is a
> problem and it makes the codeflow easier to follow.  Attempt to
> refactor it to reduce the duplication is likely to make it worse.
>

I hate to add a noise to the list, but while re-re-re-editing this
message before sending it out, I accidentally dropped an important
line here.  I said (and then removed by mistake):

	I like what this patch does.

> We used to allocate and prepare the child process structure 'conn',
> then realized that we are not going to use it anyway and discarded,
> only because the DIAG_URL check for SSH transport was done way too
> late.  That wastage is removed by this change as well.
>
> Another change I notice is that DIAG_URL code for PROTO_SSH did not
> even kick in if transport_check_allowed("ssh") said no, but with
> this new code Diag is always given, which makes it consistent with
> PROTO_GIT codepath.

Re: [RFC PATCH 2/3] connect: group CONNECT_DIAG_URL handling code

[Torsten Bögershausen]

On 29.04.16 02:43, Mike Hommey wrote:
> get_host_and_port(&ssh_host, &port);
> +			if (!port)
> +				port = get_port(ssh_host);

I'm not sure, if this is an improvement or not.

The original intention was, to check what the parser did, before
going out to the external program (like ssh) or opening a socket.

Everything should be prepared, as if the action had been taken,
and in the last nanosecond we jump out and print what would have
been done.
(The nanosecond may depend on the CPU speed)

In any case, for SSH there should be only one line calling get_host_and_port(),
or get_port().

This is to check the complete logic path in test cases, as well as it allows
the user to check the URL, especially when using SSH.

Splitting the logic into a path where we just print and one, where
the connection is really made, looses "single point of truth" - we don't know
any longer, what we test or print for diag.