[PATCH 01/24] init-db: remove unnecessary global variable & document existing bug

From: Elijah Newren via GitGitGadget <gitgitgadget@gmail.com>
Date: May 7, 2023, 03:45 UTC
Message-ID: <12a251f77f679123d01892109694f8ee19b96252.1683431151.git.gitgitgadget@gmail.com>
In-Reply-To: <pull.1525.git.1683431149.gitgitgadget@gmail.com>

From: Elijah Newren <newren@gmail.com>

This commit was prompted by a desire to move the functions which builtin/init-db.c and builtin/clone.c share out of the former file and into setup.c. One issue that made it difficult was the init_is_bare_repository global variable.

init_is_bare_repository is actually not very useful. It merely stores the return value from is_bare_repository() and only for the duration of a few additional function calls before its value is checked, and none of those functions do anything that could change is_bare_repository()'s return value. So, we can simply dispense with the global by replacing it with is_bare_repository().

However, the intervening code does talk about reading config from the config file in the specified `--templates` directory, so touching this code does lead to the question of whether core.bare could be set in such a config file and thus whether the code is doing the right thing. Long story short is that the templates directory might have a config file with core.bare set, but it has always been unconditionally ignored. While fixing that might be nice, it looks to be a can of worms, and cannot be fixed within this function anyway. Instead of opening that can of worms, document the problem with a TODO comment and a couple test_expect_failure testcases.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/init-db.c        | 20 ++++++++++++++++----
 t/t1301-shared-repo.sh   | 22 ++++++++++++++++++++++
 t/t5606-clone-options.sh | 10 ++++++++++
 3 files changed, 48 insertions(+), 4 deletions(-)

diff --git a/builtin/init-db.c b/builtin/init-db.c
index aef40361052..18733ef05aa 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -31,7 +31,6 @@
 
 #define GIT_DEFAULT_HASH_ENVIRONMENT "GIT_DEFAULT_HASH"
 
-static int init_is_bare_repository = 0;
 static int init_shared_repository = -1;
 
 static void copy_templates_1(struct strbuf *path, struct strbuf *template_path,
@@ -231,9 +230,24 @@ static int create_default_files(const char *template_path,
 	 * We must make sure command-line options continue to override any
 	 * values we might have just re-read from the config.
 	 */
-	is_bare_repository_cfg = init_is_bare_repository || !work_tree;
 	if (init_shared_repository != -1)
 		set_shared_repository(init_shared_repository);
+	/*
+	 * TODO: heed core.bare from config file in templates if no
+	 *       command-line override given
+	 *
+	 * Unfortunately, this location in the code is far too late to
+	 * allow this to happen; both builtin/init.db and
+	 * builtin/clone.c setup the new repository and call
+	 * set_git_work_tree() before this point.  (Note that both do
+	 * that repository setup before calling init_db(), which in
+	 * turn calls create_default_files().)  Fixing it would
+	 * require too much refactoring, and no one seems to have
+	 * wanted this behavior in 15+ years, so we'll continue
+	 * ignoring the config for now and just override
+	 * is_bare_repository_cfg unconditionally.
+	 */
+	is_bare_repository_cfg = is_bare_repository() || !work_tree;
 
 	/*
 	 * We would have created the above under user's umask -- under
@@ -422,8 +436,6 @@ int init_db(const char *git_dir, const char *real_git_dir,
 
 	safe_create_dir(git_dir, 0);
 
-	init_is_bare_repository = is_bare_repository();
-
 	/* Check to see if the repository version is right.
 	 * Note that a newly created repository does not have
 	 * config file, so this will not fail.  What we are catching
diff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh
index 1b6437ec079..c02fd64793b 100755
--- a/t/t1301-shared-repo.sh
+++ b/t/t1301-shared-repo.sh
@@ -52,6 +52,28 @@ test_expect_success 'shared=all' '
 	test 2 = $(git config core.sharedrepository)
 '
 
+test_expect_failure 'template can set core.bare' '
+	test_when_finished "rm -rf subdir" &&
+	test_when_finished "rm -rf templates" &&
+	test_config core.bare true &&
+	umask 0022 &&
+	mkdir -p templates/ &&
+	cp .git/config templates/config &&
+	git init --template=templates subdir &&
+	test_path_exists subdir/HEAD
+'
+
+test_expect_success 'template can set core.bare but overridden by command line' '
+	test_when_finished "rm -rf subdir" &&
+	test_when_finished "rm -rf templates" &&
+	test_config core.bare true &&
+	umask 0022 &&
+	mkdir -p templates/ &&
+	cp .git/config templates/config &&
+	git init --no-bare --template=templates subdir &&
+	test_path_exists subdir/.git/HEAD
+'
+
 test_expect_success POSIXPERM 'update-server-info honors core.sharedRepository' '
 	: > a1 &&
 	git add a1 &&
diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh
index 27f9f776389..5890319b97b 100755
--- a/t/t5606-clone-options.sh
+++ b/t/t5606-clone-options.sh
@@ -120,6 +120,16 @@ test_expect_success 'prefers -c config over --template config' '
 
 '
 
+test_expect_failure 'prefers --template config even for core.bare' '
+
+	template="$TRASH_DIRECTORY/template-with-bare-config" &&
+	mkdir "$template" &&
+	git config --file "$template/config" core.bare true &&
+	git clone "--template=$template" parent clone-bare-config &&
+	test "$(git -C clone-bare-config config --local core.bare)" = "true" &&
+	test_path_is_file clone-bare-config/HEAD
+'
+
 test_expect_success 'prefers config "clone.defaultRemoteName" over default' '
 
 	test_config_global clone.defaultRemoteName from_config &&

-- 
gitgitgadget