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

From: Phillip Wood <phillip.wood123@gmail.com>
To: Calvin Wan <calvinwan@google.com>, phillip.wood@dunelm.org.uk
Cc: git@vger.kernel.org, gitster@pobox.com, jonathantanmy@google.com,
	philipoakley@iee.email, johncai86@gmail.com
Subject: Re: [PATCH v5 5/6] transport: add client support for object-info
Date: Mon, 8 Aug 2022 11:07:15 +0100	[thread overview]
Message-ID: <edc7763d-420f-a1f9-de13-fb2cb4ddfb2a@gmail.com> (raw)
In-Reply-To: <CAFySSZDvgwbbHCHfyuaqX3tKsr-GjJ9iihygg6rNNe46Ys7_EA@mail.gmail.com>

Hi Calvin

On 04/08/2022 23:20, Calvin Wan wrote:
>>> 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?

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

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