git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 1/3] connect: Rename name to command in connect_git()

From
Jeff King <peff@peff.net>
Date
Mar 27, 2026, 21:33 UTC
Message-ID
<20260327213308.GA598533@coredump.intra.peff.net>
In-Reply-To
<20260326233739.2911354-2-wesleys@opperschaap.net>
On Thu, Mar 26, 2026 at 07:37:36PM -0400, Wesley Schwengle wrote:
Show 5 quoted lines
> connect_git has `char *name' in its signature and it caught me a little
> offguard. I initially thought it was the remote name. But when you look
> closer at the various call sites it is actually a command that is send
> over the wire, eg . `git-receive-pack'. Change the naming makes it
> easier to read the code and understand its intention.

I agree that "name" is not all that descriptive, but I think there's a hidden gotcha in the explanation above. This string is _not_ the command that we send over the wire. That's "prog" in the same function. And the reason that "name" exists is that it is a stable name for the operation we are performing, like "git-receive-pack", even if configuration or command-line parameters (like "--receive-pack=foo") tell us to use a different command name.

So probably "op" or "type" is a more accurate description. This conceptually ought to be an enum, too, since it is selecting from a limited set of operations we know about.

I took a quick stab at converting it to an enum (see below) and it's mostly an improvement, but:

  1. The ripple effect went much farther than I expected, since the
     transport code is passing these values, too. If we are going to
     update one function in the chain, we should probably do all of them
     (even if it is just a change of the variable name).
  2. We end up having to convert to a string at some points anyway for
     producing error messages, and for passing across the remote-helper
     barrier. But I think we are still better off, because it's more
     clear where we are using the string-ified version and what values
     it could take.
-Peff
---
diff --git a/builtin/archive.c b/builtin/archive.c
index 13ea7308c8..3c1288a123 100644
--- 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 d9e42bad58..316badd969 100644
--- 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 8b81c8a848..1412b49bc8 100644
--- 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 a02583a102..dad1cff1a8 100644
--- a/connect.c
+++ b/connect.c
@@ -1428,6 +1428,7 @@ static void fill_ssh_args(struct child_process *conn, const char *ssh_host,
  */
 struct child_process *git_connect(int fd[2], const char *url,
 				  const char *name,
+				  enum git_connect_type type,
 				  const char *prog, int flags)
 {
 	char *hostandport, *path;
@@ -1441,7 +1442,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 && type != 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 1645126c17..641498c759 100644
--- 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_type {
+    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_type, 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 4d95d84f9e..c7fab6f560 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -620,8 +620,22 @@ static int run_connect(struct transport *transport, struct strbuf *cmdbuf)
 	return ret;
 }
 
+static const char *connect_type_to_command(enum git_connect_type type)
+{
+	switch (type) {
+	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", type);
+}
+
 static int process_connect_service(struct transport *transport,
-				   const char *name, const char *exec)
+				   enum git_connect_type type,
+				   const char *exec)
 {
 	struct helper_data *data = transport->data;
 	struct strbuf cmdbuf = STRBUF_INIT;
@@ -631,7 +645,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_type_to_command(type), exec)) {
 		int r = set_helper_option(transport, "servpath", exec);
 		if (r > 0)
 			warning(_("setting remote service path not supported by protocol"));
@@ -640,13 +654,13 @@ 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_type_to_command(type));
 		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);
+		   (type == GIT_CONNECT_UPLOAD_PACK ||
+		    type == GIT_CONNECT_UPLOAD_ARCHIVE)) {
+		strbuf_addf(&cmdbuf, "stateless-connect %s\n", connect_type_to_command(type));
 		ret = run_connect(transport, &cmdbuf);
 		if (ret)
 			transport->stateless_rpc = 1;
@@ -660,32 +674,33 @@ static int process_connect(struct transport *transport,
 				     int for_push)
 {
 	struct helper_data *data = transport->data;
-	const char *name;
+	enum git_connect_type type;
 	const char *exec;
 	int ret;
 
-	name = for_push ? "git-receive-pack" : "git-upload-pack";
+	type = 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, type, 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_type type,
+			  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, type, exec))
+		die(_("can't connect to subservice %s"),
+		    connect_type_to_command(type));
 
 	fd[0] = data->helper->out;
 	fd[1] = data->helper->in;
diff --git a/transport-internal.h b/transport-internal.h
index 90ea749e5c..1a86c63ce0 100644
--- a/transport-internal.h
+++ b/transport-internal.h
@@ -58,7 +58,7 @@ 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_type type,
 		       const char *executable, int fd[2]);
 
 	/** get_refs_list(), fetch(), and push_refs() can keep
diff --git a/transport.c b/transport.c
index cb1befba8c..2fd94d701f 100644
--- 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,12 @@ 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_type type,
 		       const char *executable, int fd[2])
 {
 	struct git_transport_data *data = transport->data;
 	data->conn = git_connect(data->fd, transport->url,
-				 name, executable, 0);
+				 type, executable, 0);
 	fd[0] = data->fd[0];
 	fd[1] = data->fd[1];
 	return 0;
@@ -1650,11 +1650,11 @@ 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_type type,
 		      const char *exec, int fd[2])
 {
 	if (transport->vtable->connect)
-		return transport->vtable->connect(transport, name, exec, fd);
+		return transport->vtable->connect(transport, type, exec, fd);
 	else
 		die(_("operation not supported by protocol"));
 }
diff --git a/transport.h b/transport.h
index 892f19454a..1e6fd263f6 100644
--- 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,7 @@ 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_type type,
 		      const char *exec, int fd[2]);
 
 /* Transport methods defined outside transport.c */
Previous: Wesley SchwengleNext: Wesley
Message 3 of 24 in “Add support for per-remote and per-namespace SSH options”
  1. 0/3 Add support for per-remote and per-namespace SSH optionsWesley Schwengle, Mar 26, 2026
  2. 1/3 connect: Rename name to command in connect_git()Wesley Schwengle, Mar 26, 2026
  3. Jeff KingMar 27, 2026
  4. WesleyMar 28, 2026
  5. Jeff KingMar 28, 2026
  6. WesleyMar 28, 2026
  7. 2/3 connect: Add transport->remote->name to git_connect()Wesley Schwengle, Mar 26, 2026
  8. Jeff KingMar 27, 2026
  9. 3/3 connect: Add support for per-remote and per-namespace SSH optionsWesley Schwengle, Mar 26, 2026
  10. Jeff KingMar 27, 2026
  11. WesleyMar 28, 2026
  12. Jeff KingMar 28, 2026
  13. WesleyMar 28, 2026
  14. Johannes SixtMar 27, 2026
  15. WesleyMar 27, 2026
  16. Junio C HamanoMar 27, 2026
  17. WesleyMar 27, 2026
  18. brian m. carlsonMar 27, 2026
  19. WesleyMar 28, 2026
  20. Johannes SixtMar 28, 2026
  21. WesleyMar 28, 2026
  22. Ben KnobleMar 29, 2026
  23. brian m. carlsonMar 27, 2026
  24. Junio C HamanoMar 27, 2026

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.