{"thread":{"id":"35380","subject":"[PATCH v6 04/10] git_connect: factor out discovery of the protocol and its parts","startedAt":"2013-11-21T20:39:50Z","lastAt":"2013-11-21T23:22:53Z","messageCount":2,"participants":["Torsten Bögershausen","Junio C Hamano"],"isPatch":true,"patchVersion":6,"patchTotal":10},"messages":[{"id":"230905","messageId":"201311212139.51214.tboegi@web.de","threadId":"35380","inReplyTo":null,"subject":"[PATCH v6 04/10] git_connect: factor out discovery of the protocol and its parts","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2013-11-21T20:39:50Z","receivedAt":"2013-11-21T20:39:50Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"git_connect has grown large due to the many different protocols syntaxes\nthat are supported. Move the part of the function that parses the URL to\nconnect to into a separate function for readability.\n\nSigned-off-by: Johannes Sixt <j6t@kdbg.org>\n---\n connect.c | 80 ++++++++++++++++++++++++++++++++++++++++++---------------------\n 1 file changed, 53 insertions(+), 27 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex 6cc1f8d..a6cf345 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -543,37 +543,20 @@ static char *get_port(char *host)\n \treturn NULL;\n }\n \n-static struct child_process no_fork;\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- * finish the connection with finish_connect() with the value returned from\n- * this function (it is safe to call finish_connect() with NULL to support\n- * the former case).\n- *\n- * If it returns, the connect is successful; it just dies on errors (this\n- * will hopefully be changed in a libification effort, to return NULL when\n- * the connection failed).\n+ * Extract protocol and relevant parts from the specified connection URL.\n+ * The caller must free() the returned strings.\n  */\n-struct child_process *git_connect(int fd[2], const char *url_orig,\n-\t\t\t\t  const char *prog, int flags)\n+static enum protocol parse_connect_url(const char *url_orig, char **ret_host,\n+\t\t\t\t       char **ret_port, char **ret_path)\n {\n \tchar *url;\n \tchar *host, *path;\n \tchar *end;\n \tint c;\n-\tstruct child_process *conn = &no_fork;\n \tenum protocol protocol = PROTO_LOCAL;\n \tint free_path = 0;\n \tchar *port = NULL;\n-\tconst char **arg;\n-\tstruct strbuf cmd = STRBUF_INIT;\n-\n-\t/* Without this we cannot rely on waitpid() to tell\n-\t * what happened to our children.\n-\t */\n-\tsignal(SIGCHLD, SIG_DFL);\n \n \tif (is_url(url_orig))\n \t\turl = url_decode(url_orig);\n@@ -645,6 +628,49 @@ struct child_process *git_connect(int fd[2], const char *url_orig,\n \tif (protocol == PROTO_SSH && host != url)\n \t\tport = get_port(end);\n \n+\t*ret_host = xstrdup(host);\n+\tif (port)\n+\t\t*ret_port = xstrdup(port);\n+\telse\n+\t\t*ret_port = NULL;\n+\tif (free_path)\n+\t\t*ret_path = path;\n+\telse\n+\t\t*ret_path = xstrdup(path);\n+\tfree(url);\n+\treturn protocol;\n+}\n+\n+static struct child_process no_fork;\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+ * finish the connection with finish_connect() with the value returned from\n+ * this function (it is safe to call finish_connect() with NULL to support\n+ * the former case).\n+ *\n+ * If it returns, the connect is successful; it just dies on errors (this\n+ * will hopefully be changed in a libification effort, to return NULL when\n+ * the connection failed).\n+ */\n+struct child_process *git_connect(int fd[2], const char *url,\n+\t\t\t\t  const char *prog, int flags)\n+{\n+\tchar *host, *path;\n+\tstruct child_process *conn = &no_fork;\n+\tenum protocol protocol;\n+\tchar *port;\n+\tconst char **arg;\n+\tstruct strbuf cmd = STRBUF_INIT;\n+\n+\t/* Without this we cannot rely on waitpid() to tell\n+\t * what happened to our children.\n+\t */\n+\tsignal(SIGCHLD, SIG_DFL);\n+\n+\tprotocol = parse_connect_url(url, &host, &port, &path);\n+\n \tif (protocol == PROTO_GIT) {\n \t\t/* These underlying connection commands die() if they\n \t\t * cannot connect.\n@@ -666,9 +692,9 @@ struct child_process *git_connect(int fd[2], const char *url_orig,\n \t\t\t     prog, path, 0,\n \t\t\t     target_host, 0);\n \t\tfree(target_host);\n-\t\tfree(url);\n-\t\tif (free_path)\n-\t\t\tfree(path);\n+\t\tfree(host);\n+\t\tfree(port);\n+\t\tfree(path);\n \t\treturn conn;\n \t}\n \n@@ -709,9 +735,9 @@ struct child_process *git_connect(int fd[2], const char *url_orig,\n \tfd[0] = conn->out; /* read from child's stdout */\n \tfd[1] = conn->in;  /* write to child's stdin */\n \tstrbuf_release(&cmd);\n-\tfree(url);\n-\tif (free_path)\n-\t\tfree(path);\n+\tfree(host);\n+\tfree(port);\n+\tfree(path);\n \treturn conn;\n }\n \n-- \n1.8.4.457.g424cb08\n"},{"id":"230922","messageId":"xmqqk3g12vci.fsf@gitster.dls.corp.google.com","threadId":"35380","inReplyTo":"201311212139.51214.tboegi@web.de","subject":"Re: [PATCH v6 04/10] git_connect: factor out discovery of the protocol and its parts","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-11-21T23:22:53Z","receivedAt":"2013-11-21T23:22:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Torsten Bögershausen <tboegi@web.de> writes:\n\n> git_connect has grown large due to the many different protocols syntaxes\n> that are supported. Move the part of the function that parses the URL to\n> connect to into a separate function for readability.\n>\n> Signed-off-by: Johannes Sixt <j6t@kdbg.org>\n> ---\n\nI lost track, but was this authored by j6t?  If so please:\n\n (1) begin the body of the message like so:\n\n\tFrom: Johannes Sixt <j6t@kdbg.org>\n\n        git_connect has grown ...\n\n     so that the resulting commit will have him as the author; and\n\n (2) have your own sign-off after his at the end, i.e.\n\n\t....\n        connect to into a separate function for readability.\n\n\tSigned-off-by: Johannes Sixt <j6t@kdbg.org>\n        Signed-off-by: Torsten Bögershausen <tboegi@web.de>\n\n     to record the flow of the patch.\n\nThe resulting code looks fine from a cursory view.\n\nThanks.\n\n\n>  connect.c | 80 ++++++++++++++++++++++++++++++++++++++++++---------------------\n>  1 file changed, 53 insertions(+), 27 deletions(-)\n>\n> diff --git a/connect.c b/connect.c\n> index 6cc1f8d..a6cf345 100644\n> --- a/connect.c\n> +++ b/connect.c\n> @@ -543,37 +543,20 @@ static char *get_port(char *host)\n>  \treturn NULL;\n>  }\n>  \n> -static struct child_process no_fork;\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> - * finish the connection with finish_connect() with the value returned from\n> - * this function (it is safe to call finish_connect() with NULL to support\n> - * the former case).\n> - *\n> - * If it returns, the connect is successful; it just dies on errors (this\n> - * will hopefully be changed in a libification effort, to return NULL when\n> - * the connection failed).\n> + * Extract protocol and relevant parts from the specified connection URL.\n> + * The caller must free() the returned strings.\n>   */\n> -struct child_process *git_connect(int fd[2], const char *url_orig,\n> -\t\t\t\t  const char *prog, int flags)\n> +static enum protocol parse_connect_url(const char *url_orig, char **ret_host,\n> +\t\t\t\t       char **ret_port, char **ret_path)\n>  {\n>  \tchar *url;\n>  \tchar *host, *path;\n>  \tchar *end;\n>  \tint c;\n> -\tstruct child_process *conn = &no_fork;\n>  \tenum protocol protocol = PROTO_LOCAL;\n>  \tint free_path = 0;\n>  \tchar *port = NULL;\n> -\tconst char **arg;\n> -\tstruct strbuf cmd = STRBUF_INIT;\n> -\n> -\t/* Without this we cannot rely on waitpid() to tell\n> -\t * what happened to our children.\n> -\t */\n> -\tsignal(SIGCHLD, SIG_DFL);\n>  \n>  \tif (is_url(url_orig))\n>  \t\turl = url_decode(url_orig);\n> @@ -645,6 +628,49 @@ struct child_process *git_connect(int fd[2], const char *url_orig,\n>  \tif (protocol == PROTO_SSH && host != url)\n>  \t\tport = get_port(end);\n>  \n> +\t*ret_host = xstrdup(host);\n> +\tif (port)\n> +\t\t*ret_port = xstrdup(port);\n> +\telse\n> +\t\t*ret_port = NULL;\n> +\tif (free_path)\n> +\t\t*ret_path = path;\n> +\telse\n> +\t\t*ret_path = xstrdup(path);\n> +\tfree(url);\n> +\treturn protocol;\n> +}\n> +\n> +static struct child_process no_fork;\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> + * finish the connection with finish_connect() with the value returned from\n> + * this function (it is safe to call finish_connect() with NULL to support\n> + * the former case).\n> + *\n> + * If it returns, the connect is successful; it just dies on errors (this\n> + * will hopefully be changed in a libification effort, to return NULL when\n> + * the connection failed).\n> + */\n> +struct child_process *git_connect(int fd[2], const char *url,\n> +\t\t\t\t  const char *prog, int flags)\n> +{\n> +\tchar *host, *path;\n> +\tstruct child_process *conn = &no_fork;\n> +\tenum protocol protocol;\n> +\tchar *port;\n> +\tconst char **arg;\n> +\tstruct strbuf cmd = STRBUF_INIT;\n> +\n> +\t/* Without this we cannot rely on waitpid() to tell\n> +\t * what happened to our children.\n> +\t */\n> +\tsignal(SIGCHLD, SIG_DFL);\n> +\n> +\tprotocol = parse_connect_url(url, &host, &port, &path);\n> +\n>  \tif (protocol == PROTO_GIT) {\n>  \t\t/* These underlying connection commands die() if they\n>  \t\t * cannot connect.\n> @@ -666,9 +692,9 @@ struct child_process *git_connect(int fd[2], const char *url_orig,\n>  \t\t\t     prog, path, 0,\n>  \t\t\t     target_host, 0);\n>  \t\tfree(target_host);\n> -\t\tfree(url);\n> -\t\tif (free_path)\n> -\t\t\tfree(path);\n> +\t\tfree(host);\n> +\t\tfree(port);\n> +\t\tfree(path);\n>  \t\treturn conn;\n>  \t}\n>  \n> @@ -709,9 +735,9 @@ struct child_process *git_connect(int fd[2], const char *url_orig,\n>  \tfd[0] = conn->out; /* read from child's stdout */\n>  \tfd[1] = conn->in;  /* write to child's stdin */\n>  \tstrbuf_release(&cmd);\n> -\tfree(url);\n> -\tif (free_path)\n> -\t\tfree(path);\n> +\tfree(host);\n> +\tfree(port);\n> +\tfree(path);\n>  \treturn conn;\n>  }\n"}]}