| Message ID | edfd7caa39ab990faf5b49a6c572a612a35dbdd5.1715339393.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | builtin/config: remove global state | expand |
On Fri, May 10, 2024 at 4:26 AM Patrick Steinhardt <ps@pks.im> wrote: > > 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. Minor nit/question since I'm still pretty inexperienced w/ the project norms: Since this is a bug fix/behavior change, can we reorder the series so this comes before (or after) the rest of the cleanups? I'm assuming this fix would be something that could stand alone in its own series even if we weren't making the other changes. > > 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 >
On Fri, May 10, 2024 at 01:46:43PM -0700, Kyle Lippincott wrote: > On Fri, May 10, 2024 at 4:26 AM Patrick Steinhardt <ps@pks.im> wrote: > > > > 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. > > Minor nit/question since I'm still pretty inexperienced w/ the project norms: > Since this is a bug fix/behavior change, can we reorder the series so > this comes before (or after) the rest of the cleanups? I'm assuming > this fix would be something that could stand alone in its own series > even if we weren't making the other changes. Yeah, it can indeed stand on its own. The reason why I moved it into the middle of the series is so that the subsequent patch will immediately refactor the reason why this bug was able to sneak in, namely the implicit dependency on a global variable. I thus lean towards keeping the order as-is, also because the patch itself can be cleanly cherry-picked on top of ps/config-subcommands regardless of the order. Patrick
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
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(-)