Hi, On Oct 24, 2017, Junio C Hamano wrote: > Jonathan Nieder <jrnieder@xxxxxxxxx> writes: >> +static struct child_process *git_connect_git(int fd[2], char *hostandport, >> + const char *path, const char *prog, >> + int flags) >> +{ >> + struct child_process *conn = &no_fork; >> + struct strbuf request = STRBUF_INIT; > > As this one decides what "conn" to return, including the fallback > &no_fork instance,... > >> + ... >> + 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, >> @@ -881,50 +939,7 @@ struct child_process *git_connect(int fd[2], const char *url, > > Each of the if/elseif/ cascade, one of which calls the new helper, > now makes an explicit assignment to "conn" declared in > git_connect(). > > Which means the defaulting of git_connect::conn to &no_fork is now > unneeded. One of the things that made the original cascade a bit > harder to follow than necessary, aside from the physical length of > the PROTO_GIT part, was that the case where conn remains to point at > no_fork looked very special and it was buried in that long PROTO_GIT > part. Good idea. Here's what I'll include in the reroll. -- >8 -- Subject: connect: move no_fork fallback to git_tcp_connect git_connect has the structure struct child_process *conn = &no_fork; ... switch (protocol) { case PROTO_GIT: if (git_use_proxy(hostandport)) conn = git_proxy_connect(fd, hostandport); else git_tcp_connect(fd, hostandport, flags); ... break; case PROTO_SSH: conn = xmalloc(sizeof(*conn)); child_process_init(conn); argv_array_push(&conn->args, ssh); ... break; ... return conn; In all cases except the git_tcp_connect case, conn is explicitly assigned a value. Make the code clearer by explicitly assigning 'conn = &no_fork' in the tcp case and eliminating the default so the compiler can ensure conn is always correctly assigned. Noticed-by: Junio C Hamano <gitster@xxxxxxxxx> Signed-off-by: Jonathan Nieder <jrnieder@xxxxxxxxx> --- connect.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/connect.c b/connect.c index 7fbd396b35..b6accf71cb 100644 --- a/connect.c +++ b/connect.c @@ -582,12 +582,21 @@ static int git_tcp_connect_sock(char *host, int flags) #endif /* NO_IPV6 */ -static void git_tcp_connect(int fd[2], char *host, int flags) +static struct child_process no_fork = CHILD_PROCESS_INIT; + +int git_connection_is_socket(struct child_process *conn) +{ + return conn == &no_fork; +} + +static struct child_process *git_tcp_connect(int fd[2], char *host, int flags) { int sockfd = git_tcp_connect_sock(host, flags); fd[0] = sockfd; fd[1] = dup(sockfd); + + return &no_fork; } @@ -761,8 +770,6 @@ static enum protocol parse_connect_url(const char *url_orig, char **ret_host, return protocol; } -static struct child_process no_fork = CHILD_PROCESS_INIT; - static const char *get_ssh_command(void) { const char *ssh; @@ -865,7 +872,7 @@ 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; + struct child_process *conn; enum protocol protocol; /* Without this we cannot rely on waitpid() to tell @@ -901,7 +908,7 @@ struct child_process *git_connect(int fd[2], const char *url, if (git_use_proxy(hostandport)) conn = git_proxy_connect(fd, hostandport); else - git_tcp_connect(fd, hostandport, flags); + conn = git_tcp_connect(fd, hostandport, flags); /* * Separate original protocol components prog and path * from extended host header with a NUL byte. @@ -1041,11 +1048,6 @@ struct child_process *git_connect(int fd[2], const char *url, return conn; } -int git_connection_is_socket(struct child_process *conn) -{ - return conn == &no_fork; -} - int finish_connect(struct child_process *conn) { int code; -- 2.15.0.448.gf294e3d99a