Re: [PATCH 2/2] git_connect: factor out discovery of the protocol and its parts
From: Torsten Bögershausen <hidden>
Date: 2016-06-15 22:59:11
On 2013-11-05 20.39, Johannes Sixt wrote: Thanks for picking this up, please see some minor nits inline, and git_connect() is at the end
-struct child_process *git_connect(int fd[2], const char *url_orig,
- const char *prog, int flags)
+static enum protocol parse_connect_url(const char *url_orig, char **ret_host,
+ char **ret_port, char **ret_path)
{
char *url;
char *host, *path;
char *end;Can we put all the char * into one single line?
quoted hunk ↗ jump to hunk
int c;@@ -645,6 +628,49 @@ struct child_process *git_connect(int fd[2], const char *url_orig, if (protocol == PROTO_SSH && host != url) port = get_port(end); + *ret_host = xstrdup(host); + if (port) + *ret_port = xstrdup(port); + else + *ret_port = NULL; + if (free_path) + *ret_path = path; + else + *ret_path = xstrdup(path); + free(url); + return protocol; +} + +static struct child_process no_fork; + +/* + * 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, + * finish the connection with finish_connect() with the value returned from + * this function (it is safe to call finish_connect() with NULL to support + * the former case). + * + * If it returns, the connect is successful; it just dies on errors (this + * will hopefully be changed in a libification effort, to return NULL when + * the connection failed). + */ +struct child_process *git_connect(int fd[2], const char *url, + const char *prog, int flags) +{ + char *host, *path; + struct child_process *conn = &no_fork; + enum protocol protocol; + char *port; + const char **arg; + struct strbuf cmd = STRBUF_INIT; + + /* Without this we cannot rely on waitpid() to tell + * what happened to our children. + */ + signal(SIGCHLD, SIG_DFL); + + protocol = parse_connect_url(url, &host, &port, &path); + if (protocol == PROTO_GIT) { /* These underlying connection commands die() if they * cannot connect.@@ -666,9 +692,9 @@ struct child_process *git_connect(int fd[2], const char *url_orig, prog, path, 0, target_host, 0); free(target_host);
This is hard to see in the diff, I think we don't need target_host any more.
quoted hunk ↗ jump to hunk
- free(url); - if (free_path) - free(path); + free(host); + free(port); + free(path); return conn; }@@ -709,9 +735,9 @@ struct child_process *git_connect(int fd[2], const char *url_orig, fd[0] = conn->out; /* read from child's stdout */ fd[1] = conn->in; /* write to child's stdin */ strbuf_release(&cmd); - free(url); - if (free_path) - free(path);
This "end of function, free everything and return conn", could we re-arange so that it is in the code only once ?
+ free(host); + free(port); + free(path); return conn; }
====================
struct child_process *git_connect(int fd[2], const char *url,
const char *prog, int flags)
{
char *host, *port, *path;
struct child_process *conn = &no_fork;
enum protocol protocol;
const char **arg;
struct strbuf cmd = STRBUF_INIT;
/* Without this we cannot rely on waitpid() to tell
* what happened to our children.
*/
signal(SIGCHLD, SIG_DFL);
protocol = parse_connect_url(url, &host, &port, &path);
if (protocol == PROTO_GIT) {
/* These underlying connection commands die() if they
* cannot connect.
*/
if (git_use_proxy(host))
conn = git_proxy_connect(fd, host);
else
git_tcp_connect(fd, host, flags);
/*
* Separate original protocol components prog and path
* from extended host header with a NUL byte.
*
* Note: Do not add any other headers here! Doing so
* will cause older git-daemon servers to crash.
*/
packet_write(fd[1],
"%s %s%chost=%s%c",
prog, path, 0,
host, 0);
}
else
{
conn = xcalloc(1, sizeof(*conn));
strbuf_addstr(&cmd, prog);
strbuf_addch(&cmd, ' ');
sq_quote_buf(&cmd, path);
conn->in = conn->out = -1;
conn->argv = arg = xcalloc(7, sizeof(*arg));
if (protocol == PROTO_SSH) {
const char *ssh = getenv("GIT_SSH");
int putty = ssh && strcasestr(ssh, "plink");
if (!ssh) ssh = "ssh";
*arg++ = ssh;
if (putty && !strcasestr(ssh, "tortoiseplink"))
*arg++ = "-batch";
if (port) {
/* P is for PuTTY, p is for OpenSSH */
*arg++ = putty ? "-P" : "-p";
*arg++ = port;
}
*arg++ = host;
}
else {
/* remove repo-local variables from the environment */
conn->env = local_repo_env;
conn->use_shell = 1;
}
*arg++ = cmd.buf;
*arg = NULL;
if (start_command(conn))
die("unable to fork");
fd[0] = conn->out; /* read from child's stdout */
fd[1] = conn->in; /* write to child's stdin */
strbuf_release(&cmd);
}
free(host);
free(port);
free(path);
return conn;
}