Hi Calvin
On 29/07/2022 00:02, Calvin Wan wrote:
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.
+ 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.
+ 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?
@@ -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.
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
