Re: [PATCH v3 12/14] builtin/pack-objects: use `packfile_store_for_each_object()`

[Patrick Steinhardt <ps@pks.im>]

On Thu, Jan 29, 2026 at 06:08:39AM -0500, Jeff King wrote:
> On Mon, Jan 26, 2026 at 09:53:18AM +0100, Patrick Steinhardt wrote:
> 
> > > Yes, the end result is the same, both your patch and what I wrote here
> > > implement the same GC-specific definition of an object's "mtime". I am
> > > not following the argument about pluggability, though. The concern I
> > > have above is that we are pushing domain-specific logic into the object
> > > storage backend, not the other way around.
> > 
> > To expand on the pluggability bit: every time you add a new backend
> > you'll have to extend the above logic to understand how it represents
> > the mtime. That by itself might be doable, but let's for example
> > consider a backend that is a black box to us (like a shared library that
> > may plug in arbitrary storage logic). In that case you would not even be
> > able to derive the information unless you have a generic layer that lets
> > you convey it to the caller.
> > 
> > So overall I agree with you that there are nuances here, and that the
> > mtimep pointer _can_ be used incorrectly. But I still think that the
> > concept is generic enough across backends, and the refactored logic
> > still works as extended. I'll try to expand the docs and commit message
> > a bit to cover this discussion.
> 
> There's a related concept that I saw while reading some of the earlier
> patches. When you converted fsck, I wondered how you would handle the
> call to read_loose_object(), which takes an actual path. And it needs to
> do so, because we want to make sure we are opening and reading that
> particular copy of the object, and not one from elsewhere.
> 
> The answer is that you punted on it for this series, and we still get
> the path via for_each_loose_file_in_source(). ;) That is OK, but I think
> it will eventually run into the same issue: we will need some kind of
> cursor or context for the iterator to be able to get extended
> information about a particular copy of an object.
> 
> I think there are probably two approaches here:
> 
>   1. The abstract odb API tries to share as little as possible. It gives
>      the caller back an opaque context struct, and that struct can be
>      handed back to the odb to get object contents or other information
>      (perhaps even an mtime!). Under the hood for the current odb
>      implementation this is probably just a pointer to a string with the
>      filesystem path for loose objects, and the usual packed_git/offset
>      pair for packed objects.
> 
>   2. The odb API provides a set of information that a particular backend
>      _might_ implement, and callers can poke at that information and
>      decide how to handle it when it's not available. And so that might
>      include a filesystem path for loose objects, which some backends
>      may choose to leave NULL.
> 
> Option (1) presents a cleaner API for the odb, but it's also more
> restrictive. Anything that a caller _might_ want to do has to be pushed
> down into the API, and it has to start learning about things like
> mtimes. And how to decide what "mtime" means for non-filesystem
> backends.
> 
> Option (2) pushes more work onto the callers. They need to not only look
> up the mtimes themselves (like they do now), but they have to decide how
> to handle the case when no path is available. Which in the worst case
> means a special case for each type of backend, though I think in
> practice they'd probably fall into rough groups.
> 
> I think one thing that appeals to me about option 2, though, is that it
> keeps a lot of the specialized "business logic" together in those
> callers. Most code doesn't are about concepts like mtime or specific
> copies of objects. But when it does, like in repack or fsck, there are
> often subtle assumptions and interpretations. I'd rather see all of that
> lumped together in the fsck code than have it split half-and-half
> between them and the odb code (which is really going to be some backends
> idea of how its concepts can be shoe-horned into the abstract API).

Yup. One thing that I'm planning to do in one of the subsequent patch
series is to expand `struct object_info` to handle this.

Right now, the sturcture contains a `whence` pointer that tells us which
backend the information is stored in. But that concept can be extended
to surface more info: instead of only telling the caller the type, we
can instead return the actual source that the object has been looked up
in.

Furthermore, the `struct object_info::u` union already contains enough
information for us to uniquely identify a specific option. So what we
would do then is to call `odb_source_read_object_info()` on the specific
source and pass it the union.

The loose source wouldn't have to do anything in that case, as the
location of the object is deterministic and there can only be one copy.
But the packed source would inspect `u.packed.pack` and thus know which
specific object we refer to.

This still hinges on a couple intermediate steps, but I think with this
plan we should be able to handle this issue in a way where the caller
doesn't need to know _anything_ about how exactly the ODB source itself
works.

Thanks!

Patrick