Re: [PATCH GSoC v3 4/8] fetch-object-info: use dedicated struct for the results

On Mon Aug 3, 2026 at 8:28 PM CEST, Junio C Hamano wrote:
> Pablo Sabater <[email protected]> writes:
>
>> fetch_object_info() collects information about N objects, but it stores
>> the results in an array of object_info. That struct holds the extended
>> parameters of read_object_info() (The optional outputs the caller wants
>> filled). Its pointers tell that function where to write the answers for
>> a single object. object_info is not meant to be the final storage, and
>> since fetch_object_info() does not call read_object_info(), there is no
>> reason to use it. Using it means allocating one scalar per object per
>> attribute just to have those pointers somewhere to point at.
>>
>> Add struct fetch_object_info_results. The caller sets the wants_* flags
>> to say what it is interested in, and fetch_object_info() allocates one
>> array per attribute. A set wants_* flag means "asked for", while a
>> non-NULL array means "available". The caller releases the arrays with
>> free_fetch_object_info_results().
>>
>> The object_info_options string list is no longer needed. Filtering
>> against the server's advertisement now sets local ask_* flags, and
>> send_object_info_request() turns those into the v2 protocol option
>> strings. remote_atom_map[] existed only to map those strings back into
>> atom names, so drop it and build remote_allowed_atoms from the result
>> arrays.
>>
>> free_object_info_contents() loses its only caller and is dropped.
>>
>> Helped-by: Jeff King <[email protected]>
>> Helped-by: Junio C Hamano <[email protected]>
>> Mentored-by: Karthik Nayak <[email protected]>
>> Mentored-by: Chandra Pratap <[email protected]>
>> Signed-off-by: Pablo Sabater <[email protected]>
>> ---
>>  builtin/cat-file.c  | 59 +++++++++--------------------------
>>  fetch-object-info.c | 90 ++++++++++++++++++++++++++++------------------=
-------
>>  fetch-object-info.h | 28 ++++++++++++-----
>>  object-file.c       | 10 ------
>>  odb.h               |  3 --
>>  transport.c         |  3 +-
>>  transport.h         |  5 +--
>>  7 files changed, 88 insertions(+), 110 deletions(-)
>
> The direction this step wants to take us looks good, but at this
> point we only support "size" and the client side starts parsing
> "type" only in [6/8], while the server side starts advertising
> "type" only in [7/8].  If the software at this step talks to a newer
> server that does support "type", it will hit BUG() if the user
> requests %(objecttype), no?  IOW, introduction of "ask_type" smells
> a bit premature.

True, if a client asks type and size in this patch and the server
supports it, "wanted" will be 2 and because in the loop over wanted we
only expect size, we will end up BUG()'ing out for something that is
not a BUG(), but an old client vs a newer server.

I will move ask_type int a later patch in this series where it fits
correctly.

Thanks for noticing it,
Pablo