Re: [PATCH v5 1/6] fetch-pack: refactor packet writing

>
> On Mon, Oct 28, 2024 at 9:35 PM Eric Ju <eric.peijian@gmail.com> wrote:
>
> >  connect.c    | 34 ++++++++++++++++++++++++++++++++++
> >  connect.h    |  4 ++++
> >  fetch-pack.c | 36 ++----------------------------------
> >  3 files changed, 40 insertions(+), 34 deletions(-)
> >
> > diff --git a/connect.c b/connect.c
> > index 58f53d8dcb..bb4e4eec44 100644
> > --- a/connect.c
> > +++ b/connect.c
> > @@ -688,6 +688,40 @@ int server_supports(const char *feature)
> >         return !!server_feature_value(feature, NULL);
> >  }
> >
> > +void write_command_and_capabilities(struct strbuf *req_buf, const char *command,
> > +                                                                       const struct string_list *server_options)
>
> When I apply your patches this line doesn't seem well indented.
>

> > +{
> > +       const char *hash_name;
> > +       int advertise_sid;
> > +
> > +       git_config_get_bool("transfer.advertisesid", &advertise_sid);
>
> It looks like moving the function to connect.c required adding the
> above line into it. There are a few other small changes, including
> probably spurious indentation changes, in the moved function which
> make it a bit more difficult than necessary to check that the moved
> code is the same as the original one.
>
> This makes me wonder if it was actually a good idea to move the
> function, or if moving the function should have been done in a
> separate step than the step making the small changes. Perhaps patch
> 5/6 "cat-file: add declaration of variable i inside its for loop"
> could have been moved before this patch and could have included some
> of the small changes related to the i variable that are made in this
> patch.
>
> It might have been nice to mention the changes in the commit message anyway.
>

> > +       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"))
> > +               packet_buf_write(req_buf, "session-id=%s", trace2_session_id());
> > +       if (server_options && server_options->nr) {
> > +               ensure_server_supports_v2("server-option");
> > +               for (int i = 0; i < server_options->nr; i++)
> > +                       packet_buf_write(req_buf, "server-option=%s",
> > +                                        server_options->items[i].string);
> > +       }
> > +
> > +       if (server_feature_v2("object-format", &hash_name)) {
> > +               const int hash_algo = hash_algo_by_name(hash_name);
> > +               if (hash_algo_by_ptr(the_hash_algo) != hash_algo)
> > +                       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) {
> > +               die(_("the server does not support algorithm '%s'"),
> > +                       the_hash_algo->name);
> > +       }
> > +       packet_buf_delim(req_buf);
> > +}
> > +
> >  enum protocol {
> >         PROTO_LOCAL = 1,
> >         PROTO_FILE,
> > diff --git a/connect.h b/connect.h
> > index 1645126c17..2ed009066e 100644
> > --- a/connect.h
> > +++ b/connect.h
> > @@ -1,6 +1,7 @@
> >  #ifndef CONNECT_H
> >  #define CONNECT_H
> >
> > +#include "string-list.h"
> >  #include "protocol.h"
> >
> >  #define CONNECT_VERBOSE       (1u << 0)
> > @@ -30,4 +31,7 @@ void check_stateless_delimiter(int stateless_rpc,
> >                                struct packet_reader *reader,
> >                                const char *error);
> >
> > +void write_command_and_capabilities(struct strbuf *req_buf, const char *command,
> > +                                                                       const struct string_list *server_options);
>
> When I apply your patches the above line doesn't seem well indented either.
>
> You might want to make sure that your editor uses 8 spaces for each
> tab, see Documentation/CodingGuidelines, or just that your editor
> properly follows our .editorconfig file.
>
> It looks like other patches in the series, like patch 4/6, have
> similar issues. Otherwise the other patches in the series look good to
> me.
>
> Thanks.