Re: [PATCH 2/2] config: allow specifying config entries via envvar pairs

On Tue, Nov 17, 2020 at 07:37:34AM +0100, Patrick Steinhardt wrote:

quoted

then I'd feel comfortable making it a public-facing feature. And for
most cases it would be pretty pleasant to use (and for the unpleasant
ones, I'm not sure that a little quoting is any worse than the paired
environment variables found here).

I tend to disagree there. As long as you control keys and values
yourself it's not too hard, that's true. But as soon as you start
processing untrusted keys or values, then it's getting a lot harder.

E.g. suppose you create a fetch mirror for a user, where the source is
protected by a password. We don't want to write the password into the
gitconfig of the mirror repository. Passing it via `-C` will show up in
ps(1). Using GIT_CONFIG_PARAMETERS requires you to quote the value,
which contains arbitrary data, and if you fail to do that correctly you
now have an avenue for arbitrary config injection.

That scenario is roughly why I came up with the _KEY/_VALUE schema. It
requires no quoting, is trivial to set up (at least for its target
audience, which is mostly scripts or programs) and wouldn't show up in
ps(1).

True. Shell-quoting is pretty easy, but it's still a thing that the
caller has to do (and get right). Within Git we have nice routines for
that, but if you're calling from another program, you probably don't.

quoted

Yeah, scripts can currently assume that:

  unset $(git rev-parse --local-env-vars)

will drop any config from the environment. In some cases, having
rev-parse enumerate the GIT_CONFIG_KEY_* variables that are set would be
sufficient. But that implies that rev-parse is seeing the same
environment we're planning to clear. As it is now, a script is free to
use rev-parse to generate that list, hold on to it, and then use it
later.

Good point. Adjusting it would be trivial, though: unset all consecutive
GIT_CONFIG_KEY_$n keys and potentially also GIT_CONFIG_VALUE_$n until we
hit a gap. The parser would stop on the first gap anyway.

That doesn't help this case, which currently works:

  # remember the list so we don't have to invoke rev-parse in
  # each iteration of the loop
  vars=$(git rev-parse --local-env-vars)

  for repo in $repos; do
	# start with a clean slate
	unset $vars

	export GIT_DIR=$repo
	git do-some-thing

	# oops, these won't get cleared in the next go-round because
	# they weren't set when we called rev-parse
	export GIT_CONFIG_KEY_1=foo.bar
	export GIT_CONFIG_VALUE_1=whatever
	git do-another-thing
  done

Now I have no idea if anybody is doing something along those lines, and
it's a bit convoluted (I'd probably use a subshell). And obviously
nobody is doing this _exact_ thing yet, because the key/value variables
don't exist yet. So maybe it would all work out (the caller of this
script could have set git vars, but in that case we'd pick them up in
the rev-parse call).

So I dunno. It's probably unlikely to bite somebody, but it is a
departure from the current design.

-Peff