Re: [PATCH 3/6] odb: add `source` field to struct object_info_source

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

Patrick Steinhardt <ps@pks.im> writes:

> On Mon, Jun 29, 2026 at 01:47:41PM -0700, Junio C Hamano wrote:
>> Justin Tobler <jltobler@gmail.com> writes:
>> 
>> >> @@ -1424,6 +1424,10 @@ int packed_object_info_with_index_pos(struct odb_source_packed *source UNUSED,
>> >>  	oi->whence = OI_PACKED;
>> >>  
>> >>  	if (oi->sourcep) {
>> >> +		if (!source)
>> >> +			BUG("cannot request source without an owning source");
>> >> +		oi->sourcep->source = &source->base;
>> >
>> > And here it is set for the packed backend. Looks good.
>> >
>> > Naive question: I understand that some `packed_info_object()` callers
>> > may not have the `struct odb_source` on hand, but when the `struct
>> > packed_git` is intially setup, is it not always known the ODB source it
>> > comes from? It makes me wonder if the ODB source should also be recorded
>> > when `struct packed_git` is initialized.
>
> I've addressed this comment on patch 1.
>
>> As with your reaction to [PATCH 1/6], I do share this puzzlement: if
>> the source can almost always be NULL, what is it good for and isn't
>> it something that can be computed from the available information?
>
> It's not almost always NULL, even though it looks like this because we
> ended up adapting more callers to pass `NULL` than we adapted callers to
> pass an actual source. But in the end it's rather the opposite: there
> are very few low-level callers that don't have the source info
> available, and everyone else instead uses `odb_read_object_info()`,
> where we do have it available. But those callers don't need to be
> adjusted, so they weren't visible in the diff.
>
>> Perhaps it is the naming?
>
> Yeah, as Justin pointed out, calling this `sourcep` is confusing.

OK, everything makes sense.

>> I am confused what the above quoted code actually is doing ("if you
>> have a source, then grab its base and set it to .source member of
>> the struct the out parameter points at", makes it sound like the out
>> parameter sourcep should be pointing at a structure with .base
>> member, not .source member, or perhaps the caller should be passing
>> &oi->sourcep->source as *base to be assigned to, or something).
>
> We have to return the generic source here, not the specialized source,
> so that this interface can be used by every implementation. Other sites
> would end up storing their own source, which of course would have a
> different specialized backend.
>
> So an alternative to write this would have been:
>
>     oi->sourcep->source = (struct odb_source *) source;
>
> But by assigning the base we avoid having to cast.

Yuck.

I guess it may be OK as the caller or whategver the caller calls
later may have to downcast this pointer the usual way to access what
we return here anyway.  As a pointer to a struct object, when
suitably converted, points to its first member, this upcast should
always be safe.  And taking the address of the .base member is far
more explicit.

While it hides the fact that there is such an upcast involved, which
might be confusing to readers not familiar with this corner of the
codebase, I am fine with the way it was written.

Thanks.