Re: [PATCH v3 3/3] object-info: add option for retrieving object info
- From
- Jonathan Tan <jonathantanmy@google.com>
- Date
- Mar 30, 2022, 22:07 UTC
- Message-ID
- <20220330220703.2304827-1-jonathantanmy@google.com>
- In-Reply-To
- <20220328191112.3092139-4-calvinwan@google.com>
Calvin Wan <calvinwan@google.com> writes:
Show 20 quoted lines
> @@ -1604,6 +1625,10 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
> if (args->depth > 0 || args->deepen_since || args->deepen_not)
> args->deepen = 1;
>
> + if (args->object_info) {
> + state = FETCH_SEND_REQUEST;
> + }
> +
> while (state != FETCH_DONE) {
> switch (state) {
> case FETCH_CHECK_LOCAL:
> @@ -1613,7 +1638,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
> /* Filter 'ref' by 'sought' and those that aren't local */
> mark_complete_and_common_ref(negotiator, args, &ref);
> filter_refs(args, &ref, sought, nr_sought);
> - if (everything_local(args, &ref))
> + if (!args->object_info && everything_local(args, &ref))
> state = FETCH_DONE;
> else
> state = FETCH_SEND_REQUEST;I haven't looked into this deeply, but one thing to consider is to do this in a separate function (instead of reusing do_fetch_pack_v2()) because the object-info request and response has almost nothing in common with fetch.
> diff --git a/t/t5583-fetch-object-info.sh b/t/t5583-fetch-object-info.sh
[skip tests]
Also test when: the user gives an invalid --object-info (e.g. "--object-info=foo") and when the user gives two parameters, one valid and one invalid. (Both should fail.)
Show 10 quoted lines
> diff --git a/transport-internal.h b/transport-internal.h
> index c4ca0b733a..04fa015011 100644
> --- a/transport-internal.h
> +++ b/transport-internal.h
> @@ -59,6 +59,7 @@ struct transport_vtable {
> * use. disconnect() releases these resources.
> **/
> int (*disconnect)(struct transport *connection);
> + int (*fetch_object_info)(struct transport *transport, struct oid_array *oids);
> };Do we need another entry in the vtable, or can we do without? (As it is, I don't think we need it, as evidenced by the fact that this patch did not add any entries to the transport_vtable instances.)
Show 22 quoted lines
> 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;I think we can avoid code duplication in this function too. So from here (the connect_setup line below)...
Show 20 quoted lines
> + 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"));
> + case protocol_unknown_version:
> + BUG("unknown protocol version");
> + }...up to here can be a function (except that we need to take out send_object_info_request() and maybe parameterize the server_supports_v2 call).
From here...
Show 9 quoted lines
> + 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);
> + }...until here is object-info-specific, yes.
And this...
> + check_stateless_delimiter(transport->stateless_rpc, &reader, "stateless delimiter expected"); > + > + return 1; > +}
is common code once again.
Also instead of returning 0 from this function if the server doesn't support object-info, it's probably clearer to have one function that checks what the server supports, and then have an "if" block which either does the object-info part or the fetch part depending on what the server says.