Re: [PATCH v5 1/6] fetch-pack: refactor packet writing
- From
Christian Couder <christian.couder@gmail.com>
- Date
- Nov 5, 2024, 17:44 UTC
- Message-ID
- <CAP8UFD0Yk1Twv9DMOYwO4R+7P0YHj1CgK8xN=2YdE8sdi4ukow@mail.gmail.com>
- In-Reply-To
- <20241028203457.19715-2-eric.peijian@gmail.com>
On Mon, Oct 28, 2024 at 9:35 PM Eric Ju <eric.peijian@gmail.com> wrote:
Show 15 quoted lines
> 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.
Show 5 quoted lines
> +{
> + 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.
Show 47 quoted lines
> + 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.