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.)

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;
-
-	expected_mode = normalize_filemode(expected_mode, core_filemode);
+	if (!core_filemode) {
+		expected_st.st_mode = expected_st.st_mode & ~0111;
+		st.st_mode = st.st_mode & ~0111;
+	}
 
-	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;
+	}
+
+	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_equal_i((int)expected, (int)st.st_mode);
+	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)