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

["Torsten Bögershausen" <tboegi@web.de>]

On 2013-11-05 22.22, Johannes Sixt wrote:
> Am 05.11.2013 21:45, schrieb Torsten Bögershausen:
>> 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?
> 
> The idea here was to keep the diff minimal, and that further slight
> cleanups should be combined with subsequent rewrites that should happen
> to this function.
> 
>>>  	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.
> 
> I though that as well first, but no, we still need it. Further rewrites
> are needed that move the port discovery from git_proxy_connect() and
> git_tcp_connect() to the new parse_connect_url() before target_host can
> go away. And even then it is questionable because target_host is used in
> an error message and is intended to reflect the original combined
> host+port portion of the URL, if I read the code correctly.
> 
>>> -		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 ?
> 
> That would be quite simple now; just place the part after the first
> return into the else branch. That opens opportunities to move variable
> declarations from the top of the function into the else branch.
> 
> But all of these changes should go into a separate commit, IMO, so that
> the function splitting that happens here can be verified more easily.
> 
> -- Hannes
Agreed on all points, (some re-reading was needed)

I will first focus on the test cases,
since having god test cases eases us the re-factoring later on.
Thanks
/Torsten