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

[Jeff King <peff@peff.net>]

On Sun, Aug 02, 2026 at 02:33:49PM +0200, Pablo Sabater wrote:

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

IMHO it is worth doing now. I've spent a bit of time looking through
this code and I found it kind of confusing. In particular:

  - there are a lot of semi-opaque structs, like object_info_args. It
    would seem simpler to me to pass those elements around independently
    to the functions that need them. Likewise, we seem to stuff a lot of
    data into the transport struct rather than passing it to the
    relevant functions, even though many of those elements are really
    just used for one function call, and aren't a property of the
    transport at all.

  - it's hard to see who is ultimately responsible for deciding whether
    a field is requested. I _think_ it comes down to sticking those
    strings into the object_info_options struct (which I might have
    called "attrs" or "atoms" or something; they are syntactically
    "options" in the v2 protocol, but I think from the perspective of
    this code that is not what they are).

    One thing you could do is have the caller pass in pointers for
    "types" and "sizes", and use their NULLness as an indication of what
    they want.  That would be more like how object_info works, and then
    it really becomes a low-level protocol details to form those into
    the v2 protocol option strings.

    But unlike local object_info, we sometimes find that the remote is
    not willing or able to provide a particular type. So we have to
    return a flag somehow for "I was / was not able to get types". You
    could do that with a bool in the response struct.

    Or you could flip it on its head. Have the caller provide a bool
    saying "I want types", and then the low-level code is responsible
    for allocating the "types" array, and leaves it NULL if types were
    not available. That means the caller has to clean up the result, but
    they'd have had to clean up their local arrays anyway.

    I _think_ that's what you're getting at with your example below.

  - the protocol makes this unnecessarily complex. In particular, if we
    ask for "type size", the server is free to return them in an
    arbitrary order, or even to omit one of them (even if it told us it
    supported it!). So we have to map their returned ordering onto our
    arrays. Gross. I guess we are stuck with it, though, as the server
    side has been around for a while. And it does indeed choose its own
    ordering independent of what the client sent.

    I also find the framing needlessly restrictive. Rather than one
    pkt-line per item, we get packets with space-separated values. What
    happens when a future item value has spaces in it?

    As a side note, I think this is a good reason not to ship half of a
    protocol implementation. Without seeing both sides, you don't know
    what gotchas are lurking. But once one side ships, then it's hard to
    change the protocol later. This critique may all just me being
    cranky, though. ;)

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

Yeah. At first I thought your wants_size is redundant, but I guess if
the goal is for the low-level code to allocate the "sizes" array, then
we cannot use it as a signal (i.e., this is the "flip it on its head"
direction I gave above).

I do think you want to be careful with how wants_size interacts with the
"object_info_options" string list. IMHO it would make things easier if
that string-ification happened deep down, probably in
send_object_info_request(). And then the rest of the code can
consistently use wants_size to see if we want sizes (and checking
"sizes" for NULL to cover the case that the server did not support it).

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

Yeah, that would make sense.

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

I think the "after the dust settles" suggestion was for changing the
interface of "struct object_info". That all becomes moot if we stop
using it here entirely. So I think you should proceed along the lines of
the object_info_results you showed above.

-Peff