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

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

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

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

Yeah, that would make sense.

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