[PATCH] config: git_config_from_file(): handle "-" filename as stdin

[PATCH] config: git_config_from_file(): handle "-" filename as stdin

[Kirill A. Shutemov]

The patch extends git config --file interface to allow read config from
stdin.

Signed-off-by: Kirill A. Shutemov <kirill@shutemov.name>
---
 config.c               | 10 ++++++----
 t/t1300-repo-config.sh |  4 ++++
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/config.c b/config.c
index d969a5aefc2b..f80cc7e657e8 100644
--- a/config.c
+++ b/config.c
@@ -1032,10 +1032,11 @@ static int do_config_from(struct config_source *top, config_fn_t fn, void *data)
 
 int git_config_from_file(config_fn_t fn, const char *filename, void *data)
 {
-	int ret;
-	FILE *f = fopen(filename, "r");
+	int from_stdin = !strcmp(filename, "-");
+	int ret = -1;
+	FILE *f;
 
-	ret = -1;
+	f = from_stdin ? stdin : fopen(filename, "r");
 	if (f) {
 		struct config_source top;
 
@@ -1048,7 +1049,8 @@ int git_config_from_file(config_fn_t fn, const char *filename, void *data)
 
 		ret = do_config_from(&top, fn, data);
 
-		fclose(f);
+		if (!from_stdin)
+			fclose(f);
 	}
 	return ret;
 }
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 967359344dab..f1a63075e34f 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -484,6 +484,10 @@ test_expect_success 'alternative GIT_CONFIG (--file)' '
 	test_cmp expect output
 '
 
+test_expect_success 'alternative GIT_CONFIG (--file=-)' '
+	git config --file - -l < other-config > output &&
+	test_cmp expect output
+'
 test_expect_success 'refer config from subdirectory' '
 	mkdir x &&
 	(
-- 
1.8.5.2

Re: [PATCH] config: git_config_from_file(): handle "-" filename as stdin

[Junio C Hamano]

"Kirill A. Shutemov" <kirill@shutemov.name> writes:

> The patch extends git config --file interface to allow read config from
> stdin.

Thanks.  The external interface proposed by this change that behaves
the way your new test expects is a good addition to the system.  I
would describe it as:

  Subject: config: teach "git config --file -" to read from the standard input

I however think the patch implements it at the level that is too low
in the callchain.  It will affect a lot more than the dash given to
"git config --file -".  Fortunately, it does not make it possible
for users to make this mistake

	[include]
        	path = -

and scratch their heads, wondering why "git config" is not answering
until they hit ^D.  But that is _only_ because we check if a file
whose name is "-" actually exists in the current directory before
falling into this codepath (and usually no such file exists).  If
such a funnily-named file does exist, we read from that file, not
the standard input.  So that "include" codepath happens to be safe,
but who knows what dragons lie in other codepaths that call this
function.

I recall that an earlier implementation of "git diff --no-index"
that made "-" read one side to be compared from the standard input
had exactly the same issue of comparing filename with "-", which we
had to fix with code reorganization recently.  I'd prefer to see
this update to "git config --file -" done the right way from the
start.

> diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
> index 967359344dab..f1a63075e34f 100755
> --- a/t/t1300-repo-config.sh
> +++ b/t/t1300-repo-config.sh
> @@ -484,6 +484,10 @@ test_expect_success 'alternative GIT_CONFIG (--file)' '
>  	test_cmp expect output
>  '
>  
> +test_expect_success 'alternative GIT_CONFIG (--file=-)' '
> +	git config --file - -l < other-config > output &&

Please leave no SP between redirection operator and its file, i.e.

	git config --file - --list <other-config >output

Thanks.

Re: [PATCH] config: git_config_from_file(): handle "-" filename as stdin

[Kirill A. Shutemov]

On Fri, Feb 14, 2014 at 10:27:11AM -0800, Junio C Hamano wrote:
> "Kirill A. Shutemov" <kirill@shutemov.name> writes:
> 
> > The patch extends git config --file interface to allow read config from
> > stdin.
> 
> Thanks.  The external interface proposed by this change that behaves
> the way your new test expects is a good addition to the system.  I
> would describe it as:
> 
>   Subject: config: teach "git config --file -" to read from the standard input
> 
> I however think the patch implements it at the level that is too low
> in the callchain.  It will affect a lot more than the dash given to
> "git config --file -".  Fortunately, it does not make it possible
> for users to make this mistake
> 
> 	[include]
>         	path = -
> 
> and scratch their heads, wondering why "git config" is not answering
> until they hit ^D.  But that is _only_ because we check if a file
> whose name is "-" actually exists in the current directory before
> falling into this codepath (and usually no such file exists).  If
> such a funnily-named file does exist, we read from that file, not
> the standard input.  So that "include" codepath happens to be safe,
> but who knows what dragons lie in other codepaths that call this
> function.
> 
> I recall that an earlier implementation of "git diff --no-index"
> that made "-" read one side to be compared from the standard input
> had exactly the same issue of comparing filename with "-", which we
> had to fix with code reorganization recently.  I'd prefer to see
> this update to "git config --file -" done the right way from the
> start.

Okay, reworked version is below. It's slightly more invasive then the
original.

>From 199e6a995bb5228578e66189ef586421a4d8d9ba Mon Sep 17 00:00:00 2001
From: "Kirill A. Shutemov" <kirill@shutemov.name>
Date: Fri, 14 Feb 2014 21:59:39 +0200
Subject: [PATCH] config: teach "git config --file -" to read from the standard
 input

The patch extends git config --file interface to allow read config from
stdin.

Editing stdin or setting value in stdin is an error.

Signed-off-by: Kirill A. Shutemov <kirill@shutemov.name>
---
 builtin/config.c       | 39 ++++++++++++++++++++++++++-------------
 cache.h                |  1 +
 config.c               | 41 +++++++++++++++++++++++++++--------------
 t/t1300-repo-config.sh | 17 +++++++++++++++--
 4 files changed, 69 insertions(+), 29 deletions(-)

diff --git a/builtin/config.c b/builtin/config.c
index 92ebf23f0a9a..625f914c44a0 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -21,6 +21,7 @@ static char key_delim = ' ';
 static char term = '\n';
 
 static int use_global_config, use_system_config, use_local_config;
+static int use_stdin;
 static const char *given_config_file;
 static const char *given_config_blob;
 static int actions, types;
@@ -224,7 +225,7 @@ static int get_value(const char *key_, const char *regex_)
 	}
 
 	git_config_with_options(collect_config, &values,
-				given_config_file, given_config_blob,
+				use_stdin, given_config_file, given_config_blob,
 				respect_includes);
 
 	ret = !values.nr;
