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

On 2020-11-17 at 02:34:54, Jeff King wrote:

On Mon, Nov 16, 2020 at 11:39:35AM -0800, Junio C Hamano wrote:

quoted

quoted

quoted

While not document, it is currently possible to specify config entries
[in GIT_CONFIG_PARAMETERS]

[...]

"While not documented" yes, for sure, but we do not document it for
a good reason---it is a pure implementation detail between Git
process that runs another one as its internal implementation detail.

I have actually been quite tempted to document and promise that it will
continue to work. Because it really is useful in some instances. The
thing that has held me back is that the documentation would reveal how
unforgiving the parser is. ;)

It insists that key/value pairs are shell-quoted as a whole. I think if
we made it accept a some reasonable inputs:

  - do not require quoting for values that do not need it

  - allow any amount of shell-style single-quoting (whole parameters,
    just values, etc).

  - do not bother allowing other quoting, like double-quotes with
    backslashes. However, document backslash and double-quote as
    meta-characters that must not appear outside of single-quotes, to
    allow for future expansion.

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).

What if we didn't document it but provided a command that produced a
suitable value?  Maybe something like this:

  GIT_CONFIG_PARAMETERS=$(git rev-parse --quote-parameters a.b.c ENV_VAR d.e.f OTHER_ENV_VAR)

Or whatever we decide.

I don't personally love shell quoting as an interchange mechanism; I'd
prefer something like URI-encoding, which is a bit more standardized and
easier to reason about from a security perspective.  But if we decide to
change it, it doesn't matter, since it's still undocumented and this
would be the only acceptable way to pass config through the environment.

Alternatively, we could just do this:

  git with-config --key a.b.c --value ENV_VAR --key d.e.f --value OTHER_ENV_VAR --command git foo

That would also leave it undocumented, but make it easier to work with.

quoted

I especially do not think we want to read from unbounded number of
GIT_CONFIG_KEY_<N> variables like this patch does.  How would a
script cleanse its environment to protect itself from stray such
environment variable pair?  Count up from 1 to forever?  Run "env"
and grep for "GIT_CONFIG_KEY_[0-9]*=" (the answer is No.  What if
some environment variables have newline in its values?)

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.

I'm also uncomfortable with an arbitrary number of keys and values.  It
becomes very tricky to cleanse the environment, and even if the code
stops at the first gap, if you then add more entries, then you have to
cleanse again or risk a security problem.  I feel like this is only
going to bite us in the future.
-- 
brian m. carlson (he/him or they/them)
Houston, Texas, US