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