[PATCH] Documentation/git-add.txt: add `add.ginore-errors` configuration variable

[PATCH] Documentation/git-add.txt: add `add.ginore-errors` configuration variable

[Alexander Kuleshov]

'git add' supports not only `add.ignoreErrors`, but also `add.ignore-errors`
configuration variable.

Signed-off-by: Alexander Kuleshov <kuleshovmail@gmail.com>
---
 Documentation/git-add.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
index 1c74907..f68c2a2 100644
--- a/Documentation/git-add.txt
+++ b/Documentation/git-add.txt
@@ -155,8 +155,8 @@ for "git add --no-all <pathspec>...", i.e. ignored removed files.
 	If some files could not be added because of errors indexing
 	them, do not abort the operation, but continue adding the
 	others. The command shall still exit with non-zero status.
-	The configuration variable `add.ignoreErrors` can be set to
-	true to make this the default behaviour.
+	The configuration variable `add.ignoreErrors` or `add.ignore-errors`
+	can be set to true to make this the default behaviour.
 
 --ignore-missing::
 	This option can only be used together with --dry-run. By using
-- 
2.3.0.rc1.275.g028c360

Re: [PATCH] Documentation/git-add.txt: add `add.ginore-errors` configuration variable

[Eric Sunshine]

On Mon, Jan 26, 2015 at 11:55 AM, Alexander Kuleshov
<kuleshovmail@gmail.com> wrote:
> 'git add' supports not only `add.ignoreErrors`, but also `add.ignore-errors`
> configuration variable.

See 6b3020a2 (add: introduce add.ignoreerrors synonym for
add.ignore-errors,  2010-12-01) for why this patch is undesirable.

> Signed-off-by: Alexander Kuleshov <kuleshovmail@gmail.com>
> ---
>  Documentation/git-add.txt | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
> index 1c74907..f68c2a2 100644
> --- a/Documentation/git-add.txt
> +++ b/Documentation/git-add.txt
> @@ -155,8 +155,8 @@ for "git add --no-all <pathspec>...", i.e. ignored removed files.
>         If some files could not be added because of errors indexing
>         them, do not abort the operation, but continue adding the
>         others. The command shall still exit with non-zero status.
> -       The configuration variable `add.ignoreErrors` can be set to
> -       true to make this the default behaviour.
> +       The configuration variable `add.ignoreErrors` or `add.ignore-errors`
> +       can be set to true to make this the default behaviour.
>
>  --ignore-missing::
>         This option can only be used together with --dry-run. By using
> --
> 2.3.0.rc1.275.g028c360

Re: [PATCH] Documentation/git-add.txt: add `add.ginore-errors` configuration variable

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Mon, Jan 26, 2015 at 11:55 AM, Alexander Kuleshov
> <kuleshovmail@gmail.com> wrote:
>> 'git add' supports not only `add.ignoreErrors`, but also `add.ignore-errors`
>> configuration variable.
>
> See 6b3020a2 (add: introduce add.ignoreerrors synonym for
> add.ignore-errors,  2010-12-01) for why this patch is undesirable.

We might want to do this instead, as the correction to the
misspelled configuration variable name was from the 1.7.0-era that
is sufficiently old (v1.7.0.8 was done in December 2010).

-- >8 --
Subject: doc: clarify naming rules for configuration variables

Also mark deprecated variables in the documentation more clearly.

The text for the rules are partly taken from the log message of
Jonathan's 6b3020a2 (add: introduce add.ignoreerrors synonym for
add.ignore-errors, 2010-12-01).

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/CodingGuidelines | 25 +++++++++++++++++++++++++
 Documentation/config.txt       | 15 +++++++--------
 2 files changed, 32 insertions(+), 8 deletions(-)

diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
index 894546d..8fbac15 100644
--- a/Documentation/CodingGuidelines
+++ b/Documentation/CodingGuidelines
@@ -413,6 +413,31 @@ Error Messages
  - Say what the error is first ("cannot open %s", not "%s: cannot open")
 
 
+Externally Visible Names
+
+ - For configuration variable names, follow the existing convention:
+
+   . The section name indicates the affected subsystem.
+
+   . The subsection name, if any, indicates which of an unbounded set
+     of things to set the value for.
+
+   . The variable name describes the effect of tweaking this knob.
+
+   The section and variable names that consist of multiple words are
+   formed by concatenating the words without punctuations (e.g. `-`),
+   and are broken using bumpyCaps in documentation as a hint to the
+   reader.
+
+   When choosing the variable namespace, do not use variable name for
+   specifying possibly unbounded set of things, most notably anything
+   an end user can freely come up with (e.g. branch names), but also
+   large fixed set defined by the system that can grow over time
+   (e.g. what kind of common whitespace problems to notice).  Use
+   subsection names or variable values, like existing variables
+   branch.<name>.description and core.whitespace do, instead.
+
+
 Writing Documentation:
 
  Most (if not all) of the documentation pages are written in the
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 0a4d22a..8a76d1d 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -664,14 +664,13 @@ core.abbrev::
 	for abbreviated object names to stay unique for sufficiently long
 	time.
 
