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:
signature.asc
Description: PGP signature
