Re: [PATCH GSoC v14 10/13] transport: add client support for object-info

From: Karthik Nayak <karthik.188@gmail.com>
Date: Jun 27, 2026, 00:22 UTC
Message-ID: <CAOLa=ZT8u32YUqhnq-pMCCK8Qzx+7k-3E-+yAoM_miJ7BQjxTA@mail.gmail.com>
In-Reply-To: <20260625-ps-eric-work-rebase-v14-10-09f7ffe21a53@gmail.com>

Pablo Sabater <pabloosabaterr@gmail.com> writes:

Show 67 quoted lines

> From: Calvin Wan <calvinwan@google.com>
>
> 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 <jonathantanmy@google.com>
> Helped-by: Christian Couder <chriscool@tuxfamily.org>
> Signed-off-by: Calvin Wan <calvinwan@google.com>
> Signed-off-by: Eric Ju <eric.peijian@gmail.com>
> Signed-off-by: Pablo Sabater <pabloosabaterr@gmail.com>
> ---
>  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.

Show 9 quoted lines

> +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?

Show 8 quoted lines

> +	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.

Show 14 quoted lines

> +	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.

Show 8 quoted lines

> +		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.

Show 17 quoted lines

> +	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?

Show 12 quoted lines

> +	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.

Show 9 quoted lines

> +				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.

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 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.

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?

Show 88 quoted lines

> +	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.

Show 14 quoted lines

>  	 */
>  	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?

Show 21 quoted lines

>  	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...

Show 24 quoted lines

> +	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?

Show 27 quoted lines

> -	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`.

Show 17 quoted lines

>  	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.