From: Pablo Sabater Date: Sat, 27 Jun 2026 19:44:42 GMT Subject: Re: [PATCH GSoC v14 10/13] transport: add client support for object-info Message-ID: In-Reply-To: El sáb, 27 jun 2026 a las 2:22, Karthik Nayak () escribió: > > Pablo Sabater writes: > > > From: Calvin Wan > > > > 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)." And the wire > > format is documented at > > https://git-scm.com/docs/protocol-v2#_object_info. > > > > 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. > > > > Notice that the entire request is written into req_buf before being > > sent to the remote. This approach follows the pattern used in the > > `send_fetch_request()` logic within fetch-pack.c. > > Streaming the request is not addressed in this patch. > > > > Helped-by: Jonathan Tan > > Helped-by: Christian Couder > > Signed-off-by: Calvin Wan > > Signed-off-by: Eric Ju > > Signed-off-by: Pablo Sabater > > --- > > Makefile | 1 + > > fetch-object-info.c | 90 +++++++++++++++++++++++++++++++++++++++++++++++++++++ > > fetch-object-info.h | 22 +++++++++++++ > > fetch-pack.c | 3 ++ > > fetch-pack.h | 2 ++ > > meson.build | 1 + > > transport-helper.c | 11 +++++-- > > transport.c | 28 ++++++++++++++++- > > transport.h | 11 +++++++ > > 9 files changed, 166 insertions(+), 3 deletions(-) > > > > diff --git a/Makefile b/Makefile > > index 1cec251f43..ec4df39a6b 100644 > > --- a/Makefile > > +++ b/Makefile > > @@ -1159,6 +1159,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..9c4ae9bd11 > > --- /dev/null > > +++ b/fetch-object-info.c > > @@ -0,0 +1,90 @@ > > +#include "git-compat-util.h" > > +#include "gettext.h" > > +#include "hex.h" > > +#include "pkt-line.h" > > +#include "connect.h" > > +#include "oid-array.h" > > +#include "odb.h" > > +#include "fetch-object-info.h" > > +#include "string-list.h" > > + > > +/* Sends git-cat-file object-info command and its arguments into the request buffer. */ > > This file doesn't know about git-cat-file(1), nor should it care about > it, right? Since git-cat-file(1) is simply a user of this function. > Would it make more sense to structure the document around what this > function is supposed to do? > > Also the comment for `fetch_object_info` seems to be similar, perhaps > worthwhile changing both without referring to git-cat-file(1). > Theoretically there could be other users in the future too. `git cat-file` is currently the only consumer of `object-info` but you are right, this file shouldn't know about it nor mention it in a comment, I'll drop it. > > > +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"); > > + > > Okay so if the user requests 'size', we forward that to the server. But > what about the 'else' condition? Should we BUG() out? In patch [13/13] the options are built dynamically and filtered against what the server advertises, so by the time we reach here only valid options remain. For this commit there is no client support yet so there is no way of this to fail and in the next commit [11/13]: get_remote_info() [snip] string_list_append(&object_info_options, "size"); if (object_info_options.nr > 0) { gtransport->smart_options->object_info_options = &object_info_options; gtransport->smart_options->object_info_data = *remote_object_info; retval = transport_fetch_refs(gtransport, NULL); } [snip] get_remote_info() appends "size" always to object_info_options and this is changed in commit [12/13] with the allow-list where only asked object-info is appended. I could move that check to this commit as if size always gets appended the `if` below will always be true. Also because size is always appended there can't be an else condition to BUG() out. > > > + 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")); > > + > > We write out all the oids, flush and write to the fd. Okay. > > > + strbuf_release(&req_buf); > > +} > > + > > +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); > > So if the server does support 'object-info', we call > `send_object_info_request()`. Makes sense. > > > + break; > > + case protocol_v1: > > + case protocol_v0: > > + die(_("unsupported protocol version. expected v2")); > > + case protocol_unknown_version: > > + BUG("unknown protocol version"); > > + } > > + > > Now that we've sent the request, we should start parsing the response. > > > + 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 (!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(*object_info_data[j].sizep)); > > + } > > + } > > + > > So this function seems to iterate over the list of options to only find > and store the indexes. If the server does support size we also allocate > the pointers to store the size. > > Shouldn't we similarly BUG() if there is anything apart from 'size' here? Because only size is appended and the server size support comes with the series, there shouldn't be a scenario where anything else appears. 1. Full response with size. 2. OID unrecognized by the server. We can quickly check this because the server response when the OID is unrecognized is `OID SP` if there's nothing after the SP the object is unrecognized. > > > + 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, "")) { > > + FREE_AND_NULL(object_info_data[i].sizep); > > + string_list_clear(&object_info_values, 0); > > + continue; > > + } > > + if (strtoul_szt(object_info_values.items[1 + size_index].string, > > + 10, object_info_data[i].sizep)) > > This is now no longer correctly aligned. I will fix it. > > > + die("object-info: ref %s has invalid size %s", > > + object_info_values.items[0].string, > > + object_info_values.items[1 + size_index].string); > > + } > > + > > + string_list_clear(&object_info_values, 0); > > + } > > + check_stateless_delimiter(stateless_rpc, reader, "stateless delimiter expected"); > > + > > We parse each line and obtain the size and parse it into > `object_info_data[i].sizep`. If the value is missing, we simply continue > the iteration. Related to that above, if `size` is missing we FREE_AND_NULL so later on `parse_cmd_remote_object_info()` can detect that and print "missing" for that oid. This "missing" behavior follows what the local option `info` does for unrecognized OIDs. parse_cmd_remote_object_info() [snip] for (size_t i = 0; i < object_info_oids.nr; i++) { data->oid = object_info_oids.oid[i]; if (remote_object_info[i].sizep) { /* * When reaching here, it means remote-object-info can retrieve * information from server without downloading them. */ data->size = *remote_object_info[i].sizep; opt->batch_mode = BATCH_MODE_INFO; batch_object_write(argv[i + 1], output, opt, data, NULL, 0); } else { report_object_status(opt, oid_to_hex(&data->oid), &data->oid, "missing"); } } [snip] > > I think we had this discussion off the list about how this means that > oids which do not have a size will not error out but rather display a > missing info value. The following commit [11/13] that uses the functions introduced in this commit, will print "oid missing" in case of the object being unrecognized by the server, the no error out (empty string) would be in the scenario where the object is recognized by the server but the capability is not. Because this same series introduces the capability advertisement at [9/13] commit we can be sure that size will always be supported. Subsequent commits with the allow-list aim to tackle this with the empty-strings when a server doesn't support a capability. > > The argument for not error'ing out was better user experience where the > command would complete without exiting. I still think we should error > out, because: > > 1. Without error'ing out, we'd have to display to the user a missing > value token. There is contention around what this token should be, > as such a token shouldn't be a valid value for the info type being > displayed. Future options must be considered here. I believe that better user experience was my argument last time we discussed and I still believe that. But a stronger argument is that the local option `info` doesn't die either but rather it prints "OID missing" and the next commit where the cmd-related functions are implemented tries to be consistent with it for unrecognized OIDs. > > 2. What will the error code of such a situation be? Do we consider it a > success or a failure? Is there a situation where an object can have a > missing size? There are two different scenarios to consider here: 1. OID unrecognized by the server: the server responds with `oid SP` (no value after the space). In this case, the next commit [11/13] prints " missing" using `report_object_status()`, which is consistent with how the local `info` command handles missing objects in `--batch-command`. 2. OID recognized but a capability is not supported by the server: subsequent commits [12/13] and [13/13] introduce an allow-list that filters options against what the server advertises. Unsupported placeholders return an empty string, following how `for-each-ref`handles known but inapplicable atoms. For the exit code, both cases return 0. In case 1, this matches the local `info` behavior. In case 2, we are not failing to fetch, we simply see that the server does not support the requested capabilities and skip the request to the server. For the missing token (your point 1), we reuse the existing "missing" token from `report_object_status()`, which is what the local code path already uses, so no new token is introduced. > > > + return 0; > > +} > > diff --git a/fetch-object-info.h b/fetch-object-info.h > > new file mode 100644 > > index 0000000000..d35284bd6b > > --- /dev/null > > +++ b/fetch-object-info.h > > @@ -0,0 +1,22 @@ > > +#ifndef FETCH_OBJECT_INFO_H > > +#define FETCH_OBJECT_INFO_H > > + > > +#include "pkt-line.h" > > +#include "protocol.h" > > +#include "odb.h" > > + > > +struct object_info_args { > > + struct string_list *object_info_options; > > + const struct string_list *server_options; > > + struct oid_array *oids; > > +}; > > + > > +/* > > + * Sends git-cat-file object-info command into the request buf and read the > > + * results from packets. > > + */ > > +int fetch_object_info(enum protocol_version version, struct object_info_args *args, > > + struct packet_reader *reader, struct object_info *object_info_data, > > + int stateless_rpc, int fd_out); > > + > > +#endif /* FETCH_OBJECT_INFO_H */ > > diff --git a/fetch-pack.c b/fetch-pack.c > > index cdebd3476f..a86c93fc52 100644 > > --- a/fetch-pack.c > > +++ b/fetch-pack.c > > @@ -1742,6 +1742,9 @@ 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: > > diff --git a/fetch-pack.h b/fetch-pack.h > > index 6d0dec7f41..5a428f11ed 100644 > > --- a/fetch-pack.h > > +++ b/fetch-pack.h > > @@ -16,6 +16,7 @@ struct fetch_pack_args { > > const struct string_list *deepen_not; > > struct list_objects_filter_options filter_options; > > const struct string_list *server_options; > > + struct object_info *object_info_data; > > > > /* > > * If not NULL, during packfile negotiation, fetch-pack will send "have" > > @@ -43,6 +44,7 @@ struct fetch_pack_args { > > unsigned reject_shallow_remote:1; > > unsigned deepen:1; > > unsigned refetch:1; > > + unsigned object_info:1; > > > > /* > > * Indicate that the remote of this request is a promisor remote. The > > diff --git a/meson.build b/meson.build > > index 3247697f74..145c6882eb 100644 > > --- a/meson.build > > +++ b/meson.build > > @@ -347,6 +347,7 @@ libgit_sources = [ > > 'exec-cmd.c', > > 'fetch-negotiator.c', > > 'fetch-pack.c', > > + 'fetch-object-info.c', > > 'fmt-merge-msg.c', > > 'fsck.c', > > 'fsmonitor.c', > > diff --git a/transport-helper.c b/transport-helper.c > > index f195070788..c77599f6fb 100644 > > --- a/transport-helper.c > > +++ b/transport-helper.c > > @@ -727,8 +727,8 @@ 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. > > This is not really true as of this commit. This only comes into effect > in the next commit. So shouldn't this be added there? That said, I would > modify this to only talk about object-info requiring v2 and drop the > reference to cat-file. Agreed, I will do that, thanks. > > > */ > > if (data->transport_options.acked_commits) { > > warning(_("--negotiate-only requires protocol v2")); > > @@ -744,6 +744,13 @@ static int fetch_refs(struct transport *transport, > > free_refs(dummy); > > } > > > > + /* fail the command explicitly to avoid further commands input. */ > > + if (transport->smart_options->object_info) > > + die(_("remote-object-info requires protocol v2")); > > + > > + if (!data->get_refs_list_called) > > + get_refs_list_using_list(transport, 0); > > + > > Don't we already do this right above? Why is this needed again? True, I'll drop the second `if`. > > > count = 0; > > for (i = 0; i < nr_heads; i++) > > if (!(to_fetch[i]->status & REF_STATUS_UPTODATE)) > > diff --git a/transport.c b/transport.c > > index 0f5ec30247..7d3246e12b 100644 > > --- a/transport.c > > +++ b/transport.c > > @@ -9,6 +9,7 @@ > > #include "hook.h" > > #include "pkt-line.h" > > #include "fetch-pack.h" > > +#include "fetch-object-info.h" > > #include "remote.h" > > #include "connect.h" > > #include "send-pack.h" > > @@ -467,8 +468,33 @@ static int fetch_refs_via_pack(struct transport *transport, > > args.negotiation_restrict_tips = data->options.negotiation_restrict_tips; > > args.negotiation_include_tips = data->options.negotiation_include_tips; > > args.reject_shallow_remote = transport->smart_options->reject_shallow; > > + args.object_info = transport->smart_options->object_info; > > + > > Hmm, so we piggy-back on top of the `fetch_refs_via_pack()` function... > > > + if (transport->smart_options->object_info > > + && transport->smart_options->object_info_oids->nr > 0) { > > + struct packet_reader reader; > > + struct object_info_args obj_info_args = { 0 }; > > + > > + obj_info_args.server_options = transport->server_options; > > + obj_info_args.oids = transport->smart_options->object_info_oids; > > + obj_info_args.object_info_options = transport->smart_options->object_info_options; > > + string_list_sort(obj_info_args.object_info_options); > > + > > + 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; > > + > > + ret = fetch_object_info(data->version, &obj_info_args, &reader, > > + data->options.object_info_data, transport->stateless_rpc, > > + data->fd[1]); > > + goto cleanup; > > > > ... and we jump to exit when we only want object info information. This > skips the call to fetch_pack(). I'm a bit uneasy with this. Ideally we > should be adding a new function to the vtable to only fetch object info. > While this works, this doesn't fit the contract of what this function is > supposed to do. See the comment around `fetch_refs` in `struct > transport_vtable`. Shouldn't we update that documentation at the very > least? Now that you point that out, piggy-backing doesn't look very good, while it works, updating the documentation leaves this in a bad situation, so I will move it to a new function and add it to the vtable for the next version. Thanks for noticing it. > > > - if (!data->finished_handshake) { > > + } else if (!data->finished_handshake) { > > int i; > > int must_list_refs = 0; > > for (i = 0; i < nr_heads; i++) { > > diff --git a/transport.h b/transport.h > > index 7e5867cffa..bd60b10af4 100644 > > --- a/transport.h > > +++ b/transport.h > > @@ -6,6 +6,7 @@ > > #include "list-objects-filter-options.h" > > #include "string-list.h" > > #include "connect.h" > > +#include "odb.h" > > > > struct git_transport_options { > > unsigned thin : 1; > > @@ -31,6 +32,12 @@ struct git_transport_options { > > */ > > unsigned connectivity_checked:1; > > > > + /* > > + * Transport will attempt to retrieve only object-info. > > + * If object-info is not supported, the operation will error and exit. > > + */ > > + unsigned object_info : 1; > > + > > According to our style, this should be `unsigned object_info:1`. I'll fix it to properly follow the style, I kept it like this because other bit fields on these files seem to be outdated in that style. > > > int depth; > > const char *deepen_since; > > const struct string_list *deepen_not; > > @@ -55,6 +62,10 @@ struct git_transport_options { > > * common commits to this oidset instead of fetching any packfiles. > > */ > > struct oidset *acked_commits; > > + > > + struct oid_array *object_info_oids; > > + struct object_info *object_info_data; > > + struct string_list *object_info_options; > > }; > > > > enum transport_family { > > > > -- > > 2.54.0 > > Nit: I do think the commit message doesn't sufficiently capture the > entirety of this patch. We do not talk about: > > 1. How we piggy-back on top of `fetch_refs_via_pack()` and don't > introduce our own pointer in the vtable. > 2. The changes made to 'transport-helper.c' and why that is required. > 3. How we don't error out when there is no size value provided for an > OID and what the implication of this is. I will extend the commit message to include all 3 points (I'll change the piggy-backing to have its own function). Thanks for the review, Pablo.