Re: [PATCH GSoC RFC v13 06/12] connect: refactor packet writing
- From
Karthik Nayak <karthik.188@gmail.com>
- Date
- Jun 22, 2026, 20:43 UTC
- Message-ID
- <CAOLa=ZSvxXuf_bSzKMvViNQ5MuDAqxnQdo4asF9vfMhJaDQcVw@mail.gmail.com>
- In-Reply-To
- <20260619-ps-eric-work-rebase-v13-6-3d4c7315d2f8@gmail.com>
Pablo Sabater <pabloosabaterr@gmail.com> writes:
[snip]
Show 34 quoted lines
> diff --git a/connect.c b/connect.c
> index 1dced8e632..78c69d4485 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -700,16 +700,16 @@ int server_supports(const char *feature)
> return !!server_feature_value(feature, NULL);
> }
>
> -void write_fetch_command_and_capabilities(struct strbuf *req_buf,
> - const struct string_list *server_options)
> +void write_command_and_capabilities(struct strbuf *req_buf, const char *command,
> + 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");
> + ensure_server_supports_v2(command);
> + packet_buf_write(req_buf, "command=%s", command);
> if (server_supports_v2("agent"))
> packet_buf_write(req_buf, "agent=%s", git_user_agent_sanitized());
> if (advertise_sid && server_supports_v2("session-id"))
> @@ -727,7 +727,7 @@ void write_fetch_command_and_capabilities(struct strbuf *req_buf,
> die(_("mismatched algorithms: client %s; server %s"),
> the_hash_algo->name, hash_name);
> packet_buf_write(req_buf, "object-format=%s", the_hash_algo->name);
> - } else if (hash_algo_by_ptr(the_hash_algo) != GIT_HASH_SHA1_LEGACY) {
> + } else if (hash_algo_by_ptr(the_hash_algo) != GIT_HASH_SHA1) {
> die(_("the server does not support algorithm '%s'"),
> the_hash_algo->name);
> }Why did we make this change? If the server doesn't support v2, then the object format should be `GIT_HASH_SHA1_LEGACY`. While the value of it is indeed `GIT_HASH_SHA1`, it indicates a scenario where there was no option to select object hash, which is the scenario here.
If there is a reason to make such a change, perhaps we should highlight this in the commit message.
Show 13 quoted lines
> diff --git a/connect.h b/connect.h > index c4f6ea4b0a..8f4c523892 100644 > --- a/connect.h > +++ b/connect.h > @@ -34,8 +34,12 @@ void check_stateless_delimiter(int stateless_rpc, > struct packet_reader *reader, > const char *error); > > +/* > + * Writes a command along with the requested server capabilities/features into a > + * request buffer. > + */ > struct string_list;
The comment should be above the function and not the forward declaration.
While we're here, why not `#include "string-list.h"` and remove the forward declaration, is there a circular dependency?
Show 31 quoted lines
> -void write_fetch_command_and_capabilities(struct strbuf *req_buf, > - const struct string_list *server_options); > +void write_command_and_capabilities(struct strbuf *req_buf, const char *command, > + const struct string_list *server_options); > > #endif > diff --git a/fetch-pack.c b/fetch-pack.c > index 4a8a70b5f3..3d32114907 100644 > --- a/fetch-pack.c > +++ b/fetch-pack.c > @@ -1387,7 +1387,7 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out, > int done_sent = 0; > struct strbuf req_buf = STRBUF_INIT; > > - write_fetch_command_and_capabilities(&req_buf, args->server_options); > + write_command_and_capabilities(&req_buf, "fetch", args->server_options); > > if (args->use_thin_pack) > packet_buf_write(&req_buf, "thin-pack"); > @@ -2255,7 +2255,7 @@ void negotiate_using_fetch(const struct oid_array *negotiation_restrict_tips, > the_repository, "%d", > negotiation_round); > strbuf_reset(&req_buf); > - write_fetch_command_and_capabilities(&req_buf, server_options); > + write_command_and_capabilities(&req_buf, "fetch", server_options); > > packet_buf_write(&req_buf, "wait-for-done"); > > > -- > 2.54.0