git clone: is an URL local or ssh

Bug fix for msygit in t5601 : use $PWD insted of $(pwd)

Can you please make this into a series of small patches so that we can more easily see the good and the bad parts? One of the patches could be a clean-up of the current protocol determination and URL dissection, which is indigestible spaghetti right now.

connect.c +43 -0

 	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.

connect.c +3 -3

-		free(url);
-		if (free_path)
-			free(path);
+		free(host);
+		free(port);
+		free(path);
 		return conn;
 	}
 

Thanks for picking this up, please see some minor nits inline, and git_connect() is at the end

connect.c static char *get_port(char *host) · line 555

Quoted 8 lines

connect.c +2 -2

-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 48 lines

connect.c +43 -0

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

connect.c struct child_process *git_connect(int fd[2], const char *url_orig, · line 694

Quoted 7 lines

connect.c +0 -0

 	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 9 lines

connect.c +3 -3

-		free(url);
-		if (free_path)
-			free(path);
+		free(host);
+		free(port);
+		free(path);
 		return conn;
 	}
 

connect.c struct child_process *git_connect(int fd[2], const char *url_orig, · line 714

Quoted 7 lines

connect.c +0 -3

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

Am 05.11.2013 21:45, schrieb Torsten Bögershausen:

Quoted 13 lines · Johannes Sixt

Thanks for picking this up, please see some minor nits inline, and git_connect() is at the end

connect.c static char *get_port(char *host) · line 555

Quoted 8 lines

connect.c +2 -2

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

Quoted 48 lines

Quoted 48 lines

connect.c +43 -0

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

Quoted 8 lines

connect.c struct child_process *git_connect(int fd[2], const char *url_orig, · line 694

Quoted 7 lines

connect.c +0 -0

 	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.

Quoted 9 lines

Quoted 9 lines

connect.c +3 -3

-		free(url);
-		if (free_path)
-			free(path);
+		free(host);
+		free(port);
+		free(path);
 		return conn;
 	}
 

Quoted 10 lines

connect.c struct child_process *git_connect(int fd[2], const char *url_orig, · line 714

Quoted 7 lines

connect.c +0 -3

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