@@ -309,7 +310,7 @@ static void get_color(const char *def_color)
 	get_color_found = 0;
 	parsed_color[0] = '\0';
 	git_config_with_options(git_get_color_config, NULL,
-				given_config_file, given_config_blob,
+				use_stdin, given_config_file, given_config_blob,
 				respect_includes);
 
 	if (!get_color_found && def_color)
@@ -339,7 +340,7 @@ static int get_colorbool(int print)
 	get_diff_color_found = -1;
 	get_color_ui_found = -1;
 	git_config_with_options(git_get_colorbool_config, NULL,
-				given_config_file, given_config_blob,
+				use_stdin, given_config_file, given_config_blob,
 				respect_includes);
 
 	if (get_colorbool_found < 0) {
@@ -362,8 +363,11 @@ static int get_colorbool(int print)
 		return get_colorbool_found ? 0 : 1;
 }
 
-static void check_blob_write(void)
+static void check_write(void)
 {
+	if (use_stdin)
+		die("writing to stdin is not supported");
+
 	if (given_config_blob)
 		die("writing config blobs is not supported");
 }
@@ -435,7 +439,8 @@ static int get_urlmatch(const char *var, const char *url)
 	}
 
 	git_config_with_options(urlmatch_config_entry, &config,
-				given_config_file, NULL, respect_includes);
+				use_stdin, given_config_file, NULL,
+				respect_includes);
 
 	for_each_string_list_item(item, &values) {
 		struct urlmatch_current_candidate_value *matched = item->util;
@@ -476,6 +481,11 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		usage_with_options(builtin_config_usage, builtin_config_options);
 	}
 
