Jeff King [off-list ref] writes:
On Mon, Aug 14, 2017 at 04:15:40PM -0700, Stefan Beller wrote:
quoted
+ llvm-dev@lists.llvm.org
The Git community is currently discussing adopting a coding style
defined by clang-format, here is a bug report:
Since we've added a cc, let me try to give a little more context.
Thanks for relaying; I've dropped them from CC: for this message, as
the remainder is not of interest to them.
I'm tempted to say that "config --list" should normalize this case into:
mysection.mykey=true
Normally we avoid coercing values without knowing the context in which
they'll be used. But the syntax in the original file means the user is
telling us it's a boolean and they expect it to be treated that way.
The only downside is if the user is wrong, it might be coerced into
the string "true" instead of throwing an error. That seems like a minor
drawback for eliminating a potentially confusing corner case from the
plumbing output.
Because we cannot sensibly always normalize a variable set to 'yes',
'on', etc. to all "true", the degree it can help the reading scripts
is quite limited, as they need to be prepared to see other
representation of the truth values anyway. Even though I too found
the approach somewhat tempting, because there is no ambiguity in
"[section] var" that it means a boolean "true", I doubt it would
help very much.
The way they pass "non_string_options" dict to the loader is quite
sensible for that purpose, as it allows the reader to say "I care
about this and that variables, and I know I want them interpreted as
int (e.g. 1M) and bool (e.g. 'on') and returned in a normalized
form".
I do not mind adding "git config --list --autotype" option, though,
with which the reading script tells us that it accepts the chance of
false conversion, so that
[my]
intVal = 2k
boolVal
someVal = on
otherVal = 1
moreVal = 2
anotherVal = 0
no = no
might be listed as
my.intval=2048
my.boolval=true
my.someval=true
my.otherval=1
my.moreval=2
my.anotherval=0
my.no=false
Disambiguation rules are up to debate; the above illustrates an
extreme position that coerses anything that could be taken as an int
and a string that can be taken as a bool as such, but it may not
help very much if the reader wants to see boolean values.
On Mon, Aug 14, 2017 at 08:03:19PM -0700, Junio C Hamano wrote:
quoted
I'm tempted to say that "config --list" should normalize this case into:
mysection.mykey=true
Normally we avoid coercing values without knowing the context in which
they'll be used. But the syntax in the original file means the user is
telling us it's a boolean and they expect it to be treated that way.
The only downside is if the user is wrong, it might be coerced into
the string "true" instead of throwing an error. That seems like a minor
drawback for eliminating a potentially confusing corner case from the
plumbing output.
Because we cannot sensibly always normalize a variable set to 'yes',
'on', etc. to all "true", the degree it can help the reading scripts
is quite limited, as they need to be prepared to see other
representation of the truth values anyway. Even though I too found
the approach somewhat tempting, because there is no ambiguity in
"[section] var" that it means a boolean "true", I doubt it would
help very much.
Good point. This is the only case that is _syntactically_ a problem at
the key/value level, which is why we noticed it (the reader barfed on
the unknown input). But that same reader could be interpreting values
incorrectly and we'd have no idea. And that applies to types beyond
booleans (you showed numbers like "2k" below, but there are others, like
--path or --get-color).
The one nice thing about fixing this syntactic issue is that the current
behavior affected this reader even though it didn't care about
particular config key in question. I.e., in this output:
my.boolVal
my.intVal=2k
my.valueWeCareAbout=some string
if we don't care about the meaning of boolVal or intVal, we could still
parse this output fine if not for the syntactic irregularity of
my.boolVal.
That's what might tempt me to fix this independent of the deeper
problem. But I really think we should try to address that deeper
problem.
The way they pass "non_string_options" dict to the loader is quite
sensible for that purpose, as it allows the reader to say "I care
about this and that variables, and I know I want them interpreted as
int (e.g. 1M) and bool (e.g. 'on') and returned in a normalized
form".
I do not mind adding "git config --list --autotype" option, though,
with which the reading script tells us that it accepts the chance of
false conversion, so that
[...]
I think an "--autotype" is always going to have false positives.
Integers and booleans we can make guesses at. But "--path" or "--color"
are much tougher.
The right answer is to make it easier for that non_string_options
information to make it to git-config so it can do the interpretation for
the caller. The way that happens now is:
git config --int my.intVal
git config --bool my.boolVal
git config --path my.pathVal
and so on. But I think one reason people turn to --list is that it
requires only a single process invocation, rather than repeatedly
calling git-config for each variable which might be of interest. I know
that diff-highlight's startup is measurably slower due to the six "git
config --get-color" calls it must make, and I've been looking for a way
to do it with a single invocation.
I suspect we need a "--get-stdin" which can accept (key,type) tuples and
return the result over stdout. And in some cases it's more than just a
pair; for example, colors need an extra "parse this default" argument).
-Peff