Re: [PATCH 0/9] Split up "object-file.c"

[Patrick Steinhardt <ps@pks.im>]

On Wed, Apr 09, 2025 at 07:42:38AM -0700, Elijah Newren wrote:
> On Tue, Apr 8, 2025 at 3:35 AM Patrick Steinhardt <ps@pks.im> wrote:
> >
> > Hi,
> >
> > "object-file.c" is quite a grab-bag of all kinds of different functions.
> > Many of these functions aren't really a good fit though and should be
> > owned by a different subsystem. This patch series tries to split up
> > concerns a bit better by splitting out this functionality into other
> > files:
> >
> >   - `safe_create_leading_directories()` is moved into "dir.c".
> >   - `xmmap()` is moved into "wrapper.c".
> >   - `git_open_cloexec()` is moved into "compat/open.c".
> >   - Several functions attached to `struct index_state` are moved into
> >     "read-cache.c".
> >   - Several functions related to `struct object_store` are moved into a
> >     new file "object-store.c".
> >
> > "object-file.c" now mostly contains logic to read and write loose object
> > files, whereas "object-store.c" contains the higher-level logic to
> > manage different object directories for a repository. Eventually, these
> > will become the loose object backend as well as the `struct ref_store`
> > equivalent for objects, respectively.
> >
> > The series is built on top of 9d22ac51228 (The third batch, 2025-04-07)
> > with ps/object-wo-the-repository at 9442b1c919a (Merge remote-tracking
> > branch 'junio/ps/object-wo-the-repository' into HEAD, 2025-04-08) merged
> > into it.
> 
> I was able to apply with these instructions, and looking over the
> series locally with --color-moved was very helpful to verify that you
> were moving functions without modifying them (or, to notice where you
> did modify them and why, e.g. removing the static declaration).
> 
> I very much appreciate the grouping of functions by area that is done
> here with read-cache.c, wrapper.c, compat/open.c, dir.c, and the new
> object-store.c, as well as the new enums to make the purpose of the
> HASH_* flags clearer, and the reduction in the number of globals.
> 
> I only had a few comments on a couple of the patch commit messages.

Thanks for your review!

Patrick