Re: [PATCH 1/9] object-file: move `safe_create_leading_directories()` into "dir.c"

[Patrick Steinhardt <ps@pks.im>]

On Fri, Apr 11, 2025 at 10:11:52AM -0700, Elijah Newren wrote:
> On Fri, Apr 11, 2025 at 2:27 AM Patrick Steinhardt <ps@pks.im> wrote:
> >
> > On Wed, Apr 09, 2025 at 07:36:47AM -0700, Elijah Newren wrote:
> > > On Tue, Apr 8, 2025 at 3:37 AM Patrick Steinhardt <ps@pks.im> wrote:
> > > >
> > > > The `safe_create_leading_directories()` function and its relatives
> > >
> > > How is mkdir_in_gitdir() a relative of safe_create_leading_directories()?
> > >
> > > I assumed the relation was "called by", but there is no such
> > > relationship.  The rest of the patch looked fine, but I was puzzled
> > > for a while trying to figure out what this relationship is.
> >
> > It's more of a sibling than a child/parent in this case, true. I still
> > think it makes sense to move it around as it is rather generic in the
> > functionality it provides and doesn't have anything to do with objects.
> >
> > Patrick
> 
> I fully agree it makes sense to move it and that dir.c is a good place
> for it, I just think it also makes sense to fix the commit message to
> avoid the misleading/confusing text by calling out mkdir_in_gitdir()
> separately since it isn't related to
> safe_create_leading_directories().  For example, highlighting the text
> I added between asterisks, you could make it read:
> 
> The `safe_create_leading_directories()` function and its relatives*,
> as well as mkdir_in_gitdir()*, are
> located in "object-file.c", which is not a good fit as they provide
> generic functionality not related to objects at all. Move them into
> "dir.c".
> 
> However, this is a nitpick and probably not worth another re-roll;
> especially since everything else in your v2 looks great to me.

Eric has suggested moving it into "path.c", which I think is indeed a
better fix. I'm using that as an opportunity to rename the function to
`safe_create_dir_in_gitdir()` so that it matches `safe_create_dir()`,
which is functionally similar. And because "path.c" does not depend on
`the_repository` anymore I'll also inject a repository via a parameter.

All to say: there's a bunch of additional changes now, so I'll split
this out into a separate commit.

Patrick