From: Pablo Sabater <pabloosabaterr@gmail.com>
Date: Sat, 18 Jul 2026 19:00:34 GMT
Subject: Re: [PATCH GSoC v19 05/13] fetch-pack: move write_fetch_command_and_capabilities() to connect.c
Message-ID: <DK1XG2LQP0PS.33L27O783LXE4@gmail.com>
In-Reply-To: <alvHUX68McwlEau2@szeder.dev>

On Sat Jul 18, 2026 at 8:34 PM CEST, SZEDER Gábor wrote:
> 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 <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 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.

Thanks for noticing it.

Yes, then the prep patch needs to be done in fetch-pack.c before moving
it to connect.c.

I had it after because 2 hunks of that patch fixes calls for connect.c
so this way there were all for connect.c instead of mixing.

[snip]

Thanks,
Pablo