From: SZEDER Gábor Date: Sat, 18 Jul 2026 18:34:57 GMT Subject: Re: [PATCH GSoC v19 05/13] fetch-pack: move write_fetch_command_and_capabilities() to connect.c Message-ID: In-Reply-To: <20260717-ps-eric-work-rebase-v19-5-d4faee35764b@gmail.com> On Fri, Jul 17, 2026 at 05:05:15PM +0200, Pablo Sabater wrote: > In a subsequent commit write_fetch_command_and_capabilities() will be > refactored to a more general-purpose function, making it more accessible > to additional commands in the future. > > Move write_fetch_command_and_capabilities() to 'connect.c', where > there are similar purpose functions. > > Because string_list is only used as a pointer, use a forward > declaration [1]. > > [1]: https://lore.kernel.org/git/Z0RIqUAoEob8lGfM@pks.im/ > > Helped-by: Jonathan Tan > Helped-by: Christian Couder > Signed-off-by: Calvin Wan > Signed-off-by: Eric Ju > Signed-off-by: Pablo Sabater > --- > connect.c | 34 ++++++++++++++++++++++++++++++++++ > connect.h | 4 ++++ > fetch-pack.c | 34 ---------------------------------- > 3 files changed, 38 insertions(+), 34 deletions(-) > > diff --git a/connect.c b/connect.c > index 47e39d2a73..31e5ab958b 100644 > --- a/connect.c > +++ b/connect.c > @@ -700,6 +700,40 @@ 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) > +{ > + const char *hash_name; > + int advertise_sid = 0; > + > + repo_config_get_bool(the_repository, "transfer.advertisesid", &advertise_sid); > + > + ensure_server_supports_v2("fetch"); > + packet_buf_write(req_buf, "command=fetch"); > + if (server_supports_v2("agent")) > + packet_buf_write(req_buf, "agent=%s", git_user_agent_sanitized()); > + if (advertise_sid && server_supports_v2("session-id")) > + packet_buf_write(req_buf, "session-id=%s", trace2_session_id()); > + if (server_options && server_options->nr) { > + ensure_server_supports_v2("server-option"); > + for (size_t i = 0; i < server_options->nr; i++) > + packet_buf_write(req_buf, "server-option=%s", > + server_options->items[i].string); > + } > + > + if (server_feature_v2("object-format", &hash_name)) { > + int hash_algo = hash_algo_by_name(hash_name); > + if (hash_algo_by_ptr(the_hash_algo) != hash_algo) hash_algo_by_ptr() returns an unsigned integer, and comparing that to a signed integer results in: connect.c: In function ‘write_fetch_command_and_capabilities’: connect.c:726:53: error: comparison of integer expressions of different signedness: ‘uint32_t’ {aka ‘unsigned int’} and ‘int’ [-Werror=sign-compare] 726 | if (hash_algo_by_ptr(the_hash_algo) != hash_algo) | ^~ cc1: all warnings being treated as errors make: *** [Makefile:2924: connect.o] Error 1 This was not an issue while the function lived in "fetch-pack.c", because that file uses #define DISABLE_SIGN_COMPARE_WARNINGS, but "connect.c" doesn't. I think as a preparatory step the type of the 'hash_algo' variable should be changed to unsigned int while the function is still in "fetch-pack.c" to avoid this; you'll convert it in the next patch anyway. > + 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) { > + die(_("the server does not support algorithm '%s'"), > + the_hash_algo->name); > + } > + packet_buf_delim(req_buf); > +} > + > static const char *url_scheme_name(enum url_scheme scheme) > { > switch (scheme) { > diff --git a/connect.h b/connect.h > index aa482a37fb..c4f6ea4b0a 100644 > --- a/connect.h > +++ b/connect.h > @@ -34,4 +34,8 @@ void check_stateless_delimiter(int stateless_rpc, > struct packet_reader *reader, > const char *error); > > +struct string_list; > +void write_fetch_command_and_capabilities(struct strbuf *req_buf, > + const struct string_list *server_options); > + > #endif > diff --git a/fetch-pack.c b/fetch-pack.c > index 65ebfec09f..f7789e8456 100644 > --- a/fetch-pack.c > +++ b/fetch-pack.c > @@ -1375,40 +1375,6 @@ static int add_haves(struct fetch_negotiator *negotiator, > return haves_added; > } > > -static void write_fetch_command_and_capabilities(struct strbuf *req_buf, > - const struct string_list *server_options) > -{ > - const char *hash_name; > - int advertise_sid = 0; > - > - repo_config_get_bool(the_repository, "transfer.advertisesid", &advertise_sid); > - > - ensure_server_supports_v2("fetch"); > - packet_buf_write(req_buf, "command=fetch"); > - if (server_supports_v2("agent")) > - packet_buf_write(req_buf, "agent=%s", git_user_agent_sanitized()); > - if (advertise_sid && server_supports_v2("session-id")) > - packet_buf_write(req_buf, "session-id=%s", trace2_session_id()); > - if (server_options && server_options->nr) { > - ensure_server_supports_v2("server-option"); > - for (size_t i = 0; i < server_options->nr; i++) > - packet_buf_write(req_buf, "server-option=%s", > - server_options->items[i].string); > - } > - > - if (server_feature_v2("object-format", &hash_name)) { > - int hash_algo = hash_algo_by_name(hash_name); > - if (hash_algo_by_ptr(the_hash_algo) != hash_algo) > - 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) { > - die(_("the server does not support algorithm '%s'"), > - the_hash_algo->name); > - } > - packet_buf_delim(req_buf); > -} > - > static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out, > struct fetch_pack_args *args, > const struct ref *wants, struct oidset *common, > > -- > 2.54.0