Re: [RFC PATCH 2/3] connect: group CONNECT_DIAG_URL handling code
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Apr 29, 2016, 15:59 UTC
- Message-ID
- <xmqqeg9o1k40.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <1461890625-23222-2-git-send-email-mh@glandium.org>
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.
Show 5 quoted lines
> --- > connect.c | 27 ++++++++++++--------------- > 1 file changed, 12 insertions(+), 15 deletions(-) > > Note this makes http://marc.info/?l=git&m=146183714532394 irrelevant.
Indeed.
Show 48 quoted lines
>
> 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;