Re: [PATCH GSoC v14 05/13] fetch-pack: prepare function to be moved
- From
Karthik Nayak <karthik.188@gmail.com>
- Date
- Jun 26, 2026, 16:54 UTC
- Message-ID
- <CAOLa=ZScS3Gmm5BAgJF69phpaDXGnP_j9jx+bMhn_tfF65RXEg@mail.gmail.com>
- In-Reply-To
- <20260625-ps-eric-work-rebase-v14-5-09f7ffe21a53@gmail.com>
Pablo Sabater <pabloosabaterr@gmail.com> 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()`Show 6 quoted lines
> `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...Show 7 quoted lines
> > 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.
Show 43 quoted lines
> 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 <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 | 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.
Show 6 quoted lines
> 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