Re: [PATCH v5 5/6] transport: add client support for object-info

From: Calvin Wan <calvinwan@google.com>
Date: Aug 4, 2022, 22:20 UTC
Message-ID: <CAFySSZDvgwbbHCHfyuaqX3tKsr-GjJ9iihygg6rNNe46Ys7_EA@mail.gmail.com>
In-Reply-To: <b33ec478-8858-faa7-2678-f02559426539@gmail.com>

Show 8 quoted lines

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

Show 6 quoted lines

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

Show 6 quoted lines

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

Show 5 quoted lines

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

Show 16 quoted lines

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

Show 6 quoted lines

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

Show 7 quoted lines

> > +     /*
> > +      * 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!