threads / patch / 42170

patch, 4 partsgit_connect: extend to take a pseudo format string for the program to run

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

## tl;dr

15 messages between Apr 28, 2016 and Apr 29, 2016. Diffs are folded; open one to read it.

replies: 14people: 4as markdown or json

Mike Hommey· Apr 28, 2016, 14:12 UTC · lore

[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
Mike Hommey· Apr 28, 2016, 14:12 UTC · re: Mike Hommey · lore

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(-)
Show changes to connect.c +24 −5
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
Mike Hommey· Apr 28, 2016, 14:12 UTC · re: Mike Hommey · lore

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

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(+)
Show changes to connect.c +11 −0
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
Stefan Beller· Apr 28, 2016, 16:14 UTC · re: Mike Hommey · lore

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

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

Show 34 quoted lines
> 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
Mike Hommey· Apr 28, 2016, 14:12 UTC · re: Mike Hommey · lore

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

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(+)
Show changes to 2 files +3 −0

connect.c, connect.h

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
Mike Hommey· Apr 28, 2016, 14:12 UTC · re: Mike Hommey · lore

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

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(-)
Show changes to 2 files +8 −3

connect.c, connect.h

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
Junio C Hamano· Apr 28, 2016, 17:41 UTC · re: Mike Hommey · lore

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

Mike Hommey <mh@glandium.org> writes:
Show 8 quoted lines
> 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?

Mike Hommey· Apr 28, 2016, 23:29 UTC · re: Junio C Hamano · lore

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

On Thu, Apr 28, 2016 at 10:41:12AM -0700, Junio C Hamano wrote:
Show 20 quoted lines
> 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
Mike Hommey· Apr 29, 2016, 00:43 UTC · re: Mike Hommey · lore

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

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

connect.c, connect.h

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
Mike Hommey· Apr 29, 2016, 00:43 UTC · re: Mike Hommey · lore

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

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.
Show changes to connect.c +12 −15
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
Junio C Hamano· Apr 29, 2016, 15:59 UTC · re: Mike Hommey · lore

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

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.

Show 5 quoted lines
> ---
>  connect.c | 27 ++++++++++++---------------
>  1 file changed, 12 insertions(+), 15 deletions(-)
>
> Note this makes http://marc.info/?l=git&m=146183714532394 irrelevant.
Indeed.
Show 48 quoted lines
>
> 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;
Junio C Hamano· Apr 29, 2016, 17:13 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 13 quoted lines
> 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.
Show 9 quoted lines
> 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.
Torsten Bögershausen· Apr 29, 2016, 20:00 UTC · re: Mike Hommey · lore

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

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.

Mike Hommey· Apr 29, 2016, 00:43 UTC · re: Mike Hommey · lore

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

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

connect.c, connect.h

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
Junio C Hamano· Apr 29, 2016, 16:58 UTC · re: Mike Hommey · lore

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

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.

← back to recent threads