Re: [PATCH v2 0/2] clang-format

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Aug 14, 2017 at 08:03:19PM -0700, Junio C Hamano wrote:

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



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux