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

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 2520 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]