From: Chandra Pratap Date: Fri, 26 Jun 2026 12:14:49 GMT Subject: Re: [PATCH GSoC v14 05/13] fetch-pack: prepare function to be moved Message-ID: In-Reply-To: <20260625-ps-eric-work-rebase-v14-5-09f7ffe21a53@gmail.com> On Thu, 25 Jun 2026 at 17:43, Pablo Sabater wrote: > > `write_fetch_command_and_capabilities()` will be refactored and moved in > subsequent commits where it will become a more general-purpose function, > making it more accessible to additional commands in the future. > > To move `write_fetch_command_and_capabilities()` to `connect.c`, we > previously need to adjust how `advertise_sid` is managed. Currently in > `fetch_pack.c`, `advertise_sid` is a static variable, modified using > `repo_config_get_bool()`. > > 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 (!server_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. Nit: This only explains the `advertise_sid` change in this patch. We should also add a few lines explaining the `hash_algo` change. Maybe something like: While at it, change `hash_algo`'s type to `hash_algo_by_name()`'s actual return type (`unsigned int`) and make it `const`. > Helped-by: Jonathan Tan > Helped-by: Christian Couder > Signed-off-by: Calvin Wan > Signed-off-by: Eric Ju > Signed-off-by: Pablo Sabater > --- > fetch-pack.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/fetch-pack.c b/fetch-pack.c > index f13951d154..ad07603755 100644 > --- a/fetch-pack.c > +++ b/fetch-pack.c > @@ -1380,6 +1380,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; > + > + repo_config_get_bool(the_repository, "transfer.advertisesid", &advertise_sid); > > ensure_server_supports_v2("fetch"); > packet_buf_write(req_buf, "command=fetch"); > @@ -1395,7 +1398,7 @@ static void write_fetch_command_and_capabilities(struct strbuf *req_buf, > } > > if (server_feature_v2("object-format", &hash_name)) { > - int hash_algo = hash_algo_by_name(hash_name); > + const unsigned 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); > > -- > 2.54.0