-add.ignore-errors::
 add.ignoreErrors::
+add.ignore-errors (deprecated)::
 	Tells 'git add' to continue adding files when some files cannot be
 	added due to indexing errors. Equivalent to the '--ignore-errors'
-	option of linkgit:git-add[1].  Older versions of Git accept only
-	`add.ignore-errors`, which does not follow the usual naming
-	convention for configuration variables.  Newer versions of Git
-	honor `add.ignoreErrors` as well.
+	option of linkgit:git-add[1].  `add.ignore-errors` is deprecated,
+	as it does not follow the usual naming convention for configuration
+	variables.
 
 alias.*::
 	Command aliases for the linkgit:git[1] command wrapper - e.g.
@@ -1924,7 +1923,7 @@ pack.useBitmaps::
 	true. You should not generally need to turn this off unless
 	you are debugging pack bitmaps.
 
-pack.writebitmaps::
+pack.writebitmaps (deprecated)::
 	This is a deprecated synonym for `repack.writeBitmaps`.
 
 pack.writeBitmapHashCache::
@@ -2235,7 +2234,7 @@ sendemail.smtpencryption::
 	See linkgit:git-send-email[1] for description.  Note that this
 	setting is not subject to the 'identity' mechanism.
 
-sendemail.smtpssl::
+sendemail.smtpssl (deprecated)::
 	Deprecated alias for 'sendemail.smtpencryption = ssl'.
 
 sendemail.smtpsslcertpath::
@@ -2273,7 +2272,7 @@ sendemail.thread::
 sendemail.validate::
 	See linkgit:git-send-email[1] for description.
 
-sendemail.signedoffcc::
+sendemail.signedoffcc (deprecated)::
 	Deprecated alias for 'sendemail.signedoffbycc'.
 
 showbranch.default::

[PATCH 0/3] Documenting naming rules for configuration variables

[Junio C Hamano]

So here is a rewrite of the same but split into three patches,
after seeing "add.ignore-errors" not marked exactly as deprecated.

The update to CodingGuidelines goes a bit beyond how to name things
(i.e. bumpyCase vs dashed-words, the latter of which is strongly
discouraged) but how the names should be structured (i.e. no
unbounded set of things as variable names).

We may have to also be careful about underscores (there is one I
know about: gitweb.remote_heads that is used by instaweb), which
might be a good starter topic for somebody new to the codebase.


Junio C Hamano (3):
  config.txt: clarify that add.ignore-errors is deprecated
  config.txt: mark deprecated variables more prominently
  CodingGuidelines: describe naming rules for configuration variables

 Documentation/CodingGuidelines | 25 +++++++++++++++++++++++++
 Documentation/config.txt       | 15 +++++++--------
 2 files changed, 32 insertions(+), 8 deletions(-)

-- 
2.3.0-rc2-149-gdd42ee9

[PATCH 1/3] config.txt: clarify that add.ignore-errors is deprecated

[Junio C Hamano]

The old text gave an impression that even in a new repository using
old form might be safer.  Only Git from pre 1.7.0 days choke on the
correctly named variable, which is ancient by today's standard.

We have no intention to remove the support for deprecated ones, but
let's make sure that we do not give room for confused questions such
as "why does core.sparse-checkout not work, when add.ignore-errors
does?"

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/config.txt | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 0a4d22a..69f30d6 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -664,14 +664,13 @@ core.abbrev::
 	for abbreviated object names to stay unique for sufficiently long
 	time.
 
-add.ignore-errors::
 add.ignoreErrors::
+add.ignore-errors::
 	Tells 'git add' to continue adding files when some files cannot be
 	added due to indexing errors. Equivalent to the '--ignore-errors'
-	option of linkgit:git-add[1].  Older versions of Git accept only
-	`add.ignore-errors`, which does not follow the usual naming
-	convention for configuration variables.  Newer versions of Git
-	honor `add.ignoreErrors` as well.
+	option of linkgit:git-add[1].  `add.ignore-errors` is deprecated,
+	as it does not follow the usual naming convention for configuration
+	variables.
 
 alias.*::
 	Command aliases for the linkgit:git[1] command wrapper - e.g.
-- 
2.3.0-rc2-149-gdd42ee9

[PATCH 2/3] config.txt: mark deprecated variables more prominently

[Junio C Hamano]

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/config.txt | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 69f30d6..8a76d1d 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -665,7 +665,7 @@ core.abbrev::
 	time.
 
 add.ignoreErrors::
-add.ignore-errors::
+add.ignore-errors (deprecated)::
 	Tells 'git add' to continue adding files when some files cannot be
 	added due to indexing errors. Equivalent to the '--ignore-errors'
 	option of linkgit:git-add[1].  `add.ignore-errors` is deprecated,
@@ -1923,7 +1923,7 @@ pack.useBitmaps::
 	true. You should not generally need to turn this off unless
 	you are debugging pack bitmaps.
 
-pack.writebitmaps::
+pack.writebitmaps (deprecated)::
 	This is a deprecated synonym for `repack.writeBitmaps`.
 
 pack.writeBitmapHashCache::
