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

[Peijian Ju <eric.peijian@gmail.com>]

On Tue, Sep 24, 2024 at 7:45 AM Christian Couder
<christian.couder@gmail.com> wrote:
>
> On Sat, Jul 20, 2024 at 5:43 AM Eric Ju <eric.peijian@gmail.com> wrote:
>
> [...]
>
> >  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.


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

> > +               die(_("remote-object-info requires protocol v2"));
> > +       }
>
> [...]
>
> > +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.

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

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

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

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



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