On Tue, Nov 5, 2024 at 12:44 PM Christian Couder
<christian.couder@xxxxxxxxx> wrote:
>
> On Mon, Oct 28, 2024 at 9:35 PM Eric Ju <eric.peijian@xxxxxxxxx> 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.
>
Thank you. I will make sure my IDE respects
the .editorconfig and revise them in v6.
> > +{
> > + 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.
>
Thank you. In v6, I will move small changes commit "cat-file: add
declaration of variable i inside its for loop" to the very first
commit, where I will include small changes related to the i variable
that are made in this patch.
About the extra charges related to `advertise_sid`. I did a bit of
analysis, please feel free to correct me.
In the original fetch-pack.c code, there are only two places that
write `advertise_sid` :
1. line 1221 (function do_fetch_pack):
if (!server_supports("session-id"))
advertise_sid = 0;
2. line 1895 (function fetch_pack_config)
git_config_get_bool("transfer.advertisesid", &advertise_sid);
In 1, do_fetch_pack() is called when protocol is NOT v2. While
write_fetch_command_and_capabilities() or the new
write_command_and_capabilities() is only used in protocol v2, I think
it is safe so it is safe to ignore 1, and only consider 2.
In 2, git_config_get_bool is from config.h and it is an out-of-box
dependency of connect.c, so I just directly use it.
> > + 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.
Thank you. I will make sure my IDE respects
the .editorconfig and revise them in v6.