@@ -2234,7 +2234,7 @@ sendemail.smtpencryption::
 	See linkgit:git-send-email[1] for description.  Note that this
 	setting is not subject to the 'identity' mechanism.
 
-sendemail.smtpssl::
+sendemail.smtpssl (deprecated)::
 	Deprecated alias for 'sendemail.smtpencryption = ssl'.
 
 sendemail.smtpsslcertpath::
@@ -2272,7 +2272,7 @@ sendemail.thread::
 sendemail.validate::
 	See linkgit:git-send-email[1] for description.
 
-sendemail.signedoffcc::
+sendemail.signedoffcc (deprecated)::
 	Deprecated alias for 'sendemail.signedoffbycc'.
 
 showbranch.default::
-- 
2.3.0-rc2-149-gdd42ee9

[PATCH 3/3] CodingGuidelines: describe naming rules for configuration variables

[Junio C Hamano]

We may want to say something about command line option names in the
new section as well, but for now, let's make sure everybody is clear
on how to structure and name their configuration variables.

The text for the rules are partly taken from the log message of
Jonathan's 6b3020a2 (add: introduce add.ignoreerrors synonym for
add.ignore-errors, 2010-12-01).

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/CodingGuidelines | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
index 894546d..8fbac15 100644
--- a/Documentation/CodingGuidelines
+++ b/Documentation/CodingGuidelines
@@ -413,6 +413,31 @@ Error Messages
  - Say what the error is first ("cannot open %s", not "%s: cannot open")
 
 
+Externally Visible Names
+
+ - For configuration variable names, follow the existing convention:
+
+   . The section name indicates the affected subsystem.
+
+   . The subsection name, if any, indicates which of an unbounded set
+     of things to set the value for.
+
+   . The variable name describes the effect of tweaking this knob.
+
+   The section and variable names that consist of multiple words are
+   formed by concatenating the words without punctuations (e.g. `-`),
+   and are broken using bumpyCaps in documentation as a hint to the
+   reader.
+
+   When choosing the variable namespace, do not use variable name for
+   specifying possibly unbounded set of things, most notably anything
+   an end user can freely come up with (e.g. branch names), but also
+   large fixed set defined by the system that can grow over time
+   (e.g. what kind of common whitespace problems to notice).  Use
+   subsection names or variable values, like existing variables
+   branch.<name>.description and core.whitespace do, instead.
+
+
 Writing Documentation:
 
  Most (if not all) of the documentation pages are written in the
-- 
2.3.0-rc2-149-gdd42ee9

Re: [PATCH 3/3] CodingGuidelines: describe naming rules for configuration variables

[Michael Haggerty]

On 01/28/2015 11:33 PM, Junio C Hamano wrote:
> We may want to say something about command line option names in the
> new section as well, but for now, let's make sure everybody is clear
> on how to structure and name their configuration variables.
> 
> The text for the rules are partly taken from the log message of
> Jonathan's 6b3020a2 (add: introduce add.ignoreerrors synonym for
> add.ignore-errors, 2010-12-01).
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  Documentation/CodingGuidelines | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
> index 894546d..8fbac15 100644
> --- a/Documentation/CodingGuidelines
> +++ b/Documentation/CodingGuidelines
> @@ -413,6 +413,31 @@ Error Messages
>   - Say what the error is first ("cannot open %s", not "%s: cannot open")
>  
>  
> +Externally Visible Names
> +
> + - For configuration variable names, follow the existing convention:
> +
> +   . The section name indicates the affected subsystem.
> +
> +   . The subsection name, if any, indicates which of an unbounded set
> +     of things to set the value for.
> +
> +   . The variable name describes the effect of tweaking this knob.
> +
> +   The section and variable names that consist of multiple words are
> +   formed by concatenating the words without punctuations (e.g. `-`),
> +   and are broken using bumpyCaps in documentation as a hint to the
> +   reader.
> +
> +   When choosing the variable namespace, do not use variable name for
> +   specifying possibly unbounded set of things, most notably anything
> +   an end user can freely come up with (e.g. branch names), but also
> +   large fixed set defined by the system that can grow over time
> +   (e.g. what kind of common whitespace problems to notice).

I think we can all agree with this rule for "anything an end user can
freely come up with". Such sets are truly unbounded.

But what is the justification for applying it to "large fixed set
defined by the system that can grow over time"? Any set of items that
needs to be programmed one by one is not unbounded in the same sense. It
is true that it can grow over time, but there is a practical limit on
how many such options we would ever implement, and at any given time the
set has a well-defined, finite number of members.

I suppose that this is a reaction to Johannes's proposal [1] to add
configuration settings like

    git config fsck.badDate ignore

[2] which you didn't like [3] but I did [4]. (Did you miss [4], in which
I think I made some good arguments for Johannes's proposal? I don't
think you responded to it.)

I think it would be more productive to finish the concrete discussion
about the "fsck" proposal, and to use the insight gained in that
specific case to inform the decision about whether we need the new
general rule to cover "large fixed set[s] defined by the system".

> + [...] Use
> +   subsection names or variable values, like existing variables
> +   branch.<name>.description and core.whitespace do, instead.

