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

[Kyle Lippincott <spectral@google.com>]

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
>