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

@@ -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:
+			case EEXIST:
-					if (!lastch && (flags & GIT_MKDIR_VERIFY_DIR) != 0 &&
+				if (!lastch && (flags & GIT_MKDIR_VERIFY_DIR) != 0 &&
-						!git_path_isdir(make_path.ptr)) {
+					!git_path_isdir(make_path.ptr)) {
-						giterr_set(
+					giterr_set(
-							GITERR_OS, "Existing path is not a directory '%s'",
+						GITERR_OS, "Existing path is not a directory '%s'",
-							make_path.ptr);
+						make_path.ptr);
-						error = GIT_ENOTFOUND;
+					error = GIT_ENOTFOUND;
-						goto fail;
+					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 */
+				/* Fall through */
-					errno = ENOSYS;
+				errno = ENOSYS;
-				default:
+			default:
-					giterr_set(GITERR_OS, "Failed to make directory '%s'",
+				giterr_set(GITERR_OS, "Failed to make directory '%s'",
-						make_path.ptr);
+					make_path.ptr);
-					goto fail;
+				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)
+	if ((error = git_path_lstat(from->ptr, &from_st)) < 0)
-		return -1;
+		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(
+			error = _cp_r_mkdir(info, from);
-				info->to.ptr, NULL, info->dirmode, info->mkdir_flags);
 
 		/* 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(
+		(error = _cp_r_mkdir(info, from)) < 0)
-			info->to.ptr, NULL, info->dirmode, info->mkdir_flags) < 0)
+		return error;
-		return -1;
 
 	/* 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);
+		error = cp_link(from->ptr, info->to.ptr, (size_t)from_st.st_size);
-	else
+	else {
-		return git_futils_cp(from->ptr, info->to.ptr, from_st.st_mode);
+		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;
 	}

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_CREATE_EMPTY_DIRS = (1u << 0),
-	GIT_CPDIR_COPY_SYMLINKS = 2,
+	GIT_CPDIR_COPY_SYMLINKS     = (1u << 1),
-	GIT_CPDIR_COPY_DOTFILES = 4,
+	GIT_CPDIR_COPY_DOTFILES     = (1u << 2),
-	GIT_CPDIR_OVERWRITE = 8,
+	GIT_CPDIR_OVERWRITE         = (1u << 3),
-	GIT_CPDIR_CHMOD = 16
+	GIT_CPDIR_CHMOD             = (1u << 4),
 } git_futils_cpdir_flags;
 
 /**

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) {
+	if ((opts->flags & GIT_REPOSITORY_INIT_MKPATH) != 0) {
-		git_buf p = GIT_BUF_INIT;
+		/* create path #5 */
-		if ((error = git_path_dirname_r(&p, repo_path->ptr)) >= 0)
+		if (wd_path->size > 0 &&
-			error = git_futils_mkdir(p.ptr, NULL, dirmode, 0);
+			(error = mkdir_parent(wd_path, dirmode, false)) < 0)
-		git_buf_free(&p);
+			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;
+		/* create path #1 */
-		if ((opts->flags & GIT_REPOSITORY_INIT_MKPATH) != 0)
+		if ((error = git_futils_mkdir(
-			mkflag |= GIT_MKDIR_PATH;
+				repo_path->ptr, NULL, dirmode,
-		error = git_futils_mkdir(repo_path->ptr, NULL, dirmode, mkflag);
+				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) {

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_git_pass(git_futils_readbuffer(
-	cl_assert(git_futils_readbuffer(&actual,"templated.git/description") == GIT_OK);
+		&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");
 }

tests-clar/resources/template/hooks/link.sample

@@ -0,0 +1 @@
+update.sample

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"