But there is also a precedent for the opposite approach: "advice.*".

    [advice]
            pushUpdateRejected = true|false
            pushNonFFCurrent   = true|false
            statusHints        = true|false
            resolveConflict    = true|false

etc.

In fact, I would argue that

    [core]
            whitespace = blank-at-eol -indent-with-non-tab

is a bad model to follow, and would better have been expressed in a
style like

    [whitespace]
            blankAtEOL = warn
            indentWithNonTab = ignore

for the same reasons that I argued for "fsck.*". The latter style would
also have made it easier to add other choices like
"whitespace.spaceBeforeTab = error". Not that I am advocating that
particular enhancement; I am just giving an example of how the current
style of configuration is less flexible.

> [...]

Michael

[1] http://article.gmane.org/gmane.comp.version-control.git/261068
[2] Actually, the original proposal was for "fsck.bad-date", but I think
we agree that "fsck.badDate" would be the correct version of this proposal.
[3] http://article.gmane.org/gmane.comp.version-control.git/261738
[4] http://article.gmane.org/gmane.comp.version-control.git/262841

-- 
Michael Haggerty
mhagger@alum.mit.edu

Re: [PATCH 3/3] CodingGuidelines: describe naming rules for configuration variables

[Jeff King]

On Sun, Feb 01, 2015 at 06:12:38AM +0100, Michael Haggerty wrote:

> > +   When choosing the variable namespace, do not use variable name for
> > +   specifying possibly unbounded set of things, most notably anything
> > +   an end user can freely come up with (e.g. branch names), but also
> > +   large fixed set defined by the system that can grow over time
> > +   (e.g. what kind of common whitespace problems to notice).
> 
> I think we can all agree with this rule for "anything an end user can
> freely come up with". Such sets are truly unbounded.
> 
> But what is the justification for applying it to "large fixed set
> defined by the system that can grow over time"? Any set of items that
> needs to be programmed one by one is not unbounded in the same sense. It
> is true that it can grow over time, but there is a practical limit on
> how many such options we would ever implement, and at any given time the
> set has a well-defined, finite number of members.

I had the same reaction on reading this.

We should be striving to break config options down as much as possible
to single scalar values, because that is the only format that is
understood systematically by the config code.

If a config option's value is a list, then we have to come up with an
ad-hoc syntax for the list, which we parse in the config callback. And
that leaves users of "git config" to reinvent that parsing themselves
when they want to do simple things like "remove item B from the list".
I think the examples you gave over on the fsck thread all make the same
point.

> > + [...] Use
> > +   subsection names or variable values, like existing variables
> > +   branch.<name>.description and core.whitespace do, instead.
> 
> But there is also a precedent for the opposite approach: "advice.*".

The pager.*, color.* (and color.$program.*) examples come to mind. For
example, we did not add:

  [core]
  usePagerFor = log, diff, -status

but instead:

  [pager]
  log = true
  diff = true
  status = false

Not only is the latter easier to manipulate and examine with the
existing config tools, I think it is more flexible in the long run. We
later extended the syntax to allow:

  [pager]
  log = diff-highlight | less

which would have been even more awkward in the "userPagerFor" format
(you could use "log=...", of course, but now you need to get into
whitespace quoting and other complexities, all of which are handled
already by the config code in the latter case).

-Peff

Re: [PATCH 3/3] CodingGuidelines: describe naming rules for configuration variables

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> On 01/28/2015 11:33 PM, Junio C Hamano wrote:
>> +   When choosing the variable namespace, do not use variable name for
>> +   specifying possibly unbounded set of things, most notably anything
>> +   an end user can freely come up with (e.g. branch names), but also
>> +   large fixed set defined by the system that can grow over time
>> +   (e.g. what kind of common whitespace problems to notice).
>
> I think we can all agree with this rule for "anything an end user can
> freely come up with". Such sets are truly unbounded.
>
> But what is the justification for applying it to "large fixed set
> defined by the system that can grow over time"? Any set of items that
> needs to be programmed one by one is not unbounded in the same sense. It
> is true that it can grow over time, but there is a practical limit on
> how many such options we would ever implement, and at any given time the
> set has a well-defined, finite number of members.
>
> I suppose that this is a reaction to Johannes's proposal [1] to add
> configuration settings like
>
>     git config fsck.badDate ignore

Not really.

This started after I looked at the add.ignore-errors issue discussed
and I was trying to codify existing practices by running greps,
blames and logs on 'master' to see what potential pitfalls are, and
what good existing practices to follow suit we already had.

The fsck changes were not in my mind at all.

I can see it argued that for things that are completely independent
(e.g. the consequence of setting fsck.badDate can never be affected
by how fsck.missingTagger is configured), separate configuration
variables may not be wrong per-se, but I can see that a set of knobs
that would have been originally independent, as the operation grow
richer, gain more semantics to make them related, and at that point,
I doubt "they are internally independent; expose them as independent
to the end users" argument would hold water that well.

A good example is "core.whitespace", where you would have started
with a simple set of booleans ("blank-at-eol" and "space-before-tab"
are conceptually independent and will stay to be), but once you
start supporting "indent-with-non-tab" and "tab-in-indent" you
cannot pretend that they are independent.

