connect: use "service" enum for "name" argument

The git_connect() function takes a "name" argument which is a bit
confusing. It is _not_ the program to run on the remote repo, which is
specified by the "prog" argument. It should instead be one of a few
well-known strings specifying the type of operation (e.g.,
"git-upload-pack"). But to add to the confusion, unless otherwise
configured, those well-known strings will also be the same as the
programs we run, making it easy to mistake which variable is which.

This confusion comes from eaa0fd6584 (git_connect(): fix corner cases in
downgrading v2 to v0, 2023-03-17), though in its defense, the term
"name" and the use of a string are found in other connect code, going
all the way back to b236752a87 (Support remote archive from all smart
transports, 2009-12-09).

But let's see if we can clean things up a bit. The term "name" is overly
vague. We use "service" in other places, including in the smart-http
protocol, so let's use it here, too.

Using a string invites the notion that it can be anything, not one of a
defined set. Let's instead introduce an enum, which has the added bonus
that the compiler can catch typos for us, rather than quietly choosing
the wrong service from an unexpected strcmp() result.

We do still have to turn our enum into those well-known strings to pass
along in the remote-helper protocol (e.g., for a stateless-connect
directive). But now we do so explicitly and in a way that I think is
much more obvious to follow.

This is a pure cleanup; there should be no behavior change.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/builtin/archive.c b/builtin/archive.c
index 13ea7308c8b8b99afd59586e2454fc870cd0aedf..3c1288a1231ab79209ee3ff130616ecf7884dc60 100644 (file)
--- a/builtin/archive.c
+++ b/builtin/archive.c
@@ -31,7 +31,7 @@ static int run_remote_archiver(int argc, const char **argv,
 
        _remote = remote_get(remote);
        transport = transport_get(_remote, _remote->url.v[0]);
-       transport_connect(transport, "git-upload-archive", exec, fd);
+       transport_connect(transport, GIT_CONNECT_UPLOAD_ARCHIVE, exec, fd);
 
        /*
         * Inject a fake --format field at the beginning of the

diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index d9e42bad58430dceae7076eeb1676be3851e5f1f..316badd969613fe994cacd61d9127eba4a39679d 100644 (file)
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -223,7 +223,7 @@ int cmd_fetch_pack(int argc,
                int flags = args.verbose ? CONNECT_VERBOSE : 0;
                if (args.diag_url)
                        flags |= CONNECT_DIAG_URL;
-               conn = git_connect(fd, dest, "git-upload-pack",
+               conn = git_connect(fd, dest, GIT_CONNECT_UPLOAD_PACK,
                                   args.uploadpack, flags);
                if (!conn)
                        return args.diag_url ? 0 : 1;

diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index 8b81c8a84863eb0b0bb20a9fdcecffd5dada5c6c..1412b49bc845b1e70ba111f3d65821d3be13dce1 100644 (file)
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -273,8 +273,9 @@ int cmd_send_pack(int argc,
                fd[0] = 0;
                fd[1] = 1;
        } else {
-               conn = git_connect(fd, dest, "git-receive-pack", receivepack,
-                       args.verbose ? CONNECT_VERBOSE : 0);
+               conn = git_connect(fd, dest, GIT_CONNECT_RECEIVE_PACK,
+                                  receivepack,
+                                  args.verbose ? CONNECT_VERBOSE : 0);
        }
 
        packet_reader_init(&reader, fd[0], NULL, 0,

diff --git a/connect.c b/connect.c
index fcd35c5539a76e4f694f8392615de262c5b40c6d..ebc6c552d6a906961ad0dd4e83fa8c7c9384c8f3 100644 (file)
--- a/connect.c
+++ b/connect.c
@@ -1429,7 +1429,7 @@ static void fill_ssh_args(struct child_process *conn, const char *ssh_host,
  * the connection failed).
  */
 struct child_process *git_connect(int fd[2], const char *url,
-                                 const char *name,
+                                 enum git_connect_service service,
                                  const char *prog, int flags)
 {
        char *hostandport, *path;
@@ -1443,7 +1443,7 @@ struct child_process *git_connect(int fd[2], const char *url,
         * fetch, ls-remote, etc), then fallback to v0 since we don't know how
         * to do anything else (like push or remote archive) via v2.
         */
-       if (version == protocol_v2 && strcmp("git-upload-pack", name))
+       if (version == protocol_v2 && service != GIT_CONNECT_UPLOAD_PACK)
                version = protocol_v0;
 
        /* Without this we cannot rely on waitpid() to tell

diff --git a/connect.h b/connect.h
index 1645126c17f889659d84174210a43939ef67507f..c56ecddc0e94df988d99e50fc25f99850256a1b8 100644 (file)
--- a/connect.h
+++ b/connect.h
@@ -7,7 +7,12 @@
 #define CONNECT_DIAG_URL      (1u << 1)
 #define CONNECT_IPV4          (1u << 2)
 #define CONNECT_IPV6          (1u << 3)
-struct child_process *git_connect(int fd[2], const char *url, const char *name, const char *prog, int flags);
+enum git_connect_service {
+    GIT_CONNECT_UPLOAD_PACK,
+    GIT_CONNECT_RECEIVE_PACK,
+    GIT_CONNECT_UPLOAD_ARCHIVE,
+};
+struct child_process *git_connect(int fd[2], const char *url, enum git_connect_service, const char *prog, int flags);
 int finish_connect(struct child_process *conn);
 int git_connection_is_socket(struct child_process *conn);
 int server_supports(const char *feature);

diff --git a/transport-helper.c b/transport-helper.c
index 4e5d1d914fb12a6e8abb532985140683489773b8..826b35ac8c9c7034dfbf20607575d01cb640733c 100644 (file)
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -622,8 +622,22 @@ static int run_connect(struct transport *transport, struct strbuf *cmdbuf)
        return ret;
 }
 
+static const char *connect_service_cmd(enum git_connect_service service)
+{
+       switch (service) {
+       case GIT_CONNECT_UPLOAD_PACK:
+               return "git-upload-pack";
+       case GIT_CONNECT_RECEIVE_PACK:
+               return "git-receive-pack";
+       case GIT_CONNECT_UPLOAD_ARCHIVE:
+               return "git-upload-archive";
+       }
+       BUG("unknown git_connect_type: %d", service);
+}
+
 static int process_connect_service(struct transport *transport,
-                                  const char *name, const char *exec)
+                                  enum git_connect_service service,
+                                  const char *exec)
 {
        struct helper_data *data = transport->data;
        struct strbuf cmdbuf = STRBUF_INIT;
@@ -633,7 +647,7 @@ static int process_connect_service(struct transport *transport,
         * Handle --upload-pack and friends. This is fire and forget...
         * just warn if it fails.
         */
-       if (strcmp(name, exec)) {
+       if (strcmp(connect_service_cmd(service), exec)) {
                int r = set_helper_option(transport, "servpath", exec);
                if (r > 0)
                        warning(_("setting remote service path not supported by protocol"));
@@ -642,13 +656,15 @@ static int process_connect_service(struct transport *transport,
        }
 
        if (data->connect) {
-               strbuf_addf(&cmdbuf, "connect %s\n", name);
+               strbuf_addf(&cmdbuf, "connect %s\n",
+                           connect_service_cmd(service));
                ret = run_connect(transport, &cmdbuf);
        } else if (data->stateless_connect &&
                   (get_protocol_version_config() == protocol_v2) &&
-                  (!strcmp("git-upload-pack", name) ||
-                   !strcmp("git-upload-archive", name))) {
-               strbuf_addf(&cmdbuf, "stateless-connect %s\n", name);
+                  (service == GIT_CONNECT_UPLOAD_PACK ||
+                   service == GIT_CONNECT_UPLOAD_ARCHIVE)) {
+               strbuf_addf(&cmdbuf, "stateless-connect %s\n",
+                           connect_service_cmd(service));
                ret = run_connect(transport, &cmdbuf);
                if (ret)
                        transport->stateless_rpc = 1;
@@ -662,32 +678,33 @@ static int process_connect(struct transport *transport,
                                     int for_push)
 {
        struct helper_data *data = transport->data;
-       const char *name;
+       enum git_connect_service service;
        const char *exec;
        int ret;
 
-       name = for_push ? "git-receive-pack" : "git-upload-pack";
+       service = for_push ? GIT_CONNECT_RECEIVE_PACK : GIT_CONNECT_UPLOAD_PACK;
        if (for_push)
                exec = data->transport_options.receivepack;
        else
                exec = data->transport_options.uploadpack;
 
-       ret = process_connect_service(transport, name, exec);
+       ret = process_connect_service(transport, service, exec);
        if (ret)
                do_take_over(transport);
        return ret;
 }
 
-static int connect_helper(struct transport *transport, const char *name,
-                  const char *exec, int fd[2])
+static int connect_helper(struct transport *transport, enum git_connect_service service,
+                         const char *exec, int fd[2])
 {
        struct helper_data *data = transport->data;
 
        /* Get_helper so connect is inited. */
        get_helper(transport);
 
-       if (!process_connect_service(transport, name, exec))
-               die(_("can't connect to subservice %s"), name);
+       if (!process_connect_service(transport, service, exec))
+               die(_("can't connect to subservice %s"),
+                   connect_service_cmd(service));
 
        fd[0] = data->helper->out;
        fd[1] = data->helper->in;

diff --git a/transport-internal.h b/transport-internal.h
index 90ea749e5cfbf7de994a8747186ad5fb3ae7c653..051f3ab0dc95ec714235252685d160fe7b9c1152 100644 (file)
--- a/transport-internal.h
+++ b/transport-internal.h
@@ -1,6 +1,8 @@
 #ifndef TRANSPORT_INTERNAL_H
 #define TRANSPORT_INTERNAL_H
 
+#include "connect.h"
+
 struct ref;
 struct transport;
 struct strvec;
@@ -58,7 +60,8 @@ struct transport_vtable {
         * process involved generating new commits.
         **/
        int (*push_refs)(struct transport *transport, struct ref *refs, int flags);
-       int (*connect)(struct transport *connection, const char *name,
+       int (*connect)(struct transport *connection,
+                      enum git_connect_service service,
                       const char *executable, int fd[2]);
 
        /** get_refs_list(), fetch(), and push_refs() can keep

diff --git a/transport.c b/transport.c
index e53936d87b641fbb3f3fe81bdf8a44165970f5f3..f2179057efdb3bcb184254e2bf655a1dfcd3d857 100644 (file)
--- a/transport.c
+++ b/transport.c
@@ -308,8 +308,8 @@ static int connect_setup(struct transport *transport, int for_push)
 
        data->conn = git_connect(data->fd, transport->url,
                                 for_push ?
-                                       "git-receive-pack" :
-                                       "git-upload-pack",
+                                       GIT_CONNECT_RECEIVE_PACK :
+                                       GIT_CONNECT_UPLOAD_PACK,
                                 for_push ?
                                        data->options.receivepack :
                                        data->options.uploadpack,
@@ -956,12 +956,13 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re
        return ret;
 }
 
-static int connect_git(struct transport *transport, const char *name,
+static int connect_git(struct transport *transport,
+                      enum git_connect_service service,
                       const char *executable, int fd[2])
 {
        struct git_transport_data *data = transport->data;
        data->conn = git_connect(data->fd, transport->url,
-                                name, executable, 0);
+                                service, executable, 0);
        fd[0] = data->fd[0];
        fd[1] = data->fd[1];
        return 0;
@@ -1651,11 +1652,12 @@ void transport_unlock_pack(struct transport *transport, unsigned int flags)
                string_list_clear(&transport->pack_lockfiles, 0);
 }
 
-int transport_connect(struct transport *transport, const char *name,
+int transport_connect(struct transport *transport,
+                     enum git_connect_service service,
                      const char *exec, int fd[2])
 {
        if (transport->vtable->connect)
-               return transport->vtable->connect(transport, name, exec, fd);
+               return transport->vtable->connect(transport, service, exec, fd);
        else
                die(_("operation not supported by protocol"));
 }

diff --git a/transport.h b/transport.h
index 892f19454a75d602ec69cb251f503bff0b93aa58..78e9ea8ad1271d02b869a9a0fceb4eb7cef6137c 100644 (file)
--- a/transport.h
+++ b/transport.h
@@ -5,6 +5,7 @@
 #include "remote.h"
 #include "list-objects-filter-options.h"
 #include "string-list.h"
+#include "connect.h"
 
 struct git_transport_options {
        unsigned thin : 1;
@@ -324,7 +325,8 @@ char *transport_anonymize_url(const char *url);
 void transport_take_over(struct transport *transport,
                         struct child_process *child);
 
-int transport_connect(struct transport *transport, const char *name,
+int transport_connect(struct transport *transport,
+                     enum git_connect_service service,
                      const char *exec, int fd[2]);
 
 /* Transport methods defined outside transport.c */