On Fri, May 10, 2024 at 01:46:43PM -0700, Kyle Lippincott wrote:

On Fri, May 10, 2024 at 4:26 AM Patrick Steinhardt [off-list ref] wrote:

quoted

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