Re: [PATCH GSoC RFC v13 05/12] fetch-pack: move function to connect.c
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.
> 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?
Show 22 quoted lines
>
> 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?
[snip]
Karthik Nayak <karthik.188@gmail.com>