Re: [PATCH GSoC v18 05/13] fetch-pack: drop static advertise_sid variable
- From
Karthik Nayak <karthik.188@gmail.com>
- Date
- Jul 15, 2026, 21:48 UTC
- Message-ID
- <CAOLa=ZSy1Z-R38cqFiz-Ejj9CNJkp4x_6rFk_wdfhyBytYH9fw@mail.gmail.com>
- In-Reply-To
- <20260715-ps-eric-work-rebase-v18-5-34d7adb051bb@gmail.com>
Pablo Sabater <pabloosabaterr@gmail.com> writes:
Show 11 quoted lines
> write_fetch_command_and_capabilities() is moved to 'connect.c' in a
> subsequent commit. To prepare for that, drop the static variable usage
> of advertise_sid. Currently advertise_sid is used in two places:
>
> 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);
>Nit: But #2 isn't a usecase, it's where it is set no? Looking at the usecase, it seems like we have two:
#1 like you stated
#2 within `write_fetch_command_and_capabilities()`. But the flow is
that, the variable is set in `fetch_pack_config()`, right?Show 57 quoted lines
> About 1, it is only relevant for v0/v1 protocol, move it into
> find_common().
>
> About 2, call repo_config_get_bool() inside of
> write_fetch_command_and_capabilities() and find_common() replacing the
> static variable.
>
> Because repo_config_get_bool() leaves advertise_sid as is if it is not
> set, initialize it to 0 matching its default.
>
> 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 | 13 +++++++------
> 1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/fetch-pack.c b/fetch-pack.c
> index eea72b2500..8e04db8640 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -49,7 +49,6 @@ static int fetch_fsck_objects = -1;
> static int transfer_fsck_objects = -1;
> static int agent_supported;
> static int server_supports_filtering;
> -static int advertise_sid;
> static struct shallow_lock shallow_lock;
> static const char *alternate_shallow_file;
> static struct strbuf fsck_msg_types = STRBUF_INIT;
> @@ -363,6 +362,9 @@ static int find_common(struct fetch_negotiator *negotiator,
> size_t state_len = 0;
> struct packet_reader reader;
> struct oidset negotiation_include_oids = OIDSET_INIT;
> + int advertise_sid = 0;
> +
> + repo_config_get_bool(the_repository, "transfer.advertisesid", &advertise_sid);
>
> if (args->stateless_rpc && multi_ack == 1)
> die(_("the option '%s' requires '%s'"), "--stateless-rpc", "multi_ack_detailed");
> @@ -414,7 +416,7 @@ static int find_common(struct fetch_negotiator *negotiator,
> if (deepen_not_ok) strbuf_addstr(&c, " deepen-not");
> if (agent_supported) strbuf_addf(&c, " agent=%s",
> git_user_agent_sanitized());
> - if (advertise_sid)
> + if (advertise_sid && server_supports("session-id"))
> strbuf_addf(&c, " session-id=%s", trace2_session_id());
> if (args->filter_options.choice)
> strbuf_addstr(&c, " filter");
> @@ -1160,9 +1162,6 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
> (int)agent_len, agent_feature);
> }
>
> - if (!server_supports("session-id"))
> - advertise_sid = 0;
> -So earlier we'd set the `advertise_sid` to 0 if the server didn't support 'session-id'. But we could directly check where it is needed, which is `find_common()`. Is `find_common()` only called in v0/v1 as stated in the commit message?
Show 24 quoted lines
> if (server_supports("shallow"))
> print_verbose(args, _("Server supports %s"), "shallow");
> else if (args->depth > 0 || is_repository_shallow(r))
> @@ -1380,6 +1379,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 = 0;
> +
> + repo_config_get_bool(the_repository, "transfer.advertisesid", &advertise_sid);
>
> ensure_server_supports_v2("fetch");
> packet_buf_write(req_buf, "command=fetch");
> @@ -1998,7 +2000,6 @@ static void fetch_pack_config(void)
> repo_config_get_bool(the_repository, "repack.usedeltabaseoffset", &prefer_ofs_delta);
> repo_config_get_bool(the_repository, "fetch.fsckobjects", &fetch_fsck_objects);
> repo_config_get_bool(the_repository, "transfer.fsckobjects", &transfer_fsck_objects);
> - repo_config_get_bool(the_repository, "transfer.advertisesid", &advertise_sid);
> if (!uri_protocols.nr) {
> char *str;
>
>
> --
> 2.54.0