Re: [PATCH GSoC v19 05/13] fetch-pack: move write_fetch_command_and_capabilities() to connect.c
- From
SZEDER Gábor <szeder.dev@gmail.com>
- Date
- Jul 18, 2026, 18:34 UTC
- Message-ID
- <alvHUX68McwlEau2@szeder.dev>
- 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:
Show 55 quoted lines
> 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 <jonathantanmy@google.com>
> Helped-by: Christian Couder <chriscool@tuxfamily.org>
> Signed-off-by: Calvin Wan <calvinwan@google.com>
> Signed-off-by: Eric Ju <eric.peijian@gmail.com>
> Signed-off-by: Pablo Sabater <pabloosabaterr@gmail.com>
> ---
> 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 1This 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.
Show 74 quoted lines
> + 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