Re: [PATCH v8 5/6] transport: add client support for object-info

>
> Thanks for picking up this series btw!
>
> On Mon, Dec 23, 2024 at 3:25 PM Eric Ju <eric.peijian@gmail.com> wrote:
> >
> > From: Calvin Wan <calvinwan@google.com>
> >
> > Sometimes, it is beneficial to retrieve information about an object
> > without downloading it entirely. The server-side logic for this
> > functionality was implemented in commit "a2ba162cda (object-info:
> > support for retrieving object info, 2021-04-20)."
> >
> > This commit introduces client functions to interact with the server.
> >
> > Currently, the client supports requesting a list of object IDs with
> > the ‘size’ feature from a v2 server. If the server does not advertise
> > this feature (i.e., transfer.advertiseobjectinfo is set to false),
> > the client will return an error and exit.
> >
> > Helped-by: Jonathan Tan <jonathantanmy@google.com>
> > Helped-by: Christian Couder <chriscool@tuxfamily.org>
> > Signed-off-by: Calvin Wan <calvinwan@google.com>
> > Signed-off-by: Eric Ju  <eric.peijian@gmail.com>
> > ---
> >  Makefile            |  1 +
> >  fetch-object-info.c | 92 +++++++++++++++++++++++++++++++++++++++++++++
> >  fetch-object-info.h | 18 +++++++++
> >  fetch-pack.c        |  3 ++
> >  fetch-pack.h        |  2 +
> >  transport-helper.c  | 11 +++++-
> >  transport.c         | 28 +++++++++++++-
> >  transport.h         | 11 ++++++
> >  8 files changed, 163 insertions(+), 3 deletions(-)
> >  create mode 100644 fetch-object-info.c
> >  create mode 100644 fetch-object-info.h
> >
> > diff --git a/Makefile b/Makefile
> > index 3fa4bf0d06..70e9ec0464 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -1020,6 +1020,7 @@ LIB_OBJS += ewah/ewah_rlw.o
> >  LIB_OBJS += exec-cmd.o
> >  LIB_OBJS += fetch-negotiator.o
> >  LIB_OBJS += fetch-pack.o
> > +LIB_OBJS += fetch-object-info.o
> >  LIB_OBJS += fmt-merge-msg.o
> >  LIB_OBJS += fsck.o
> >  LIB_OBJS += fsmonitor.o
> > diff --git a/fetch-object-info.c b/fetch-object-info.c
> > new file mode 100644
> > index 0000000000..2aa9f2b70d
> > --- /dev/null
> > +++ b/fetch-object-info.c
> > @@ -0,0 +1,92 @@
> > +#include "git-compat-util.h"
> > +#include "gettext.h"
> > +#include "hex.h"
> > +#include "pkt-line.h"
> > +#include "connect.h"
> > +#include "oid-array.h"
> > +#include "object-store-ll.h"
> > +#include "fetch-object-info.h"
> > +#include "string-list.h"
> > +
> > +/**
> > + * send_object_info_request sends git-cat-file object-info command and its
> > + * arguments into the request buffer.
> > + */
> > +static void send_object_info_request(const int fd_out, struct object_info_args *args)
> > +{
> > +       struct strbuf req_buf = STRBUF_INIT;
> > +
> > +       write_command_and_capabilities(&req_buf, "object-info", args->server_options);
> > +
> > +       if (unsorted_string_list_has_string(args->object_info_options, "size"))
> > +               packet_buf_write(&req_buf, "size");
> > +
> > +       if (args->oids) {
> > +               for (size_t i = 0; i < args->oids->nr; i++)
> > +                       packet_buf_write(&req_buf, "oid %s", oid_to_hex(&args->oids->oid[i]));
> > +       }
> > +
> > +       packet_buf_flush(&req_buf);
> > +       if (write_in_full(fd_out, req_buf.buf, req_buf.len) < 0)
> > +               die_errno(_("unable to write request to remote"));
> > +
> > +       strbuf_release(&req_buf);
> > +}
> > +
> > +/**
> > + * fetch_object_info sends git-cat-file object-info command into the request buf
> > + * and read the results from packets.
> > + */
> > +int fetch_object_info(const enum protocol_version version, struct object_info_args *args,
> > +                     struct packet_reader *reader, struct object_info *object_info_data,
> > +                     const int stateless_rpc, const int fd_out)
> > +{
> > +       int size_index = -1;
> > +
> > +       switch (version) {
> > +       case protocol_v2:
> > +               if (!server_supports_v2("object-info"))
> > +                       die(_("object-info capability is not enabled on the server"));
> > +               send_object_info_request(fd_out, args);
> > +               break;
> > +       case protocol_v1:
> > +       case protocol_v0:
> > +               die(_("wrong protocol version. expected v2"));
>
> s/wrong/unsupported
>

> > +       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(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));
> > +                       }
> > +                       continue;
> > +               }
> > +               return -1;
> > +       }
>
> I think we can flatten this logic a bit more here to make it more intuitive.
>
> if (!unsorted_string_list_has_string(args->object_info_options, reader->line))
>         return -1;
> 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));
> }