| Message ID | 7ab99a27c16718ad4dff1f7862e80c52b48c3812.1715595550.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | builtin/config: remove global state | expand |
On Mon, May 13, 2024 at 12:22:28PM +0200, Patrick Steinhardt wrote: > diff --git a/builtin/config.c b/builtin/config.c > index 0842e4f198..9866d1a055 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); Nice catch. I thought about suggesting that check_write() could be inlined into handle_config_location(). But that is not a good idea, since we only care about calling check_write() when we are actually going to write something. In the spots outside of cmd_config_actions() where we explicitly call check_write(), we do so because we know we're about to write something (e.g., from cmd_config_set(), cmd_config_unset(), etc.). But in the classic mode we only want to call check_write() when the action selected tells us that we're going to write something. I do wonder if we could set some "initialized" bit on the given_config_source variable so that it is a BUG() to call check_write() before it is set. Thanks, Taylor
On Tue, May 14, 2024 at 05:45:08PM -0400, Taylor Blau wrote: > On Mon, May 13, 2024 at 12:22:28PM +0200, Patrick Steinhardt wrote: > > diff --git a/builtin/config.c b/builtin/config.c > > index 0842e4f198..9866d1a055 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); > > Nice catch. > > I thought about suggesting that check_write() could be inlined into > handle_config_location(). But that is not a good idea, since we only > care about calling check_write() when we are actually going to write > something. > > In the spots outside of cmd_config_actions() where we explicitly call > check_write(), we do so because we know we're about to write something > (e.g., from cmd_config_set(), cmd_config_unset(), etc.). > > But in the classic mode we only want to call check_write() when the > action selected tells us that we're going to write something. Yeah, I was also wondering whether we want to refactor this, e.g. by passing in an additional parameter to `handle_config_location()` that tells it whether we want to read or write. But as you noted, this would be trivial for the new subcommand modes, but harder for the acton mode. So I refrained from doing that. > I do wonder if we could set some "initialized" bit on the > given_config_source variable so that it is a BUG() to call check_write() > before it is set. We could do that, but with the subsequent patch I think it's not as important anymore. The main problem here is that it was not obvious at all that `check_write()` and `handle_config_location()` have anything to do with each other because they both accessed global state. With the next patch we make that dependency explicit by accepting it as a param, and with that it becomes clearer that `check_write()` depends on a properly initialized variable. Does that work for you? Patrick
diff --git a/builtin/config.c b/builtin/config.c index 0842e4f198..9866d1a055 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
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(-)