Subject: [PATCH v2 3/3] config: correct file reading order in read_early_config()

From: Nguyễn Thái Ngọc Duy <[email-masked]>
Date: Mon, 17 Apr 2017 17:10:02 +0700

Config file reading order is important because each file can override
values in the previous files and this is expected behavior. Normally
we read in this order, all in do_git_config_sequence():

1. $HOME/.gitconfig
2. $GIT_DIR/config
3. config from command line

However in read_early_config() the order may be swapped a bit if
setup_git_directory() has not been called:

1. $HOME/.gitconfig
2. $GIT_DIR/config is NOT read because .git dir is not found _yet_
3. config from command line
4. $GIT_DIR/config is now READ (after discover_git_directory() call)

The reading at step 4 could override config at step 3, which is not
the expectation.

Now that we could pass the .git dir around, we could feed
discover_git_directory() back to step 2, so that it works again, and
remove step 4.

Noticed-by: Jeff King <[email-masked]>
Signed-off-by: Nguyễn Thái Ngọc Duy <[email-masked]>
---
 config.c                | 26 ++++++++++++--------------
 t/helper/test-config.c  |  4 ++++
 t/t1309-early-config.sh | 17 +++++++++++++++++
 3 files changed, 33 insertions(+), 14 deletions(-)

diff --git a/config.c b/config.c
index 14f0417460..fffda653e3 100644
--- a/config.c
+++ b/config.c
@@ -1504,12 +1504,20 @@ int git_config_system(void)
 	return !git_env_bool("GIT_CONFIG_NOSYSTEM", 0);
 }
 
-static int do_git_config_sequence(config_fn_t fn, void *data)
+static int do_git_config_sequence(const struct config_options *opts,
+				  config_fn_t fn, void *data)
 {
 	int ret = 0;
 	char *xdg_config = xdg_config_home("config");
 	char *user_config = expand_user_path("~/.gitconfig");
-	char *repo_config = have_git_dir() ? git_pathdup("config") : NULL;
+	char *repo_config;
+
+	if (opts->git_dir)
+		repo_config = mkpathdup("%s/config", opts->git_dir);
+	else if (have_git_dir())
+		repo_config = git_pathdup("config");
+	else
+		repo_config = NULL;
 
 	current_parsing_scope = CONFIG_SCOPE_SYSTEM;
 	if (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK, 0))
@@ -1563,7 +1571,7 @@ int git_config_with_options(config_fn_t fn, void *data,
 	else if (config_source && config_source->blob)
 		return git_config_from_blob_ref(fn, config_source->blob, data);
 
-	return do_git_config_sequence(fn, data);
+	return do_git_config_sequence(opts, fn, data);
 }
 
 static void git_config_raw(config_fn_t fn, void *data)
@@ -1613,7 +1621,6 @@ void read_early_config(config_fn_t cb, void *data)
 {
 	struct config_options opts = {0};
 	struct strbuf buf = STRBUF_INIT;
-	char *to_free = NULL;
 
 	opts.respect_includes = 1;
 
@@ -1628,20 +1635,11 @@ void read_early_config(config_fn_t cb, void *data)
 	 * call).
 	 */
 	else if (discover_git_directory(&buf))
-		opts.git_dir = to_free = xstrdup(buf.buf);
+		opts.git_dir = buf.buf;
 
 	git_config_with_options(cb, data, NULL, &opts);
 
-	if (!have_git_dir() && opts.git_dir) {
-		struct git_config_source repo_config;
-
-		memset(&repo_config, 0, sizeof(repo_config));
-		strbuf_addstr(&buf, "/config");
-		repo_config.file = buf.buf;
-		git_config_with_options(cb, data, &repo_config, &opts);
-	}
 	strbuf_release(&buf);
-	free(to_free);
 }
 
 static void git_config_check_init(void);
diff --git a/t/helper/test-config.c b/t/helper/test-config.c
index 8e3ed6a76c..696d0a52fd 100644
--- a/t/helper/test-config.c
+++ b/t/helper/test-config.c
@@ -84,6 +84,10 @@ int cmd_main(int argc, const char **argv)
 	struct config_set cs;
 
 	if (argc == 3 && !strcmp(argv[1], "read_early_config")) {
+		const char *cmdline_config = getenv("CMDL_CFG");
+
+		if (cmdline_config)
+			git_config_push_parameter(cmdline_config);
 		read_early_config(early_config_cb, (void *)argv[2]);
 		return 0;
 	}
diff --git a/t/t1309-early-config.sh b/t/t1309-early-config.sh
index b97357b8ab..c15f18dd96 100755
--- a/t/t1309-early-config.sh
+++ b/t/t1309-early-config.sh
@@ -47,6 +47,23 @@ test_expect_success 'ceiling #2' '
 	test xdg = "$(cat output)"
 '
 
+test_expect_success 'read config file in right order' '
+	echo "[test]source = home" >>.gitconfig &&
+	git init foo &&
+	(
+		cd foo &&
+		echo "[test]source = repo" >>.git/config &&
+		CMDL_CFG=test.source=cmdline test-config \
+			read_early_config test.source >actual &&
+		cat >expected <<-\EOF &&
+		home
+		repo
+		cmdline
+		EOF
+		test_cmp expected actual
+	)
+'
+
 test_with_config () {
 	rm -rf throwaway &&
 	git init throwaway &&
-- 
2.11.0.157.gd943d85