El lun, 22 jun 2026 a las 22:43, Karthik Nayak
([off-list ref]) escribió:

Pablo Sabater [off-list ref] writes:

[snip]

quoted

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!

quoted

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].

[snip]

[1]: https://lore.kernel.org/git/20250221190451.12536-5-eric.peijian@gmail.com/ (local)
[2]: https://lore.kernel.org/git/Z0RIqUAoEob8lGfM@pks.im/ (local)

Thanks for the review,
Pablo.