Re: [PATCH v3 3/3] object-info: add option for retrieving object info

[Calvin Wan <calvinwan@google.com>]

> but the oid arguments below have a newline, so we
> should be consistent one way or the other.

To match what the other packet functions do, I will get rid of the new
line for the oid loop.

> Rather than die() if we're on protocol V1
> or V0, can we also return a failure here and let callers fallback to
> fetch?

Sounds reasonable. Will look into it.

On Wed, Mar 30, 2022 at 3:12 PM Josh Steadmon <steadmon@google.com> wrote:
>
> On 2022.03.28 19:11, Calvin Wan wrote:
> > Sometimes it is useful to get information about an object without
> > having to download it completely. The server logic has already been
> > implemented as “a2ba162cda (object-info: support for retrieving object
> > info, 2021-04-20)”. This patch implements the client option for
> > it. Currently, only 'size' is supported, however future patches can
> > implement additional options.
> >
> > Add ‘--object-info’ option to fetch. This option allows the client to
> > make an object-info command request to a server that supports protocol
> > v2. If the server is v2, but does not allow for the object-info
> > command request, fetch the objects as if it were a standard fetch
> > (however without changing any refs). Therefore, hook `fetch
> > object-info` into transport_fetch_refs() to easily fallback if needed.
> >
> > A previous patch added the `transfer.advertiseObjectInfo` config
> > option, of which this patch utilizes to test the fallback.
> >
> > ---
> >  Documentation/fetch-options.txt |   5 ++
> >  Documentation/git-fetch.txt     |   1 +
> >  builtin/fetch.c                 |  36 ++++++++-
> >  fetch-pack.c                    |  42 +++++++++-
> >  fetch-pack.h                    |   9 +++
> >  t/t5583-fetch-object-info.sh    | 138 ++++++++++++++++++++++++++++++++
> >  transport-helper.c              |   8 +-
> >  transport-internal.h            |   1 +
> >  transport.c                     |  75 ++++++++++++++++-
> >  transport.h                     |   9 +++
> >  10 files changed, 315 insertions(+), 9 deletions(-)
> >  create mode 100755 t/t5583-fetch-object-info.sh
>
> > diff --git a/fetch-pack.c b/fetch-pack.c
> > index b709a61baf..36e3d1c5d0 100644
> > --- a/fetch-pack.c
> > +++ b/fetch-pack.c
> > @@ -1269,6 +1269,27 @@ static void write_command_and_capabilities(struct strbuf *req_buf,
> >       packet_buf_delim(req_buf);
> >  }
> >
> > +void send_object_info_request(int fd_out, struct object_info_args *args)
> > +{
> > +     struct strbuf req_buf = STRBUF_INIT;
> > +     int i;
> > +
> > +     write_command_and_capabilities(&req_buf, args->server_options, "object-info");
> > +
> > +     if (string_list_has_string(args->object_info_options, "size"))
> > +             packet_buf_write(&req_buf, "size");
>
> Do we need a newline after "size" here? It's not clear to me that it's
> necessary; Documentation/technical/protocol-v2.txt just says
> "command-specific-args are packet line framed arguments defined by each
> individual command", but the oid arguments below have a newline, so we
> should be consistent one way or the other.
>
> It looks like the server-side implementation wants just a bare "size"
> string (no newline), but I suspect that either is OK because the
> packet_reader is probably using PACKET_READ_CHOMP_NEWLINE.
>
>
> > +     for (i = 0; i < args->oids->nr; i++) {
> > +             packet_buf_write(&req_buf, "oid %s\n", 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);
> > +}
> > +
>
> > diff --git a/transport.c b/transport.c
> > index 70e9840a90..65a1b1fdb3 100644
> > --- a/transport.c
> > +++ b/transport.c
> > @@ -350,6 +350,67 @@ static struct ref *handshake(struct transport *transport, int for_push,
> >       return refs;
> >  }
> >
> > +/*
> > + * Fetches object info if server supports object-info command
> > + * Make sure to fallback to normal fetch if this fails
> > + */
> > +static int fetch_object_info(struct transport *transport)
> > +{
> > +     struct git_transport_data *data = transport->data;
> > +     struct object_info_args args;
> > +     struct packet_reader reader;
> > +
> > +     memset(&args, 0, sizeof(args));
> > +     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_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", 0))
> > +                     return 0;
> > +             send_object_info_request(data->fd[1], &args);
> > +             break;
> > +     case protocol_v1:
> > +     case protocol_v0:
> > +             die(_("wrong protocol version. expected v2"));
>
> The comment at the top of this function says that callers should be
> prepared to fallback to normal fetch if this function fails. The only
> way it can currently fail is if we are using protocol V2 but the server
> doesn't support object-info. Rather than die() if we're on protocol V1
> or V0, can we also return a failure here and let callers fallback to
> fetch?
>
> > +     case protocol_unknown_version:
> > +             BUG("unknown protocol version");
> > +     }
> > +
> > +     if (packet_reader_read(&reader) != PACKET_READ_NORMAL) {
> > +             die(_("error reading object info header"));
> > +     }
> > +     if (strcmp(reader.line, "size")) {
> > +             die(_("expected 'size', received '%s'"), reader.line);
> > +     }
> > +     while (packet_reader_read(&reader) == PACKET_READ_NORMAL) {
> > +             printf("%s\n", reader.line);
> > +     }
> > +     check_stateless_delimiter(transport->stateless_rpc, &reader, "stateless delimiter expected");
> > +
> > +     return 1;
> > +}
> > +