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

On Tue Aug 4, 2026 at 10:58 PM CEST, Junio C Hamano wrote:
> Pablo Sabater <[email protected]> writes:
>
>>  builtin/cat-file.c  | 59 +++++++++-----------------------------
>>  fetch-object-info.c | 81 ++++++++++++++++++++++++++---------------------------
>>  fetch-object-info.h | 27 +++++++++++++-----
>>  object-file.c       | 10 -------
>>  odb.h               |  3 --
>>  transport.c         |  3 +-
>>  transport.h         |  5 ++--
>>  7 files changed, 77 insertions(+), 111 deletions(-)
>
> We lost ~30 lines mostly thanks to losing an intermediate structure
> and need to flip pointer members in it to point at heap allocated
> locations?

Yes, also contributes that the logic that filtered the allow-list is
dropped, and compared to the previous version, no type logic is in
this patch.

>
>> -struct protocol_placeholder_entry {
>> -	const char *option;
>> -	const char *atom;
>> -};
>> -
>> -static const struct protocol_placeholder_entry remote_atom_map[] = {
>> -	{"size", "objectsize"},
>> -	{"type", "objecttype"},
>> -	/*
>> -	 * Add new protocol options here. Even if the server doesn't support
>> -	 * them the allow_list will drop them if the server doesn't advertise
>> -	 * them.
>> -	 */
>> -};
>
> Hmph, it would not make any difference in the final result, as we
> are losing protocol_placeholder_entry and this list of atoms, but
> I am somewhat confused.   Why we already had "type" here, even
> though we are adding support for it much later in the series (and
> unlike the previous iteration, this iteration did not do the
> ask/want_type thing in the previous step)?

The list was designed so even if new placeholder were added, it would be
idemp. because nothing would happen until a server did advertise that
attribute; that's why there's no change in behavior.

It was there because it was interesting for the tests. A known
placeholder but unsupported in the server still outputs an empty string
(this would be the case of: new client vs old server).

This way we had tested:

  unknown + unsupported as %(deltabase) is.
  known + unsupported as %(objecttype) was. (that's why it's on the list)
  knwon + supported as %(objectsize) is.

Using object_info and the list made sense because object_info already had
a type field, so listing type cost nothing. Now that we have to dedicate
an array per attribute it would mean carrying a types array that nobody
fills, so I preferred to drop it here and implement type fully later.

Hope this makes sense,
Pablo