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

On Sun Aug 2, 2026 at 4:02 AM CEST, Junio C Hamano wrote:

Jeff King [off-list ref] writes:

quoted

And I guess that's what started this conversation. The fundamental
difference is asking about one object (and using pointers to tell where
to put the answer) versus asking about N.

Thanks for framing the trouble I had so cleanly.  Yes.

The origin of the pointer pattern you mentioned, 9a49059022
(sha1_object_info_extended(): expose a bit more info, 2011-05-12),
designed the object_info structure to be passed as a set of extended
parameters to sha1_object_info_extended().

Instead of passing 'size_t *size_p' (which can be NULL) as a
parameter to signal that (1) if NULL we are not interested in the
value, and (2) if not NULL, that is where you are expected to write
the answer, and having to keep adding such a pointer parameter
every time we need to optionally ask the function for a different
aspect of the object, it defined the function to take an object_info
structure to allow us to add new members to the struct as the set
of queries grows without having to change the function signature.

As a set of extended parameters, of course, it was natural for the
caller's variables that receive the answers to be pointed to by
members in the struct.  So the pointers in the struct are
justifiable, but strictly as parameters to the function.

The troubling thing I saw in the patch (and I suspect it was not a
problem introduced in this series, but by earlier changes that added
other kinds of fields) is exactly as you identified.

The pointers in this struct were meant to point at real variables or
structure members that receive values from the function, and were
never meant to be the final structure that receives and retains
returned values.  If we need 5 calls to the function, we either:

 (1) Have a single object_info, and a set of local variables that
     are pointed at by the members of the object_info structure, and
     have a loop that runs 5 times where each iteration calls the
     function to store the returned values in local variables and
     consumes them, i.e.

        struct oid oid[5];
        struct object_info oi;
        for (int i = 0; i < 5; i++) {
                size_t size;
                enum object_type type;
                oi.size_p = &size;
                oi.type_p = &type;
                object_info_extended(oid[i], &oi);
                ... use 'size' and 'type' here ...
        }

     if you can consume and forget about the object in each
     iteration, or

 (2) Have a single object_info, and 5 sets of local variables.  A
     loop runs 5 times; in the nth iteration of the loop,
     object_info points at the nth set of local variables and the
     function is called.  After the loop runs, we have 5 sets of
     local variables populated and we use them, i.e.

        struct oid oid[5];
        struct { size_t size; enum object_type type; } trait[5];
        struct object_info oi;
        for (int i = 0; i < 5; i++) {
                oi.size_p = &trait[i].size;
                oi.type_p = &trait[i].type;
                object_info_extended(oid[i], &oi);
        }
        ... now you have 'size' and 'type' for all these 5 objects ...

     if you have to return all 5 results to your caller.

In either case, you do not need more than one object_info
structure.  Having an array of object_info structures was what
looked so weird to me.


Thanks.

Thanks, I think I got it.

I have the doubt of whether this change is desired for this series as
prep or if I should keep on and later make a cleanup series as this
doesn't make a change for a user.

What I understood is that fetch_object_info shouldn't use object_info to
store the results, because it doesn't call read_object_info() like other
commands like 'info' do. Then, it should use its own data structure to
hold the results with flags like wants_size and wants_type. Something
like:

	struct object_info_results {
		enum object_type *types;
		size_t *sizes;
		unsigned *unrecognized;
		size_t nr;
		unsigned wants_size:1;
		unsigned wants_type:1;
	};

All three of the pointers are nr long.
This could be done in two patches, as I was going to do a prep to
prepare the current code (size only) and this patch would add type for
fetch_object_info().

At the start I read this:

It could be something we may want to
clean-up much later after all the dust settles from this year's
GSoC.  I dunno.

So I'm a bit lost about what to do, I'm happy to make that in this
series or as a cleanup series later after GSoC which ends in a couple
weeks.

Whatever is preferred.

Thanks,
Pablo