> 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@xxxxxxxxxx> 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;
> > +}
> > +
