From: Phillip Wood Date: Mon, 08 Aug 2022 10:07:15 GMT Subject: Re: [PATCH v5 5/6] transport: add client support for object-info Message-ID: In-Reply-To: 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 " 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