And that is the "existing practice" I primarily had in mind.  We
didn't do

	whitespace.tabInIndent = true
        whitespace.indentWithNonTab = true

to pretend they are independent and still internally having to make
sure the consistency of the setting.  We structured the syntax for
ease of the end user (not scripter) to shorter

	core.whitespace = tab-in-indent,indent-with-non-tab

as we need the consistency thing either way (and it is easier to see
the consistency mistakes when they appear next to each other).

And I am happy that we chose wisely in an early version that didn't
use one-variable-per-knob but used list-of-features-as-value instead.


> [2] which you didn't like [3] but I did [4]. (Did you miss [4], in which
> I think I made some good arguments for Johannes's proposal? I don't
> think you responded to it.)

I saw it, found it as an argument between "not good" and "weak" (see
above), kept in my mailbox while trying to decide if it was worth
responding, and then forgot about it after I got busy dealing with
other topics that have more relevance to the upcoming release ;-)

> I think it would be more productive to finish the concrete discussion
> about the "fsck" proposal,...

Surely.

 (0) fsck.bad-date is out.

 (1) fsck.badDate is OK.  I expect there is very high chance for
     them to stay independent forever.

 (2) fsck.ignore = bad-date,... is still my preference, only if it
     aligns with existing core.whitespace precedence that allows
     users to leverage the familiarity.


I see Peff cites "pager.<cmd>", but I think it was something that we
would rather shouldn't have done, similar to "alias.<cmd>".  They
are bad precedents we shouldn't encourage new things to mimic.

But that is not from "one-variable-with-list-is-better" (it is not
better for these "independent" ones) but is purely from the syntax
point of view.  There is no good reason to force case insensitivity
to the command names used as the key in these cases, but we do
because we made them the last-level variable names, and we have to
avoid hyphens in the command names while at it if we want to be
consistent (and the consistency was the point of the original
add.ignore-errors discussion that started all this, after all).

If they were

    pager.diff.enable = true
    alias.co.command = checkout

the world would have been a lot better place.

Re: [PATCH 3/3] CodingGuidelines: describe naming rules for configuration variables

[Jeff King]

On Sun, Feb 01, 2015 at 12:18:34PM -0800, Junio C Hamano wrote:

> I can see it argued that for things that are completely independent
> (e.g. the consequence of setting fsck.badDate can never be affected
> by how fsck.missingTagger is configured), separate configuration
> variables may not be wrong per-se, but I can see that a set of knobs
> that would have been originally independent, as the operation grow
> richer, gain more semantics to make them related, and at that point,
> I doubt "they are internally independent; expose them as independent
> to the end users" argument would hold water that well.

I'm not sure I buy this argument. There are two syntaxes being discussed
here. Let's look at each.

Let's call this "type 1":

  [core]
  foo = bar -moof xyzzy=whatever

We have a single variable, but the value of that variable is essentially
a list of sub-variables. The sub-variables are either booleans (like
"bar" or "-moof") or possibly have values of their own (with an "=").
The sub-variables are tied together logically by being part of a single
"core.foo". Presumably the user can infer some relationship between them
through this.

Type 2 is more like:

  [foo]
  bar = true
  moof = false
  xyzzy = whatever

So we still have our same set of sub-variables, except now each is a
first-class config variable. They are tied together logically by being
part of a single section (I called it "foo" here, which drops the
"core"; but clearly we could give it whatever descriptive name we
wanted).

>From the user's perspective, I don't see how the implied relationships
are significantly different. In type 1, they are placed inside a single
value and in type 2, they are not. Both are a form of grouping.

Moreover, I am not even sure that the syntax is an important element in
communicating semantic relationships. In these examples, are "bar" and
"moof" dependent? Clearly they are _related_ by our grouping. But does
one depend on the other? Similarly, with the existing core.whitespace,
what tells you which of the sub-variables are related. The
"blank-at-eol" and "space-before-tab" variables are both
whitespace-related, but do not depend on each other at all. But
"indent-with-non-tab" and "tab-in-indent" are. Yet those two pairs are
represented the same way syntactically.

I don't think you can infer semantic independence from syntax. It's
simply too blunt an instrument (and as you noted, it may even change
over time).

> We structured the syntax for ease of the end user (not scripter) to
> shorter
> 
> 	core.whitespace = tab-in-indent,indent-with-non-tab
> 
> as we need the consistency thing either way (and it is easier to see
> the consistency mistakes when they appear next to each other).

I agree that this provides a slight advantage to "type 1", because it
requires syntactically that the definitions are near each other (whereas
with split variables, they can literally be in different files). And if
everything else were equal, that would be enough. But I think "type 1"
has a lot of other disadvantages. For example:

  1. I'm a user who has set my preferred core.whitespace in my
     ~/.gitconfig. A particular project I am working on uses an
     alternate tabwidth. How do I set that in the repo config without
     repeating my defaults?

  2. I'm writing a hook whose behavior depends on the whitespace
     settings. How do I ask git whether blank-at-eol is enabled?

  3. I'm a user who wants to set whitespace config. I prefer using "git
     config" to editing the file manually. How do I turn off
     blank-at-eol without disrupting my existing settings?

