Re: [PATCH v2 4/6] transport: add client support for object-info

On Tue, Sep 24, 2024 at 7:45 AM Christian Couder
[off-list ref] wrote:

On Sat, Jul 20, 2024 at 5:43 AM Eric Ju [off-list ref] wrote:

[...]

quoted

 fetch-pack.c       |  24 +++++++++
 fetch-pack.h       |  10 ++++
 transport-helper.c |   8 ++-
 transport.c        | 118 +++++++++++++++++++++++++++++++++++++++++++--
 transport.h        |  11 +++++
 5 files changed, 164 insertions(+), 7 deletions(-)

Karthik suggested adding tests at this stage, but I see no tests here.
Maybe the tests are added later, but I agree with Karthik that it
would be nice to add them early if possible.

Thank you. I’m not sure if there’s an easy way to directly add unit
tests for the changes in fetch-pack.c, transport-helper.c, and
transport.c, as the relevant functions are deeply nested within the
client’s call stack. Therefore, I’m attempting to test them indirectly
through git cat-file in t/t1017-cat-file-remote-object-info.sh in the
next commit.

Specifically, in t/t1017-cat-file-remote-object-info.sh:

- The code in transport-helper.c is tested in the cases
“remote-object-info fails on server with legacy protocol” and
“remote-object-info fails on server with legacy protocol fallback”.

- The code in transport.c and fetch-pack.c is tested in the cases
where transfer.advertiseobjectinfo is set to true, such as in
“batch-command remote-object-info http://”, “batch-command
remote-object-info file://”, and “batch-command remote-object-info
git://”. In these tests, we verify that remote-object-info
successfully retrieves the size from the remote without downloading
the objects locally.

quoted

diff --git a/transport-helper.c b/transport-helper.c
index 09b3560ffd..841a32e80a 100644
--- a/transport-helper.c
+++ b/transport-helper.c

