> > 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.
I should clarify that the server does not support 'type', only the client.
Since the client falls back to 'fetch' to grab the object info if the server
doesn't support a requested option (e.g. type), I have 'type' included
as part of the supported client options.
> > + 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.
ack
> > + 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.
I gave context above -- the server does not accept 'type' so protocol-v2.txt
does not need to be updated.
> 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
ack
> > + 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"?
Correct. You'll find the same error message in upload-pack.c
> > + 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)
Hmmmm I think I either check for an object id match or update
protocol-v2.txt to mention order is consistent.
> Should we be parsing the object type here as well?
When the server starts supporting it.
> > + 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.
ack
> > + /*
> > + * 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
Yes, fetch_refs_via_pack() is where fetch_object_info() is called,
making it easy to fallback if fetch_object_info() fails.
>> + while (packet_reader_read(&reader) == PACKET_READ_NORMAL && i < args.oids->nr) {
>> + struct string_list object_info_values = STRING_LIST_INIT_DUP;
>
> I forget to say earlier that this is leaked
ack
Thank you for the review!