An astute reader will notice that case (1) is a double-edged sword. If
your defaults have "blank-at-eol" and you want to set
"indent-with-non-tab" in the repo, that's fine. If the default is
"tab-in-indent" and you want to set "indent-with-non-tab", then those
are in conflict (i.e., this is the exact thing you were complaining
about above).

But is it such a bad thing to have them in conflict? Can't we define a
set of rules that does what people expects? For example, by the "last
one wins" principle, any time we see "whitespace.tab-in-indent", it
clears the setting of "whitespace.indent-with-non-tab", and vice versa.
This isn't represented syntactically in the config file, but it is an
easy rule, does what people would want, and can be documented in
config.txt (which is where we have to talk about such semantic conflicts
anyway).

By the way, this is the exact case I am concerned about for fsck.*. Our
use case at GitHub would be something like:

  a. We set up sane defaults for fsck.* in /etc/gitconfig

  b. User complains that we will not accept their push, which contains
     objects with malformed committers.

  c. Support investigates, determines that the malformed objects are
     part of a well-established history, and that they are OK to enter.

  d. We relax fsck.committerIdent in that repo's $GIT_DIR/config file.

Copy-and-pasting the rest of the rules from (a) into the repo config
file in step (d) is not ideal.

> I see Peff cites "pager.<cmd>", but I think it was something that we
> would rather shouldn't have done, similar to "alias.<cmd>".  They
> are bad precedents we shouldn't encourage new things to mimic.
> 
> But that is not from "one-variable-with-list-is-better" (it is not
> better for these "independent" ones) but is purely from the syntax
> point of view.

Yeah, I'd agree that the problem there is orthogonal to the type 1/2
thing above. I don't think it has been a big deal in practice, just
because people with good taste do not name their commands with uppercase
anyway.

I'd be happy to transition to pager.*.enabled, etc, if we care. That is
a much more flexible system anyway, and I do not think there is any
backwards compatibility problem (we'd continue with alias.X as a synonym
for alias.X.command indefinitely). It would not even be that much work;
it is mostly that I have never seen anybody complain about it in the
real world.

-Peff

Re: [PATCH 3/3] CodingGuidelines: describe naming rules for configuration variables

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> From the user's perspective, I don't see how the implied relationships
> are significantly different. In type 1, they are placed inside a single
> value and in type 2, they are not. Both are a form of grouping.
>
> Moreover, I am not even sure that the syntax is an important element in
> communicating semantic relationships.

I think we are in agreement and I do not see how you can draw
different conclusions.  The above argument refutes the point Michael
made a big deal out of, which was that "these are individual and
independent bools in the implementation detail of the code, expose
that as such to the end users."  I do not buy that point, i.e. it is
not a good argument to favor style 2 over style 1, which was the
primary thing I wanted to illustrate in the message you are
responding to.

>   1. I'm a user who has set my preferred core.whitespace in my
>      ~/.gitconfig. A particular project I am working on uses an
>      alternate tabwidth. How do I set that in the repo config without
>      repeating my defaults?

Isn't it cumulative?  At least it should be (but I wouldn't be too
surprised if the recent config reader caching broke it).

>   2. I'm writing a hook whose behavior depends on the whitespace
>      settings. How do I ask git whether blank-at-eol is enabled?

If that becomes an issue, "git config" would have to learn about
them, just like it knows about how to do --color depending on the
tty-ness of the output.

>   3. I'm a user who wants to set whitespace config. I prefer using "git
>      config" to editing the file manually. How do I turn off
>      blank-at-eol without disrupting my existing settings?

See above 1.

>> I see Peff cites "pager.<cmd>", but I think it was something that we
>> would rather shouldn't have done, similar to "alias.<cmd>".  They
>> are bad precedents we shouldn't encourage new things to mimic.
>> 
>> But that is not from "one-variable-with-list-is-better" (it is not
>> better for these "independent" ones) but is purely from the syntax
>> point of view.
>
> Yeah, I'd agree that the problem there is orthogonal to the type 1/2
> thing above. I don't think it has been a big deal in practice, just
> because people with good taste do not name their commands with uppercase
> anyway.
>
> I'd be happy to transition to pager.*.enabled, etc, if we care.

I have no strong opinion.  It was primarily meant to illustrate why
pager.<cmd> and alias.<cmd> were bad precedents that should not be
used to support design of future things.

Re: [PATCH 3/3] CodingGuidelines: describe naming rules for configuration variables

[Michael Haggerty]

Junio,

Thanks for your thoughtful response.

