Re: [RFC PATCH 2/3] connect: group CONNECT_DIAG_URL handling code

[Junio C Hamano <gitster@pobox.com>]

Mike Hommey <mh@glandium.org> writes:

> Signed-off-by: Mike Hommey <mh@glandium.org>

I feel that this commit is under-explained.  I think you should feel
entitled to boast the goodness this brings to us louder in the log
message.

It bothers me somewhat that this ended up copying, not moving, a bit
of code to call get-host-and-port helper, but I do not think it is a
problem and it makes the codeflow easier to follow.  Attempt to
refactor it to reduce the duplication is likely to make it worse.

We used to allocate and prepare the child process structure 'conn',
then realized that we are not going to use it anyway and discarded,
only because the DIAG_URL check for SSH transport was done way too
late.  That wastage is removed by this change as well.

Another change I notice is that DIAG_URL code for PROTO_SSH did not
even kick in if transport_check_allowed("ssh") said no, but with
this new code Diag is always given, which makes it consistent with
PROTO_GIT codepath.

> ---
>  connect.c | 27 ++++++++++++---------------
>  1 file changed, 12 insertions(+), 15 deletions(-)
>
> Note this makes http://marc.info/?l=git&m=146183714532394 irrelevant.

Indeed.

>
> diff --git a/connect.c b/connect.c
> index 29569b3..ce216cb 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -676,10 +676,20 @@ struct child_process *git_connect(int fd[2], const char *url,
>  	signal(SIGCHLD, SIG_DFL);
>  
>  	protocol = parse_connect_url(url, &hostandport, &path);
> -	if ((flags & CONNECT_DIAG_URL) && (protocol != PROTO_SSH)) {
> +	if (flags & CONNECT_DIAG_URL) {
>  		printf("Diag: url=%s\n", url ? url : "NULL");
>  		printf("Diag: protocol=%s\n", prot_name(protocol));
> -		printf("Diag: hostandport=%s\n", hostandport ? hostandport : "NULL");
> +		if (protocol == PROTO_SSH) {
> +			char *ssh_host = hostandport;
> +			const char *port = NULL;
> +			get_host_and_port(&ssh_host, &port);
> +			if (!port)
> +				port = get_port(ssh_host);
> +			printf("Diag: userandhost=%s\n", ssh_host ? ssh_host : "NULL");
> +			printf("Diag: port=%s\n", port ? port : "NONE");
> +		} else {
> +			printf("Diag: hostandport=%s\n", hostandport ? hostandport : "NULL");
> +		}
>  		printf("Diag: path=%s\n", path ? path : "NULL");
>  		conn = NULL;
>  	} else if (protocol == PROTO_GIT) {
> @@ -738,19 +748,6 @@ struct child_process *git_connect(int fd[2], const char *url,
>  			if (!port)
>  				port = get_port(ssh_host);
>  
> -			if (flags & CONNECT_DIAG_URL) {
> -				printf("Diag: url=%s\n", url ? url : "NULL");
> -				printf("Diag: protocol=%s\n", prot_name(protocol));
> -				printf("Diag: userandhost=%s\n", ssh_host ? ssh_host : "NULL");
> -				printf("Diag: port=%s\n", port ? port : "NONE");
> -				printf("Diag: path=%s\n", path ? path : "NULL");
> -
> -				free(hostandport);
> -				free(path);
> -				free(conn);
> -				return NULL;
> -			}
> -
>  			ssh = getenv("GIT_SSH_COMMAND");
>  			if (!ssh) {
>  				const char *base;