Re: [PATCH v5 5/6] transport: add client support for object-info
- From
- Phillip Wood <phillip.wood123@gmail.com>
- Date
- Aug 1, 2022, 13:30 UTC
- Message-ID
- <b33ec478-8858-faa7-2678-f02559426539@gmail.com>
- In-Reply-To
- <20220728230210.2952731-6-calvinwan@google.com>
Hi Calvin
On 29/07/2022 00:02, Calvin Wan wrote:
Show 10 quoted lines
> 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 adds client functions to communicate with the > server. > > The client currently supports requesting a list of object ids with > features 'size' and 'type' from a v2 server. If a server does not > advertise either of the requested features, then the client falls back > to making the request through 'fetch'.
I'm confused by the 'type' support, I might have missed something as I'm not familiar with this code but I couldn't see where we parse the type returned by the server.
> + for (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");This is one of a number of lines in this series that are way over the 80 column limit.
Show 5 quoted lines
> + return -1;
> + }
> + if (unsorted_string_list_has_string(args.object_info_options, reader.line)) {
> + if (!strcmp(reader.line, "size"))
> + size_index = i;Should we be checking for "type" as well? Also does protocol-v2.txt need updating as it only mentions "size" as an attribute.
Show 11 quoted lines
> + continue;
> + }
> + return -1;
> + }
> +
> + i = 0;
> + while (packet_reader_read(&reader) == PACKET_READ_NORMAL && i < args.oids->nr) {
> + struct string_list object_info_values = STRING_LIST_INIT_DUP;
> +
> + string_list_split(&object_info_values, reader.line, ' ', -1);
> + if (0 <= size_index) {To avoid a possible out-of-bounds access we need to check that size_index + 1 < object_info_value.nr in case the server response is malformed
> + if (!strcmp(object_info_values.items[1 + size_index].string, ""))
> + die("object-info: not our ref %s",I'm a bit confused by this message is it trying to say "object %s is missing on the server"?
> + object_info_values.items[0].string); > + *(*object_info_data)[i].sizep = strtoul(object_info_values.items[1 + size_index].string, NULL, 10);
As Junio pointed out in his comments in v4 there is no error checking here - we should check the server has actually returned a number. Note that strtoul() will happily parse negative numbers so we probably want to do something like
const char *endp
errno = 0
if (!isdigit(*object_info_values.items[1 + size_index].string))
die("...")
*(*object_info_data)[i].sizep = strtoul(object_info_values.items[1 +
size_index].string, &endp, 10);
if (errno || *endp)
die("...")Should be we checking the object id matches what we asked for? (I'm not sure if protocol-v2.txt mentions the order in which the objects are returned)
Should we be parsing the object type here as well?
Show 18 quoted lines
> @@ -392,8 +468,25 @@ 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;
> -
> - if (!data->got_remote_heads) {
> + args.object_info = transport->smart_options->object_info;
> +
> + if (transport->smart_options && transport->smart_options->object_info) {
> + struct ref *ref = object_info_refs;
> +
> + 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 (i = 0; i < transport->smart_options->object_info_oids->nr; i++) {
> + struct ref *temp_ref = xcalloc(1, sizeof (struct ref));Using CALLOC_ARRAY() or p = xcalloc(1, sizeof(*p)) would be safer here (and everywhere else where you have used xcalloc()) as it ensures we allocate the correct size.
Show 12 quoted lines
> diff --git a/transport.h b/transport.h
> index b5bf7b3e70..5512fdb140 100644
> --- a/transport.h
> +++ b/transport.h
> @@ -31,6 +32,12 @@ struct git_transport_options {
> */
> unsigned connectivity_checked:1;
>
> + /*
> + * Transport will attempt to pull only object-info. Fallbacks
> + * to pulling entire object if object-info is not supported
> + */Is it definitely true that we fallback to pulling the entire object? - there is at least one place above where we do
> + if (transport->smart_options->object_info) {
> + die(_("--object-info requires protocol v2"));
> + }
>Best Wishes
Phillip