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@xxxxxx> 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@xxxxxx> 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
