From: Karthik Nayak Date: Mon, 22 Jun 2026 20:43:01 GMT Subject: Re: [PATCH GSoC RFC v13 06/12] connect: refactor packet writing Message-ID: In-Reply-To: <20260619-ps-eric-work-rebase-v13-6-3d4c7315d2f8@gmail.com> Pablo Sabater writes: [snip] > diff --git a/connect.c b/connect.c > index 1dced8e632..78c69d4485 100644 > --- a/connect.c > +++ b/connect.c > @@ -700,16 +700,16 @@ int server_supports(const char *feature) > return !!server_feature_value(feature, NULL); > } > > -void write_fetch_command_and_capabilities(struct strbuf *req_buf, > - const struct string_list *server_options) > +void write_command_and_capabilities(struct strbuf *req_buf, const char *command, > + const struct string_list *server_options) > { > const char *hash_name; > int advertise_sid; > > repo_config_get_bool(the_repository, "transfer.advertisesid", &advertise_sid); > > - ensure_server_supports_v2("fetch"); > - packet_buf_write(req_buf, "command=fetch"); > + ensure_server_supports_v2(command); > + packet_buf_write(req_buf, "command=%s", command); > if (server_supports_v2("agent")) > packet_buf_write(req_buf, "agent=%s", git_user_agent_sanitized()); > if (advertise_sid && server_supports_v2("session-id")) > @@ -727,7 +727,7 @@ void write_fetch_command_and_capabilities(struct strbuf *req_buf, > die(_("mismatched algorithms: client %s; server %s"), > the_hash_algo->name, hash_name); > packet_buf_write(req_buf, "object-format=%s", the_hash_algo->name); > - } else if (hash_algo_by_ptr(the_hash_algo) != GIT_HASH_SHA1_LEGACY) { > + } else if (hash_algo_by_ptr(the_hash_algo) != GIT_HASH_SHA1) { > die(_("the server does not support algorithm '%s'"), > the_hash_algo->name); > } Why did we make this change? If the server doesn't support v2, then the object format should be `GIT_HASH_SHA1_LEGACY`. While the value of it is indeed `GIT_HASH_SHA1`, it indicates a scenario where there was no option to select object hash, which is the scenario here. If there is a reason to make such a change, perhaps we should highlight this in the commit message. > diff --git a/connect.h b/connect.h > index c4f6ea4b0a..8f4c523892 100644 > --- a/connect.h > +++ b/connect.h > @@ -34,8 +34,12 @@ void check_stateless_delimiter(int stateless_rpc, > struct packet_reader *reader, > const char *error); > > +/* > + * Writes a command along with the requested server capabilities/features into a > + * request buffer. > + */ > struct string_list; The comment should be above the function and not the forward declaration. While we're here, why not `#include "string-list.h"` and remove the forward declaration, is there a circular dependency? > -void write_fetch_command_and_capabilities(struct strbuf *req_buf, > - const struct string_list *server_options); > +void write_command_and_capabilities(struct strbuf *req_buf, const char *command, > + const struct string_list *server_options); > > #endif > diff --git a/fetch-pack.c b/fetch-pack.c > index 4a8a70b5f3..3d32114907 100644 > --- a/fetch-pack.c > +++ b/fetch-pack.c > @@ -1387,7 +1387,7 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out, > int done_sent = 0; > struct strbuf req_buf = STRBUF_INIT; > > - write_fetch_command_and_capabilities(&req_buf, args->server_options); > + write_command_and_capabilities(&req_buf, "fetch", args->server_options); > > if (args->use_thin_pack) > packet_buf_write(&req_buf, "thin-pack"); > @@ -2255,7 +2255,7 @@ void negotiate_using_fetch(const struct oid_array *negotiation_restrict_tips, > the_repository, "%d", > negotiation_round); > strbuf_reset(&req_buf); > - write_fetch_command_and_capabilities(&req_buf, server_options); > + write_command_and_capabilities(&req_buf, "fetch", server_options); > > packet_buf_write(&req_buf, "wait-for-done"); > > > -- > 2.54.0