{"thread":{"id":"42170","subject":"[PATCH 1/4] git_connect: extend to take a pseudo format string for the program to run","startedAt":"2016-04-28T14:12:35Z","lastAt":"2016-04-29T20:00:52Z","messageCount":15,"participants":["Mike Hommey","Stefan Beller","Junio C Hamano","Torsten Bögershausen"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"284810","messageId":"1461852759-28429-1-git-send-email-mh@glandium.org","threadId":"42170","inReplyTo":null,"subject":"[RFC PATCH 0/4] git_connect: add some flexibility","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-04-28T14:12:35Z","receivedAt":"2016-04-28T14:12:35Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"As you may be aware, I'm working on a git remote helper to access\nmercurial repositories (https://github.com/glandium/git-cinnabar/).\n\nAt the moment, a small part is written in C, relying on the git code\nbase, but eventually, there would be more C.\n\nAs I want to get rid of the dependency on Mercurial itself, I'm planning\nto implement the wire protocol parts in git-cinnabar. And while at it, I\nfigured I'd evaluate if I can't just rely on some git internals, from C\ncode. So I've turned to the git_connect function, that implements the\nniceties around GIT_SSH and GIT_SSH_COMMAND, and also handles ssh client\nspecificities. (I'd rather not have to copy the code or reimplement it).\nIt also turns out to be a convenient wrapper around start_command() for\nlocal urls.\n\nThe git commands that git_connect is invoked for all take the repository\npath as their last argument. In mercurial's case, the command is:\n  hg -R $path serve --stdio\n\nwhich doesn't match that pattern. So one hack I was thinking about was\nscan the url on my end, extract the path, replace it with \"--stdio\",\nand pass \"hg -R $path serve\" as command. Unfortunately, parse_connect_url\nis static, which means I'd either have to change connect.c to expose it,\nor copy it. Since I'd rather avoid copying code, I figured that since I\nwas going to have to change connect.c, I might as well go with something\nless hacky, assuming it's accepted mainline.\n\nSo following here are four patches that allow me to connect, via ssh, to\nhg.mozilla.org, and access mercurial repositories there using:\n\n  git_connect(fd, url, \"hg -R %s serve --stdio\",\n              CONNECT_RELATIVE_SSH | CONNECT_WANT_STDERR)\n\nAnd this works for local urls too, invoking `hg serve` locally.\n\nNote that what the second patch does could be done in sq_quote_buf\ninstead, arguably.\n\nI'm certainly open to any better ideas as long as they can make it to\nmainline :).\n\nMike Hommey (4):\n  git_connect: extend to take a pseudo format string for the program to\n    run\n  git_connect: avoid quoting the path on the command line when it's not\n    necessary\n  git_connect: allow a file descriptor to be allocated for stderr\n  git_connect: add a flag to consider the path part of ssh urls relative\n\n connect.c | 52 ++++++++++++++++++++++++++++++++++++++++++++--------\n connect.h |  2 ++\n 2 files changed, 46 insertions(+), 8 deletions(-)\n\n-- \n2.8.1.5.g18c8a48\n"},{"id":"284808","messageId":"1461852759-28429-2-git-send-email-mh@glandium.org","threadId":"42170","inReplyTo":"1461852759-28429-1-git-send-email-mh@glandium.org","subject":"[PATCH 1/4] git_connect: extend to take a pseudo format string for the program to run","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-04-28T14:12:36Z","receivedAt":"2016-04-28T14:12:36Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"Currently, the path extracted from the url is passed as last argument to\nthe program/command passed to git_connect(). In every case the function\nis used in the git code base, it's enough, but in order to allow the\nreuse of e.g. the GIT_SSH/GIT_SSH_COMMAND logic, additional flexibility\nis welcome.\n\nWith this change, when the program/command passed to git_connect()\ncontains a \"%s\", that \"%s\" is replaced with the path from the url,\nallowing the path to be at a different position than last on the\nexecuted command line.\n\nSigned-off-by: Mike Hommey <mh@glandium.org>\n---\n connect.c | 29 ++++++++++++++++++++++++-----\n 1 file changed, 24 insertions(+), 5 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex dccf673..96c8c1d 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -658,6 +658,25 @@ static enum protocol parse_connect_url(const char *url_orig, char **ret_host,\n \n static struct child_process no_fork = CHILD_PROCESS_INIT;\n \n+static void prepare_connect_command(struct strbuf *cmd, const char *prog,\n+                                    const char *path, int quote)\n+{\n+\tconst char *found = strstr(prog, \"%s\");\n+\tif (found)\n+\t\tstrbuf_add(cmd, prog, found - prog);\n+\telse {\n+\t\tstrbuf_addstr(cmd, prog);\n+\t\tstrbuf_addch(cmd, ' ');\n+\t}\n+\tif (quote)\n+\t\tsq_quote_buf(cmd, path);\n+\telse\n+\t\tstrbuf_addstr(cmd, path);\n+\n+\tif (found)\n+\t\tstrbuf_addstr(cmd, found + 2);\n+}\n+\n /*\n  * This returns a dummy child_process if the transport protocol does not\n  * need fork(2), or a struct child_process object if it does.  Once done,\n@@ -717,18 +736,18 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\t * Note: Do not add any other headers here!  Doing so\n \t\t * will cause older git-daemon servers to crash.\n \t\t */\n+\t\tprepare_connect_command(&cmd, prog, path, 0);\n \t\tpacket_write(fd[1],\n-\t\t\t     \"%s %s%chost=%s%c\",\n-\t\t\t     prog, path, 0,\n+\t\t\t     \"%s%chost=%s%c\",\n+\t\t\t     cmd.buf, 0,\n \t\t\t     target_host, 0);\n+\t\tstrbuf_release(&cmd);\n \t\tfree(target_host);\n \t} else {\n \t\tconn = xmalloc(sizeof(*conn));\n \t\tchild_process_init(conn);\n \n-\t\tstrbuf_addstr(&cmd, prog);\n-\t\tstrbuf_addch(&cmd, ' ');\n-\t\tsq_quote_buf(&cmd, path);\n+\t\tprepare_connect_command(&cmd, prog, path, 1);\n \n \t\t/* remove repo-local variables from the environment */\n \t\tconn->env = local_repo_env;\n-- \n2.8.1.5.g18c8a48\n"},{"id":"284811","messageId":"1461852759-28429-3-git-send-email-mh@glandium.org","threadId":"42170","inReplyTo":"1461852759-28429-1-git-send-email-mh@glandium.org","subject":"[PATCH 2/4] git_connect: avoid quoting the path on the command line when it's not necessary","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-04-28T14:12:37Z","receivedAt":"2016-04-28T14:12:37Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"Some remote systems can employ restricted shells that aren't very smart\nwith quotes, so avoid quoting when it's not strictly necessary.\n\nThe list of \"safe\" characters comes from Mercurial's shell quoting\nfunction used for its ssh client side. There likely are more that could\nbe added to the list.\n\nSigned-off-by: Mike Hommey <mh@glandium.org>\n---\n connect.c | 11 +++++++++++\n 1 file changed, 11 insertions(+)\n\ndiff --git a/connect.c b/connect.c\nindex 96c8c1d..919bf9e 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -668,6 +668,17 @@ static void prepare_connect_command(struct strbuf *cmd, const char *prog,\n \t\tstrbuf_addstr(cmd, prog);\n \t\tstrbuf_addch(cmd, ' ');\n \t}\n+\tif (quote) {\n+\t\tconst char *p;\n+\t\tfor (p = path; *p; p++) {\n+\t\t\tif (!isalnum(*p) && *p != '@' && *p != '%' &&\n+\t\t\t    *p != '_' && *p != '+' && *p != '=' && *p != ':' &&\n+\t\t\t    *p != ',' && *p != '.' && *p != '/' && *p != '-')\n+\t\t\t\tbreak;\n+\t\t}\n+\t\tif (!*p)\n+\t\t\tquote = 0;\n+\t}\n \tif (quote)\n \t\tsq_quote_buf(cmd, path);\n \telse\n-- \n2.8.1.5.g18c8a48\n"},{"id":"284812","messageId":"1461852759-28429-4-git-send-email-mh@glandium.org","threadId":"42170","inReplyTo":"1461852759-28429-1-git-send-email-mh@glandium.org","subject":"[PATCH 3/4] git_connect: allow a file descriptor to be allocated for stderr","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-04-28T14:12:38Z","receivedAt":"2016-04-28T14:12:38Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"It can be useful to the caller of git_connect() to get access to stderr,\nso add a flag that makes start_command allocate a file descriptor for\nit.\n\nSigned-off-by: Mike Hommey <mh@glandium.org>\n---\n connect.c | 2 ++\n connect.h | 1 +\n 2 files changed, 3 insertions(+)\n\ndiff --git a/connect.c b/connect.c\nindex 919bf9e..9feedd8 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -764,6 +764,8 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\tconn->env = local_repo_env;\n \t\tconn->use_shell = 1;\n \t\tconn->in = conn->out = -1;\n+\t\tif (flags & CONNECT_WANT_STDERR)\n+\t\t\tconn->err = -1;\n \t\tif (protocol == PROTO_SSH) {\n \t\t\tconst char *ssh;\n \t\t\tint putty = 0, tortoiseplink = 0;\ndiff --git a/connect.h b/connect.h\nindex 01f14cd..fb3331b 100644\n--- a/connect.h\n+++ b/connect.h\n@@ -5,6 +5,7 @@\n #define CONNECT_DIAG_URL      (1u << 1)\n #define CONNECT_IPV4          (1u << 2)\n #define CONNECT_IPV6          (1u << 3)\n+#define CONNECT_WANT_STDERR   (1u << 4)\n extern struct child_process *git_connect(int fd[2], const char *url, const char *prog, int flags);\n extern int finish_connect(struct child_process *conn);\n extern int git_connection_is_socket(struct child_process *conn);\n-- \n2.8.1.5.g18c8a48\n"},{"id":"284809","messageId":"1461852759-28429-5-git-send-email-mh@glandium.org","threadId":"42170","inReplyTo":"1461852759-28429-1-git-send-email-mh@glandium.org","subject":"[PATCH 4/4] git_connect: add a flag to consider the path part of ssh urls relative","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-04-28T14:12:39Z","receivedAt":"2016-04-28T14:12:39Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"In Mercurial ssh urls, the path part of the url is relative to the home\ndirectory of the account being logged to instead of being absolute.\n\nAdd a flag allowing git_connect() to handle this kind of usecase.\n\nSigned-off-by: Mike Hommey <mh@glandium.org>\n---\n connect.c | 10 +++++++---\n connect.h |  1 +\n 2 files changed, 8 insertions(+), 3 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex 9feedd8..0df6297 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -592,7 +592,7 @@ static char *get_port(char *host)\n  * The caller must free() the returned strings.\n  */\n static enum protocol parse_connect_url(const char *url_orig, char **ret_host,\n-\t\t\t\t       char **ret_path)\n+\t\t\t\t       char **ret_path, int relative_ssh)\n {\n \tchar *url;\n \tchar *host, *path;\n@@ -642,7 +642,10 @@ static enum protocol parse_connect_url(const char *url_orig, char **ret_host,\n \tend = path; /* Need to \\0 terminate host here */\n \tif (separator == ':')\n \t\tpath++; /* path starts after ':' */\n-\tif (protocol == PROTO_GIT || protocol == PROTO_SSH) {\n+\tif (protocol == PROTO_SSH && relative_ssh) {\n+\t\tif (path[0] == separator)\n+\t\t\tpath++;\n+\t} else if (protocol == PROTO_GIT || protocol == PROTO_SSH) {\n \t\tif (path[1] == '~')\n \t\t\tpath++;\n \t}\n@@ -712,7 +715,8 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t */\n \tsignal(SIGCHLD, SIG_DFL);\n \n-\tprotocol = parse_connect_url(url, &hostandport, &path);\n+\tprotocol = parse_connect_url(url, &hostandport, &path,\n+\t                             flags & CONNECT_RELATIVE_SSH);\n \tif ((flags & CONNECT_DIAG_URL) && (protocol != PROTO_SSH)) {\n \t\tprintf(\"Diag: url=%s\\n\", url ? url : \"NULL\");\n \t\tprintf(\"Diag: protocol=%s\\n\", prot_name(protocol));\ndiff --git a/connect.h b/connect.h\nindex fb3331b..1377028 100644\n--- a/connect.h\n+++ b/connect.h\n@@ -6,6 +6,7 @@\n #define CONNECT_IPV4          (1u << 2)\n #define CONNECT_IPV6          (1u << 3)\n #define CONNECT_WANT_STDERR   (1u << 4)\n+#define CONNECT_RELATIVE_SSH  (1u << 5)\n extern struct child_process *git_connect(int fd[2], const char *url, const char *prog, int flags);\n extern int finish_connect(struct child_process *conn);\n extern int git_connection_is_socket(struct child_process *conn);\n-- \n2.8.1.5.g18c8a48\n"},{"id":"284835","messageId":"CAGZ79kYf+7r_eA3pDPiQaMP+fb5Rp6cYwts+YQzamUnVzsr-Kg@mail.gmail.com","threadId":"42170","inReplyTo":"1461852759-28429-3-git-send-email-mh@glandium.org","subject":"Re: [PATCH 2/4] git_connect: avoid quoting the path on the command line when it's not necessary","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-04-28T16:14:56Z","receivedAt":"2016-04-28T16:14:56Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Apr 28, 2016 at 7:12 AM, Mike Hommey <mh@glandium.org> wrote:\n> Some remote systems can employ restricted shells that aren't very smart\n> with quotes, so avoid quoting when it's not strictly necessary.\n>\n> The list of \"safe\" characters comes from Mercurial's shell quoting\n> function used for its ssh client side. There likely are more that could\n> be added to the list.\n>\n\nWould it make sense to move the new code into its own function and\ndocument it with this paragraph of the commit message, i.e. hinting\nat Mercurial safe characters or others?\n\n> Signed-off-by: Mike Hommey <mh@glandium.org>\n> ---\n>  connect.c | 11 +++++++++++\n>  1 file changed, 11 insertions(+)\n>\n> diff --git a/connect.c b/connect.c\n> index 96c8c1d..919bf9e 100644\n> --- a/connect.c\n> +++ b/connect.c\n> @@ -668,6 +668,17 @@ static void prepare_connect_command(struct strbuf *cmd, const char *prog,\n>                 strbuf_addstr(cmd, prog);\n>                 strbuf_addch(cmd, ' ');\n>         }\n> +       if (quote) {\n> +               const char *p;\n> +               for (p = path; *p; p++) {\n> +                       if (!isalnum(*p) && *p != '@' && *p != '%' &&\n> +                           *p != '_' && *p != '+' && *p != '=' && *p != ':' &&\n> +                           *p != ',' && *p != '.' && *p != '/' && *p != '-')\n> +                               break;\n> +               }\n> +               if (!*p)\n> +                       quote = 0;\n> +       }\n>         if (quote)\n>                 sq_quote_buf(cmd, path);\n>         else\n> --\n> 2.8.1.5.g18c8a48\n>\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at  http://vger.kernel.org/majordomo-info.html\n"},{"id":"284849","messageId":"xmqqbn4t637r.fsf@gitster.mtv.corp.google.com","threadId":"42170","inReplyTo":"1461852759-28429-1-git-send-email-mh@glandium.org","subject":"Re: [RFC PATCH 0/4] git_connect: add some flexibility","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-28T17:41:12Z","receivedAt":"2016-04-28T17:41:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mike Hommey <mh@glandium.org> writes:\n\n> As you may be aware, I'm working on a git remote helper to access\n> mercurial repositories (https://github.com/glandium/git-cinnabar/).\n>\n> At the moment, a small part is written in C, relying on the git code\n> base, but eventually, there would be more C.\n>\n> As I want to get rid of the dependency on Mercurial itself, I'm planning\n> to implement the wire protocol parts in git-cinnabar.\n\nWhile all of the above sounds like a good thing to do, what I do not\nunderstand is why you need to even touch git_connect() at all, and\nwe certainly do *not* want you to touch it to make its external\ninterface unnecessarily ugly and complex with features that are only\nnecessary if it needs to talk to non-git services.\n\nIn other words, why can't this cinnabar thing live on the other side\nof transport API, just like Git transport itself does not know about\ncURL and HTTP when talking with https:// repositories?\n"},{"id":"284882","messageId":"20160428232936.GA8663@glandium.org","threadId":"42170","inReplyTo":"xmqqbn4t637r.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC PATCH 0/4] git_connect: add some flexibility","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-04-28T23:29:36Z","receivedAt":"2016-04-28T23:29:36Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Thu, Apr 28, 2016 at 10:41:12AM -0700, Junio C Hamano wrote:\n> Mike Hommey <mh@glandium.org> writes:\n> \n> > As you may be aware, I'm working on a git remote helper to access\n> > mercurial repositories (https://github.com/glandium/git-cinnabar/).\n> >\n> > At the moment, a small part is written in C, relying on the git code\n> > base, but eventually, there would be more C.\n> >\n> > As I want to get rid of the dependency on Mercurial itself, I'm planning\n> > to implement the wire protocol parts in git-cinnabar.\n> \n> While all of the above sounds like a good thing to do, what I do not\n> understand is why you need to even touch git_connect() at all, and\n> we certainly do *not* want you to touch it to make its external\n> interface unnecessarily ugly and complex with features that are only\n> necessary if it needs to talk to non-git services.\n> \n> In other words, why can't this cinnabar thing live on the other side\n> of transport API, just like Git transport itself does not know about\n> cURL and HTTP when talking with https:// repositories?\n\nIt does live on the other side of transport API. The changes are not\nabout git itself talking to mercurial servers. They are about a remote\nhelper (git-cinnabar), using native code based on git core code, to talk\nto mercurial servers. Because I'd rather bend git_connect a little, if I\ncan, than copy it (as well as parse_connect_url). Because I want to\nbenefit from all tweaks it has (handling GIT_SSH/GIT_SSH_COMMAND\nproperly, handling tortoiseplink/plink properly, handling IPv6 forms of\nthe ssh url properly, ...)\n\nNow, tweaking git_connect is one possible way to do what I want. Another\nwould be to make parse_connect_url non-static, and move the ssh command\nline construction to a separate (non-static) function. Something with a\nsignature like:\n  void prepare_ssh_command(struct argv_array **command,\n                           const char *hostandport)\n\nthat would essentially do the part of git_connect that is in the if\n(protocol == PROTO_SSH) block (not sure how CONNECT_DIAG_URL would fit\nin this, though).\n\nThoughts?\n\nMike\n"},{"id":"284886","messageId":"1461890625-23222-1-git-send-email-mh@glandium.org","threadId":"42170","inReplyTo":"20160428232936.GA8663@glandium.org","subject":"[RFC PATCH 1/3] connect: make parse_connect_url public","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-04-29T00:43:43Z","receivedAt":"2016-04-29T00:43:43Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"Signed-off-by: Mike Hommey <mh@glandium.org>\n---\n connect.c | 11 ++---------\n connect.h |  9 +++++++++\n 2 files changed, 11 insertions(+), 9 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex c53f3f1..29569b3 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -231,13 +231,6 @@ int server_supports(const char *feature)\n \treturn !!server_feature_value(feature, NULL);\n }\n \n-enum protocol {\n-\tPROTO_LOCAL = 1,\n-\tPROTO_FILE,\n-\tPROTO_SSH,\n-\tPROTO_GIT\n-};\n-\n int url_is_local_not_ssh(const char *url)\n {\n \tconst char *colon = strchr(url, ':');\n@@ -591,8 +584,8 @@ static char *get_port(char *host)\n  * Extract protocol and relevant parts from the specified connection URL.\n  * The caller must free() the returned strings.\n  */\n-static enum protocol parse_connect_url(const char *url_orig, char **ret_host,\n-\t\t\t\t       char **ret_path)\n+enum protocol parse_connect_url(const char *url_orig, char **ret_host,\n+\t\t\t\tchar **ret_path)\n {\n \tchar *url;\n \tchar *host, *path;\ndiff --git a/connect.h b/connect.h\nindex 01f14cd..dede6e8 100644\n--- a/connect.h\n+++ b/connect.h\n@@ -1,6 +1,15 @@\n #ifndef CONNECT_H\n #define CONNECT_H\n \n+enum protocol {\n+\tPROTO_LOCAL = 1,\n+\tPROTO_FILE,\n+\tPROTO_SSH,\n+\tPROTO_GIT\n+};\n+enum protocol parse_connect_url(const char *url_orig, char **ret_host,\n+\t\t\t\tchar **ret_path);\n+\n #define CONNECT_VERBOSE       (1u << 0)\n #define CONNECT_DIAG_URL      (1u << 1)\n #define CONNECT_IPV4          (1u << 2)\n-- \n2.8.1.8.gc23d642.dirty\n"},{"id":"284884","messageId":"1461890625-23222-2-git-send-email-mh@glandium.org","threadId":"42170","inReplyTo":"1461890625-23222-1-git-send-email-mh@glandium.org","subject":"[RFC PATCH 2/3] connect: group CONNECT_DIAG_URL handling code","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-04-29T00:43:44Z","receivedAt":"2016-04-29T00:43:44Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"Signed-off-by: Mike Hommey <mh@glandium.org>\n---\n connect.c | 27 ++++++++++++---------------\n 1 file changed, 12 insertions(+), 15 deletions(-)\n\nNote this makes http://marc.info/?l=git&m=146183714532394 irrelevant.\n\n\ndiff --git a/connect.c b/connect.c\nindex 29569b3..ce216cb 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -676,10 +676,20 @@ struct child_process *git_connect(int fd[2], const char *url,\n \tsignal(SIGCHLD, SIG_DFL);\n \n \tprotocol = parse_connect_url(url, &hostandport, &path);\n-\tif ((flags & CONNECT_DIAG_URL) && (protocol != PROTO_SSH)) {\n+\tif (flags & CONNECT_DIAG_URL) {\n \t\tprintf(\"Diag: url=%s\\n\", url ? url : \"NULL\");\n \t\tprintf(\"Diag: protocol=%s\\n\", prot_name(protocol));\n-\t\tprintf(\"Diag: hostandport=%s\\n\", hostandport ? hostandport : \"NULL\");\n+\t\tif (protocol == PROTO_SSH) {\n+\t\t\tchar *ssh_host = hostandport;\n+\t\t\tconst char *port = NULL;\n+\t\t\tget_host_and_port(&ssh_host, &port);\n+\t\t\tif (!port)\n+\t\t\t\tport = get_port(ssh_host);\n+\t\t\tprintf(\"Diag: userandhost=%s\\n\", ssh_host ? ssh_host : \"NULL\");\n+\t\t\tprintf(\"Diag: port=%s\\n\", port ? port : \"NONE\");\n+\t\t} else {\n+\t\t\tprintf(\"Diag: hostandport=%s\\n\", hostandport ? hostandport : \"NULL\");\n+\t\t}\n \t\tprintf(\"Diag: path=%s\\n\", path ? path : \"NULL\");\n \t\tconn = NULL;\n \t} else if (protocol == PROTO_GIT) {\n@@ -738,19 +748,6 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\t\tif (!port)\n \t\t\t\tport = get_port(ssh_host);\n \n-\t\t\tif (flags & CONNECT_DIAG_URL) {\n-\t\t\t\tprintf(\"Diag: url=%s\\n\", url ? url : \"NULL\");\n-\t\t\t\tprintf(\"Diag: protocol=%s\\n\", prot_name(protocol));\n-\t\t\t\tprintf(\"Diag: userandhost=%s\\n\", ssh_host ? ssh_host : \"NULL\");\n-\t\t\t\tprintf(\"Diag: port=%s\\n\", port ? port : \"NONE\");\n-\t\t\t\tprintf(\"Diag: path=%s\\n\", path ? path : \"NULL\");\n-\n-\t\t\t\tfree(hostandport);\n-\t\t\t\tfree(path);\n-\t\t\t\tfree(conn);\n-\t\t\t\treturn NULL;\n-\t\t\t}\n-\n \t\t\tssh = getenv(\"GIT_SSH_COMMAND\");\n \t\t\tif (!ssh) {\n \t\t\t\tconst char *base;\n-- \n2.8.1.8.gc23d642.dirty\n"},{"id":"284885","messageId":"1461890625-23222-3-git-send-email-mh@glandium.org","threadId":"42170","inReplyTo":"1461890625-23222-1-git-send-email-mh@glandium.org","subject":"[RFC PATCH 3/3] connect: move ssh command line preparation to a separate (public) function","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-04-29T00:43:45Z","receivedAt":"2016-04-29T00:43:45Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"Signed-off-by: Mike Hommey <mh@glandium.org>\n---\n connect.c | 108 +++++++++++++++++++++++++++++++++-----------------------------\n connect.h |   2 ++\n 2 files changed, 60 insertions(+), 50 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex ce216cb..c71563f 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -649,6 +649,62 @@ enum protocol parse_connect_url(const char *url_orig, char **ret_host,\n \treturn protocol;\n }\n \n+int prepare_ssh_command(struct argv_array *cmd, char *hostandport, int flags)\n+{\n+\tconst char *ssh;\n+\tint putty = 0, tortoiseplink = 0, use_shell = 1;\n+\tchar *ssh_host = hostandport;\n+\tconst char *port = NULL;\n+\tget_host_and_port(&ssh_host, &port);\n+\n+\tif (!port)\n+\t\tport = get_port(ssh_host);\n+\n+\tssh = getenv(\"GIT_SSH_COMMAND\");\n+\tif (!ssh) {\n+\t\tconst char *base;\n+\t\tchar *ssh_dup;\n+\n+\t\t/*\n+\t\t * GIT_SSH is the no-shell version of\n+\t\t * GIT_SSH_COMMAND (and must remain so for\n+\t\t * historical compatibility).\n+\t\t */\n+\t\tuse_shell = 0;\n+\n+\t\tssh = getenv(\"GIT_SSH\");\n+\t\tif (!ssh)\n+\t\t\tssh = \"ssh\";\n+\n+\t\tssh_dup = xstrdup(ssh);\n+\t\tbase = basename(ssh_dup);\n+\n+\t\ttortoiseplink = !strcasecmp(base, \"tortoiseplink\") ||\n+\t\t\t!strcasecmp(base, \"tortoiseplink.exe\");\n+\t\tputty = tortoiseplink ||\n+\t\t\t!strcasecmp(base, \"plink\") ||\n+\t\t\t!strcasecmp(base, \"plink.exe\");\n+\n+\t\tfree(ssh_dup);\n+\t}\n+\n+\targv_array_push(cmd, ssh);\n+\tif (flags & CONNECT_IPV4)\n+\t\targv_array_push(cmd, \"-4\");\n+\telse if (flags & CONNECT_IPV6)\n+\t\targv_array_push(cmd, \"-6\");\n+\tif (tortoiseplink)\n+\t\targv_array_push(cmd, \"-batch\");\n+\tif (port) {\n+\t\t/* P is for PuTTY, p is for OpenSSH */\n+\t\targv_array_push(cmd, putty ? \"-P\" : \"-p\");\n+\t\targv_array_push(cmd, port);\n+\t}\n+\targv_array_push(cmd, ssh_host);\n+\n+\treturn use_shell;\n+}\n+\n static struct child_process no_fork = CHILD_PROCESS_INIT;\n \n /*\n@@ -738,57 +794,9 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\tconn->use_shell = 1;\n \t\tconn->in = conn->out = -1;\n \t\tif (protocol == PROTO_SSH) {\n-\t\t\tconst char *ssh;\n-\t\t\tint putty = 0, tortoiseplink = 0;\n-\t\t\tchar *ssh_host = hostandport;\n-\t\t\tconst char *port = NULL;\n \t\t\ttransport_check_allowed(\"ssh\");\n-\t\t\tget_host_and_port(&ssh_host, &port);\n-\n-\t\t\tif (!port)\n-\t\t\t\tport = get_port(ssh_host);\n-\n-\t\t\tssh = getenv(\"GIT_SSH_COMMAND\");\n-\t\t\tif (!ssh) {\n-\t\t\t\tconst char *base;\n-\t\t\t\tchar *ssh_dup;\n-\n-\t\t\t\t/*\n-\t\t\t\t * GIT_SSH is the no-shell version of\n-\t\t\t\t * GIT_SSH_COMMAND (and must remain so for\n-\t\t\t\t * historical compatibility).\n-\t\t\t\t */\n-\t\t\t\tconn->use_shell = 0;\n-\n-\t\t\t\tssh = getenv(\"GIT_SSH\");\n-\t\t\t\tif (!ssh)\n-\t\t\t\t\tssh = \"ssh\";\n-\n-\t\t\t\tssh_dup = xstrdup(ssh);\n-\t\t\t\tbase = basename(ssh_dup);\n-\n-\t\t\t\ttortoiseplink = !strcasecmp(base, \"tortoiseplink\") ||\n-\t\t\t\t\t!strcasecmp(base, \"tortoiseplink.exe\");\n-\t\t\t\tputty = tortoiseplink ||\n-\t\t\t\t\t!strcasecmp(base, \"plink\") ||\n-\t\t\t\t\t!strcasecmp(base, \"plink.exe\");\n-\n-\t\t\t\tfree(ssh_dup);\n-\t\t\t}\n-\n-\t\t\targv_array_push(&conn->args, ssh);\n-\t\t\tif (flags & CONNECT_IPV4)\n-\t\t\t\targv_array_push(&conn->args, \"-4\");\n-\t\t\telse if (flags & CONNECT_IPV6)\n-\t\t\t\targv_array_push(&conn->args, \"-6\");\n-\t\t\tif (tortoiseplink)\n-\t\t\t\targv_array_push(&conn->args, \"-batch\");\n-\t\t\tif (port) {\n-\t\t\t\t/* P is for PuTTY, p is for OpenSSH */\n-\t\t\t\targv_array_push(&conn->args, putty ? \"-P\" : \"-p\");\n-\t\t\t\targv_array_push(&conn->args, port);\n-\t\t\t}\n-\t\t\targv_array_push(&conn->args, ssh_host);\n+\t\t\tconn->use_shell = prepare_ssh_command(\n+\t\t\t\t&conn->args, hostandport, flags);\n \t\t} else {\n \t\t\ttransport_check_allowed(\"file\");\n \t\t}\ndiff --git a/connect.h b/connect.h\nindex dede6e8..5392102 100644\n--- a/connect.h\n+++ b/connect.h\n@@ -10,6 +10,8 @@ enum protocol {\n enum protocol parse_connect_url(const char *url_orig, char **ret_host,\n \t\t\t\tchar **ret_path);\n \n+int prepare_ssh_command(struct argv_array *cmd, char *hostandport, int flags);\n+\n #define CONNECT_VERBOSE       (1u << 0)\n #define CONNECT_DIAG_URL      (1u << 1)\n #define CONNECT_IPV4          (1u << 2)\n-- \n2.8.1.8.gc23d642.dirty\n"},{"id":"284945","messageId":"xmqqeg9o1k40.fsf@gitster.mtv.corp.google.com","threadId":"42170","inReplyTo":"1461890625-23222-2-git-send-email-mh@glandium.org","subject":"Re: [RFC PATCH 2/3] connect: group CONNECT_DIAG_URL handling code","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-29T15:59:43Z","receivedAt":"2016-04-29T15:59:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mike Hommey <mh@glandium.org> writes:\n\n> Signed-off-by: Mike Hommey <mh@glandium.org>\n\nI feel that this commit is under-explained.  I think you should feel\nentitled to boast the goodness this brings to us louder in the log\nmessage.\n\nIt bothers me somewhat that this ended up copying, not moving, a bit\nof code to call get-host-and-port helper, but I do not think it is a\nproblem and it makes the codeflow easier to follow.  Attempt to\nrefactor it to reduce the duplication is likely to make it worse.\n\nWe used to allocate and prepare the child process structure 'conn',\nthen realized that we are not going to use it anyway and discarded,\nonly because the DIAG_URL check for SSH transport was done way too\nlate.  That wastage is removed by this change as well.\n\nAnother change I notice is that DIAG_URL code for PROTO_SSH did not\neven kick in if transport_check_allowed(\"ssh\") said no, but with\nthis new code Diag is always given, which makes it consistent with\nPROTO_GIT codepath.\n\n> ---\n>  connect.c | 27 ++++++++++++---------------\n>  1 file changed, 12 insertions(+), 15 deletions(-)\n>\n> Note this makes http://marc.info/?l=git&m=146183714532394 irrelevant.\n\nIndeed.\n\n>\n> diff --git a/connect.c b/connect.c\n> index 29569b3..ce216cb 100644\n> --- a/connect.c\n> +++ b/connect.c\n> @@ -676,10 +676,20 @@ struct child_process *git_connect(int fd[2], const char *url,\n>  \tsignal(SIGCHLD, SIG_DFL);\n>  \n>  \tprotocol = parse_connect_url(url, &hostandport, &path);\n> -\tif ((flags & CONNECT_DIAG_URL) && (protocol != PROTO_SSH)) {\n> +\tif (flags & CONNECT_DIAG_URL) {\n>  \t\tprintf(\"Diag: url=%s\\n\", url ? url : \"NULL\");\n>  \t\tprintf(\"Diag: protocol=%s\\n\", prot_name(protocol));\n> -\t\tprintf(\"Diag: hostandport=%s\\n\", hostandport ? hostandport : \"NULL\");\n> +\t\tif (protocol == PROTO_SSH) {\n> +\t\t\tchar *ssh_host = hostandport;\n> +\t\t\tconst char *port = NULL;\n> +\t\t\tget_host_and_port(&ssh_host, &port);\n> +\t\t\tif (!port)\n> +\t\t\t\tport = get_port(ssh_host);\n> +\t\t\tprintf(\"Diag: userandhost=%s\\n\", ssh_host ? ssh_host : \"NULL\");\n> +\t\t\tprintf(\"Diag: port=%s\\n\", port ? port : \"NONE\");\n> +\t\t} else {\n> +\t\t\tprintf(\"Diag: hostandport=%s\\n\", hostandport ? hostandport : \"NULL\");\n> +\t\t}\n>  \t\tprintf(\"Diag: path=%s\\n\", path ? path : \"NULL\");\n>  \t\tconn = NULL;\n>  \t} else if (protocol == PROTO_GIT) {\n> @@ -738,19 +748,6 @@ struct child_process *git_connect(int fd[2], const char *url,\n>  \t\t\tif (!port)\n>  \t\t\t\tport = get_port(ssh_host);\n>  \n> -\t\t\tif (flags & CONNECT_DIAG_URL) {\n> -\t\t\t\tprintf(\"Diag: url=%s\\n\", url ? url : \"NULL\");\n> -\t\t\t\tprintf(\"Diag: protocol=%s\\n\", prot_name(protocol));\n> -\t\t\t\tprintf(\"Diag: userandhost=%s\\n\", ssh_host ? ssh_host : \"NULL\");\n> -\t\t\t\tprintf(\"Diag: port=%s\\n\", port ? port : \"NONE\");\n> -\t\t\t\tprintf(\"Diag: path=%s\\n\", path ? path : \"NULL\");\n> -\n> -\t\t\t\tfree(hostandport);\n> -\t\t\t\tfree(path);\n> -\t\t\t\tfree(conn);\n> -\t\t\t\treturn NULL;\n> -\t\t\t}\n> -\n>  \t\t\tssh = getenv(\"GIT_SSH_COMMAND\");\n>  \t\t\tif (!ssh) {\n>  \t\t\t\tconst char *base;\n"},{"id":"284946","messageId":"xmqqa8kc1hdu.fsf@gitster.mtv.corp.google.com","threadId":"42170","inReplyTo":"1461890625-23222-3-git-send-email-mh@glandium.org","subject":"Re: [RFC PATCH 3/3] connect: move ssh command line preparation to a separate (public) function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-29T16:58:37Z","receivedAt":"2016-04-29T16:58:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mike Hommey <mh@glandium.org> writes:\n\n> Signed-off-by: Mike Hommey <mh@glandium.org>\n> ---\n\nI think moving it out into a separate helper function is a very good\nchange to make the end result easier to follow.\n\nTurning the helper to extern and adding it to connect.h does not\nbenefit us (and [PATCH 1/3] doesn't, either).\n\nAnd in the longer term, I doubt think it (and [PATCH 1/3]) would\nbenefit you, either.  We do not mean to make what goes into libgit.a\n(which connect.o is a part of) be a stable \"library interface\", so\nall sorts of future changes we make would affect your external code,\ne.g.\n\n - some people periodically check for extern symbols that do not\n   have to be extern, and this will be marked static again any time\n   (or you somehow need to arrange it so that this function is\n   marked as special to reduce false positives);\n\n - we may need to update the external interface to this function,\n   changing parameters, or updating its return value.\n\neven if we promise not to make such a change only for the purpose of\ndeliberately breaking you.  So you have to be prepared and willing\nto follow the evolution of in-core code anyway.\n\nSo I would suggest restructuring this series to do\n\n * 2/3 (DIAG consolidation)\n * refactoring in 3/3 but not s/static/extern/\n\nand in optional follow(s)-up, do\n\n * s/static/extern/ and update to *.h in 3/3\n\n * 1/3, but I do not think it is necessary for users of\n   prepare_ssh_command()\n\nThe last patch could be kept in your fork of git.git if you want to\nkeep linking with libgit.a, but my feeling is that we'd prefer to do\nwithout it at least for now, until we gain in-tree users that live\noutside connect.c that want to call the helper.\n\n\nThanks.\n"},{"id":"284950","messageId":"xmqqvb30z6ba.fsf@gitster.mtv.corp.google.com","threadId":"42170","inReplyTo":"xmqqeg9o1k40.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC PATCH 2/3] connect: group CONNECT_DIAG_URL handling code","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-29T17:13:45Z","receivedAt":"2016-04-29T17:13:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Mike Hommey <mh@glandium.org> writes:\n>\n>> Signed-off-by: Mike Hommey <mh@glandium.org>\n>\n> I feel that this commit is under-explained.  I think you should feel\n> entitled to boast the goodness this brings to us louder in the log\n> message.\n>\n> It bothers me somewhat that this ended up copying, not moving, a bit\n> of code to call get-host-and-port helper, but I do not think it is a\n> problem and it makes the codeflow easier to follow.  Attempt to\n> refactor it to reduce the duplication is likely to make it worse.\n>\n\nI hate to add a noise to the list, but while re-re-re-editing this\nmessage before sending it out, I accidentally dropped an important\nline here.  I said (and then removed by mistake):\n\n\tI like what this patch does.\n\n> We used to allocate and prepare the child process structure 'conn',\n> then realized that we are not going to use it anyway and discarded,\n> only because the DIAG_URL check for SSH transport was done way too\n> late.  That wastage is removed by this change as well.\n>\n> Another change I notice is that DIAG_URL code for PROTO_SSH did not\n> even kick in if transport_check_allowed(\"ssh\") said no, but with\n> this new code Diag is always given, which makes it consistent with\n> PROTO_GIT codepath.\n"},{"id":"284970","messageId":"632a7629-645b-43a7-abd7-fcbc35362f60@web.de","threadId":"42170","inReplyTo":"1461890625-23222-2-git-send-email-mh@glandium.org","subject":"Re: [RFC PATCH 2/3] connect: group CONNECT_DIAG_URL handling code","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2016-04-29T20:00:52Z","receivedAt":"2016-04-29T20:00:52Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 29.04.16 02:43, Mike Hommey wrote:\n> get_host_and_port(&ssh_host, &port);\n> +\t\t\tif (!port)\n> +\t\t\t\tport = get_port(ssh_host);\n\nI'm not sure, if this is an improvement or not.\n\nThe original intention was, to check what the parser did, before\ngoing out to the external program (like ssh) or opening a socket.\n\nEverything should be prepared, as if the action had been taken,\nand in the last nanosecond we jump out and print what would have\nbeen done.\n(The nanosecond may depend on the CPU speed)\n\nIn any case, for SSH there should be only one line calling get_host_and_port(),\nor get_port().\n\nThis is to check the complete logic path in test cases, as well as it allows\nthe user to check the URL, especially when using SSH.\n\nSplitting the logic into a path where we just print and one, where\nthe connection is really made, looses \"single point of truth\" - we don't know\nany longer, what we test or print for diag.\n"}]}