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

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