{"thread":{"id":"65658","subject":"[PATCH] connect: use \"service\" enum for \"name\" argument","startedAt":"2026-05-19T05:22:21Z","lastAt":"2026-05-22T04:44:00Z","messageCount":3,"participants":["Jeff King","Patrick Steinhardt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"543587","messageId":"20260519052219.GA1703179@coredump.intra.peff.net","threadId":"65658","inReplyTo":null,"subject":"[PATCH] connect: use \"service\" enum for \"name\" argument","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-05-19T05:22:19Z","receivedAt":"2026-05-19T05:22:21Z","isPatch":true,"body":"The git_connect() function takes a \"name\" argument which is a bit\nconfusing. It is _not_ the program to run on the remote repo, which is\nspecified by the \"prog\" argument. It should instead be one of a few\nwell-known strings specifying the type of operation (e.g.,\n\"git-upload-pack\"). But to add to the confusion, unless otherwise\nconfigured, those well-known strings will also be the same as the\nprograms we run, making it easy to mistake which variable is which.\n\nThis confusion comes from eaa0fd6584 (git_connect(): fix corner cases in\ndowngrading v2 to v0, 2023-03-17), though in its defense, the term\n\"name\" and the use of a string are found in other connect code, going\nall the way back to b236752a87 (Support remote archive from all smart\ntransports, 2009-12-09).\n\nBut let's see if we can clean things up a bit. The term \"name\" is overly\nvague. We use \"service\" in other places, including in the smart-http\nprotocol, so let's use it here, too.\n\nUsing a string invites the notion that it can be anything, not one of a\ndefined set. Let's instead introduce an enum, which has the added bonus\nthat the compiler can catch typos for us, rather than quietly choosing\nthe wrong service from an unexpected strcmp() result.\n\nWe do still have to turn our enum into those well-known strings to pass\nalong in the remote-helper protocol (e.g., for a stateless-connect\ndirective). But now we do so explicitly and in a way that I think is\nmuch more obvious to follow.\n\nThis is a pure cleanup; there should be no behavior change.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThis was a cleanup that come out of discussion on another patch a month\nor two ago:\n\n  https://lore.kernel.org/git/20260327213308.GA598533@coredump.intra.peff.net/\n\n builtin/archive.c    |  2 +-\n builtin/fetch-pack.c |  2 +-\n builtin/send-pack.c  |  5 +++--\n connect.c            |  4 ++--\n connect.h            |  7 ++++++-\n transport-helper.c   | 43 ++++++++++++++++++++++++++++++-------------\n transport-internal.h |  5 ++++-\n transport.c          | 14 ++++++++------\n transport.h          |  4 +++-\n 9 files changed, 58 insertions(+), 28 deletions(-)\n\ndiff --git a/builtin/archive.c b/builtin/archive.c\nindex 13ea7308c8..3c1288a123 100644\n--- a/builtin/archive.c\n+++ b/builtin/archive.c\n@@ -31,7 +31,7 @@ static int run_remote_archiver(int argc, const char **argv,\n \n \t_remote = remote_get(remote);\n \ttransport = transport_get(_remote, _remote->url.v[0]);\n-\ttransport_connect(transport, \"git-upload-archive\", exec, fd);\n+\ttransport_connect(transport, GIT_CONNECT_UPLOAD_ARCHIVE, exec, fd);\n \n \t/*\n \t * Inject a fake --format field at the beginning of the\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex d9e42bad58..316badd969 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -223,7 +223,7 @@ int cmd_fetch_pack(int argc,\n \t\tint flags = args.verbose ? CONNECT_VERBOSE : 0;\n \t\tif (args.diag_url)\n \t\t\tflags |= CONNECT_DIAG_URL;\n-\t\tconn = git_connect(fd, dest, \"git-upload-pack\",\n+\t\tconn = git_connect(fd, dest, GIT_CONNECT_UPLOAD_PACK,\n \t\t\t\t   args.uploadpack, flags);\n \t\tif (!conn)\n \t\t\treturn args.diag_url ? 0 : 1;\ndiff --git a/builtin/send-pack.c b/builtin/send-pack.c\nindex 8b81c8a848..1412b49bc8 100644\n--- a/builtin/send-pack.c\n+++ b/builtin/send-pack.c\n@@ -273,8 +273,9 @@ int cmd_send_pack(int argc,\n \t\tfd[0] = 0;\n \t\tfd[1] = 1;\n \t} else {\n-\t\tconn = git_connect(fd, dest, \"git-receive-pack\", receivepack,\n-\t\t\targs.verbose ? CONNECT_VERBOSE : 0);\n+\t\tconn = git_connect(fd, dest, GIT_CONNECT_RECEIVE_PACK,\n+\t\t\t\t   receivepack,\n+\t\t\t\t   args.verbose ? CONNECT_VERBOSE : 0);\n \t}\n \n \tpacket_reader_init(&reader, fd[0], NULL, 0,\ndiff --git a/connect.c b/connect.c\nindex a02583a102..9af277bed6 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -1427,7 +1427,7 @@ static void fill_ssh_args(struct child_process *conn, const char *ssh_host,\n  * the connection failed).\n  */\n struct child_process *git_connect(int fd[2], const char *url,\n-\t\t\t\t  const char *name,\n+\t\t\t\t  enum git_connect_service service,\n \t\t\t\t  const char *prog, int flags)\n {\n \tchar *hostandport, *path;\n@@ -1441,7 +1441,7 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t * fetch, ls-remote, etc), then fallback to v0 since we don't know how\n \t * to do anything else (like push or remote archive) via v2.\n \t */\n-\tif (version == protocol_v2 && strcmp(\"git-upload-pack\", name))\n+\tif (version == protocol_v2 && service != GIT_CONNECT_UPLOAD_PACK)\n \t\tversion = protocol_v0;\n \n \t/* Without this we cannot rely on waitpid() to tell\ndiff --git a/connect.h b/connect.h\nindex 1645126c17..c56ecddc0e 100644\n--- a/connect.h\n+++ b/connect.h\n@@ -7,7 +7,12 @@\n #define CONNECT_DIAG_URL      (1u << 1)\n #define CONNECT_IPV4          (1u << 2)\n #define CONNECT_IPV6          (1u << 3)\n-struct child_process *git_connect(int fd[2], const char *url, const char *name, const char *prog, int flags);\n+enum git_connect_service {\n+    GIT_CONNECT_UPLOAD_PACK,\n+    GIT_CONNECT_RECEIVE_PACK,\n+    GIT_CONNECT_UPLOAD_ARCHIVE,\n+};\n+struct child_process *git_connect(int fd[2], const char *url, enum git_connect_service, const char *prog, int flags);\n int finish_connect(struct child_process *conn);\n int git_connection_is_socket(struct child_process *conn);\n int server_supports(const char *feature);\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 4614036c99..bf37c5280c 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -620,8 +620,22 @@ static int run_connect(struct transport *transport, struct strbuf *cmdbuf)\n \treturn ret;\n }\n \n+static const char *connect_service_cmd(enum git_connect_service service)\n+{\n+\tswitch (service) {\n+\tcase GIT_CONNECT_UPLOAD_PACK:\n+\t\treturn \"git-upload-pack\";\n+\tcase GIT_CONNECT_RECEIVE_PACK:\n+\t\treturn \"git-receive-pack\";\n+\tcase GIT_CONNECT_UPLOAD_ARCHIVE:\n+\t\treturn \"git-upload-archive\";\n+\t}\n+\tBUG(\"unknown git_connect_type: %d\", service);\n+}\n+\n static int process_connect_service(struct transport *transport,\n-\t\t\t\t   const char *name, const char *exec)\n+\t\t\t\t   enum git_connect_service service,\n+\t\t\t\t   const char *exec)\n {\n \tstruct helper_data *data = transport->data;\n \tstruct strbuf cmdbuf = STRBUF_INIT;\n@@ -631,7 +645,7 @@ static int process_connect_service(struct transport *transport,\n \t * Handle --upload-pack and friends. This is fire and forget...\n \t * just warn if it fails.\n \t */\n-\tif (strcmp(name, exec)) {\n+\tif (strcmp(connect_service_cmd(service), exec)) {\n \t\tint r = set_helper_option(transport, \"servpath\", exec);\n \t\tif (r > 0)\n \t\t\twarning(_(\"setting remote service path not supported by protocol\"));\n@@ -640,13 +654,15 @@ static int process_connect_service(struct transport *transport,\n \t}\n \n \tif (data->connect) {\n-\t\tstrbuf_addf(&cmdbuf, \"connect %s\\n\", name);\n+\t\tstrbuf_addf(&cmdbuf, \"connect %s\\n\",\n+\t\t\t    connect_service_cmd(service));\n \t\tret = run_connect(transport, &cmdbuf);\n \t} else if (data->stateless_connect &&\n \t\t   (get_protocol_version_config() == protocol_v2) &&\n-\t\t   (!strcmp(\"git-upload-pack\", name) ||\n-\t\t    !strcmp(\"git-upload-archive\", name))) {\n-\t\tstrbuf_addf(&cmdbuf, \"stateless-connect %s\\n\", name);\n+\t\t   (service == GIT_CONNECT_UPLOAD_PACK ||\n+\t\t    service == GIT_CONNECT_UPLOAD_ARCHIVE)) {\n+\t\tstrbuf_addf(&cmdbuf, \"stateless-connect %s\\n\",\n+\t\t\t    connect_service_cmd(service));\n \t\tret = run_connect(transport, &cmdbuf);\n \t\tif (ret)\n \t\t\ttransport->stateless_rpc = 1;\n@@ -660,32 +676,33 @@ static int process_connect(struct transport *transport,\n \t\t\t\t     int for_push)\n {\n \tstruct helper_data *data = transport->data;\n-\tconst char *name;\n+\tenum git_connect_service service;\n \tconst char *exec;\n \tint ret;\n \n-\tname = for_push ? \"git-receive-pack\" : \"git-upload-pack\";\n+\tservice = for_push ? GIT_CONNECT_RECEIVE_PACK : GIT_CONNECT_UPLOAD_PACK;\n \tif (for_push)\n \t\texec = data->transport_options.receivepack;\n \telse\n \t\texec = data->transport_options.uploadpack;\n \n-\tret = process_connect_service(transport, name, exec);\n+\tret = process_connect_service(transport, service, exec);\n \tif (ret)\n \t\tdo_take_over(transport);\n \treturn ret;\n }\n \n-static int connect_helper(struct transport *transport, const char *name,\n-\t\t   const char *exec, int fd[2])\n+static int connect_helper(struct transport *transport, enum git_connect_service service,\n+\t\t\t  const char *exec, int fd[2])\n {\n \tstruct helper_data *data = transport->data;\n \n \t/* Get_helper so connect is inited. */\n \tget_helper(transport);\n \n-\tif (!process_connect_service(transport, name, exec))\n-\t\tdie(_(\"can't connect to subservice %s\"), name);\n+\tif (!process_connect_service(transport, service, exec))\n+\t\tdie(_(\"can't connect to subservice %s\"),\n+\t\t    connect_service_cmd(service));\n \n \tfd[0] = data->helper->out;\n \tfd[1] = data->helper->in;\ndiff --git a/transport-internal.h b/transport-internal.h\nindex 90ea749e5c..051f3ab0dc 100644\n--- a/transport-internal.h\n+++ b/transport-internal.h\n@@ -1,6 +1,8 @@\n #ifndef TRANSPORT_INTERNAL_H\n #define TRANSPORT_INTERNAL_H\n \n+#include \"connect.h\"\n+\n struct ref;\n struct transport;\n struct strvec;\n@@ -58,7 +60,8 @@ struct transport_vtable {\n \t * process involved generating new commits.\n \t **/\n \tint (*push_refs)(struct transport *transport, struct ref *refs, int flags);\n-\tint (*connect)(struct transport *connection, const char *name,\n+\tint (*connect)(struct transport *connection,\n+\t\t       enum git_connect_service service,\n \t\t       const char *executable, int fd[2]);\n \n \t/** get_refs_list(), fetch(), and push_refs() can keep\ndiff --git a/transport.c b/transport.c\nindex 9cde4a4e43..132c93e665 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -309,8 +309,8 @@ static int connect_setup(struct transport *transport, int for_push)\n \n \tdata->conn = git_connect(data->fd, transport->url,\n \t\t\t\t for_push ?\n-\t\t\t\t\t\"git-receive-pack\" :\n-\t\t\t\t\t\"git-upload-pack\",\n+\t\t\t\t\tGIT_CONNECT_RECEIVE_PACK :\n+\t\t\t\t\tGIT_CONNECT_UPLOAD_PACK,\n \t\t\t\t for_push ?\n \t\t\t\t\tdata->options.receivepack :\n \t\t\t\t\tdata->options.uploadpack,\n@@ -957,12 +957,13 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re\n \treturn ret;\n }\n \n-static int connect_git(struct transport *transport, const char *name,\n+static int connect_git(struct transport *transport,\n+\t\t       enum git_connect_service service,\n \t\t       const char *executable, int fd[2])\n {\n \tstruct git_transport_data *data = transport->data;\n \tdata->conn = git_connect(data->fd, transport->url,\n-\t\t\t\t name, executable, 0);\n+\t\t\t\t service, executable, 0);\n \tfd[0] = data->fd[0];\n \tfd[1] = data->fd[1];\n \treturn 0;\n@@ -1656,11 +1657,12 @@ void transport_unlock_pack(struct transport *transport, unsigned int flags)\n \t\tstring_list_clear(&transport->pack_lockfiles, 0);\n }\n \n-int transport_connect(struct transport *transport, const char *name,\n+int transport_connect(struct transport *transport,\n+\t\t      enum git_connect_service service,\n \t\t      const char *exec, int fd[2])\n {\n \tif (transport->vtable->connect)\n-\t\treturn transport->vtable->connect(transport, name, exec, fd);\n+\t\treturn transport->vtable->connect(transport, service, exec, fd);\n \telse\n \t\tdie(_(\"operation not supported by protocol\"));\n }\ndiff --git a/transport.h b/transport.h\nindex 892f19454a..78e9ea8ad1 100644\n--- a/transport.h\n+++ b/transport.h\n@@ -5,6 +5,7 @@\n #include \"remote.h\"\n #include \"list-objects-filter-options.h\"\n #include \"string-list.h\"\n+#include \"connect.h\"\n \n struct git_transport_options {\n \tunsigned thin : 1;\n@@ -324,7 +325,8 @@ char *transport_anonymize_url(const char *url);\n void transport_take_over(struct transport *transport,\n \t\t\t struct child_process *child);\n \n-int transport_connect(struct transport *transport, const char *name,\n+int transport_connect(struct transport *transport,\n+\t\t      enum git_connect_service service,\n \t\t      const char *exec, int fd[2]);\n \n /* Transport methods defined outside transport.c */\n-- \n2.54.0.524.g198262df96\n"},{"id":"543786","messageId":"ag7AJMbav6KgSCjj@pks.im","threadId":"65658","inReplyTo":"20260519052219.GA1703179@coredump.intra.peff.net","subject":"Re: [PATCH] connect: use \"service\" enum for \"name\" argument","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-05-21T08:19:48Z","receivedAt":"2026-05-21T08:19:54Z","isPatch":true,"body":"On Tue, May 19, 2026 at 01:22:19AM -0400, Jeff King wrote:\n> diff --git a/connect.h b/connect.h\n> index 1645126c17..c56ecddc0e 100644\n> --- a/connect.h\n> +++ b/connect.h\n> @@ -7,7 +7,12 @@\n>  #define CONNECT_DIAG_URL      (1u << 1)\n>  #define CONNECT_IPV4          (1u << 2)\n>  #define CONNECT_IPV6          (1u << 3)\n> -struct child_process *git_connect(int fd[2], const char *url, const char *name, const char *prog, int flags);\n> +enum git_connect_service {\n> +    GIT_CONNECT_UPLOAD_PACK,\n> +    GIT_CONNECT_RECEIVE_PACK,\n> +    GIT_CONNECT_UPLOAD_ARCHIVE,\n> +};\n> +struct child_process *git_connect(int fd[2], const char *url, enum git_connect_service, const char *prog, int flags);\n>  int finish_connect(struct child_process *conn);\n>  int git_connection_is_socket(struct child_process *conn);\n>  int server_supports(const char *feature);\n\nThis is all quite tightly-packed, and the patch would be a good\nopportunity to maybe add some documentation. But that's certainly\nmoving the goalposts quite a bit.\n\n> diff --git a/transport-helper.c b/transport-helper.c\n> index 4614036c99..bf37c5280c 100644\n> --- a/transport-helper.c\n> +++ b/transport-helper.c\n> @@ -620,8 +620,22 @@ static int run_connect(struct transport *transport, struct strbuf *cmdbuf)\n>  \treturn ret;\n>  }\n>  \n> +static const char *connect_service_cmd(enum git_connect_service service)\n> +{\n> +\tswitch (service) {\n> +\tcase GIT_CONNECT_UPLOAD_PACK:\n> +\t\treturn \"git-upload-pack\";\n> +\tcase GIT_CONNECT_RECEIVE_PACK:\n> +\t\treturn \"git-receive-pack\";\n> +\tcase GIT_CONNECT_UPLOAD_ARCHIVE:\n> +\t\treturn \"git-upload-archive\";\n> +\t}\n> +\tBUG(\"unknown git_connect_type: %d\", service);\n> +}\n\nShouldn't this say \"unknown git_connect_service\" instead of \"_type\"?\n\nOther than that this patch looks good to me, and I agree that this makes\nthe argument a bit easier to understand.\n\nPatrick\n"},{"id":"543877","messageId":"20260522044352.GA861761@coredump.intra.peff.net","threadId":"65658","inReplyTo":"ag7AJMbav6KgSCjj@pks.im","subject":"Re: [PATCH] connect: use \"service\" enum for \"name\" argument","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-05-22T04:43:52Z","receivedAt":"2026-05-22T04:44:00Z","isPatch":true,"body":"On Thu, May 21, 2026 at 10:19:48AM +0200, Patrick Steinhardt wrote:\n\n> > +\tswitch (service) {\n> > +\tcase GIT_CONNECT_UPLOAD_PACK:\n> > +\t\treturn \"git-upload-pack\";\n> > +\tcase GIT_CONNECT_RECEIVE_PACK:\n> > +\t\treturn \"git-receive-pack\";\n> > +\tcase GIT_CONNECT_UPLOAD_ARCHIVE:\n> > +\t\treturn \"git-upload-archive\";\n> > +\t}\n> > +\tBUG(\"unknown git_connect_type: %d\", service);\n> > +}\n> \n> Shouldn't this say \"unknown git_connect_service\" instead of \"_type\"?\n\nOops, yes. As you probably guessed, I started with \"type\" before\nrealizing that \"service\" was a better word.\n\nThe patch is in next, so the fixup on top (of jk/connect-service-enum)\nis below.\n\n-- >8 --\nSubject: [PATCH] transport-helper: fix typo in BUG() message\n\nWe mistakenly refer to the git_connect_service enum as \"_type\" rather\nthan \"_service\". Users should never see this message in practice, but it\nis slightly confusing when reading the code.\n\nReported-by: Patrick Steinhardt <ps@pks.im>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n transport-helper.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/transport-helper.c b/transport-helper.c\nindex bf37c5280c..b672801ae4 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -630,7 +630,7 @@ static const char *connect_service_cmd(enum git_connect_service service)\n \tcase GIT_CONNECT_UPLOAD_ARCHIVE:\n \t\treturn \"git-upload-archive\";\n \t}\n-\tBUG(\"unknown git_connect_type: %d\", service);\n+\tBUG(\"unknown git_connect_service: %d\", service);\n }\n \n static int process_connect_service(struct transport *transport,\n-- \n2.54.0.618.gdbb63b8024\n\n"}]}