Re: [PATCH 1/5] connect: split git:// setup into a separate function

[Stefan Beller <sbeller@google.com>]

On Mon, Oct 23, 2017 at 2:29 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> The git_connect function is growing long.  Split the
> PROTO_GIT-specific portion to a separate function to make it easier to
> read.
>
> No functional change intended.
>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

This also looks good to me.

unrelated:
Patch 2 was very easy to review using "log -p -w --color-moved",
this one however was not. This is because -w caused the diff machinery
to generate a completely different diff. (Not showing the new function
completely but some weird function header trickery. The white space
mangled output is below; most of it was colored "moved")

I had to have -w as otherwise --color-moved would not work,
so maybe we want to have an option to ignore white space for the
sake of move detection only, not affecting the diff in general;
maybe '--ignore-white-space-in-move-detection'?

I think once this option is given, all we have to do is pay attention to
this option in diff.c#moved_entry_cmp/next_byte, which is best built
on top of Peffs recent fixes origin/jk/diff-color-moved-fix.
Would that be of interest for people?

Thanks,
Stefan

diff --git a/connect.c b/connect.c
index 7fbd396b35..068e70caad 100644
--- a/connect.c
+++ b/connect.c
@@ -851,36 +851,16 @@ static enum ssh_variant
determine_ssh_variant(const char *ssh_command,
 }

 /*
- * 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).
+ * Open a connection using Git's native protocol.
  *
- * 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).
+ * The caller is responsible for freeing hostandport, but this function may
+ * modify it (for example, to truncate it to remove the port part).
  */
-struct child_process *git_connect(int fd[2], const char *url,
-                                  const char *prog, int flags)
+static struct child_process *git_connect_git(int fd[2], char *hostandport,
+                                             const char *path, const
char *prog,
+                                             int flags)
 {
-        char *hostandport, *path;
         struct child_process *conn = &no_fork;
-        enum protocol protocol;
-
-        /* Without this we cannot rely on waitpid() to tell
-         * what happened to our children.
-         */
-        signal(SIGCHLD, SIG_DFL);
-
-        protocol = parse_connect_url(url, &hostandport, &path);
-        if ((flags & CONNECT_DIAG_URL) && (protocol != PROTO_SSH)) {
-                printf("Diag: url=%s\n", url ? url : "NULL");
-                printf("Diag: protocol=%s\n", prot_name(protocol));
-                printf("Diag: hostandport=%s\n", hostandport ?
hostandport : "NULL");
-                printf("Diag: path=%s\n", path ? path : "NULL");
-                conn = NULL;
-        } else if (protocol == PROTO_GIT) {
         struct strbuf request = STRBUF_INIT;
         /*
          * Set up virtual host information based on where we will
@@ -925,6 +905,41 @@ struct child_process *git_connect(int fd[2],
const char *url,

         free(target_host);
         strbuf_release(&request);
+        return conn;
+}
+
+/*
+ * 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 *hostandport, *path;
+        struct child_process *conn = &no_fork;
+        enum protocol protocol;
+
+        /* Without this we cannot rely on waitpid() to tell
+         * what happened to our children.
+         */
+        signal(SIGCHLD, SIG_DFL);
+
+        protocol = parse_connect_url(url, &hostandport, &path);
+        if ((flags & CONNECT_DIAG_URL) && (protocol != PROTO_SSH)) {
+                printf("Diag: url=%s\n", url ? url : "NULL");
+                printf("Diag: protocol=%s\n", prot_name(protocol));
+                printf("Diag: hostandport=%s\n", hostandport ?
hostandport : "NULL");
+                printf("Diag: path=%s\n", path ? path : "NULL");
+                conn = NULL;
+        } else if (protocol == PROTO_GIT) {
+                conn = git_connect_git(fd, hostandport, path, prog, flags);
         } else {
                 struct strbuf cmd = STRBUF_INIT;
                 const char *const *var;