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

[Patrick Steinhardt <ps@pks.im>]

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

On Fri, May 10, 2024 at 01:46:43PM -0700, Kyle Lippincott wrote:
> 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.

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

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