From: Pablo Sabater <pabloosabaterr@gmail.com>
Date: Wed, 24 Jun 2026 12:21:11 GMT
Subject: Re: [PATCH GSoC RFC v13 05/12] fetch-pack: move function to connect.c
Message-ID: <CAN5EUNRMZd+NoiAHd-f0Gx4CqRPs7759a4UQh=GDeUgsFKbdJg@mail.gmail.com>
In-Reply-To: <CAOLa=ZRUoBKPAjh6He0qgdZdzAzMxmeS9RMRi-czpHEfKG6EKw@mail.gmail.com>

El lun, 22 jun 2026 a las 12:30, Karthik Nayak
(<karthik.188@gmail.com>) escribió:
>
> Pablo Sabater <pabloosabaterr@gmail.com> writes:
>
> > write_fetch_command_and_capabilities will be refactored in a subsequent
> > commit where it will become a more general-purpose function, making it
> > more accessible to additional commands in the future.
>
> Okay.
>
> > To move `write_fetch_command_and_capabilities()` to `connect.c`, we need
> > to adjust how `advertise_sid` is managed. Previously in `fetch_pack.c`,
> > `advertise_sid` was a static variable, modified using
> > `repo_config_get_bool()`.
>
> Nit: What's missing is why do we need to move it to 'connect.c', I
> assume this is because it being generic means its better placed in
> connect.c over 'fetch-pack.c'. Would be nice to explicitly mention that
> perhaps?

True, it is for that reason, I'll write it explicitly in the next
version, thanks!

>
> >
> > In `connect.c`, we now 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 (!sever_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.
> >
> > Move `write_fetch_command_and_capabilities()` to `connect.c`
> >
>
> Nit: Wouldn't it then make sense to split this into two?
> 1. Drop usage of the static `advertise_sid` within
> `write_fetch_command_and_capabilities()`.
> 2. Move `write_fetch_command_and_capabilities()` to `connect.c`
>
> That way the second patch is simply a move?

Okay, seems fair, I'll do that, thanks.

>
> [snip]

Thanks for the review,
Pablo