From 3c42e4ef7428de18784e04c9cac18de3613144cb Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Tue, 26 Feb 2013 11:43:14 -0800 Subject: [PATCH 1/3] Fix initialization of repo directories When PR #1359 removed the hooks from the test resources/template directory, it made me realize that the tests for git_repository_init_ext using templates must be pretty shabby because we could not have been testing if the hooks were getting created correctly. So, this started with me recreating a couple of hooks, including a sample and symlink, and adding tests that they got created correctly in the various circumstances, including with the SHARED modes, etc. Unfortunately this uncovered some issues with how directories and symlinks were copied and chmod'ed. Also, there was a FIXME in the code related to the chmod behavior as well. Going back over the directory creation logic for setting up a repository, I found it was a little difficult to read and could result in creating and/or chmod'ing directories that the user almost certainly didn't intend. So that let to this work which makes repo initialization much more careful (and hopefully easier to follow). It required a couple of extensions / changes to core fileops utilities, but I also think those are for the better, at least for git_futils_cp_r in terms of being careful about what actions it takes. --- src/fileops.c | 119 ++++++++++++------ src/fileops.h | 14 ++- src/repository.c | 96 +++++++++----- tests-clar/repo/init.c | 117 +++++++++++++++-- .../resources/template/hooks/link.sample | 1 + .../resources/template/hooks/update.sample | 9 ++ 6 files changed, 276 insertions(+), 80 deletions(-) create mode 120000 tests-clar/resources/template/hooks/link.sample create mode 100755 tests-clar/resources/template/hooks/update.sample diff --git a/src/fileops.c b/src/fileops.c index 3531e75b8..2b2db4b37 100644 --- a/src/fileops.c +++ b/src/fileops.c @@ -272,6 +272,10 @@ int git_futils_mkdir( } /* if we are not supposed to made the last element, truncate it */ + if ((flags & GIT_MKDIR_SKIP_LAST2) != 0) { + git_buf_rtruncate_at_char(&make_path, '/'); + flags |= GIT_MKDIR_SKIP_LAST; + } if ((flags & GIT_MKDIR_SKIP_LAST) != 0) git_buf_rtruncate_at_char(&make_path, '/'); @@ -303,34 +307,34 @@ int git_futils_mkdir( int already_exists = 0; switch (errno) { - case EEXIST: - if (!lastch && (flags & GIT_MKDIR_VERIFY_DIR) != 0 && - !git_path_isdir(make_path.ptr)) { - giterr_set( - GITERR_OS, "Existing path is not a directory '%s'", - make_path.ptr); - error = GIT_ENOTFOUND; - goto fail; - } + case EEXIST: + if (!lastch && (flags & GIT_MKDIR_VERIFY_DIR) != 0 && + !git_path_isdir(make_path.ptr)) { + giterr_set( + GITERR_OS, "Existing path is not a directory '%s'", + make_path.ptr); + error = GIT_ENOTFOUND; + goto fail; + } + already_exists = 1; + break; + case ENOSYS: + /* Solaris can generate this error if you try to mkdir + * a path which is already a mount point. In that case, + * the path does already exist; but it's not implied by + * the definition of the error, so let's recheck */ + if (git_path_isdir(make_path.ptr)) { already_exists = 1; break; - case ENOSYS: - /* Solaris can generate this error if you try to mkdir - * a path which is already a mount point. In that case, - * the path does already exist; but it's not implied by - * the definition of the error, so let's recheck */ - if (git_path_isdir(make_path.ptr)) { - already_exists = 1; - break; - } + } - /* Fall through */ - errno = ENOSYS; - default: - giterr_set(GITERR_OS, "Failed to make directory '%s'", - make_path.ptr); - goto fail; + /* Fall through */ + errno = ENOSYS; + default: + giterr_set(GITERR_OS, "Failed to make directory '%s'", + make_path.ptr); + goto fail; } if (already_exists && (flags & GIT_MKDIR_EXCL) != 0) { @@ -679,8 +683,33 @@ typedef struct { mode_t dirmode; } cp_r_info; +#define GIT_CPDIR__MKDIR_DONE_FOR_TO_ROOT (1u << 10) + +static int _cp_r_mkdir(cp_r_info *info, git_buf *from) +{ + int error = 0; + + /* create root directory the first time we need to create a directory */ + if ((info->flags & GIT_CPDIR__MKDIR_DONE_FOR_TO_ROOT) == 0) { + error = git_futils_mkdir( + info->to_root, NULL, info->dirmode, + ((info->flags & GIT_CPDIR_CHMOD) != 0) ? GIT_MKDIR_CHMOD : 0); + + info->flags |= GIT_CPDIR__MKDIR_DONE_FOR_TO_ROOT; + } + + /* create directory with root as base to prevent excess chmods */ + if (!error) + error = git_futils_mkdir( + from->ptr + info->from_prefix, info->to_root, + info->dirmode, info->mkdir_flags); + + return error; +} + static int _cp_r_callback(void *ref, git_buf *from) { + int error = 0; cp_r_info *info = ref; struct stat from_st, to_st; bool exists = false; @@ -702,11 +731,10 @@ static int _cp_r_callback(void *ref, git_buf *from) } else exists = true; - if (git_path_lstat(from->ptr, &from_st) < 0) - return -1; + if ((error = git_path_lstat(from->ptr, &from_st)) < 0) + return error; if (S_ISDIR(from_st.st_mode)) { - int error = 0; mode_t oldmode = info->dirmode; /* if we are not chmod'ing, then overwrite dirmode */ @@ -715,11 +743,10 @@ static int _cp_r_callback(void *ref, git_buf *from) /* make directory now if CREATE_EMPTY_DIRS is requested and needed */ if (!exists && (info->flags & GIT_CPDIR_CREATE_EMPTY_DIRS) != 0) - error = git_futils_mkdir( - info->to.ptr, NULL, info->dirmode, info->mkdir_flags); + error = _cp_r_mkdir(info, from); /* recurse onto target directory */ - if (!exists || S_ISDIR(to_st.st_mode)) + if (!error && (!exists || S_ISDIR(to_st.st_mode))) error = git_path_direach(from, _cp_r_callback, info); if (oldmode != 0) @@ -747,15 +774,29 @@ static int _cp_r_callback(void *ref, git_buf *from) /* Make container directory on demand if needed */ if ((info->flags & GIT_CPDIR_CREATE_EMPTY_DIRS) == 0 && - git_futils_mkdir( - info->to.ptr, NULL, info->dirmode, info->mkdir_flags) < 0) - return -1; + (error = _cp_r_mkdir(info, from)) < 0) + return error; /* make symlink or regular file */ if (S_ISLNK(from_st.st_mode)) - return cp_link(from->ptr, info->to.ptr, (size_t)from_st.st_size); - else - return git_futils_cp(from->ptr, info->to.ptr, from_st.st_mode); + error = cp_link(from->ptr, info->to.ptr, (size_t)from_st.st_size); + else { + mode_t usemode = from_st.st_mode; + + /* if chmod'ing, then blend dirmode & from_st bits */ + if ((info->flags & GIT_CPDIR_CHMOD) != 0) + usemode = (info->dirmode & 0666) | (usemode & ~0666); + + error = git_futils_cp(from->ptr, info->to.ptr, usemode); + + if (!error && + (info->flags & GIT_CPDIR_CHMOD) != 0 && + (error = p_chmod(info->to.ptr, usemode)) < 0) + giterr_set(GITERR_OS, "Failed to set permissions on '%s'", + info->to.ptr); + } + + return error; } int git_futils_cp_r( @@ -768,7 +809,7 @@ int git_futils_cp_r( git_buf path = GIT_BUF_INIT; cp_r_info info; - if (git_buf_sets(&path, from) < 0) + if (git_buf_joinpath(&path, from, "") < 0) /* ensure trailing slash */ return -1; info.to_root = to; @@ -779,10 +820,14 @@ int git_futils_cp_r( /* precalculate mkdir flags */ if ((flags & GIT_CPDIR_CREATE_EMPTY_DIRS) == 0) { + /* if not creating empty dirs, then use mkdir to create the path on + * demand right before files are copied. + */ info.mkdir_flags = GIT_MKDIR_PATH | GIT_MKDIR_SKIP_LAST; if ((flags & GIT_CPDIR_CHMOD) != 0) info.mkdir_flags |= GIT_MKDIR_CHMOD_PATH; } else { + /* otherwise, we will do simple mkdir as directories are encountered */ info.mkdir_flags = ((flags & GIT_CPDIR_CHMOD) != 0) ? GIT_MKDIR_CHMOD : 0; } diff --git a/src/fileops.h b/src/fileops.h index 5495b12cd..9e1f7440e 100644 --- a/src/fileops.h +++ b/src/fileops.h @@ -65,6 +65,7 @@ extern int git_futils_mkdir_r(const char *path, const char *base, const mode_t m * * GIT_MKDIR_CHMOD says to chmod the final directory entry after creation * * GIT_MKDIR_CHMOD_PATH says to chmod each directory component in the path * * GIT_MKDIR_SKIP_LAST says to leave off the last element of the path + * * GIT_MKDIR_SKIP_LAST2 says to leave off the last 2 elements of the path * * GIT_MKDIR_VERIFY_DIR says confirm final item is a dir, not just EEXIST * * Note that the chmod options will be executed even if the directory already @@ -76,7 +77,8 @@ typedef enum { GIT_MKDIR_CHMOD = 4, GIT_MKDIR_CHMOD_PATH = 8, GIT_MKDIR_SKIP_LAST = 16, - GIT_MKDIR_VERIFY_DIR = 32, + GIT_MKDIR_SKIP_LAST2 = 32, + GIT_MKDIR_VERIFY_DIR = 64, } git_futils_mkdir_flags; /** @@ -162,11 +164,11 @@ extern int git_futils_cp( * Flags that can be passed to `git_futils_cp_r`. */ typedef enum { - GIT_CPDIR_CREATE_EMPTY_DIRS = 1, - GIT_CPDIR_COPY_SYMLINKS = 2, - GIT_CPDIR_COPY_DOTFILES = 4, - GIT_CPDIR_OVERWRITE = 8, - GIT_CPDIR_CHMOD = 16 + GIT_CPDIR_CREATE_EMPTY_DIRS = (1u << 0), + GIT_CPDIR_COPY_SYMLINKS = (1u << 1), + GIT_CPDIR_COPY_DOTFILES = (1u << 2), + GIT_CPDIR_OVERWRITE = (1u << 3), + GIT_CPDIR_CHMOD = (1u << 4), } git_futils_cpdir_flags; /** diff --git a/src/repository.c b/src/repository.c index 014b40aff..5279d8f63 100644 --- a/src/repository.c +++ b/src/repository.c @@ -1005,17 +1005,8 @@ static int repo_init_structure( tdir = GIT_TEMPLATE_DIR; } - /* FIXME: GIT_CPDIR_CHMOD cannot applied here as an attempt - * would be made to chmod() all directories up to the last - * component of repo_dir, e.g., also /home etc. Recall that - * repo_dir is prettified at this point. - * - * Best probably would be to have the same logic as in - * git_futils_mkdir(), i.e., to separate the base from - * the path. - */ error = git_futils_cp_r(tdir, repo_dir, - GIT_CPDIR_COPY_SYMLINKS /*| GIT_CPDIR_CHMOD*/, dmode); + GIT_CPDIR_COPY_SYMLINKS | GIT_CPDIR_CHMOD, dmode); if (error < 0) { if (strcmp(tdir, GIT_TEMPLATE_DIR) != 0) @@ -1050,6 +1041,14 @@ static int repo_init_structure( return error; } +static int mkdir_parent(git_buf *buf, uint32_t mode, bool skip2) +{ + return git_futils_mkdir( + buf->ptr, NULL, mode & ~(S_ISGID | S_IWOTH), + GIT_MKDIR_PATH | GIT_MKDIR_VERIFY_DIR | + (skip2 ? GIT_MKDIR_SKIP_LAST2 : GIT_MKDIR_SKIP_LAST)); +} + static int repo_init_directories( git_buf *repo_path, git_buf *wd_path, @@ -1057,14 +1056,36 @@ static int repo_init_directories( git_repository_init_options *opts) { int error = 0; - bool add_dotgit, has_dotgit, natural_wd; + bool is_bare, add_dotgit, has_dotgit, natural_wd; mode_t dirmode; + /* There are three possible rules for what we are allowed to create: + * - MKPATH means anything we need + * - MKDIR means just the .git directory and its parent and the workdir + * - Neither means only the .git directory can be created + * + * There are 5 "segments" of path that we might need to deal with: + * 1. The .git directory + * 2. The parent of the .git directory + * 3. Everything above the parent of the .git directory + * 4. The working directory (often the same as #2) + * 5. Everything above the working directory (often the same as #3) + * + * For all directories created, we start with the init_mode value for + * permissions and then strip off bits in some cases: + * + * For MKPATH, we create #3 (and #5) paths without S_ISGID or S_IWOTH + * For MKPATH and MKDIR, we create #2 (and #4) without S_ISGID + * For all rules, we create #1 using the untouched init_mode + */ + /* set up repo path */ + is_bare = ((opts->flags & GIT_REPOSITORY_INIT_BARE) != 0); + add_dotgit = (opts->flags & GIT_REPOSITORY_INIT_NO_DOTGIT_DIR) == 0 && - (opts->flags & GIT_REPOSITORY_INIT_BARE) == 0 && + !is_bare && git__suffixcmp(given_repo, "/" DOT_GIT) != 0 && git__suffixcmp(given_repo, "/" GIT_DIR) != 0; @@ -1077,7 +1098,7 @@ static int repo_init_directories( /* set up workdir path */ - if ((opts->flags & GIT_REPOSITORY_INIT_BARE) == 0) { + if (!is_bare) { if (opts->workdir_path) { if (git_path_join_unrooted( wd_path, opts->workdir_path, repo_path->ptr, NULL) < 0) @@ -1109,30 +1130,47 @@ static int repo_init_directories( dirmode = pick_dir_mode(opts); - if ((opts->flags & GIT_REPOSITORY_INIT_MKDIR) != 0 && has_dotgit) { - git_buf p = GIT_BUF_INIT; - if ((error = git_path_dirname_r(&p, repo_path->ptr)) >= 0) - error = git_futils_mkdir(p.ptr, NULL, dirmode, 0); - git_buf_free(&p); + if ((opts->flags & GIT_REPOSITORY_INIT_MKPATH) != 0) { + /* create path #5 */ + if (wd_path->size > 0 && + (error = mkdir_parent(wd_path, dirmode, false)) < 0) + return error; + + /* create path #3 (if not the same as #5) */ + if (!natural_wd && + (error = mkdir_parent(repo_path, dirmode, has_dotgit)) < 0) + return error; + } + + if ((opts->flags & GIT_REPOSITORY_INIT_MKDIR) != 0 || + (opts->flags & GIT_REPOSITORY_INIT_MKPATH) != 0) + { + /* create path #4 */ + if (wd_path->size > 0 && + (error = git_futils_mkdir( + wd_path->ptr, NULL, dirmode & ~S_ISGID, + GIT_MKDIR_VERIFY_DIR)) < 0) + return error; + + /* create path #2 (if not the same as #4) */ + if (!natural_wd && + (error = git_futils_mkdir( + repo_path->ptr, NULL, dirmode & ~S_ISGID, + GIT_MKDIR_VERIFY_DIR | GIT_MKDIR_SKIP_LAST)) < 0) + return error; } if ((opts->flags & GIT_REPOSITORY_INIT_MKDIR) != 0 || (opts->flags & GIT_REPOSITORY_INIT_MKPATH) != 0 || has_dotgit) { - uint32_t mkflag = GIT_MKDIR_CHMOD; - if ((opts->flags & GIT_REPOSITORY_INIT_MKPATH) != 0) - mkflag |= GIT_MKDIR_PATH; - error = git_futils_mkdir(repo_path->ptr, NULL, dirmode, mkflag); + /* create path #1 */ + if ((error = git_futils_mkdir( + repo_path->ptr, NULL, dirmode, + GIT_MKDIR_VERIFY_DIR | GIT_MKDIR_CHMOD)) < 0) + return error; } - if (wd_path->size > 0 && - !natural_wd && - ((opts->flags & GIT_REPOSITORY_INIT_MKDIR) != 0 || - (opts->flags & GIT_REPOSITORY_INIT_MKPATH) != 0)) - error = git_futils_mkdir(wd_path->ptr, NULL, dirmode & ~S_ISGID, - (opts->flags & GIT_REPOSITORY_INIT_MKPATH) ? GIT_MKDIR_PATH : 0); - /* prettify both directories now that they are created */ if (!error) { diff --git a/tests-clar/repo/init.c b/tests-clar/repo/init.c index 97a5ff62a..c69ea76ca 100644 --- a/tests-clar/repo/init.c +++ b/tests-clar/repo/init.c @@ -9,7 +9,7 @@ enum repo_mode { BARE_REPOSITORY = 1 }; -static git_repository *_repo; +static git_repository *_repo = NULL; void test_repo_init__initialize(void) { @@ -363,36 +363,138 @@ void test_repo_init__extended_1(void) cl_fixture_cleanup("root"); } +static void assert_hooks_match( + const char *template_dir, + const char *repo_dir, + const char *hook_path, + uint32_t expected_mode) +{ + git_buf expected = GIT_BUF_INIT; + git_buf actual = GIT_BUF_INIT; + struct stat expected_st, st; + + cl_git_pass(git_buf_joinpath(&expected, template_dir, hook_path)); + cl_git_pass(git_path_lstat(expected.ptr, &expected_st)); + + cl_git_pass(git_buf_joinpath(&actual, repo_dir, hook_path)); + cl_git_pass(git_path_lstat(actual.ptr, &st)); + + cl_assert(expected_st.st_size == st.st_size); + + if (!expected_mode) + expected_mode = (uint32_t)expected_st.st_mode; + + cl_assert_equal_i((int)expected_mode, (int)st.st_mode); + + git_buf_free(&expected); + git_buf_free(&actual); +} + +static void assert_has_mode( + const char *base, const char *path, uint32_t expected) +{ + git_buf full = GIT_BUF_INIT; + struct stat st; + + cl_git_pass(git_buf_joinpath(&full, base, path)); + cl_git_pass(git_path_lstat(full.ptr, &st)); + git_buf_free(&full); + + cl_assert_equal_i((int)expected, (int)st.st_mode); +} + void test_repo_init__extended_with_template(void) { git_buf expected = GIT_BUF_INIT; git_buf actual = GIT_BUF_INIT; - git_repository_init_options opts = GIT_REPOSITORY_INIT_OPTIONS_INIT; - opts.flags = GIT_REPOSITORY_INIT_MKPATH | GIT_REPOSITORY_INIT_BARE | GIT_REPOSITORY_INIT_EXTERNAL_TEMPLATE; + cl_set_cleanup(&cleanup_repository, "templated.git"); + + opts.flags = GIT_REPOSITORY_INIT_MKPATH | GIT_REPOSITORY_INIT_BARE | + GIT_REPOSITORY_INIT_EXTERNAL_TEMPLATE; opts.template_path = cl_fixture("template"); cl_git_pass(git_repository_init_ext(&_repo, "templated.git", &opts)); cl_assert(git_repository_is_bare(_repo)); + cl_assert(!git__suffixcmp(git_repository_path(_repo), "/templated.git/")); - cl_assert(git_futils_readbuffer(&expected,cl_fixture("template/description")) == GIT_OK); - cl_assert(git_futils_readbuffer(&actual,"templated.git/description") == GIT_OK); + cl_git_pass(git_futils_readbuffer( + &expected, cl_fixture("template/description"))); + cl_git_pass(git_futils_readbuffer( + &actual, "templated.git/description")); - cl_assert(!git_buf_cmp(&expected,&actual)); + cl_assert_equal_s(expected.ptr, actual.ptr); git_buf_free(&expected); git_buf_free(&actual); - cleanup_repository("templated.git"); + assert_hooks_match( + cl_fixture("template"), git_repository_path(_repo), + "hooks/update.sample", 0); + + assert_hooks_match( + cl_fixture("template"), git_repository_path(_repo), + "hooks/link.sample", 0); +} + +void test_repo_init__extended_with_template_and_shared_mode(void) +{ + git_buf expected = GIT_BUF_INIT; + git_buf actual = GIT_BUF_INIT; + git_repository_init_options opts = GIT_REPOSITORY_INIT_OPTIONS_INIT; + + cl_set_cleanup(&cleanup_repository, "init_shared_from_tpl"); + + opts.flags = GIT_REPOSITORY_INIT_MKPATH | + GIT_REPOSITORY_INIT_EXTERNAL_TEMPLATE; + opts.template_path = cl_fixture("template"); + opts.mode = GIT_REPOSITORY_INIT_SHARED_GROUP; + + cl_git_pass(git_repository_init_ext(&_repo, "init_shared_from_tpl", &opts)); + + cl_assert(!git_repository_is_bare(_repo)); + cl_assert(!git__suffixcmp(git_repository_path(_repo), "/init_shared_from_tpl/.git/")); + + cl_git_pass(git_futils_readbuffer( + &expected, cl_fixture("template/description"))); + cl_git_pass(git_futils_readbuffer( + &actual, "init_shared_from_tpl/.git/description")); + + cl_assert_equal_s(expected.ptr, actual.ptr); + + git_buf_free(&expected); + git_buf_free(&actual); + + assert_has_mode(git_repository_path(_repo), "hooks", + GIT_REPOSITORY_INIT_SHARED_GROUP | S_IFDIR); + assert_has_mode(git_repository_path(_repo), "info", + GIT_REPOSITORY_INIT_SHARED_GROUP | S_IFDIR); + assert_has_mode(git_repository_path(_repo), "description", + (GIT_REPOSITORY_INIT_SHARED_GROUP | S_IFREG) & ~(S_ISGID | 0111)); + + /* for a non-symlinked hook, it should have shared permissions now */ + assert_hooks_match( + cl_fixture("template"), git_repository_path(_repo), + "hooks/update.sample", + (GIT_REPOSITORY_INIT_SHARED_GROUP | S_IFREG) & ~S_ISGID); + + /* for a symlinked hook, the permissions still should match the + * source link, not the GIT_REPOSITORY_INIT_SHARED_GROUP value + */ + assert_hooks_match( + cl_fixture("template"), git_repository_path(_repo), + "hooks/link.sample", 0); } void test_repo_init__can_reinit_an_initialized_repository(void) { git_repository *reinit; + cl_set_cleanup(&cleanup_repository, "extended"); + cl_git_pass(git_futils_mkdir("extended", NULL, 0775, 0)); cl_git_pass(git_repository_init(&_repo, "extended", false)); @@ -401,5 +503,4 @@ void test_repo_init__can_reinit_an_initialized_repository(void) cl_assert_equal_s(git_repository_path(_repo), git_repository_path(reinit)); git_repository_free(reinit); - cleanup_repository("extended"); } diff --git a/tests-clar/resources/template/hooks/link.sample b/tests-clar/resources/template/hooks/link.sample new file mode 120000 index 000000000..771acc43b --- /dev/null +++ b/tests-clar/resources/template/hooks/link.sample @@ -0,0 +1 @@ +update.sample \ No newline at end of file diff --git a/tests-clar/resources/template/hooks/update.sample b/tests-clar/resources/template/hooks/update.sample new file mode 100755 index 000000000..3b5f41202 --- /dev/null +++ b/tests-clar/resources/template/hooks/update.sample @@ -0,0 +1,9 @@ +#!/bin/sh +# +# A sample hook to make sure that the `git_repository_init_ext()` function +# can correctly copy a hook over and set it up with the correct permissions. +# +# To enable a hook, you copy the file and remove the ".sample" suffix, but +# in this case, we're just making sure it gets copied correctly. + +echo "$GIT_DIR" From 0d1b094b07d836a657ad9e458e04490d1e72d365 Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Tue, 26 Feb 2013 13:15:06 -0800 Subject: [PATCH 2/3] Fix portability issues on Windows The new tests were not taking core.filemode into account when testing file modes after repo initialization. Fixed that and some other Windows warnings that have crept in. --- src/refs.c | 11 +++++---- src/repository.c | 5 +++- tests-clar/refs/branches/remote.c | 2 +- tests-clar/repo/init.c | 39 ++++++++++++++++++++++++------- 4 files changed, 43 insertions(+), 14 deletions(-) diff --git a/src/refs.c b/src/refs.c index cca3f3ec8..113cadad5 100644 --- a/src/refs.c +++ b/src/refs.c @@ -1599,7 +1599,8 @@ static int ensure_segment_validity(const char *name) { const char *current = name; char prev = '\0'; - int lock_len = strlen(GIT_FILELOCK_EXTENSION); + const int lock_len = (int)strlen(GIT_FILELOCK_EXTENSION); + int segment_len; if (*current == '.') return -1; /* Refname starts with "." */ @@ -1620,12 +1621,14 @@ static int ensure_segment_validity(const char *name) prev = *current; } + segment_len = (int)(current - name); + /* A refname component can not end with ".lock" */ - if (current - name >= lock_len && + if (segment_len >= lock_len && !memcmp(current - lock_len, GIT_FILELOCK_EXTENSION, lock_len)) return -1; - return (int)(current - name); + return segment_len; } static bool is_all_caps_and_underscore(const char *name, size_t len) @@ -1700,7 +1703,7 @@ int git_reference__normalize_name( /* No empty segment is allowed when not normalizing */ if (segment_len == 0 && !normalize) goto cleanup; - + if (current[segment_len] == '\0') break; diff --git a/src/repository.c b/src/repository.c index 5279d8f63..e120c5836 100644 --- a/src/repository.c +++ b/src/repository.c @@ -1043,8 +1043,11 @@ static int repo_init_structure( static int mkdir_parent(git_buf *buf, uint32_t mode, bool skip2) { + /* When making parent directories during repository initialization + * don't try to set gid or grant world write access + */ return git_futils_mkdir( - buf->ptr, NULL, mode & ~(S_ISGID | S_IWOTH), + buf->ptr, NULL, mode & ~(S_ISGID | 0002), GIT_MKDIR_PATH | GIT_MKDIR_VERIFY_DIR | (skip2 ? GIT_MKDIR_SKIP_LAST2 : GIT_MKDIR_SKIP_LAST)); } diff --git a/tests-clar/refs/branches/remote.c b/tests-clar/refs/branches/remote.c index 145c3182f..5272d1236 100644 --- a/tests-clar/refs/branches/remote.c +++ b/tests-clar/refs/branches/remote.c @@ -11,7 +11,7 @@ void test_refs_branches_remote__initialize(void) { g_repo = cl_git_sandbox_init("testrepo"); - expected_remote_name_length = strlen(expected_remote_name) + 1; + expected_remote_name_length = (int)strlen(expected_remote_name) + 1; } void test_refs_branches_remote__cleanup(void) diff --git a/tests-clar/repo/init.c b/tests-clar/repo/init.c index c69ea76ca..085d65f86 100644 --- a/tests-clar/repo/init.c +++ b/tests-clar/repo/init.c @@ -363,10 +363,23 @@ void test_repo_init__extended_1(void) cl_fixture_cleanup("root"); } +static uint32_t normalize_filemode(uint32_t mode, bool core_filemode) +{ + /* if no filemode support, strip SETGID, exec, and low-order bits */ + + /* cannot use constants because on platform without SETGID, that + * will have been defined to zero - must use hardcoded value to + * clear it effectively from the expected value + */ + + return core_filemode ? mode : (mode & ~02177); +} + static void assert_hooks_match( const char *template_dir, const char *repo_dir, const char *hook_path, + bool core_filemode, uint32_t expected_mode) { git_buf expected = GIT_BUF_INIT; @@ -384,6 +397,8 @@ static void assert_hooks_match( if (!expected_mode) expected_mode = (uint32_t)expected_st.st_mode; + expected_mode = normalize_filemode(expected_mode, core_filemode); + cl_assert_equal_i((int)expected_mode, (int)st.st_mode); git_buf_free(&expected); @@ -391,7 +406,7 @@ static void assert_hooks_match( } static void assert_has_mode( - const char *base, const char *path, uint32_t expected) + const char *base, const char *path, bool core_filemode, uint32_t expected) { git_buf full = GIT_BUF_INIT; struct stat st; @@ -400,6 +415,8 @@ static void assert_has_mode( cl_git_pass(git_path_lstat(full.ptr, &st)); git_buf_free(&full); + expected = normalize_filemode(expected, core_filemode); + cl_assert_equal_i((int)expected, (int)st.st_mode); } @@ -433,11 +450,11 @@ void test_repo_init__extended_with_template(void) assert_hooks_match( cl_fixture("template"), git_repository_path(_repo), - "hooks/update.sample", 0); + "hooks/update.sample", true, 0); assert_hooks_match( cl_fixture("template"), git_repository_path(_repo), - "hooks/link.sample", 0); + "hooks/link.sample", true, 0); } void test_repo_init__extended_with_template_and_shared_mode(void) @@ -445,6 +462,8 @@ void test_repo_init__extended_with_template_and_shared_mode(void) git_buf expected = GIT_BUF_INIT; git_buf actual = GIT_BUF_INIT; git_repository_init_options opts = GIT_REPOSITORY_INIT_OPTIONS_INIT; + git_config *config; + int filemode = true; cl_set_cleanup(&cleanup_repository, "init_shared_from_tpl"); @@ -458,6 +477,10 @@ void test_repo_init__extended_with_template_and_shared_mode(void) cl_assert(!git_repository_is_bare(_repo)); cl_assert(!git__suffixcmp(git_repository_path(_repo), "/init_shared_from_tpl/.git/")); + cl_git_pass(git_repository_config(&config, _repo)); + cl_git_pass(git_config_get_bool(&filemode, config, "core.filemode")); + git_config_free(config); + cl_git_pass(git_futils_readbuffer( &expected, cl_fixture("template/description"))); cl_git_pass(git_futils_readbuffer( @@ -468,17 +491,17 @@ void test_repo_init__extended_with_template_and_shared_mode(void) git_buf_free(&expected); git_buf_free(&actual); - assert_has_mode(git_repository_path(_repo), "hooks", + assert_has_mode(git_repository_path(_repo), "hooks", filemode, GIT_REPOSITORY_INIT_SHARED_GROUP | S_IFDIR); - assert_has_mode(git_repository_path(_repo), "info", + assert_has_mode(git_repository_path(_repo), "info", filemode, GIT_REPOSITORY_INIT_SHARED_GROUP | S_IFDIR); - assert_has_mode(git_repository_path(_repo), "description", + assert_has_mode(git_repository_path(_repo), "description", filemode, (GIT_REPOSITORY_INIT_SHARED_GROUP | S_IFREG) & ~(S_ISGID | 0111)); /* for a non-symlinked hook, it should have shared permissions now */ assert_hooks_match( cl_fixture("template"), git_repository_path(_repo), - "hooks/update.sample", + "hooks/update.sample", filemode, (GIT_REPOSITORY_INIT_SHARED_GROUP | S_IFREG) & ~S_ISGID); /* for a symlinked hook, the permissions still should match the @@ -486,7 +509,7 @@ void test_repo_init__extended_with_template_and_shared_mode(void) */ assert_hooks_match( cl_fixture("template"), git_repository_path(_repo), - "hooks/link.sample", 0); + "hooks/link.sample", filemode, 0); } void test_repo_init__can_reinit_an_initialized_repository(void) From 18f08264082dd436914c3c06d369e7a98ddf17aa Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Wed, 27 Feb 2013 13:44:15 -0800 Subject: [PATCH 3/3] Make mode handling during init more like git When creating files, instead of actually using GIT_FILEMODE_BLOB and the other various constants that happen to correspond to mode values, apparently I should be just using 0666 and 0777, and relying on the umask to clear bits and make the value sane. This fixes the rules for copying a template directory and fixes the checks to match that new behavior. (Further changes to the checkout logic to follow separately.) --- src/fileops.c | 19 ++++------- src/fileops.h | 15 ++++++++- src/repo_template.h | 10 +++--- src/repository.c | 13 ++++---- tests-clar/repo/init.c | 73 ++++++++++++++++++++++-------------------- 5 files changed, 70 insertions(+), 60 deletions(-) diff --git a/src/fileops.c b/src/fileops.c index 2b2db4b37..90ca11fb7 100644 --- a/src/fileops.c +++ b/src/fileops.c @@ -693,7 +693,7 @@ static int _cp_r_mkdir(cp_r_info *info, git_buf *from) if ((info->flags & GIT_CPDIR__MKDIR_DONE_FOR_TO_ROOT) == 0) { error = git_futils_mkdir( info->to_root, NULL, info->dirmode, - ((info->flags & GIT_CPDIR_CHMOD) != 0) ? GIT_MKDIR_CHMOD : 0); + (info->flags & GIT_CPDIR_CHMOD_DIRS) ? GIT_MKDIR_CHMOD : 0); info->flags |= GIT_CPDIR__MKDIR_DONE_FOR_TO_ROOT; } @@ -738,7 +738,7 @@ static int _cp_r_callback(void *ref, git_buf *from) mode_t oldmode = info->dirmode; /* if we are not chmod'ing, then overwrite dirmode */ - if ((info->flags & GIT_CPDIR_CHMOD) == 0) + if ((info->flags & GIT_CPDIR_CHMOD_DIRS) == 0) info->dirmode = from_st.st_mode; /* make directory now if CREATE_EMPTY_DIRS is requested and needed */ @@ -783,17 +783,10 @@ static int _cp_r_callback(void *ref, git_buf *from) else { mode_t usemode = from_st.st_mode; - /* if chmod'ing, then blend dirmode & from_st bits */ - if ((info->flags & GIT_CPDIR_CHMOD) != 0) - usemode = (info->dirmode & 0666) | (usemode & ~0666); + if ((info->flags & GIT_CPDIR_SIMPLE_TO_MODE) != 0) + usemode = (usemode & 0111) ? 0777 : 0666; error = git_futils_cp(from->ptr, info->to.ptr, usemode); - - if (!error && - (info->flags & GIT_CPDIR_CHMOD) != 0 && - (error = p_chmod(info->to.ptr, usemode)) < 0) - giterr_set(GITERR_OS, "Failed to set permissions on '%s'", - info->to.ptr); } return error; @@ -824,12 +817,12 @@ int git_futils_cp_r( * demand right before files are copied. */ info.mkdir_flags = GIT_MKDIR_PATH | GIT_MKDIR_SKIP_LAST; - if ((flags & GIT_CPDIR_CHMOD) != 0) + if ((flags & GIT_CPDIR_CHMOD_DIRS) != 0) info.mkdir_flags |= GIT_MKDIR_CHMOD_PATH; } else { /* otherwise, we will do simple mkdir as directories are encountered */ info.mkdir_flags = - ((flags & GIT_CPDIR_CHMOD) != 0) ? GIT_MKDIR_CHMOD : 0; + ((flags & GIT_CPDIR_CHMOD_DIRS) != 0) ? GIT_MKDIR_CHMOD : 0; } error = _cp_r_callback(&info, &path); diff --git a/src/fileops.h b/src/fileops.h index 9e1f7440e..988cc661a 100644 --- a/src/fileops.h +++ b/src/fileops.h @@ -162,13 +162,26 @@ extern int git_futils_cp( /** * Flags that can be passed to `git_futils_cp_r`. + * + * - GIT_CPDIR_CREATE_EMPTY_DIRS: create directories even if there are no + * files under them (otherwise directories will only be created lazily + * when a file inside them is copied). + * - GIT_CPDIR_COPY_SYMLINKS: copy symlinks, otherwise they are ignored. + * - GIT_CPDIR_COPY_DOTFILES: copy files with leading '.', otherwise ignored. + * - GIT_CPDIR_OVERWRITE: overwrite pre-existing files with source content, + * otherwise they are silently skipped. + * - GIT_CPDIR_CHMOD_DIRS: explicitly chmod directories to `dirmode` + * - GIT_CPDIR_SIMPLE_TO_MODE: default tries to replicate the mode of the + * source file to the target; with this flag, always use 0666 (or 0777 if + * source has exec bits set) for target. */ typedef enum { GIT_CPDIR_CREATE_EMPTY_DIRS = (1u << 0), GIT_CPDIR_COPY_SYMLINKS = (1u << 1), GIT_CPDIR_COPY_DOTFILES = (1u << 2), GIT_CPDIR_OVERWRITE = (1u << 3), - GIT_CPDIR_CHMOD = (1u << 4), + GIT_CPDIR_CHMOD_DIRS = (1u << 4), + GIT_CPDIR_SIMPLE_TO_MODE = (1u << 5), } git_futils_cpdir_flags; /** diff --git a/src/repo_template.h b/src/repo_template.h index 90ffe851b..099279aa7 100644 --- a/src/repo_template.h +++ b/src/repo_template.h @@ -11,10 +11,10 @@ #define GIT_OBJECTS_PACK_DIR GIT_OBJECTS_DIR "pack/" #define GIT_HOOKS_DIR "hooks/" -#define GIT_HOOKS_DIR_MODE 0755 +#define GIT_HOOKS_DIR_MODE 0777 #define GIT_HOOKS_README_FILE GIT_HOOKS_DIR "README.sample" -#define GIT_HOOKS_README_MODE 0755 +#define GIT_HOOKS_README_MODE 0777 #define GIT_HOOKS_README_CONTENT \ "#!/bin/sh\n"\ "#\n"\ @@ -23,16 +23,16 @@ "# more information.\n" #define GIT_INFO_DIR "info/" -#define GIT_INFO_DIR_MODE 0755 +#define GIT_INFO_DIR_MODE 0777 #define GIT_INFO_EXCLUDE_FILE GIT_INFO_DIR "exclude" -#define GIT_INFO_EXCLUDE_MODE 0644 +#define GIT_INFO_EXCLUDE_MODE 0666 #define GIT_INFO_EXCLUDE_CONTENT \ "# File patterns to ignore; see `git help ignore` for more information.\n"\ "# Lines that start with '#' are comments.\n" #define GIT_DESC_FILE "description" -#define GIT_DESC_MODE 0644 +#define GIT_DESC_MODE 0666 #define GIT_DESC_CONTENT \ "Unnamed repository; edit this file 'description' to name the repository.\n" diff --git a/src/repository.c b/src/repository.c index e120c5836..cd1e658cf 100644 --- a/src/repository.c +++ b/src/repository.c @@ -934,7 +934,7 @@ static int repo_write_gitlink( error = git_buf_printf(&buf, "%s %s", GIT_FILE_CONTENT_PREFIX, to_repo); if (!error) - error = repo_write_template(in_dir, true, DOT_GIT, 0644, true, buf.ptr); + error = repo_write_template(in_dir, true, DOT_GIT, 0666, true, buf.ptr); cleanup: git_buf_free(&buf); @@ -944,7 +944,7 @@ cleanup: static mode_t pick_dir_mode(git_repository_init_options *opts) { if (opts->mode == GIT_REPOSITORY_INIT_SHARED_UMASK) - return 0755; + return 0777; if (opts->mode == GIT_REPOSITORY_INIT_SHARED_GROUP) return (0775 | S_ISGID); if (opts->mode == GIT_REPOSITORY_INIT_SHARED_ALL) @@ -1006,7 +1006,8 @@ static int repo_init_structure( } error = git_futils_cp_r(tdir, repo_dir, - GIT_CPDIR_COPY_SYMLINKS | GIT_CPDIR_CHMOD, dmode); + GIT_CPDIR_COPY_SYMLINKS | GIT_CPDIR_CHMOD_DIRS | + GIT_CPDIR_SIMPLE_TO_MODE, dmode); if (error < 0) { if (strcmp(tdir, GIT_TEMPLATE_DIR) != 0) @@ -1168,10 +1169,8 @@ static int repo_init_directories( has_dotgit) { /* create path #1 */ - if ((error = git_futils_mkdir( - repo_path->ptr, NULL, dirmode, - GIT_MKDIR_VERIFY_DIR | GIT_MKDIR_CHMOD)) < 0) - return error; + error = git_futils_mkdir(repo_path->ptr, NULL, dirmode, + GIT_MKDIR_VERIFY_DIR | ((dirmode & S_ISGID) ? GIT_MKDIR_CHMOD : 0)); } /* prettify both directories now that they are created */ diff --git a/tests-clar/repo/init.c b/tests-clar/repo/init.c index 085d65f86..e6f53083b 100644 --- a/tests-clar/repo/init.c +++ b/tests-clar/repo/init.c @@ -363,24 +363,11 @@ void test_repo_init__extended_1(void) cl_fixture_cleanup("root"); } -static uint32_t normalize_filemode(uint32_t mode, bool core_filemode) -{ - /* if no filemode support, strip SETGID, exec, and low-order bits */ - - /* cannot use constants because on platform without SETGID, that - * will have been defined to zero - must use hardcoded value to - * clear it effectively from the expected value - */ - - return core_filemode ? mode : (mode & ~02177); -} - static void assert_hooks_match( const char *template_dir, const char *repo_dir, const char *hook_path, - bool core_filemode, - uint32_t expected_mode) + bool core_filemode) { git_buf expected = GIT_BUF_INIT; git_buf actual = GIT_BUF_INIT; @@ -394,19 +381,20 @@ static void assert_hooks_match( cl_assert(expected_st.st_size == st.st_size); - if (!expected_mode) - expected_mode = (uint32_t)expected_st.st_mode; + if (!core_filemode) { + expected_st.st_mode = expected_st.st_mode & ~0111; + st.st_mode = st.st_mode & ~0111; + } - expected_mode = normalize_filemode(expected_mode, core_filemode); - - cl_assert_equal_i((int)expected_mode, (int)st.st_mode); + cl_assert_equal_i((int)expected_st.st_mode, (int)st.st_mode); git_buf_free(&expected); git_buf_free(&actual); } -static void assert_has_mode( - const char *base, const char *path, bool core_filemode, uint32_t expected) +static void assert_mode_seems_okay( + const char *base, const char *path, + git_filemode_t expect_mode, bool expect_setgid, bool core_filemode) { git_buf full = GIT_BUF_INIT; struct stat st; @@ -415,9 +403,25 @@ static void assert_has_mode( cl_git_pass(git_path_lstat(full.ptr, &st)); git_buf_free(&full); - expected = normalize_filemode(expected, core_filemode); + if (!core_filemode) { + expect_mode = expect_mode & ~0111; + st.st_mode = st.st_mode & ~0111; + expect_setgid = false; + } - cl_assert_equal_i((int)expected, (int)st.st_mode); + if (S_ISGID != 0) { + if (expect_setgid) + cl_assert((st.st_mode & S_ISGID) != 0); + else + cl_assert((st.st_mode & S_ISGID) == 0); + } + + if ((expect_mode & 0111) != 0) + cl_assert((st.st_mode & 0111) != 0); + else + cl_assert((st.st_mode & 0111) == 0); + + cl_assert((expect_mode & 0170000) == (st.st_mode & 0170000)); } void test_repo_init__extended_with_template(void) @@ -450,11 +454,11 @@ void test_repo_init__extended_with_template(void) assert_hooks_match( cl_fixture("template"), git_repository_path(_repo), - "hooks/update.sample", true, 0); + "hooks/update.sample", true); assert_hooks_match( cl_fixture("template"), git_repository_path(_repo), - "hooks/link.sample", true, 0); + "hooks/link.sample", true); } void test_repo_init__extended_with_template_and_shared_mode(void) @@ -464,6 +468,7 @@ void test_repo_init__extended_with_template_and_shared_mode(void) git_repository_init_options opts = GIT_REPOSITORY_INIT_OPTIONS_INIT; git_config *config; int filemode = true; + const char *repo_path = NULL; cl_set_cleanup(&cleanup_repository, "init_shared_from_tpl"); @@ -491,25 +496,25 @@ void test_repo_init__extended_with_template_and_shared_mode(void) git_buf_free(&expected); git_buf_free(&actual); - assert_has_mode(git_repository_path(_repo), "hooks", filemode, - GIT_REPOSITORY_INIT_SHARED_GROUP | S_IFDIR); - assert_has_mode(git_repository_path(_repo), "info", filemode, - GIT_REPOSITORY_INIT_SHARED_GROUP | S_IFDIR); - assert_has_mode(git_repository_path(_repo), "description", filemode, - (GIT_REPOSITORY_INIT_SHARED_GROUP | S_IFREG) & ~(S_ISGID | 0111)); + repo_path = git_repository_path(_repo); + assert_mode_seems_okay(repo_path, "hooks", + GIT_FILEMODE_TREE | GIT_REPOSITORY_INIT_SHARED_GROUP, true, filemode); + assert_mode_seems_okay(repo_path, "info", + GIT_FILEMODE_TREE | GIT_REPOSITORY_INIT_SHARED_GROUP, true, filemode); + assert_mode_seems_okay(repo_path, "description", + GIT_FILEMODE_BLOB, false, filemode); /* for a non-symlinked hook, it should have shared permissions now */ assert_hooks_match( cl_fixture("template"), git_repository_path(_repo), - "hooks/update.sample", filemode, - (GIT_REPOSITORY_INIT_SHARED_GROUP | S_IFREG) & ~S_ISGID); + "hooks/update.sample", filemode); /* for a symlinked hook, the permissions still should match the * source link, not the GIT_REPOSITORY_INIT_SHARED_GROUP value */ assert_hooks_match( cl_fixture("template"), git_repository_path(_repo), - "hooks/link.sample", filemode, 0); + "hooks/link.sample", filemode); } void test_repo_init__can_reinit_an_initialized_repository(void)