Re: [PATCH 2/2] git_connect: factor out discovery of the protocol and its parts

From: Torsten Bögershausen <tboegi@web.de>
Date: Nov 5, 2013, 20:45 UTC
Message-ID: <527958E1.2080805@web.de>
In-Reply-To: <5279496A.2090202@kdbg.org>

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

Show 8 quoted lines

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

Show 55 quoted lines

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

Show 16 quoted lines

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

Show 5 quoted lines

> +	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;
}