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

Jeff King <peff@peff.net> writes:

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.