On 02/01/2015 09:18 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> On 01/28/2015 11:33 PM, Junio C Hamano wrote:
>>> +   When choosing the variable namespace, do not use variable name for
>>> +   specifying possibly unbounded set of things, most notably anything
>>> +   an end user can freely come up with (e.g. branch names), but also
>>> +   large fixed set defined by the system that can grow over time
>>> +   (e.g. what kind of common whitespace problems to notice).
>>
>> I think we can all agree with this rule for "anything an end user can
>> freely come up with". Such sets are truly unbounded.
>>
>> But what is the justification for applying it to "large fixed set
>> defined by the system that can grow over time"?
> [...]
> 
> I can see it argued that for things that are completely independent
> (e.g. the consequence of setting fsck.badDate can never be affected
> by how fsck.missingTagger is configured), separate configuration
> variables may not be wrong per-se, but I can see that a set of knobs
> that would have been originally independent, as the operation grow
> richer, gain more semantics to make them related, and at that point,
> I doubt "they are internally independent; expose them as independent
> to the end users" argument would hold water that well.
> 
> A good example is "core.whitespace", where you would have started
> with a simple set of booleans ("blank-at-eol" and "space-before-tab"
> are conceptually independent and will stay to be), but once you
> start supporting "indent-with-non-tab" and "tab-in-indent" you
> cannot pretend that they are independent.
> 
> And that is the "existing practice" I primarily had in mind.  We
> didn't do
> 
> 	whitespace.tabInIndent = true
>         whitespace.indentWithNonTab = true
> 
> to pretend they are independent and still internally having to make
> sure the consistency of the setting.  We structured the syntax for
> ease of the end user (not scripter) to shorter
> 
> 	core.whitespace = tab-in-indent,indent-with-non-tab
> 
> as we need the consistency thing either way (and it is easier to see
> the consistency mistakes when they appear next to each other).
> 
> And I am happy that we chose wisely in an early version that didn't
> use one-variable-per-knob but used list-of-features-as-value instead.

You make an interesting point: values that start as a list of
independent booleans can grow dependencies over time.

In retrospect, ISTM that a better interface for the indentation-related
"whitespace" settings would have been something like

* "whitespace.indent" -- a single value chosen from "tabs-only |
spaces-only | tabs-when-possible | anything"
* "whitespace.tabwidth" -- an integer value

This would have made the mutual-exclusivity of those choices manifest in
the style of configuration rather than hoping that the user notices that
his settings contradict each other.

Let's dig into this example some more by imagining some other
hypothetical future extensions. Suppose we wanted to support different
whitespace rules for different types of files [1]. For example, I might
never want to forbid "cr-at-eol" everywhere, and might usually like to
uses spaces for my indentation, but for Makefiles need to indent using
tabs. The "type 2" syntax, I think, is pretty straightforward:

    [whitespace]
            cr-at-eol = error
            indent = spaces-only

    [whitespace "makefile"]
            indent = tabs-only

Our usual rules, "last setting wins" and "foo.*.bar, if present, takes
precedence overrides foo.bar", make it pretty clear how the above should
be interpreted.

What would be the "type 1" syntax for this? Would "cr-at-eol" be
inherited from "core.whitespace" to "core.makefile.whitespace"? If not,
then I have to repeat "cr-at-eol":

    [core]
            whitespace = cr-at-eol tab-in-indent
    [core "makefile"]
            whitespace = cr-at-eol indent-with-non-tab

[2]. If values are inherited, then do I also have to countermand
"tab-in-indent" in the "makefile" rule?

    [core]
            whitespace = cr-at-eol tab-in-indent
    [core "makefile"]
            whitespace = indent-with-non-tab -tab-in-indent

Or does "indent-with-non-tab" automatically supersede "tab-in-indent",
according to last-setting-wins (an interpretation that starts requiring
the config parser to have domain-specific knowledge)?:

    [core]
            whitespace = cr-at-eol tab-in-indent
    [core "makefile"]
            whitespace = indent-with-non-tab

But if that is the case, which setting wins in this scenario?:

    [core]
            whitespace = cr-at-eol tab-in-indent
    [core "makefile"]
            whitespace = indent-with-non-tab
    # In another config file maybe:
    [core]
            whitespace = space-before-tab

Does "core.whitespace = space-before-tab" supersede
"core.makefile.whitespace = indent-with-non-tab" here?

No matter which of the "type 1" variants we choose, we would have to
invent new rules for users and config parsers to learn, and some of
those rules would require domain-specific knowledge to interpret.
Whereas the "type 2" style is pretty straightforward and leans only on
existing conventions.

> [...]

Michael

[1] I'm not claiming that this specific extension makes sense. It might
make more sense to configure the whitespace rules one-by-one using
gitattributes. But I think it is nevertheless a typical way that
features are extended and therefore an interesting gedankenexperiment.

[2] For the purposes of this discussion, let's ignore the fact that
there is no precedent for a three-level "core" setting like
"core.makefile.whitespace". It could just as easily be spelled
"whitespace.makefile.check" or something in the "type 1" syntax.

-- 
Michael Haggerty
mhagger@alum.mit.edu

Re: [PATCH 3/3] CodingGuidelines: describe naming rules for configuration variables

[Johannes Schindelin]

Hi,

On 2015-02-01 23:34, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
> 
>>   1. I'm a user who has set my preferred core.whitespace in my
>>      ~/.gitconfig. A particular project I am working on uses an
>>      alternate tabwidth. How do I set that in the repo config without
>>      repeating my defaults?
> 
> Isn't it cumulative?  At least it should be (but I wouldn't be too
> surprised if the recent config reader caching broke it).

