Re: [PATCH v3 24/35] connect: refactor git_connect to only get the protocol version once

[Jonathan Tan]

On Tue,  6 Feb 2018 17:13:01 -0800
Brandon Williams  wrote:

> Instead of having each builtin transport asking for which protocol
> version the user has configured in 'protocol.version' by calling
> `get_protocol_version_config()` multiple times, factor this logic out
> so there is just a single call at the beginning of `git_connect()`.
> 
> This will be helpful in the next patch where we can have centralized
> logic which determines if we need to request a different protocol
> version than what the user has configured.
> 
> Signed-off-by: Brandon Williams 

Reviewed-by: Jonathan Tan 

[PATCH v3 24/35] connect: refactor git_connect to only get the protocol version once

[Brandon Williams]

Instead of having each builtin transport asking for which protocol
version the user has configured in 'protocol.version' by calling
`get_protocol_version_config()` multiple times, factor this logic out
so there is just a single call at the beginning of `git_connect()`.

This will be helpful in the next patch where we can have centralized
logic which determines if we need to request a different protocol
version than what the user has configured.

Signed-off-by: Brandon Williams 
---
 connect.c | 27 +++
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/connect.c b/connect.c
index 9577528f3..dbf4def65 100644
--- a/connect.c
+++ b/connect.c
@@ -1029,6 +1029,7 @@ static enum ssh_variant determine_ssh_variant(const char 
*ssh_command,
  */
 static struct child_process *git_connect_git(int fd[2], char *hostandport,
 const char *path, const char *prog,
+enum protocol_version version,
 int flags)
 {
struct child_process *conn;
@@ -1067,10 +1068,10 @@ static struct child_process *git_connect_git(int fd[2], 
char *hostandport,
target_host, 0);
 
/* If using a new version put that stuff here after a second null byte 
*/
-   if (get_protocol_version_config() > 0) {
+   if (version > 0) {
strbuf_addch(, '\0');
strbuf_addf(, "version=%d%c",
-   get_protocol_version_config(), '\0');
+   version, '\0');
}
 
packet_write(fd[1], request.buf, request.len);
@@ -1086,14 +1087,14 @@ static struct child_process *git_connect_git(int fd[2], 
char *hostandport,
  */
 static void push_ssh_options(struct argv_array *args, struct argv_array *env,
 enum ssh_variant variant, const char *port,
-int flags)
+enum protocol_version version, int flags)
 {
if (variant == VARIANT_SSH &&
-   get_protocol_version_config() > 0) {
+   version > 0) {
argv_array_push(args, "-o");
argv_array_push(args, "SendEnv=" GIT_PROTOCOL_ENVIRONMENT);
argv_array_pushf(env, GIT_PROTOCOL_ENVIRONMENT "=version=%d",
-get_protocol_version_config());
+version);
}
 
if (flags & CONNECT_IPV4) {
@@ -1146,7 +1147,8 @@ static void push_ssh_options(struct argv_array *args, 
struct argv_array *env,
 
 /* Prepare a child_process for use by Git's SSH-tunneled transport. */
 static void fill_ssh_args(struct child_process *conn, const char *ssh_host,
- const char *port, int flags)
+ const char *port, enum protocol_version version,
+ int flags)
 {
const char *ssh;
enum ssh_variant variant;
@@ -1180,14 +1182,14 @@ static void fill_ssh_args(struct child_process *conn, 
const char *ssh_host,
argv_array_push(, ssh);
argv_array_push(, "-G");
push_ssh_options(, _array,
-VARIANT_SSH, port, flags);
+VARIANT_SSH, port, version, flags);
argv_array_push(, ssh_host);
 
variant = run_command() ? VARIANT_SIMPLE : VARIANT_SSH;
}
 
argv_array_push(>args, ssh);
-   push_ssh_options(>args, >env_array, variant, port, flags);
+   push_ssh_options(>args, >env_array, variant, port, version, 
flags);
argv_array_push(>args, ssh_host);
 }
 
@@ -1208,6 +1210,7 @@ struct child_process *git_connect(int fd[2], const char 
*url,
char *hostandport, *path;
struct child_process *conn;
enum protocol protocol;
+   enum protocol_version version = get_protocol_version_config();
 
/* Without this we cannot rely on waitpid() to tell
 * what happened to our children.
@@ -1222,7 +1225,7 @@ struct child_process *git_connect(int fd[2], const char 
*url,
printf("Diag: path=%s\n", path ? path : "NULL");
conn = NULL;
} else if (protocol == PROTO_GIT) {
-   conn = git_connect_git(fd, hostandport, path, prog, flags);
+   conn = git_connect_git(fd, hostandport, path, prog, version, 
flags);
} else {
struct strbuf cmd = STRBUF_INIT;
const char *const *var;
@@ -1265,12 +1268,12 @@ struct child_process *git_connect(int fd[2], const char 
*url,
strbuf_release();
return NULL;
}
-   fill_ssh_args(conn, ssh_host, port, flags);
+   fill_ssh_args(conn, ssh_host, port, version, flags);
} else {