Re: [PATCH GSoC v4 4/9] fetch-object-info: use dedicated struct for the results
- From
Pablo Sabater <pabloosabaterr@gmail.com>
- Date
- Aug 4, 2026, 21:42 UTC
- Message-ID
- <DKGHIZDB5W00.QN7C3PZ77K7B@gmail.com>
- In-Reply-To
- <xmqqh5l9txg6.fsf@gitster.g>
On Tue Aug 4, 2026 at 10:58 PM CEST, Junio C Hamano wrote:
Show 14 quoted lines
> Pablo Sabater <pabloosabaterr@gmail.com> 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.
Show 22 quoted lines
>
>> -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