@@ -699,13 +699,17 @@ static int fetch_refs(struct transport *transport,

        /*
         * If we reach here, then the server, the client, and/or the transport
-        * helper does not support protocol v2. --negotiate-only requires
-        * protocol v2.
+        * helper does not support protocol v2. --negotiate-only and cat-file remote-object-info
+        * require protocol v2.
         */
        if (data->transport_options.acked_commits) {
                warning(_("--negotiate-only requires protocol v2"));
                return -1;
        }
+       if (transport->smart_options->object_info) {
+               // fail the command explicitly to avoid further commands input

We use "/* stuff */" for one line comments instead of "// stuff". Also
the comment could go before the if (...) above and the "{" and "}"
could be dropped.

Thank you. Revised in V3.

quoted

+               die(_("remote-object-info requires protocol v2"));
+       }

[...]

quoted

+static int fetch_object_info(struct transport *transport, struct object_info *object_info_data)
+{
+       int size_index = -1;
+       struct git_transport_data *data = transport->data;
+       struct object_info_args args = { 0 };
+       struct packet_reader reader;
+
+       args.server_options = transport->server_options;
+       args.object_info_options = transport->smart_options->object_info_options;
+       args.oids = transport->smart_options->object_info_oids;
+
+       connect_setup(transport, 0);
+       packet_reader_init(&reader, data->fd[0], NULL, 0,
+                       PACKET_READ_CHOMP_NEWLINE |
+                       PACKET_READ_GENTLE_ON_EOF |
+                       PACKET_READ_DIE_ON_ERR_PACKET);
+       data->version = discover_version(&reader);
+
+       transport->hash_algo = reader.hash_algo;
+
+       switch (data->version) {
+       case protocol_v2:
+               if (!server_supports_v2("object-info"))
+                       return -1;
+               if (unsorted_string_list_has_string(args.object_info_options, "size")
+                               && !server_supports_feature("object-info", "size", 0)) {
+                       return -1;
+               }

The "{" and "}" can be dropped here too.

Thank you. Fixed in V3.

quoted

+               send_object_info_request(data->fd[1], &args);
+               break;
+       case protocol_v1:
+       case protocol_v0:
+               die(_("wrong protocol version. expected v2"));
+       case protocol_unknown_version:
+               BUG("unknown protocol version");
+       }
+
+       for (size_t i = 0; i < args.object_info_options->nr; i++) {
+               if (packet_reader_read(&reader) != PACKET_READ_NORMAL) {
+                       check_stateless_delimiter(transport->stateless_rpc, &reader, "stateless delimiter expected");
+                       return -1;
+               }
+               if (unsorted_string_list_has_string(args.object_info_options, reader.line)) {
+                       if (!strcmp(reader.line, "size")) {
+                               size_index = i;
+                               for (size_t j = 0; j < args.oids->nr; j++) {
+                                       object_info_data[j].sizep = xcalloc(1, sizeof(long));
+                               }

The "{" and "}" can be dropped here too.

Thank you. Fixed in V3.

quoted

+                       }
+                       continue;
+               }
+               return -1;
+       }
+
+       for (size_t i = 0; packet_reader_read(&reader) == PACKET_READ_NORMAL && i < args.oids->nr; i++){
+               struct string_list object_info_values = STRING_LIST_INIT_DUP;
+
+               string_list_split(&object_info_values, reader.line, ' ', -1);
+               if (0 <= size_index) {
+                       if (!strcmp(object_info_values.items[1 + size_index].string, ""))
+                               die("object-info: not our ref %s",
+                                       object_info_values.items[0].string);
+
+                       *object_info_data[i].sizep = strtoul(object_info_values.items[1 + size_index].string, NULL, 10);
+

This blank line can be removed.

Thank you. Fixed in V3.

quoted

+               }
+
+               string_list_clear(&object_info_values, 0);
+       }
+       check_stateless_delimiter(transport->stateless_rpc, &reader, "stateless delimiter expected");
+
+       return 0;
+}
+
 static struct ref *get_refs_via_connect(struct transport *transport, int for_push,
                                        struct transport_ls_refs_options *options)
 {

@@ -413,6 +487,7 @@ static int fetch_refs_via_pack(struct transport *transport,
        struct ref *refs = NULL;
        struct fetch_pack_args args;
        struct ref *refs_tmp = NULL;
+       struct ref *object_info_refs = NULL;

        memset(&args, 0, sizeof(args));
        args.uploadpack = data->options.uploadpack;

@@ -439,11 +514,36 @@ static int fetch_refs_via_pack(struct transport *transport,
        args.server_options = transport->server_options;
        args.negotiation_tips = data->options.negotiation_tips;
        args.reject_shallow_remote = transport->smart_options->reject_shallow;
+       args.object_info = transport->smart_options->object_info;
+
+       if (transport->smart_options
+               && transport->smart_options->object_info
+               && transport->smart_options->object_info_oids->nr > 0) {
+               struct ref *ref_itr = object_info_refs = alloc_ref("");
+
+               if (!fetch_object_info(transport, data->options.object_info_data))
+                       goto cleanup;
+
+               args.object_info_data = data->options.object_info_data;
+               args.quiet = 1;
+               args.no_progress = 1;
+               for (size_t i = 0; i < transport->smart_options->object_info_oids->nr; i++) {
+                       ref_itr->old_oid = transport->smart_options->object_info_oids->oid[i];
+                       ref_itr->exact_oid = 1;
+                       if (i == transport->smart_options->object_info_oids->nr - 1)
+                               /* last element, no need to allocat to next */

s/allocat/allocate/

Thank you. Fixed in V3.

quoted

+                               ref_itr -> next = NULL;
+                       else
+                               ref_itr->next = alloc_ref("");

-       if (!data->finished_handshake) {
-               int i;
+                       ref_itr = ref_itr->next;
+               }
+
+               transport->remote_refs = object_info_refs;
+
+       } else if (!data->finished_handshake) {
                int must_list_refs = 0;
-               for (i = 0; i < nr_heads; i++) {
+               for (int i = 0; i < nr_heads; i++) {
                        if (!to_fetch[i]->exact_oid) {
                                must_list_refs = 1;
                                break;

@@ -481,23 +581,31 @@ static int fetch_refs_via_pack(struct transport *transport,
                          &transport->pack_lockfiles, data->version);

        data->finished_handshake = 0;
+       if (args.object_info) {
+               struct ref *ref_cpy_reader = object_info_refs;
+               for (int i = 0; ref_cpy_reader; i++) {
+                       oid_object_info_extended(the_repository, &ref_cpy_reader->old_oid, &args.object_info_data[i], OBJECT_INFO_LOOKUP_REPLACE);

This line might want to be folded.

Thank you. Fixed in V3.

quoted

+                       ref_cpy_reader = ref_cpy_reader->next;
+               }
+       }
+