Re: [PATCH GSoC v14 05/13] fetch-pack: prepare function to be moved
From: Karthik Nayak <hidden>
Date: 2026-06-26 16:54:57
Pablo Sabater [off-list ref] 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.
quoted hunk ↗ jump to hunk
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 [off-list ref] Helped-by: Christian Couder [off-list ref] Signed-off-by: Calvin Wan <redacted> Signed-off-by: Eric Ju <redacted> Signed-off-by: Pablo Sabater <redacted> --- 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.0Attachments
- signature.asc [application/pgp-signature] 690 bytes