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

From: Taylor Blau <me@ttaylorr.com>
Date: May 14, 2024, 21:45 UTC
Message-ID: <ZkPbZGPq6xFrhngN@nand.local>
In-Reply-To: <7ab99a27c16718ad4dff1f7862e80c52b48c3812.1715595550.git.ps@pks.im>

On Mon, May 13, 2024 at 12:22:28PM +0200, Patrick Steinhardt wrote:

Show 19 quoted lines

> 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