On Mon, Sep 27 2021, Jeff King wrote:
On Mon, Sep 27, 2021 at 09:30:41AM -0700, Junio C Hamano wrote:
quoted
quoted
quoted
This asserts what? FOO=" bar"?
Whoops, that should have been "envIs", asserting that $FOO contains
"bar".
Oh, "can we check with a literal with leading whitespace?" was what
my question was about ;-)
My assumption was that nobody would really care about doing so. It is
true that it's less flexible, though (and is a decision we can't easily
take back later).
Yeah, I think nobody really cares about stripspace() or not for this
sort of feature.
I do think having implicit and explicit complexity like that makes it
harder to document, implement and understand for users though. I.e. is
"env:FOO == bar" the same as "env:FOO ==bar" etc., what whitespace
exactly is accepted etc.
quoted
quoted
As I said, I think it matters more with the infix operators, as:
[includeIf "env:FOO == bar"]
is more readable than:
[includeIf "env:FOO==bar"]
Sure, but at that point, we'd probably want some quoting mechanism
for the literal to be compared, e.g.
[includeIf "env:PATH ~= \"(:|^)/usr/bin(:|$)\""]
Ick. The extra quoting of the internal double-quotes is pretty horrid to
look at. Also, how does one match a double-quote in the value? \\\"?
If it were optional, that would make the common cases easy (no dq, no
whitespace), and the hard ones possible.
An implicit assumption of mine in the simpler positive-match-only
version (which I should have made clear) is that anyone who needs this
sort of complexity can just arrange to wrap their "git" in a function,
or do this sort of thing in their ~/.bashrc, i.e. just:
if code_of_arbitrary_complexity
then
export GIT_DO_XYZ_INCLUDES=1
fi
Then in your config:
includeIf.envBool:GIT_DO_XYZ_INCLUDES.path=~/.gitconfig.d/xyz.cfg
And having written that out I think the best thing to do is probably to
have a version that only does the envExists and envBool version (or just
envBool), and skip envIs and envMatch entirely.
In the case of env:PATH we're just setting users up for some buggy or
unexpected interaction with something that would be better done either
via a gitdir include, or if they really need $PATH they can just wrap
"git" in a function that sets a boolean inclusion variable.
That would get us out of having to support emergent behavior where some
git tool invoked via run_command() or something chdir's somewhere as an
implementation detail, and such an env:PATH match means we'd need to
support that, or potentially break existing user config.
Or, since we might not be invoked via a shell, the same issue with a
$PATH being "stale" from the POV of a user who's wondering why say a
command like:
# status in the "t" subdirectory
git -C t <cmd> <question>
Doesn't have the "right" $PWD, which we might not have as some future
shortcut in <cmd> decided not to bother chdir()-ing to answer the user's
<question>.
I think this is getting into a bit of a digression, though. I'm willing
to defer to Ævar, who is doing the actual work, and I don't know if he
has found any of this compelling. ;)
quoted
quoted
But I do think:
[includeIf "envIs:FOO:bar"]
is harder to read than even:
[includeIf "envIs:FOO: bar"]
Hmph, that's quite subjective, I am afraid. When I see the latter
in the configuration file, "do I have to have a single space before
'bar' in the value of $FOO" would be the first question that would
come to my mind.
I think it's just the mashed-up colons that I find ugly in the first
one. But I agree the latter isn't that nice either, and introduces the
ambiguity you describe.
FWIW I hacked up a --config-key --config-value pairing so you could set
keys with "=" in them on the command-line, I'm not sure I like the
interface, but it gets rid of that ":" v.s. "=" edge case:
https://github.com/avar/git/commit/a86053df48b
quoted
With an understanding that our syntax is so limited that we cannot
even write '=' and need to resort to Is: instead, I'd actually find
that the former less confusing than the latter.
That I think is the most interesting question: is the "=" actually
out-of-bounds? I tend to think not, based on our responses earlier in
the thread.