+	if (given_config_file && !strcmp(given_config_file, "-")) {
+		given_config_file = NULL;
+		use_stdin = 1;
+	}
+
 	if (use_global_config) {
 		char *user_config = NULL;
 		char *xdg_config = NULL;
@@ -549,6 +559,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 	if (actions == ACTION_LIST) {
 		check_argc(argc, 0, 0);
 		if (git_config_with_options(show_all_config, NULL,
+					    use_stdin,
 					    given_config_file,
 					    given_config_blob,
 					    respect_includes) < 0) {
@@ -563,6 +574,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		check_argc(argc, 0, 0);
 		if (!given_config_file && nongit)
 			die("not in a git directory");
+		if (use_stdin)
+			die("editing stdin is not supported");
 		if (given_config_blob)
 			die("editing blobs is not supported");
 		git_config(git_default_config, NULL);
@@ -572,7 +585,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 	}
 	else if (actions == ACTION_SET) {
 		int ret;
-		check_blob_write();
+		check_write();
 		check_argc(argc, 2, 2);
 		value = normalize_value(argv[0], argv[1]);
 		ret = git_config_set_in_file(given_config_file, argv[0], value);
@@ -582,21 +595,21 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		return ret;
 	}
 	else if (actions == ACTION_SET_ALL) {
-		check_blob_write();
+		check_write();
 		check_argc(argc, 2, 3);
 		value = normalize_value(argv[0], argv[1]);
 		return git_config_set_multivar_in_file(given_config_file,
 						       argv[0], value, argv[2], 0);
 	}
 	else if (actions == ACTION_ADD) {
-		check_blob_write();
+		check_write();
 		check_argc(argc, 2, 2);
 		value = normalize_value(argv[0], argv[1]);
 		return git_config_set_multivar_in_file(given_config_file,
 						       argv[0], value, "^$", 0);
 	}
 	else if (actions == ACTION_REPLACE_ALL) {
-		check_blob_write();
+		check_write();
 		check_argc(argc, 2, 3);
 		value = normalize_value(argv[0], argv[1]);
 		return git_config_set_multivar_in_file(given_config_file,
@@ -623,7 +636,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		return get_urlmatch(argv[0], argv[1]);
 	}
 	else if (actions == ACTION_UNSET) {
-		check_blob_write();
+		check_write();
 		check_argc(argc, 1, 2);
 		if (argc == 2)
 			return git_config_set_multivar_in_file(given_config_file,
@@ -633,14 +646,14 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 						      argv[0], NULL);
 	}
 	else if (actions == ACTION_UNSET_ALL) {
-		check_blob_write();
+		check_write();
 		check_argc(argc, 1, 2);
 		return git_config_set_multivar_in_file(given_config_file,
 						       argv[0], NULL, argv[1], 1);
 	}
 	else if (actions == ACTION_RENAME_SECTION) {
 		int ret;
-		check_blob_write();
+		check_write();
 		check_argc(argc, 2, 2);
 		ret = git_config_rename_section_in_file(given_config_file,
 							argv[0], argv[1]);
@@ -651,7 +664,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 	}
 	else if (actions == ACTION_REMOVE_SECTION) {
 		int ret;
-		check_blob_write();
+		check_write();
 		check_argc(argc, 1, 1);
 		ret = git_config_rename_section_in_file(given_config_file,
 							argv[0], NULL);
diff --git a/cache.h b/cache.h
index dc040fb1aa99..538c28a1564a 100644
--- a/cache.h
+++ b/cache.h
@@ -1155,6 +1155,7 @@ extern void git_config_push_parameter(const char *text);
 extern int git_config_from_parameters(config_fn_t fn, void *data);
 extern int git_config(config_fn_t fn, void *);
 extern int git_config_with_options(config_fn_t fn, void *,
+				   int use_stdin,
 				   const char *filename,
 				   const char *blob_ref,
 				   int respect_includes);
diff --git a/config.c b/config.c
index d969a5aefc2b..53dd39f0b9ef 100644
--- a/config.c
+++ b/config.c
@@ -1030,24 +1030,34 @@ 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,
+			       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 = 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 = 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, "<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, data);
 		fclose(f);
 	}
 	return ret;
@@ -1170,6 +1180,7 @@ int git_config_early(config_fn_t fn, void *data, const char *repo_config)
 }
 
 int git_config_with_options(config_fn_t fn, void *data,
+			    int use_stdin,
 			    const char *filename,
 			    const char *blob_ref,
 			    int respect_includes)
@@ -1189,6 +1200,8 @@ 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 (use_stdin)
+		return git_config_from_stdin(fn, data);
 	if (filename)
 		return git_config_from_file(fn, filename, data);
 	else if (blob_ref)
@@ -1203,7 +1216,7 @@ int git_config_with_options(config_fn_t fn, void *data,
 
 int git_config(config_fn_t fn, void *data)
 {
-	return git_config_with_options(fn, data, NULL, NULL, 1);
+	return git_config_with_options(fn, data, 0, NULL, NULL, 1);
 }
 
 /*
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 967359344dab..c9c426c273e5 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 &&
 	(
-- 
 Kirill A. Shutemov

Re: [PATCH] config: git_config_from_file(): handle "-" filename as stdin

[Junio C Hamano]

"Kirill A. Shutemov" <kirill@shutemov.name> writes:

> On Fri, Feb 14, 2014 at 10:27:11AM -0800, Junio C Hamano wrote:
> ...
>> I recall that an earlier implementation of "git diff --no-index"
>> that made "-" read one side to be compared from the standard input
>> had exactly the same issue of comparing filename with "-", which we
>> had to fix with code reorganization recently.  I'd prefer to see
>> this update to "git config --file -" done the right way from the
>> start.
>
> Okay, reworked version is below. It's slightly more invasive then the
> original.

Thanks.

It looks more invasive mostly because you renamed check_blob_write()
to check_write().  While I think that rename is a right thing to do
(it used to be "if we are attempting to write to blob, signal an
error", but we could have called it check_write(), meaning "we are
attempting to write; error out if that should not be allowed"), it
would have been much easier to review and comment if this were done
as a separate clean-up preparatory patch.  It wouldn't have had to
touch this many lines, I would think.

And "clean-up existing code as a separate step, without changing the
behaviour, before starting to work on a new feature" is actively
encouraged around here.

> From 199e6a995bb5228578e66189ef586421a4d8d9ba Mon Sep 17 00:00:00 2001
> From: "Kirill A. Shutemov" <kirill@shutemov.name>
> Date: Fri, 14 Feb 2014 21:59:39 +0200
> Subject: [PATCH] config: teach "git config --file -" to read from the standard
>  input

I do not see a need for these four lines in your e-mail.  All the
necessary information is already in your e-mail header.  Please drop
them, perhaps except for the "Subject:" in-body header.

If you are going to have a discussion and then your proposed patch,
please do it this way (without the 8-space indentation I added for
illustration) using the "-- >8 --" scissors line:

        ...
        Okay, reworked version is below.

        -- >8 --
        Subject: config: teach "git config --file -" to read from the standard input

        Extend "git config --file" interface to allow reading from
        the standard input

        Editing or setting value is an error.

        Signed-off-by: ...
        ---
         diffstat and patch here
        
The in-body header "Subject: " is there to let you retitle the
commit.

> Editing stdin or setting value in stdin is an error.

Good thinking.  Even comes with tests to cover these cases.  Good.

> diff --git a/builtin/config.c b/builtin/config.c
> index 92ebf23f0a9a..625f914c44a0 100644
> --- a/builtin/config.c
> +++ b/builtin/config.c
> @@ -21,6 +21,7 @@ static char key_delim = ' ';
>  static char term = '\n';
>  
>  static int use_global_config, use_system_config, use_local_config;
> +static int use_stdin;
>  static const char *given_config_file;
>  static const char *given_config_blob;

If we are changing the function signature of git_config_with_options()
anyway, would it be even a better idea to introduce something like:

	struct config_source {
        	int use_stdin;
                const char *config_file;
                const char *config_blob;
	};

        static struct config_source given_config_source;

and have one _fewer_ parameter to the function, instead of adding
one extra parameter?

> diff --git a/config.c b/config.c
> index d969a5aefc2b..53dd39f0b9ef 100644
> --- a/config.c
> +++ b/config.c
> @@ -1030,24 +1030,34 @@ 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,
> +			       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 = 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 = 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, "<stdin>", stdin, data);
> +}

So the above callchain will set top.name to the string "<stdin>".
How would that interact with the code in handle_path_include() that
makes sure that relative config includes are only allowed in file
with known location?

Other than that, I didn't spot any issue in this round looks.  It
seems to be almost there.

Thanks.