Subject: Re: [PATCH] config: teach "git config --file -" to read from the standard input

From: Junio C Hamano <[email-masked]>
Date: Tue, 18 Feb 2014 15:02:46 -0800

Junio C Hamano <[email-masked]> writes:

>> I think I preferred the earlier version where you had "<stdin>" in the
>> name field, and this hunk could just go away. I know you switched it to
>> NULL here to avoid making bogus relative filenames in includes.
>
> Exactly the same comment here.  I really like the way how this
> series becomes more polished every time we see it.
>
>> But that would be pretty straightforward to fix by splitting the "name"
>> field into an additional "path" field. It looks like "git config --blob"
>> has the same problem (it will try relative lookups in the filesystem,
>> even though that is nonsensical from the blob). So we could fix the
>> existing bug at the same time. :)
>>
>> Perhaps this could go at the start of your series?
>
> Sounds like the right thing to do.
>
> Thanks.

And [PATCH 3/3] rebased on the others may look like this.

 builtin/config.c          | 11 +++++++++++
 cache.h                   |  1 +
 config.c                  | 42 ++++++++++++++++++++++++++++--------------
 t/t1300-repo-config.sh    | 17 +++++++++++++++--
 t/t1305-config-include.sh | 16 +++++++++++++++-
 5 files changed, 70 insertions(+), 17 deletions(-)

diff --git a/builtin/config.c b/builtin/config.c
index de41ba5..5677c94 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -360,6 +360,9 @@ static int get_colorbool(int print)
 
 static void check_write(void)
 {
+	if (given_config_source.use_stdin)
+		die("writing to stdin is not supported");
+
 	if (given_config_source.blob)
 		die("writing config blobs is not supported");
 }
@@ -472,6 +475,12 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		usage_with_options(builtin_config_usage, builtin_config_options);
 	}
 
+	if (given_config_source.file &&
+			!strcmp(given_config_source.file, "-")) {
+		given_config_source.file = NULL;
+		given_config_source.use_stdin = 1;
+	}
+
 	if (use_global_config) {
 		char *user_config = NULL;
 		char *xdg_config = NULL;
@@ -558,6 +567,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		check_argc(argc, 0, 0);
 		if (!given_config_source.file && nongit)
 			die("not in a git directory");
+		if (given_config_source.use_stdin)
+			die("editing stdin is not supported");
 		if (given_config_source.blob)
 			die("editing blobs is not supported");
 		git_config(git_default_config, NULL);
diff --git a/cache.h b/cache.h
index 9d94bd6..4db19b5 100644
--- a/cache.h
+++ b/cache.h
@@ -1147,6 +1147,7 @@ extern int update_server_info(int);
 #define CONFIG_GENERIC_ERROR 7
 
 struct git_config_source {
+	unsigned int use_stdin:1;
 	const char *file;
 	const char *blob;
 };
diff --git a/config.c b/config.c
index a21b0dd..9dd0bdb 100644
--- a/config.c
+++ b/config.c
@@ -1031,24 +1031,36 @@ static int do_config_from(struct config_source *top, config_fn_t fn, void *data)
 	return ret;
 }
 
-int git_config_from_file(config_fn_t fn, const char *filename, void *data)
+static int do_config_from_file(config_fn_t fn,
+			       const char *filename, FILE *f,
+			       const char *label, void *data)
 {
-	int ret;
-	FILE *f = fopen(filename, "r");
+	struct config_source top;
 
-	ret = -1;
-	if (f) {
-		struct config_source top;
+	top.u.file = f;
+	top.name = label;
+	top.path = filename;
+	top.die_on_error = 1;
+	top.do_fgetc = config_file_fgetc;
+	top.do_ungetc = config_file_ungetc;
+	top.do_ftell = config_file_ftell;
+
+	return do_config_from(&top, fn, data);
+}
 
-		top.u.file = f;
-		top.name = top.path = filename;
-		top.die_on_error = 1;
-		top.do_fgetc = config_file_fgetc;
-		top.do_ungetc = config_file_ungetc;
-		top.do_ftell = config_file_ftell;
+static int git_config_from_stdin(config_fn_t fn, void *data)
+{
+	return do_config_from_file(fn, NULL, stdin, "<stdin>", data);
+}
 
-		ret = do_config_from(&top, fn, data);
+int git_config_from_file(config_fn_t fn, const char *filename, void *data)
+{
+	int ret = -1;
+	FILE *f;
 
+	f = fopen(filename, "r");
+	if (f) {
+		ret = do_config_from_file(fn, filename, f, filename, data);
 		fclose(f);
 	}
 	return ret;
@@ -1190,7 +1202,9 @@ int git_config_with_options(config_fn_t fn, void *data,
 	 * If we have a specific filename, use it. Otherwise, follow the
 	 * regular lookup sequence.
 	 */
-	if (config_source && config_source->file)
+	if (config_source && config_source->use_stdin)
+		return git_config_from_stdin(fn, data);
+	else if (config_source && config_source->file)
 		return git_config_from_file(fn, config_source->file, data);
 	else if (config_source && config_source->blob)
 		return git_config_from_blob_ref(fn, config_source->blob, data);
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 9673593..c9c426c 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -475,15 +475,28 @@ ein.bahn=strasse
 EOF
 
 test_expect_success 'alternative GIT_CONFIG' '
-	GIT_CONFIG=other-config git config -l >output &&
+	GIT_CONFIG=other-config git config --list >output &&
 	test_cmp expect output
 '
 
 test_expect_success 'alternative GIT_CONFIG (--file)' '
-	git config --file other-config -l > output &&
+	git config --file other-config --list >output &&
 	test_cmp expect output
 '
 
+test_expect_success 'alternative GIT_CONFIG (--file=-)' '
+	git config --file - --list <other-config >output &&
+	test_cmp expect output
+'
+
+test_expect_success 'setting a value in stdin is an error' '
+	test_must_fail git config --file - some.value foo
+'
+
+test_expect_success 'editing stdin is an error' '
+	test_must_fail git config --file - --edit
+'
+
 test_expect_success 'refer config from subdirectory' '
 	mkdir x &&
 	(
diff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh
index 6edd38b..9ba2ba1 100755
--- a/t/t1305-config-include.sh
+++ b/t/t1305-config-include.sh
@@ -113,7 +113,7 @@ test_expect_success 'missing include files are ignored' '
 test_expect_success 'absolute includes from command line work' '
 	echo "[test]one = 1" >one &&
 	echo 1 >expect &&
-	git -c include.path="$PWD/one" config test.one >actual &&
+	git -c include.path="$(pwd)/one" config test.one >actual &&
 	test_cmp expect actual
 '
 
@@ -138,6 +138,20 @@ test_expect_success 'relative includes from blobs fail' '
 	test_must_fail git config --blob=$blob test.one
 '
 
+test_expect_success 'absolute includes from stdin work' '
+	echo "[test]one = 1" >one &&
+	echo 1 >expect &&
+	echo "[include]path=\"$(pwd)/one\"" |
+	git config --file - test.one >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'relative includes from stdin line fail' '
+	echo "[test]one = 1" >one &&
+	echo "[include]path=one" |
+	test_must_fail git config --file - test.one
+'
+
 test_expect_success 'include cycles are detected' '
 	cat >.gitconfig <<-\EOF &&
 	[test]value = gitconfig