From: Pablo Sabater <pabloosabaterr@gmail.com>
Date: Sat, 27 Jun 2026 21:06:58 GMT
Subject: Re: [PATCH GSoC v14 05/13] fetch-pack: prepare function to be moved
Message-ID: <CAN5EUNR2Sv_JtSwQ66m1CwYxgDs4Zb_RxpkpnDTWevPVAOm4xA@mail.gmail.com>
In-Reply-To: <CAOLa=ZScS3Gmm5BAgJF69phpaDXGnP_j9jx+bMhn_tfF65RXEg@mail.gmail.com>

El vie, 26 jun 2026 a las 18:54, Karthik Nayak
(<karthik.188@gmail.com>) escribió:
>
> 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()`
>
> > `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 <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.
>
> >               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

I'll reword the commit message with all you and Chandra reviewed.

Thanks for the feedback,
Pablo.