Re: [PATCH GSoC v2 4/6] fetch-object-info: parse type from server response

On Sat Aug 1, 2026 at 3:38 PM CEST, Junio C Hamano wrote:
> Junio C Hamano <[email protected]> writes:
>
>>> +		} else if (!strcmp(reader->line, "type")) {
>>> +			type_index =3D (int)i;
>>> +			for (size_t j =3D 0; j < args->oids->nr; j++)
>>> +				object_info_data[j].typep =3D
>>> +					xcalloc(1, sizeof(*object_info_data[j].typep));
>>
>> Do object_info_data[j].typep and object_info_data[k].typep need to
>> be independently freeable?  Separate allocations by calling calloc
>> args->oids->nr times would allow that, but if there is no such need,
>> nr contiguous allocation of them,
>
> Stepping back a bit, the design of "odb.h:struct object_info" look
> rather curious.
>
> Why does the struct store scalar values like "enum object_type" and
> "size_t" as a pointer to elsewhere, and does not store the values
> right there in the structure itself?  By forcing the caller to
> allocate an "enum object_type" for each of these object_info[]
> elements, the design requires 8-byte for a pointer to the heap and
> malloc overhead, probably ~16 bytes or more, in addition to store a
> single "enum object_type" that can be stored in a single byte.
>
> We are probably using this pointer indirection to say "ah, typep is
> NULL so the caller did not ask for this information and the object
> layer does not have to provide one", plus "typep is NULL so the
> engine did not give this information for the object".  But we can do
> so with two bitfields "unsigned typep_asked:1, typep_valid:1;"
> instead of paying ~24-byte or more heap allocation overhead.
>
> Again, this is not something we can change in the middle of this
> topic, but since I noticed it and found iffy, I'll leave a note here
> to stir the pot anyway.
>
> Stepping back a bit, the design of odb.h:struct object_info looks
> rather curious.
>
> Why does the struct store scalar values like enum object_type and
> size_t as pointers to elsewhere, rather than storing the values
> right there in the structure itself?  By forcing the caller to
> allocate an enum object_type for each of these object_info[]
> elements, the design requires an 8-byte pointer to the heap and
> malloc overhead, probably ~16 bytes or more, to store a single
> enum object_type that could fit in a single byte.
>
> We are probably using this pointer indirection to say "ah, '.typep'
> is NULL so the caller did not ask for this information and the
> object layer does not have to provide it", plus "'.typep' is NULL
> so the engine did not give this information for the object".  But we
> can do so with two bitfields
>
>     unsigned type_asked:1,
>              type_valid:1;
>
> instead of paying ~24 bytes or more of heap allocation overhead.
>
> Again, this is not something we can change in the middle of this
> topic, but since I noticed it and found it iffy, I'll leave a note
> here to stir the pot anyway.  It could be something we may want to
> clean-up much later after all the dust settles from this year's
> GSoC.  I dunno.

[CC'ing peff]

Hi!

I haven't stopped to think about that but it does look strange.

This is related to what had to be done to fix a bug at "contents"
commands a few days ago [1].

In that patch it had to save the previous state of typep and then
restore it, because other commands like "info" and this series one
"remote-object-info" use this pointer for the "is this asked?" question.

If we take a look at expand_atom():

	...
	} else if (is_atom("objecttype", atom, len)) {
		if (data->mark_query) {
			data->info.typep =3D &data->type;
		} else {
			const char *t =3D type_name(data->type);
			strbuf_addstr(sb, t ? t : "");
		}
	...

expand_atom() has two responsibilities, it is called at the start to map
which atoms are asked (when data->mark_query), and a second to expand
those atoms.

For example, typep being non-NULL does this effect on these commands:

info: makes a type lookup, and fills type.

remote-object-info: typep is directly used to know whether a client has
                    asked for %(objecttype).

For both commands what we pay is extra work because at the end the data
shown is the one expanded from the format.

It's out of scope for this series but I wanted to add what I know.

[1]: https://lore.kernel.org/git/[email protected]=
.net/

Thanks,
Pablo