From: Karthik Nayak Date: Fri, 26 Jun 2026 16:54:55 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> Pablo Sabater writes: The subject doesn't really give much insight into what the patch does. Perhaps something like: fetch-pack: use repo config in `write_fetch_command_and_capabilities()` fetch-pack: drop static variable use in `write_fetch_command_and_capabilities()` > `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 I don't think 'previously' makes sense here. > `fetch_pack.c`, `advertise_sid` is a static variable, modified using > `repo_config_get_bool()`. > Perhaps: To move `write_fetch_command_and_capabilities()` to `connect.c`, drop the usage of file static variable `advertise_sid` within the function. Currently, `advertise_sid` is modified... > > 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`: > This needs to be modified no? This is from the prev patch, where we moved and refactored in the same patch, this no longer is the case. > 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. > > 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); > Agreed with Chandra, this needs to be assessed. > 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