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

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

> 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.