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

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Aug 8, 2022, 10:07 UTC
Message-ID: <edc7763d-420f-a1f9-de13-fb2cb4ddfb2a@gmail.com>
In-Reply-To: <CAFySSZDvgwbbHCHfyuaqX3tKsr-GjJ9iihygg6rNNe46Ys7_EA@mail.gmail.com>

Hi Calvin

On 04/08/2022 23:20, Calvin Wan wrote:

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

Does that mean all the type client code is effectively unused and so untested?

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

I find the message confusing as we're talking about oids not refs. I have also realized that dying here is incompatible with the normal cat-file behavior of printing "<objectname> missing" rather than dying when a missing object is queried.

Show 6 quoted lines

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

If we can then updating the protocol makes sense. Even if we do that a buggy or malicious server could return the objects in a different order. A malicious server can also lie about the size so I'm not sure how much benefit there is in checking the oids in terms of validating the response but I think we should definitely check that the server returns the expected number of sizes.

Best Wishes

Phillip