| Message ID | 20250408-pks-split-object-file-v1-1-f1fd50191143@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Split up "object-file.c" | expand |
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. > 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". Makes sense. > > Signed-off-by: Patrick Steinhardt <ps@pks.im> > --- > builtin/bugreport.c | 2 +- > builtin/credential-cache--daemon.c | 2 +- > builtin/diagnose.c | 2 +- > builtin/fsck.c | 1 + > builtin/gc.c | 2 +- > builtin/init-db.c | 2 +- > builtin/log.c | 2 +- > commit-graph.c | 1 + > dir.c | 107 ++++++++++++++++++++++++++++++++++++- > dir.h | 35 ++++++++++++ > midx-write.c | 1 + > object-file.c | 106 ------------------------------------ > object-file.h | 35 ------------ > 13 files changed, 150 insertions(+), 148 deletions(-) > > diff --git a/builtin/bugreport.c b/builtin/bugreport.c > index 66d64bfd5ae..d07fa91c247 100644 > --- a/builtin/bugreport.c > +++ b/builtin/bugreport.c > @@ -1,6 +1,7 @@ > #define USE_THE_REPOSITORY_VARIABLE > #include "builtin.h" > #include "abspath.h" > +#include "dir.h" > #include "editor.h" > #include "gettext.h" > #include "parse-options.h" > @@ -10,7 +11,6 @@ > #include "hook.h" > #include "hook-list.h" > #include "diagnose.h" > -#include "object-file.h" > #include "setup.h" > #include "version.h" > > diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c > index e707618e743..80d29b4f5c0 100644 > --- a/builtin/credential-cache--daemon.c > +++ b/builtin/credential-cache--daemon.c > @@ -1,8 +1,8 @@ > #define USE_THE_REPOSITORY_VARIABLE > #include "builtin.h" > #include "abspath.h" > +#include "dir.h" > #include "gettext.h" > -#include "object-file.h" > #include "parse-options.h" > > #ifndef NO_UNIX_SOCKETS > diff --git a/builtin/diagnose.c b/builtin/diagnose.c > index 33c39bd5981..d5dadd6a48b 100644 > --- a/builtin/diagnose.c > +++ b/builtin/diagnose.c > @@ -2,8 +2,8 @@ > > #include "builtin.h" > #include "abspath.h" > +#include "dir.h" > #include "gettext.h" > -#include "object-file.h" > #include "parse-options.h" > #include "diagnose.h" > > diff --git a/builtin/fsck.c b/builtin/fsck.c > index 9c8a6d6a8df..32d40d8f9fc 100644 > --- a/builtin/fsck.c > +++ b/builtin/fsck.c > @@ -1,5 +1,6 @@ > #define USE_THE_REPOSITORY_VARIABLE > #include "builtin.h" > +#include "dir.h" > #include "gettext.h" > #include "hex.h" > #include "config.h" > diff --git a/builtin/gc.c b/builtin/gc.c > index 99431fd4674..b069629676c 100644 > --- a/builtin/gc.c > +++ b/builtin/gc.c > @@ -16,6 +16,7 @@ > #include "builtin.h" > #include "abspath.h" > #include "date.h" > +#include "dir.h" > #include "environment.h" > #include "hex.h" > #include "config.h" > @@ -28,7 +29,6 @@ > #include "commit.h" > #include "commit-graph.h" > #include "packfile.h" > -#include "object-file.h" > #include "object-store-ll.h" > #include "pack.h" > #include "pack-objects.h" > diff --git a/builtin/init-db.c b/builtin/init-db.c > index 196dccdd77a..39730c1b0ce 100644 > --- a/builtin/init-db.c > +++ b/builtin/init-db.c > @@ -6,9 +6,9 @@ > #define USE_THE_REPOSITORY_VARIABLE > #include "builtin.h" > #include "abspath.h" > +#include "dir.h" > #include "environment.h" > #include "gettext.h" > -#include "object-file.h" > #include "parse-options.h" > #include "path.h" > #include "refs.h" > diff --git a/builtin/log.c b/builtin/log.c > index 0d4c579dad7..06ffaa93e86 100644 > --- a/builtin/log.c > +++ b/builtin/log.c > @@ -10,11 +10,11 @@ > #include "builtin.h" > #include "abspath.h" > #include "config.h" > +#include "dir.h" > #include "environment.h" > #include "gettext.h" > #include "hex.h" > #include "refs.h" > -#include "object-file.h" > #include "object-name.h" > #include "object-store-ll.h" > #include "pager.h" > diff --git a/commit-graph.c b/commit-graph.c > index 8286d5dda24..3fae20dc21b 100644 > --- a/commit-graph.c > +++ b/commit-graph.c > @@ -4,6 +4,7 @@ > #include "git-compat-util.h" > #include "config.h" > #include "csum-file.h" > +#include "dir.h" > #include "gettext.h" > #include "hex.h" > #include "lockfile.h" > diff --git a/dir.c b/dir.c > index 28b0e03feb4..16ae3b5169d 100644 > --- a/dir.c > +++ b/dir.c > @@ -17,7 +17,6 @@ > #include "environment.h" > #include "gettext.h" > #include "name-hash.h" > -#include "object-file.h" > #include "object-store-ll.h" > #include "path.h" > #include "refs.h" > @@ -4132,3 +4131,109 @@ int path_match_flags(const char *const str, const enum path_match_flags flags) > return is_xplatform_dir_sep(*p); > BUG("unreachable"); > } > + > +int mkdir_in_gitdir(const char *path) > +{ > + if (mkdir(path, 0777)) { > + int saved_errno = errno; > + struct stat st; > + struct strbuf sb = STRBUF_INIT; > + > + if (errno != EEXIST) > + return -1; > + /* > + * Are we looking at a path in a symlinked worktree > + * whose original repository does not yet have it? > + * e.g. .git/rr-cache pointing at its original > + * repository in which the user hasn't performed any > + * conflict resolution yet? > + */ > + if (lstat(path, &st) || !S_ISLNK(st.st_mode) || > + strbuf_readlink(&sb, path, st.st_size) || > + !is_absolute_path(sb.buf) || > + mkdir(sb.buf, 0777)) { > + strbuf_release(&sb); > + errno = saved_errno; > + return -1; > + } > + strbuf_release(&sb); > + } > + return adjust_shared_perm(the_repository, path); > +} > + > +static enum scld_error safe_create_leading_directories_1(char *path, int share) > +{ > + char *next_component = path + offset_1st_component(path); > + enum scld_error ret = SCLD_OK; > + > + while (ret == SCLD_OK && next_component) { > + struct stat st; > + char *slash = next_component, slash_character; > + > + while (*slash && !is_dir_sep(*slash)) > + slash++; > + > + if (!*slash) > + break; > + > + next_component = slash + 1; > + while (is_dir_sep(*next_component)) > + next_component++; > + if (!*next_component) > + break; > + > + slash_character = *slash; > + *slash = '\0'; > + if (!stat(path, &st)) { > + /* path exists */ > + if (!S_ISDIR(st.st_mode)) { > + errno = ENOTDIR; > + ret = SCLD_EXISTS; > + } > + } else if (mkdir(path, 0777)) { > + if (errno == EEXIST && > + !stat(path, &st) && S_ISDIR(st.st_mode)) > + ; /* somebody created it since we checked */ > + else if (errno == ENOENT) > + /* > + * Either mkdir() failed because > + * somebody just pruned the containing > + * directory, or stat() failed because > + * the file that was in our way was > + * just removed. Either way, inform > + * the caller that it might be worth > + * trying again: > + */ > + ret = SCLD_VANISHED; > + else > + ret = SCLD_FAILED; > + } else if (share && adjust_shared_perm(the_repository, path)) { > + ret = SCLD_PERMS; > + } > + *slash = slash_character; > + } > + return ret; > +} > + > +enum scld_error safe_create_leading_directories(char *path) > +{ > + return safe_create_leading_directories_1(path, 1); > +} > + > +enum scld_error safe_create_leading_directories_no_share(char *path) > +{ > + return safe_create_leading_directories_1(path, 0); > +} > + > +enum scld_error safe_create_leading_directories_const(const char *path) > +{ > + int save_errno; > + /* path points to cache entries, so xstrdup before messing with it */ > + char *buf = xstrdup(path); > + enum scld_error result = safe_create_leading_directories(buf); > + > + save_errno = errno; > + free(buf); > + errno = save_errno; > + return result; > +} > diff --git a/dir.h b/dir.h > index d7e71aa8daa..02c1f9420b0 100644 > --- a/dir.h > +++ b/dir.h > @@ -676,4 +676,39 @@ static inline int starts_with_dot_dot_slash_native(const char *const path) > return path_match_flags(path, what | PATH_MATCH_NATIVE); > } > > +/* > + * Create the directory containing the named path, using care to be > + * somewhat safe against races. Return one of the scld_error values to > + * indicate success/failure. On error, set errno to describe the > + * problem. > + * > + * SCLD_VANISHED indicates that one of the ancestor directories of the > + * path existed at one point during the function call and then > + * suddenly vanished, probably because another process pruned the > + * directory while we were working. To be robust against this kind of > + * race, callers might want to try invoking the function again when it > + * returns SCLD_VANISHED. > + * > + * safe_create_leading_directories() temporarily changes path while it > + * is working but restores it before returning. > + * safe_create_leading_directories_const() doesn't modify path, even > + * temporarily. Both these variants adjust the permissions of the > + * created directories to honor core.sharedRepository, so they are best > + * suited for files inside the git dir. For working tree files, use > + * safe_create_leading_directories_no_share() instead, as it ignores > + * the core.sharedRepository setting. > + */ > +enum scld_error { > + SCLD_OK = 0, > + SCLD_FAILED = -1, > + SCLD_PERMS = -2, > + SCLD_EXISTS = -3, > + SCLD_VANISHED = -4 > +}; > +enum scld_error safe_create_leading_directories(char *path); > +enum scld_error safe_create_leading_directories_const(const char *path); > +enum scld_error safe_create_leading_directories_no_share(char *path); > + > +int mkdir_in_gitdir(const char *path); > + > #endif > diff --git a/midx-write.c b/midx-write.c > index a628ac24dcb..e01a867c583 100644 > --- a/midx-write.c > +++ b/midx-write.c > @@ -3,6 +3,7 @@ > #include "git-compat-util.h" > #include "abspath.h" > #include "config.h" > +#include "dir.h" > #include "hex.h" > #include "lockfile.h" > #include "packfile.h" > diff --git a/object-file.c b/object-file.c > index 772c311f188..23b2c8560be 100644 > --- a/object-file.c > +++ b/object-file.c > @@ -91,112 +91,6 @@ static int get_conv_flags(unsigned flags) > } > > > -int mkdir_in_gitdir(const char *path) > -{ > - if (mkdir(path, 0777)) { > - int saved_errno = errno; > - struct stat st; > - struct strbuf sb = STRBUF_INIT; > - > - if (errno != EEXIST) > - return -1; > - /* > - * Are we looking at a path in a symlinked worktree > - * whose original repository does not yet have it? > - * e.g. .git/rr-cache pointing at its original > - * repository in which the user hasn't performed any > - * conflict resolution yet? > - */ > - if (lstat(path, &st) || !S_ISLNK(st.st_mode) || > - strbuf_readlink(&sb, path, st.st_size) || > - !is_absolute_path(sb.buf) || > - mkdir(sb.buf, 0777)) { > - strbuf_release(&sb); > - errno = saved_errno; > - return -1; > - } > - strbuf_release(&sb); > - } > - return adjust_shared_perm(the_repository, path); > -} > - > -static enum scld_error safe_create_leading_directories_1(char *path, int share) > -{ > - char *next_component = path + offset_1st_component(path); > - enum scld_error ret = SCLD_OK; > - > - while (ret == SCLD_OK && next_component) { > - struct stat st; > - char *slash = next_component, slash_character; > - > - while (*slash && !is_dir_sep(*slash)) > - slash++; > - > - if (!*slash) > - break; > - > - next_component = slash + 1; > - while (is_dir_sep(*next_component)) > - next_component++; > - if (!*next_component) > - break; > - > - slash_character = *slash; > - *slash = '\0'; > - if (!stat(path, &st)) { > - /* path exists */ > - if (!S_ISDIR(st.st_mode)) { > - errno = ENOTDIR; > - ret = SCLD_EXISTS; > - } > - } else if (mkdir(path, 0777)) { > - if (errno == EEXIST && > - !stat(path, &st) && S_ISDIR(st.st_mode)) > - ; /* somebody created it since we checked */ > - else if (errno == ENOENT) > - /* > - * Either mkdir() failed because > - * somebody just pruned the containing > - * directory, or stat() failed because > - * the file that was in our way was > - * just removed. Either way, inform > - * the caller that it might be worth > - * trying again: > - */ > - ret = SCLD_VANISHED; > - else > - ret = SCLD_FAILED; > - } else if (share && adjust_shared_perm(the_repository, path)) { > - ret = SCLD_PERMS; > - } > - *slash = slash_character; > - } > - return ret; > -} > - > -enum scld_error safe_create_leading_directories(char *path) > -{ > - return safe_create_leading_directories_1(path, 1); > -} > - > -enum scld_error safe_create_leading_directories_no_share(char *path) > -{ > - return safe_create_leading_directories_1(path, 0); > -} > - > -enum scld_error safe_create_leading_directories_const(const char *path) > -{ > - int save_errno; > - /* path points to cache entries, so xstrdup before messing with it */ > - char *buf = xstrdup(path); > - enum scld_error result = safe_create_leading_directories(buf); > - > - save_errno = errno; > - free(buf); > - errno = save_errno; > - return result; > -} > - > int odb_mkstemp(struct strbuf *temp_filename, const char *pattern) > { > int fd; > diff --git a/object-file.h b/object-file.h > index 81b30d269c8..922f2bba8c9 100644 > --- a/object-file.h > +++ b/object-file.h > @@ -21,41 +21,6 @@ extern int fetch_if_missing; > int index_fd(struct index_state *istate, struct object_id *oid, int fd, struct stat *st, enum object_type type, const char *path, unsigned flags); > int index_path(struct index_state *istate, struct object_id *oid, const char *path, struct stat *st, unsigned flags); > > -/* > - * Create the directory containing the named path, using care to be > - * somewhat safe against races. Return one of the scld_error values to > - * indicate success/failure. On error, set errno to describe the > - * problem. > - * > - * SCLD_VANISHED indicates that one of the ancestor directories of the > - * path existed at one point during the function call and then > - * suddenly vanished, probably because another process pruned the > - * directory while we were working. To be robust against this kind of > - * race, callers might want to try invoking the function again when it > - * returns SCLD_VANISHED. > - * > - * safe_create_leading_directories() temporarily changes path while it > - * is working but restores it before returning. > - * safe_create_leading_directories_const() doesn't modify path, even > - * temporarily. Both these variants adjust the permissions of the > - * created directories to honor core.sharedRepository, so they are best > - * suited for files inside the git dir. For working tree files, use > - * safe_create_leading_directories_no_share() instead, as it ignores > - * the core.sharedRepository setting. > - */ > -enum scld_error { > - SCLD_OK = 0, > - SCLD_FAILED = -1, > - SCLD_PERMS = -2, > - SCLD_EXISTS = -3, > - SCLD_VANISHED = -4 > -}; > -enum scld_error safe_create_leading_directories(char *path); > -enum scld_error safe_create_leading_directories_const(const char *path); > -enum scld_error safe_create_leading_directories_no_share(char *path); > - > -int mkdir_in_gitdir(const char *path); > - > int git_open_cloexec(const char *name, int flags); > #define git_open(name) git_open_cloexec(name, O_RDONLY) > > > -- > 2.49.0.682.gc9b6a7b2b0.dirty > >
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
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.
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
Patrick Steinhardt <ps@pks.im> writes: > 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. Excellent. Thanks.
diff --git a/builtin/bugreport.c b/builtin/bugreport.c index 66d64bfd5ae..d07fa91c247 100644 --- a/builtin/bugreport.c +++ b/builtin/bugreport.c @@ -1,6 +1,7 @@ #define USE_THE_REPOSITORY_VARIABLE #include "builtin.h" #include "abspath.h" +#include "dir.h" #include "editor.h" #include "gettext.h" #include "parse-options.h" @@ -10,7 +11,6 @@ #include "hook.h" #include "hook-list.h" #include "diagnose.h" -#include "object-file.h" #include "setup.h" #include "version.h" diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c index e707618e743..80d29b4f5c0 100644 --- a/builtin/credential-cache--daemon.c +++ b/builtin/credential-cache--daemon.c @@ -1,8 +1,8 @@ #define USE_THE_REPOSITORY_VARIABLE #include "builtin.h" #include "abspath.h" +#include "dir.h" #include "gettext.h" -#include "object-file.h" #include "parse-options.h" #ifndef NO_UNIX_SOCKETS diff --git a/builtin/diagnose.c b/builtin/diagnose.c index 33c39bd5981..d5dadd6a48b 100644 --- a/builtin/diagnose.c +++ b/builtin/diagnose.c @@ -2,8 +2,8 @@ #include "builtin.h" #include "abspath.h" +#include "dir.h" #include "gettext.h" -#include "object-file.h" #include "parse-options.h" #include "diagnose.h" diff --git a/builtin/fsck.c b/builtin/fsck.c index 9c8a6d6a8df..32d40d8f9fc 100644 --- a/builtin/fsck.c +++ b/builtin/fsck.c @@ -1,5 +1,6 @@ #define USE_THE_REPOSITORY_VARIABLE #include "builtin.h" +#include "dir.h" #include "gettext.h" #include "hex.h" #include "config.h" diff --git a/builtin/gc.c b/builtin/gc.c index 99431fd4674..b069629676c 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -16,6 +16,7 @@ #include "builtin.h" #include "abspath.h" #include "date.h" +#include "dir.h" #include "environment.h" #include "hex.h" #include "config.h" @@ -28,7 +29,6 @@ #include "commit.h" #include "commit-graph.h" #include "packfile.h" -#include "object-file.h" #include "object-store-ll.h" #include "pack.h" #include "pack-objects.h" diff --git a/builtin/init-db.c b/builtin/init-db.c index 196dccdd77a..39730c1b0ce 100644 --- a/builtin/init-db.c +++ b/builtin/init-db.c @@ -6,9 +6,9 @@ #define USE_THE_REPOSITORY_VARIABLE #include "builtin.h" #include "abspath.h" +#include "dir.h" #include "environment.h" #include "gettext.h" -#include "object-file.h" #include "parse-options.h" #include "path.h" #include "refs.h" diff --git a/builtin/log.c b/builtin/log.c index 0d4c579dad7..06ffaa93e86 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -10,11 +10,11 @@ #include "builtin.h" #include "abspath.h" #include "config.h" +#include "dir.h" #include "environment.h" #include "gettext.h" #include "hex.h" #include "refs.h" -#include "object-file.h" #include "object-name.h" #include "object-store-ll.h" #include "pager.h" diff --git a/commit-graph.c b/commit-graph.c index 8286d5dda24..3fae20dc21b 100644 --- a/commit-graph.c +++ b/commit-graph.c @@ -4,6 +4,7 @@ #include "git-compat-util.h" #include "config.h" #include "csum-file.h" +#include "dir.h" #include "gettext.h" #include "hex.h" #include "lockfile.h" diff --git a/dir.c b/dir.c index 28b0e03feb4..16ae3b5169d 100644 --- a/dir.c +++ b/dir.c @@ -17,7 +17,6 @@ #include "environment.h" #include "gettext.h" #include "name-hash.h" -#include "object-file.h" #include "object-store-ll.h" #include "path.h" #include "refs.h" @@ -4132,3 +4131,109 @@ int path_match_flags(const char *const str, const enum path_match_flags flags) return is_xplatform_dir_sep(*p); BUG("unreachable"); } + +int mkdir_in_gitdir(const char *path) +{ + if (mkdir(path, 0777)) { + int saved_errno = errno; + struct stat st; + struct strbuf sb = STRBUF_INIT; + + if (errno != EEXIST) + return -1; + /* + * Are we looking at a path in a symlinked worktree + * whose original repository does not yet have it? + * e.g. .git/rr-cache pointing at its original + * repository in which the user hasn't performed any + * conflict resolution yet? + */ + if (lstat(path, &st) || !S_ISLNK(st.st_mode) || + strbuf_readlink(&sb, path, st.st_size) || + !is_absolute_path(sb.buf) || + mkdir(sb.buf, 0777)) { + strbuf_release(&sb); + errno = saved_errno; + return -1; + } + strbuf_release(&sb); + } + return adjust_shared_perm(the_repository, path); +} + +static enum scld_error safe_create_leading_directories_1(char *path, int share) +{ + char *next_component = path + offset_1st_component(path); + enum scld_error ret = SCLD_OK; + + while (ret == SCLD_OK && next_component) { + struct stat st; + char *slash = next_component, slash_character; + + while (*slash && !is_dir_sep(*slash)) + slash++; + + if (!*slash) + break; + + next_component = slash + 1; + while (is_dir_sep(*next_component)) + next_component++; + if (!*next_component) + break; + + slash_character = *slash; + *slash = '\0'; + if (!stat(path, &st)) { + /* path exists */ + if (!S_ISDIR(st.st_mode)) { + errno = ENOTDIR; + ret = SCLD_EXISTS; + } + } else if (mkdir(path, 0777)) { + if (errno == EEXIST && + !stat(path, &st) && S_ISDIR(st.st_mode)) + ; /* somebody created it since we checked */ + else if (errno == ENOENT) + /* + * Either mkdir() failed because + * somebody just pruned the containing + * directory, or stat() failed because + * the file that was in our way was + * just removed. Either way, inform + * the caller that it might be worth + * trying again: + */ + ret = SCLD_VANISHED; + else + ret = SCLD_FAILED; + } else if (share && adjust_shared_perm(the_repository, path)) { + ret = SCLD_PERMS; + } + *slash = slash_character; + } + return ret; +} + +enum scld_error safe_create_leading_directories(char *path) +{ + return safe_create_leading_directories_1(path, 1); +} + +enum scld_error safe_create_leading_directories_no_share(char *path) +{ + return safe_create_leading_directories_1(path, 0); +} + +enum scld_error safe_create_leading_directories_const(const char *path) +{ + int save_errno; + /* path points to cache entries, so xstrdup before messing with it */ + char *buf = xstrdup(path); + enum scld_error result = safe_create_leading_directories(buf); + + save_errno = errno; + free(buf); + errno = save_errno; + return result; +} diff --git a/dir.h b/dir.h index d7e71aa8daa..02c1f9420b0 100644 --- a/dir.h +++ b/dir.h @@ -676,4 +676,39 @@ static inline int starts_with_dot_dot_slash_native(const char *const path) return path_match_flags(path, what | PATH_MATCH_NATIVE); } +/* + * Create the directory containing the named path, using care to be + * somewhat safe against races. Return one of the scld_error values to + * indicate success/failure. On error, set errno to describe the + * problem. + * + * SCLD_VANISHED indicates that one of the ancestor directories of the + * path existed at one point during the function call and then + * suddenly vanished, probably because another process pruned the + * directory while we were working. To be robust against this kind of + * race, callers might want to try invoking the function again when it + * returns SCLD_VANISHED. + * + * safe_create_leading_directories() temporarily changes path while it + * is working but restores it before returning. + * safe_create_leading_directories_const() doesn't modify path, even + * temporarily. Both these variants adjust the permissions of the + * created directories to honor core.sharedRepository, so they are best + * suited for files inside the git dir. For working tree files, use + * safe_create_leading_directories_no_share() instead, as it ignores + * the core.sharedRepository setting. + */ +enum scld_error { + SCLD_OK = 0, + SCLD_FAILED = -1, + SCLD_PERMS = -2, + SCLD_EXISTS = -3, + SCLD_VANISHED = -4 +}; +enum scld_error safe_create_leading_directories(char *path); +enum scld_error safe_create_leading_directories_const(const char *path); +enum scld_error safe_create_leading_directories_no_share(char *path); + +int mkdir_in_gitdir(const char *path); + #endif diff --git a/midx-write.c b/midx-write.c index a628ac24dcb..e01a867c583 100644 --- a/midx-write.c +++ b/midx-write.c @@ -3,6 +3,7 @@ #include "git-compat-util.h" #include "abspath.h" #include "config.h" +#include "dir.h" #include "hex.h" #include "lockfile.h" #include "packfile.h" diff --git a/object-file.c b/object-file.c index 772c311f188..23b2c8560be 100644 --- a/object-file.c +++ b/object-file.c @@ -91,112 +91,6 @@ static int get_conv_flags(unsigned flags) } -int mkdir_in_gitdir(const char *path) -{ - if (mkdir(path, 0777)) { - int saved_errno = errno; - struct stat st; - struct strbuf sb = STRBUF_INIT; - - if (errno != EEXIST) - return -1; - /* - * Are we looking at a path in a symlinked worktree - * whose original repository does not yet have it? - * e.g. .git/rr-cache pointing at its original - * repository in which the user hasn't performed any - * conflict resolution yet? - */ - if (lstat(path, &st) || !S_ISLNK(st.st_mode) || - strbuf_readlink(&sb, path, st.st_size) || - !is_absolute_path(sb.buf) || - mkdir(sb.buf, 0777)) { - strbuf_release(&sb); - errno = saved_errno; - return -1; - } - strbuf_release(&sb); - } - return adjust_shared_perm(the_repository, path); -} - -static enum scld_error safe_create_leading_directories_1(char *path, int share) -{ - char *next_component = path + offset_1st_component(path); - enum scld_error ret = SCLD_OK; - - while (ret == SCLD_OK && next_component) { - struct stat st; - char *slash = next_component, slash_character; - - while (*slash && !is_dir_sep(*slash)) - slash++; - - if (!*slash) - break; - - next_component = slash + 1; - while (is_dir_sep(*next_component)) - next_component++; - if (!*next_component) - break; - - slash_character = *slash; - *slash = '\0'; - if (!stat(path, &st)) { - /* path exists */ - if (!S_ISDIR(st.st_mode)) { - errno = ENOTDIR; - ret = SCLD_EXISTS; - } - } else if (mkdir(path, 0777)) { - if (errno == EEXIST && - !stat(path, &st) && S_ISDIR(st.st_mode)) - ; /* somebody created it since we checked */ - else if (errno == ENOENT) - /* - * Either mkdir() failed because - * somebody just pruned the containing - * directory, or stat() failed because - * the file that was in our way was - * just removed. Either way, inform - * the caller that it might be worth - * trying again: - */ - ret = SCLD_VANISHED; - else - ret = SCLD_FAILED; - } else if (share && adjust_shared_perm(the_repository, path)) { - ret = SCLD_PERMS; - } - *slash = slash_character; - } - return ret; -} - -enum scld_error safe_create_leading_directories(char *path) -{ - return safe_create_leading_directories_1(path, 1); -} - -enum scld_error safe_create_leading_directories_no_share(char *path) -{ - return safe_create_leading_directories_1(path, 0); -} - -enum scld_error safe_create_leading_directories_const(const char *path) -{ - int save_errno; - /* path points to cache entries, so xstrdup before messing with it */ - char *buf = xstrdup(path); - enum scld_error result = safe_create_leading_directories(buf); - - save_errno = errno; - free(buf); - errno = save_errno; - return result; -} - int odb_mkstemp(struct strbuf *temp_filename, const char *pattern) { int fd; diff --git a/object-file.h b/object-file.h index 81b30d269c8..922f2bba8c9 100644 --- a/object-file.h +++ b/object-file.h @@ -21,41 +21,6 @@ extern int fetch_if_missing; int index_fd(struct index_state *istate, struct object_id *oid, int fd, struct stat *st, enum object_type type, const char *path, unsigned flags); int index_path(struct index_state *istate, struct object_id *oid, const char *path, struct stat *st, unsigned flags); -/* - * Create the directory containing the named path, using care to be - * somewhat safe against races. Return one of the scld_error values to - * indicate success/failure. On error, set errno to describe the - * problem. - * - * SCLD_VANISHED indicates that one of the ancestor directories of the - * path existed at one point during the function call and then - * suddenly vanished, probably because another process pruned the - * directory while we were working. To be robust against this kind of - * race, callers might want to try invoking the function again when it - * returns SCLD_VANISHED. - * - * safe_create_leading_directories() temporarily changes path while it - * is working but restores it before returning. - * safe_create_leading_directories_const() doesn't modify path, even - * temporarily. Both these variants adjust the permissions of the - * created directories to honor core.sharedRepository, so they are best - * suited for files inside the git dir. For working tree files, use - * safe_create_leading_directories_no_share() instead, as it ignores - * the core.sharedRepository setting. - */ -enum scld_error { - SCLD_OK = 0, - SCLD_FAILED = -1, - SCLD_PERMS = -2, - SCLD_EXISTS = -3, - SCLD_VANISHED = -4 -}; -enum scld_error safe_create_leading_directories(char *path); -enum scld_error safe_create_leading_directories_const(const char *path); -enum scld_error safe_create_leading_directories_no_share(char *path); - -int mkdir_in_gitdir(const char *path); - int git_open_cloexec(const char *name, int flags); #define git_open(name) git_open_cloexec(name, O_RDONLY)
The `safe_create_leading_directories()` function and its relatives 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". Signed-off-by: Patrick Steinhardt <ps@pks.im> --- builtin/bugreport.c | 2 +- builtin/credential-cache--daemon.c | 2 +- builtin/diagnose.c | 2 +- builtin/fsck.c | 1 + builtin/gc.c | 2 +- builtin/init-db.c | 2 +- builtin/log.c | 2 +- commit-graph.c | 1 + dir.c | 107 ++++++++++++++++++++++++++++++++++++- dir.h | 35 ++++++++++++ midx-write.c | 1 + object-file.c | 106 ------------------------------------ object-file.h | 35 ------------ 13 files changed, 150 insertions(+), 148 deletions(-)