git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH 06/21] builtin/config: check for writeability after source is set up

From
Patrick Steinhardt <ps@pks.im>
Date
May 10, 2024, 11:24 UTC
Message-ID
<edfd7caa39ab990faf5b49a6c572a612a35dbdd5.1715339393.git.ps@pks.im>
In-Reply-To
<cover.1715339393.git.ps@pks.im>

The `check_write()` function verifies that we do not try to write to a config source that cannot be written to, like for example stdin. But while the new subcommands do call this function, they do so before calling `handle_config_location()`. Consequently, we only end up checking the default config location for writeability, not the location that was actually specified by the caller of git-config(1).

Fix this by calling `check_write()` after `handle_config_location()`. We will further clarify the relationship between those two functions in a subsequent commit where we remove the global state that both implicitly rely on.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/config.c  | 10 +++++-----
 t/t1300-config.sh |  6 ++++++
 2 files changed, 11 insertions(+), 5 deletions(-)
diff --git a/builtin/config.c b/builtin/config.c
index 8f3342b593..9295a2f737 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -843,7 +843,6 @@ static int cmd_config_set(int argc, const char **argv, const char *prefix)
 
 	argc = parse_options(argc, argv, prefix, opts, builtin_config_set_usage,
 			     PARSE_OPT_STOP_AT_NON_OPTION);
-	check_write();
 	check_argc(argc, 2, 2);
 
 	if ((flags & CONFIG_FLAGS_FIXED_VALUE) && !value_pattern)
@@ -856,6 +855,7 @@ static int cmd_config_set(int argc, const char **argv, const char *prefix)
 	comment = git_config_prepare_comment_string(comment_arg);
 
 	handle_config_location(prefix);
+	check_write();
 
 	value = normalize_value(argv[0], argv[1], &default_kvi);
 
@@ -891,13 +891,13 @@ static int cmd_config_unset(int argc, const char **argv, const char *prefix)
 
 	argc = parse_options(argc, argv, prefix, opts, builtin_config_unset_usage,
 			     PARSE_OPT_STOP_AT_NON_OPTION);
-	check_write();
 	check_argc(argc, 1, 1);
 
 	if ((flags & CONFIG_FLAGS_FIXED_VALUE) && !value_pattern)
 		die(_("--fixed-value only applies with 'value-pattern'"));
 
 	handle_config_location(prefix);
