Re: [PATCH v4 1/3] config: rename `git_etc_config()`

From: Jeff King <peff@peff.net>
Date: Apr 23, 2021, 09:27 UTC
Message-ID: <YIKTDjOX3WtBjqAq@coredump.intra.peff.net>
In-Reply-To: <YH1jnIhuF4UjXVNW@tanuki>

On Mon, Apr 19, 2021 at 01:03:56PM +0200, Patrick Steinhardt wrote:

Show 12 quoted lines

> It's only by accident that I dropped the call to `git_config_system()`,
> must've happened when resolving conflicts somewhere. The issue with just
> returning `/dev/null` from `git_system_config()` is that git-config(1)
> would be broken, as you hint at. We do not honor GIT_CONFIG_NOSYSTEM
> there if the "--system" flag was given.
> 
> So yes, we could change it to return `/dev/null`, but that would change
> semantics of GIT_CONFIG_NOSYSTEM. I'm not sure doing this in the same
> series is a good idea. Even more so because with returning `/dev/null`,
> the conversion would be silent: whereas previous versions simply wrote
> to or read from the system-level config, we now pretend to have read or
> written something even though we didn't.

I'm OK being more cautious here, and leaving NOSYSTEM as it is.

My original thinking was that returning /dev/null would be an improvement even for the existing call in "git config --system". And I do think it would be for reading. But it actually gets pretty weird for writing, because of our atomic-rename strategy.

And that is actually true of your new variable too. Doing this:

  GIT_CONFIG_SYSTEM=/dev/null git config --system foo.bar

would return nothing, which makes sense to me. But this:

  GIT_CONFIG_SYSTEM=/dev/null git config --system foo.bar value

would try to write /dev/null.lock, and then rename it into place over the real /dev/null! That may be slightly surprising, but it is not really any different than any other non-regular-file entry.

The stakes are slightly higher here, as it breaks all sorts of things that can be hard to recover from (even if you know the correct mknod incantation, important things like bash seem to get crabby if they can't open /dev/null for writing). And it may be more likely to happen by mistake (as opposed to "git config --file=/dev/null", which behaves the same). But unless you are root, you are likely to just get an error in the first place, so it seems like an unlikely mistake in practice.

So I think it may fall into "if it hurts, don't do it".

-Peff