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.
> +{
> + 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.
