From: Karthik Nayak Date: Mon, 22 Jun 2026 10:30:13 GMT Subject: Re: [PATCH GSoC RFC v13 05/12] fetch-pack: move function to connect.c Message-ID: In-Reply-To: <20260619-ps-eric-work-rebase-v13-5-3d4c7315d2f8@gmail.com> Pablo Sabater writes: > write_fetch_command_and_capabilities will be refactored in a subsequent > commit where it will become a more general-purpose function, making it > more accessible to additional commands in the future. Okay. > To move `write_fetch_command_and_capabilities()` to `connect.c`, we need > to adjust how `advertise_sid` is managed. Previously in `fetch_pack.c`, > `advertise_sid` was a static variable, modified using > `repo_config_get_bool()`. Nit: What's missing is why do we need to move it to 'connect.c', I assume this is because it being generic means its better placed in connect.c over 'fetch-pack.c'. Would be nice to explicitly mention that perhaps? > > In `connect.c`, we now initialize `advertise_sid` at the begining by > directly using `repo_config_get_bool()`. This change is safe because: > > In the original `fetch-pack.c` code, there are only two places that write > `advertise_sid`: > > 1. In function `do_fetch_pack()`: > if (!sever_supports("session_id")) > advertise_sid = 0; > 2. In function `fetch_pack_config()`: > repo_config_get_bool("transfer.advertisesid", &advertise_sid); > > About 1, since `do_fetch_pack()` is only relevant for protocol v1, this > assignment can be ignored, as `write_fetch_command_and_capabilities()` > is only used in v2. > > About 2, `repo_config_get_bool()` is from `config.h` and it's an out-of-box > dependency of `connect.c`, so we can reuse it directly. > > Move `write_fetch_command_and_capabilities()` to `connect.c` > Nit: Wouldn't it then make sense to split this into two? 1. Drop usage of the static `advertise_sid` within `write_fetch_command_and_capabilities()`. 2. Move `write_fetch_command_and_capabilities()` to `connect.c` That way the second patch is simply a move? [snip]