From: Karthik Nayak <karthik.188@gmail.com>
Date: Wed, 24 Jun 2026 13:25:09 GMT
Subject: Re: [PATCH GSoC RFC v13 06/12] connect: refactor packet writing
Message-ID: <CAOLa=ZQUCrAgnyoUCpd_G8PEdYunN7on=unR-TYcUe=yURyrtA@mail.gmail.com>
In-Reply-To: <CAN5EUNRN4_5PS3cbQtQfpyRuwByvV=qvAVKnVbgT-pirKGnnTg@mail.gmail.com>

Pablo Sabater <pabloosabaterr@gmail.com> writes:

> El lun, 22 jun 2026 a las 22:43, Karthik Nayak
> (<karthik.188@gmail.com>) escribió:
>>
>> Pablo Sabater <pabloosabaterr@gmail.com> writes:
>>
>> [snip]
>>
>> > 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.
>
> Hi!
> There should be no diff related to that line, In some point between
> Eric's last version (v11) and mine's firs (v12) the original code
> changed. On the diff from v11 [1] the object format is the same, i
> didn't notice this change and it's wrong, I'll fix it for v14, Thanks!
>
>>
>> > 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.
>
> True, I'll fix it for v14.
>
>>
>> While we're here, why not `#include "string-list.h"` and remove the
>> forward declaration, is there a circular dependency?
>
> I believe this was right because from what I know forward declarations
> are prefered in headers when in this case, the struct is only used as
> a pointer. Investigating, this came from a review from patrick [2].
>

That's fair.

Nit: Maybe add this context to the commit message?

Thanks

> [snip]
>
> [1]: https://lore.kernel.org/git/20250221190451.12536-5-eric.peijian@gmail.com/
> [2]: https://lore.kernel.org/git/Z0RIqUAoEob8lGfM@pks.im/
>
> Thanks for the review,
> Pablo.