Please note that it is really, really difficult for a regular Git user to figure out which whitespace settings are in effect using just `git config` whether it is cumulative or not.

Also, please note it makes it hard on users that there are a bunch of config settings which *override* previous settings, and others that are *cumulative*.

The latter we cannot change, and the former we cannot change for the whitespace settings. However, when introducing new settings (such as the fsck severity levels), we should go out of our way to keep things as simple as possible. For example, if a *simple* `git config` can answer the question "Is feature XYZ turned on", I would consider the design superior to a design that requires additional code just to figure out whether feature XYZ is turned on, let alone to turn it on without affecting other settings.

In other words, I would like to caution against recommending the whitespace setting as an example to follow: anybody who is unfamiliar with the whitespace setting will find the cumulative last-setting-wins (per item inside the whitespace-separated list, not per config setting) strategy confusing.

On the other hand, if you offer `whitespace.tabWidth` and `whitespace.indentWithNoTab` separately, everything is really crystal clear to new users without having much explaining to do, let alone having to introduce new tooling.

Ciao,
Dscho

P.S.: Of course I know that it is too late for `whitespace`, but it offers itself as a good subject to demonstrate the merits of the different suggestions.

Re: [PATCH 3/3] CodingGuidelines: describe naming rules for configuration variables

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

I didn't reply to the latter part of this message yesterday, because
I wanted to think more on it.

> But is it such a bad thing to have them in conflict? Can't we define a
> set of rules that does what people expects? For example, by the "last
> one wins" principle, any time we see "whitespace.tab-in-indent", it
> clears the setting of "whitespace.indent-with-non-tab", and vice versa.
> This isn't represented syntactically in the config file, ...

I agree.  Both one-variable-per-knob and value-with-list-of-knobs do
not express the semantic linkage between knobs; once we convince the
users that one-variable-per-knob format does not mean they represent
independent and orthgonal settings, the issue becomes a trade-off
between

 * Is it concise to let end users skim through?
 * Is it easy to parse by scripters?

> By the way, this is the exact case I am concerned about for fsck.*. Our
> use case at GitHub would be something like:
>
>   a. We set up sane defaults for fsck.* in /etc/gitconfig
>
>   b. User complains that we will not accept their push, which contains
>      objects with malformed committers.
>
>   c. Support investigates, determines that the malformed objects are
>      part of a well-established history, and that they are OK to enter.
>
>   d. We relax fsck.committerIdent in that repo's $GIT_DIR/config file.
>
> Copy-and-pasting the rest of the rules from (a) into the repo config
> file in step (d) is not ideal.

It probably can be worked around by the later-one-wins rule per
item, e.g. after seeing "fsck.ignore = A B C" in /etc/gitconfig and
then seeing "fsck.error = B" in $GIT_DIR/config, the latter will
flip the three-way radio button for B from ignore to error (the
other possible setting of the radio button is 'warn'), while leaving
the three-way radio buttons for A and C set to ignore.

We can (and have to) do the same with "fsck.B = ignore" in
/etc/gitconfig that gets overridden by "fsck.B = error" in
$GIT_DIR/config, and that comes _free_, which makes it an
attractive proposition.

As I already said, I am fine with "fsck.missingTagger = ignore".

Re: [PATCH 3/3] CodingGuidelines: describe naming rules for configuration variables

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> You make an interesting point: values that start as a list of
> independent booleans can grow dependencies over time.
>
> In retrospect, ISTM that a better interface for the indentation-related
> "whitespace" settings would have been something like
>
> * "whitespace.indent" -- a single value chosen from "tabs-only |
> spaces-only | tabs-when-possible | anything"
> * "whitespace.tabwidth" -- an integer value
>
> This would have made the mutual-exclusivity of those choices manifest in
> the style of configuration rather than hoping that the user notices that
> his settings contradict each other.
>
> Let's dig into this example some more by imagining some other
> hypothetical future extensions.

Let's not; that line of thought entirely misses the point.  If you
start from one set of variables, you can define a structure
(e.g. "there are indentation-related and you must choose only one
among them") over it after the fact.

Once you have chosen a structure, you have to live with it.  Either
you make sure that a structure itself is extensible, or you make
sure you accept a new variable only if it fits within a structure.
Either way, you lose.  You cannot predict the future, and you do not
want to constrain those who will contribute to the project in the
future.

My aversion to one-variable-per-knob was primarily against the
"because that is how the variables are internally represented; a
collection of enums that can be independently set" argument.  If we
assume that one-variable-per-knob style implies variables that can
be independently set, that _is_ defining the structure the future
work has to live within.

But as I and Peff discussed in the other sub(sub)thread, having two
variables placed flatly in the namespace, e.g. ws.indentWithTab and
ws.noTabInIndent, does not have to mean they are independent.

And the opposite is also true; having these two knobs as possible
elements of the value of a single variable does not imply they
always have meaningful interactions.

So I am fine with "fsck.missingTagger = ignore/warn/error", as long
as the argument that supports the style is not "because
fsck.missingTagger and fsck.malformedIdent are independent".