+	check_write();
 
 	if ((flags & CONFIG_FLAGS_MULTI_REPLACE) || value_pattern)
 		return git_config_set_multivar_in_file_gently(given_config_source.file,
@@ -918,10 +918,10 @@ static int cmd_config_rename_section(int argc, const char **argv, const char *pr
 
 	argc = parse_options(argc, argv, prefix, opts, builtin_config_rename_section_usage,
 			     PARSE_OPT_STOP_AT_NON_OPTION);
-	check_write();
 	check_argc(argc, 2, 2);
 
 	handle_config_location(prefix);
+	check_write();
 
 	ret = git_config_rename_section_in_file(given_config_source.file,
 						argv[0], argv[1]);
@@ -943,10 +943,10 @@ static int cmd_config_remove_section(int argc, const char **argv, const char *pr
 
 	argc = parse_options(argc, argv, prefix, opts, builtin_config_remove_section_usage,
 			     PARSE_OPT_STOP_AT_NON_OPTION);
-	check_write();
 	check_argc(argc, 1, 1);
 
 	handle_config_location(prefix);
+	check_write();
 
 	ret = git_config_rename_section_in_file(given_config_source.file,
 						argv[0], NULL);
@@ -997,10 +997,10 @@ static int cmd_config_edit(int argc, const char **argv, const char *prefix)
 	};
 
 	argc = parse_options(argc, argv, prefix, opts, builtin_config_edit_usage, 0);
-	check_write();
 	check_argc(argc, 0, 0);
 
 	handle_config_location(prefix);
+	check_write();
 
 	return show_editor();
 }
diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index d90a69b29f..9de2d95f06 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -2835,6 +2835,12 @@ test_expect_success 'specifying multiple modes causes failure' '
 	test_cmp expect err
 '
 
+test_expect_success 'writing to stdin is rejected' '
+	echo "fatal: writing to stdin is not supported" >expect &&
+	test_must_fail git config ${mode_set} --file - foo.bar baz 2>err &&
+	test_cmp expect err
+'
+
 done
 
 test_done
-- 
2.45.GIT
Previous: Patrick SteinhardtNext: Kyle Lippincott
Message 9 of 81 in “builtin/config: remove global state”
  1. 00/21 builtin/config: remove global statePatrick Steinhardt, May 10, 2024
  2. 01/21 builtin/config: stop printing full usage on misusePatrick Steinhardt, May 10, 2024
  3. 02/21 builtin/config: move legacy mode into its own functionPatrick Steinhardt, May 10, 2024
  4. 03/21 builtin/config: move subcommand options into `cmd_config()`Patrick Steinhardt, May 10, 2024
  5. 04/21 builtin/config: move legacy options into `cmd_config()`Patrick Steinhardt, May 10, 2024
  6. 05/21 builtin/config: move actions into `cmd_config_actions()`Patrick Steinhardt, May 10, 2024
  7. Kyle LippincottMay 10, 2024
  8. Patrick SteinhardtMay 13, 2024
  9. 06/21 builtin/config: check for writeability after source is set upPatrick Steinhardt, May 10, 2024
  10. Kyle LippincottMay 10, 2024
  11. Patrick SteinhardtMay 13, 2024
  12. 07/21 config: make the config source constPatrick Steinhardt, May 10, 2024
  13. 08/21 builtin/config: refactor functions to have common exit pathsPatrick Steinhardt, May 10, 2024
  14. 09/21 builtin/config: move location options into local variablesPatrick Steinhardt, May 10, 2024
  15. Patrick SteinhardtMay 10, 2024
  16. 10/21 builtin/config: move display options into local variablesPatrick Steinhardt, May 10, 2024
  17. 11/21 builtin/config: move type options into display optionsPatrick Steinhardt, May 10, 2024
  18. 12/21 builtin/config: move default value into display optionsPatrick Steinhardt, May 10, 2024
  19. 13/21 builtin/config: move `respect_includes_opt` into location optionsPatrick Steinhardt, May 10, 2024
  20. 14/21 builtin/config: convert `do_not_match` to a local variablePatrick Steinhardt, May 10, 2024
  21. Eric SunshineMay 11, 2024
  22. 15/21 builtin/config: convert `value_pattern` to a local variablePatrick Steinhardt, May 10, 2024
  23. 16/21 builtin/config: convert `regexp` to a local variablePatrick Steinhardt, May 10, 2024
  24. 17/21 builtin/config: convert `key_regexp` to a local variablePatrick Steinhardt, May 10, 2024
  25. 18/21 builtin/config: convert `key` to a local variablePatrick Steinhardt, May 10, 2024
  26. 19/21 builtin/config: track "fixed value" option via flags onlyPatrick Steinhardt, May 10, 2024
  27. Eric SunshineMay 11, 2024
  28. 20/21 builtin/config: convert flags to a local variablePatrick Steinhardt, May 10, 2024
  29. 21/21 builtin/config: pass data between callbacks via local variablesPatrick Steinhardt, May 10, 2024
  30. Junio C HamanoMay 10, 2024
  31. Kyle LippincottMay 10, 2024
  32. 00/21 builtin/config: remove global statePatrick Steinhardt, May 13, 2024
  33. 01/21 builtin/config: stop printing full usage on misusePatrick Steinhardt, May 13, 2024
  34. 02/21 builtin/config: move legacy mode into its own functionPatrick Steinhardt, May 13, 2024
  35. 03/21 builtin/config: move subcommand options into `cmd_config()`Patrick Steinhardt, May 13, 2024
  36. 04/21 builtin/config: move legacy options into `cmd_config()`Patrick Steinhardt, May 13, 2024
  37. 05/21 builtin/config: move actions into `cmd_config_actions()`Patrick Steinhardt, May 13, 2024
  38. 06/21 builtin/config: check for writeability after source is set upPatrick Steinhardt, May 13, 2024
  39. Taylor BlauMay 14, 2024
  40. Patrick SteinhardtMay 15, 2024
  41. 07/21 config: make the config source constPatrick Steinhardt, May 13, 2024
  42. 08/21 builtin/config: refactor functions to have common exit pathsPatrick Steinhardt, May 13, 2024
  43. 09/21 builtin/config: move location options into local variablesPatrick Steinhardt, May 13, 2024
  44. 10/21 builtin/config: move display options into local variablesPatrick Steinhardt, May 13, 2024
  45. 11/21 builtin/config: move type options into display optionsPatrick Steinhardt, May 13, 2024
  46. 12/21 builtin/config: move default value into display optionsPatrick Steinhardt, May 13, 2024
  47. 13/21 builtin/config: move `respect_includes_opt` into location optionsPatrick Steinhardt, May 13, 2024
  48. 14/21 builtin/config: convert `do_not_match` to a local variablePatrick Steinhardt, May 13, 2024
  49. 15/21 builtin/config: convert `value_pattern` to a local variablePatrick Steinhardt, May 13, 2024
  50. 16/21 builtin/config: convert `regexp` to a local variablePatrick Steinhardt, May 13, 2024
  51. 17/21 builtin/config: convert `key_regexp` to a local variablePatrick Steinhardt, May 13, 2024
  52. 18/21 builtin/config: convert `key` to a local variablePatrick Steinhardt, May 13, 2024
  53. 19/21 builtin/config: track "fixed value" option via flags onlyPatrick Steinhardt, May 13, 2024
  54. 20/21 builtin/config: convert flags to a local variablePatrick Steinhardt, May 13, 2024
  55. 21/21 builtin/config: pass data between callbacks via local variablesPatrick Steinhardt, May 13, 2024
  56. Kyle LippincottMay 13, 2024
  57. Junio C HamanoMay 14, 2024
  58. Patrick SteinhardtMay 14, 2024
  59. Patrick SteinhardtMay 15, 2024
  60. 00/21 builtin/config: remove global statePatrick Steinhardt, May 15, 2024
  61. 01/21 builtin/config: stop printing full usage on misusePatrick Steinhardt, May 15, 2024
  62. 02/21 builtin/config: move legacy mode into its own functionPatrick Steinhardt, May 15, 2024
  63. 03/21 builtin/config: move subcommand options into `cmd_config()`Patrick Steinhardt, May 15, 2024
  64. 04/21 builtin/config: move legacy options into `cmd_config()`Patrick Steinhardt, May 15, 2024
  65. 05/21 builtin/config: move actions into `cmd_config_actions()`Patrick Steinhardt, May 15, 2024
  66. 06/21 builtin/config: check for writeability after source is set upPatrick Steinhardt, May 15, 2024
  67. 07/21 config: make the config source constPatrick Steinhardt, May 15, 2024
  68. 08/21 builtin/config: refactor functions to have common exit pathsPatrick Steinhardt, May 15, 2024
  69. 09/21 builtin/config: move location options into local variablesPatrick Steinhardt, May 15, 2024
  70. 10/21 builtin/config: move display options into local variablesPatrick Steinhardt, May 15, 2024
  71. 11/21 builtin/config: move type options into display optionsPatrick Steinhardt, May 15, 2024
  72. 12/21 builtin/config: move default value into display optionsPatrick Steinhardt, May 15, 2024
  73. 13/21 builtin/config: move `respect_includes_opt` into location optionsPatrick Steinhardt, May 15, 2024
  74. 14/21 builtin/config: convert `do_not_match` to a local variablePatrick Steinhardt, May 15, 2024
  75. 15/21 builtin/config: convert `value_pattern` to a local variablePatrick Steinhardt, May 15, 2024
  76. 16/21 builtin/config: convert `regexp` to a local variablePatrick Steinhardt, May 15, 2024
  77. 17/21 builtin/config: convert `key_regexp` to a local variablePatrick Steinhardt, May 15, 2024
  78. 18/21 builtin/config: convert `key` to a local variablePatrick Steinhardt, May 15, 2024
  79. 19/21 builtin/config: track "fixed value" option via flags onlyPatrick Steinhardt, May 15, 2024
  80. 20/21 builtin/config: convert flags to a local variablePatrick Steinhardt, May 15, 2024
  81. 21/21 builtin/config: pass data between callbacks via local variablesPatrick Steinhardt, May 15, 2024

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.