From: Pablo Sabater <pabloosabaterr@gmail.com>
Date: Thu, 16 Jul 2026 14:52:28 GMT
Subject: Re: [PATCH GSoC v18 05/13] fetch-pack: drop static advertise_sid variable
Message-ID: <DK02X0ULSX7S.24CCVGDZC6W9Y@gmail.com>
In-Reply-To: <CAOLa=ZSy1Z-R38cqFiz-Ejj9CNJkp4x_6rFk_wdfhyBytYH9fw@mail.gmail.com>

On Wed Jul 15, 2026 at 11:48 PM CEST, Karthik Nayak wrote:
> Pablo Sabater <pabloosabaterr@gmail.com> writes:
>
>> write_fetch_command_and_capabilities() is moved to 'connect.c' in a
>> subsequent commit. To prepare for that, drop the static variable usage
>> of advertise_sid. Currently advertise_sid is used in two places:
>>
>> 1. In function do_fetch_pack():
>>         if (!server_supports("session-id"))
>>                advertise_sid = 0;
>>
>> 2. In function fetch_pack_config():
>>         repo_config_get_bool("transfer.advertisesid", &advertise_sid);
>>
>
> Nit: But #2 isn't a usecase, it's where it is set no? Looking at the
> usecase, it seems like we have two:
>
>   #1 like you stated
>   #2 within `write_fetch_command_and_capabilities()`. But the flow is
>     that, the variable is set in `fetch_pack_config()`, right?

Completly true, fetch_pack_config() is where it's set, but not where
it's used.

I'll fix #2 to be where it is used.

Thanks.

>
>> About 1, it is only relevant for v0/v1 protocol, move it into
>> find_common().
>>
>> About 2, call repo_config_get_bool() inside of
>> write_fetch_command_and_capabilities() and find_common() replacing the
>> static variable.
>>
>> Because repo_config_get_bool() leaves advertise_sid as is if it is not
>> set, initialize it to 0 matching its default.
>>
>> 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>
>> ---
>>  fetch-pack.c | 13 +++++++------
>>  1 file changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/fetch-pack.c b/fetch-pack.c
>> index eea72b2500..8e04db8640 100644
>> --- a/fetch-pack.c
>> +++ b/fetch-pack.c
>> @@ -49,7 +49,6 @@ static int fetch_fsck_objects = -1;
>>  static int transfer_fsck_objects = -1;
>>  static int agent_supported;
>>  static int server_supports_filtering;
>> -static int advertise_sid;
>>  static struct shallow_lock shallow_lock;
>>  static const char *alternate_shallow_file;
>>  static struct strbuf fsck_msg_types = STRBUF_INIT;
>> @@ -363,6 +362,9 @@ static int find_common(struct fetch_negotiator *negotiator,
>>  	size_t state_len = 0;
>>  	struct packet_reader reader;
>>  	struct oidset negotiation_include_oids = OIDSET_INIT;
>> +	int advertise_sid = 0;
>> +
>> +	repo_config_get_bool(the_repository, "transfer.advertisesid", &advertise_sid);
>>
>>  	if (args->stateless_rpc && multi_ack == 1)
>>  		die(_("the option '%s' requires '%s'"), "--stateless-rpc", "multi_ack_detailed");
>> @@ -414,7 +416,7 @@ static int find_common(struct fetch_negotiator *negotiator,
>>  			if (deepen_not_ok)      strbuf_addstr(&c, " deepen-not");
>>  			if (agent_supported)    strbuf_addf(&c, " agent=%s",
>>  							    git_user_agent_sanitized());
>> -			if (advertise_sid)
>> +			if (advertise_sid && server_supports("session-id"))
>>  				strbuf_addf(&c, " session-id=%s", trace2_session_id());
>>  			if (args->filter_options.choice)
>>  				strbuf_addstr(&c, " filter");
>> @@ -1160,9 +1162,6 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
>>  				      (int)agent_len, agent_feature);
>>  	}
>>
>> -	if (!server_supports("session-id"))
>> -		advertise_sid = 0;
>> -
>
> So earlier we'd set the `advertise_sid` to 0 if the server didn't support
> 'session-id'. But we could directly check where it is needed, which is
> `find_common()`. Is `find_common()` only called in v0/v1 as stated in
> the commit message?

Yes, find_common() is only called from do_fetch_pack() which is the v0/v1
path, if we take a look at fetch_pack():

	if (version == protocol_v2) {
		[snip]
		ref_cpy = do_fetch_pack_v2(args, fd, ref, sought, nr_sought,
					   &shallows_scratch, &si,
					   pack_lockfiles);
	} else {
		[snip]
		ref_cpy = do_fetch_pack(args, fd, ref, sought, nr_sought,
					&si, pack_lockfiles);
	}

>
>>  	if (server_supports("shallow"))
>>  		print_verbose(args, _("Server supports %s"), "shallow");
>>  	else if (args->depth > 0 || is_repository_shallow(r))
>> @@ -1380,6 +1379,9 @@ 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");
>> @@ -1998,7 +2000,6 @@ static void fetch_pack_config(void)
>>  	repo_config_get_bool(the_repository, "repack.usedeltabaseoffset", &prefer_ofs_delta);
>>  	repo_config_get_bool(the_repository, "fetch.fsckobjects", &fetch_fsck_objects);
>>  	repo_config_get_bool(the_repository, "transfer.fsckobjects", &transfer_fsck_objects);
>> -	repo_config_get_bool(the_repository, "transfer.advertisesid", &advertise_sid);
>>  	if (!uri_protocols.nr) {
>>  		char *str;
>>
>>
>> --
>> 2.54.0

Thanks for the feedback, will make it clear next reroll,
Pablo