Possible bug in includeIf / conditional includes

Possible bug in includeIf / conditional includes

[raphael.stolt]

Hi there,

I might have stumbled over a bug in includeIf / conditional includes or maybe it's just as intended.

Current configuration which finds the conditional configuration.

~/.gitconfig
[includeIf "gitdir:~/Work/git-repos/oss/"]
  path = ~/Work/git-repos/oss/.oss-gitconfig

Expected configuration which doesn't find the conditional configuration:

~/.gitconfig
[includeIf "gitdir:~/Work/git-repos/oss/"]
  path = .oss-gitconfig

Best regards,

Raphael Stolt

Re: Possible bug in includeIf / conditional includes

[Sebastian Schuberth]

On 2017-05-10 19:00, raphael.stolt@gmail.com wrote:

> Current configuration which finds the conditional configuration.
> 
> ~/.gitconfig
> [includeIf "gitdir:~/Work/git-repos/oss/"]
>    path = ~/Work/git-repos/oss/.oss-gitconfig
> 
> Expected configuration which doesn't find the conditional configuration:
> 
> ~/.gitconfig
> [includeIf "gitdir:~/Work/git-repos/oss/"]
>    path = .oss-gitconfig

My guess is, because includeIf might contain other conditionals than 
"gitdir", the generic convention is to always use an absolute path for 
"path".

-- 
Sebastian Schuberth

Re: Possible bug in includeIf / conditional includes

[Ævar Arnfjörð Bjarmason]

On Wed, May 10, 2017 at 8:58 PM, Sebastian Schuberth
<sschuberth@gmail.com> wrote:
> On 2017-05-10 19:00, raphael.stolt@gmail.com wrote:
>
>> Current configuration which finds the conditional configuration.
>>
>> ~/.gitconfig
>> [includeIf "gitdir:~/Work/git-repos/oss/"]
>>    path = ~/Work/git-repos/oss/.oss-gitconfig
>>
>> Expected configuration which doesn't find the conditional configuration:
>>
>> ~/.gitconfig
>> [includeIf "gitdir:~/Work/git-repos/oss/"]
>>    path = .oss-gitconfig
>
>
> My guess is, because includeIf might contain other conditionals than
> "gitdir", the generic convention is to always use an absolute path for
> "path".

[CC'd OP Raphael Stolt, please reply-all]

In both cases the conditional is the same, but the path is relative
v.s. absolute.

Raphael: Does the config get included if you cd to
~/Work/git-repos/oss/? From git-config(1):

---cut---
The included file is expanded immediately, as if its contents had been
found at the location of the include directive. If the value of the
`include.path` variable is a relative path, the path is considered to
be relative to the configuration file in which the include directive
was found.  See below for examples.
---cut---

The commit that added IncludeIf (3efd0bedc6) does something with
relative path (just skimming, need to get to other things), but unlike
[Include] the docs don't explicitly mention what it's supposed to do
with that, and all the examples show absolute paths.

So whether this is a bug in the code or not it seems to definitely be
a doc bug, whatever it does with relative files should be in the docs.

Re: Possible bug in includeIf / conditional includes

[Raphael Stolt]

> 
> Am 10.05.2017 um 21:48 schrieb Ævar Arnfjörð Bjarmason <avarab@gmail.com>:
> 
> On Wed, May 10, 2017 at 8:58 PM, Sebastian Schuberth
> <sschuberth@gmail.com> wrote:
>> On 2017-05-10 19:00, raphael.stolt@gmail.com wrote:
>> 
>>> Current configuration which finds the conditional configuration.
>>> 

a)

>>> ~/.gitconfig
>>> [includeIf "gitdir:~/Work/git-repos/oss/"]
>>>   path = ~/Work/git-repos/oss/.oss-gitconfig
>>> 
>>> Expected configuration which doesn't find the conditional configuration:
>>> 

b)

>>> ~/.gitconfig
>>> [includeIf "gitdir:~/Work/git-repos/oss/"]
>>>   path = .oss-gitconfig
>> 
>> 
>> My guess is, because includeIf might contain other conditionals than
>> "gitdir", the generic convention is to always use an absolute path for
>> "path".
> 
> [CC'd OP Raphael Stolt, please reply-all]
> 
> In both cases the conditional is the same, but the path is relative
> v.s. absolute.
> 
> Raphael: Does the config get included if you cd to
> ~/Work/git-repos/oss/? From git-config(1):

Given I’m in a repo in ~/Work/git-repos/oss/ e.g. ~/Work/git-repos/oss/project-repo-a and I’m using config a) 
the config is used from ~/Work/git-repos/oss/.oss-gitconfig

Given I’m in a repo in ~/Work/git-repos/oss/ e.g. ~/Work/git-repos/oss/project-repo-a and I’m using config b) 
the global config is used because there is no .oss-gitconfig in $HOME.

I guess it’s an intended behavior since conditional configuration files __SHOULD__ reside in $HOME rather than 
in my case in ~/Work/git-repos/oss.

> 
> ---cut---
> The included file is expanded immediately, as if its contents had been
> found at the location of the include directive. If the value of the
> `include.path` variable is a relative path, the path is considered to
> be relative to the configuration file in which the include directive
> was found.  See below for examples.
> ---cut---
> 
> The commit that added IncludeIf (3efd0bedc6) does something with
> relative path (just skimming, need to get to other things), but unlike
> [Include] the docs don't explicitly mention what it's supposed to do
> with that, and all the examples show absolute paths.
> 
> So whether this is a bug in the code or not it seems to definitely be
> a doc bug, whatever it does with relative files should be in the docs.
+ 1

Re: Possible bug in includeIf / conditional includes

[Raphael Stolt]

> Am 10.05.2017 um 23:21 schrieb Raphael Stolt <raphael.stolt@gmail.com>:
> 
>> 
>> Am 10.05.2017 um 21:48 schrieb Ævar Arnfjörð Bjarmason <avarab@gmail.com>:
>> 
>> On Wed, May 10, 2017 at 8:58 PM, Sebastian Schuberth
>> <sschuberth@gmail.com> wrote:
>>> On 2017-05-10 19:00, raphael.stolt@gmail.com wrote:
>>> 
>>>> Current configuration which finds the conditional configuration.
>>>> 
> 
> a)
> 
>>>> ~/.gitconfig
>>>> [includeIf "gitdir:~/Work/git-repos/oss/"]
>>>>  path = ~/Work/git-repos/oss/.oss-gitconfig
>>>> 
>>>> Expected configuration which doesn't find the conditional configuration:
>>>> 
> 
> b)
> 
>>>> ~/.gitconfig
>>>> [includeIf "gitdir:~/Work/git-repos/oss/"]
>>>>  path = .oss-gitconfig
>>> 
>>> 
>>> My guess is, because includeIf might contain other conditionals than
>>> "gitdir", the generic convention is to always use an absolute path for
>>> "path".
>> 
>> [CC'd OP Raphael Stolt, please reply-all]
>> 
>> In both cases the conditional is the same, but the path is relative
>> v.s. absolute.
>> 
>> Raphael: Does the config get included if you cd to
>> ~/Work/git-repos/oss/? From git-config(1):
> 
> Given I’m in a repo in ~/Work/git-repos/oss/ e.g. ~/Work/git-repos/oss/project-repo-a and I’m using config a) 
> the config is used from ~/Work/git-repos/oss/.oss-gitconfig
> 
> Given I’m in a repo in ~/Work/git-repos/oss/ e.g. ~/Work/git-repos/oss/project-repo-a and I’m using config b) 
> the global config is used because there is no .oss-gitconfig in $HOME.

Given I’m in ~/Work/git-repos/oss and I’m using config b)
also the global config (~/.gitconfig) is used.

> 
> I guess it’s an intended behavior since conditional configuration files __SHOULD__ reside in $HOME rather than 
> in my case in ~/Work/git-repos/oss.
> 
>> 
>> ---cut---
>> The included file is expanded immediately, as if its contents had been
>> found at the location of the include directive. If the value of the
>> `include.path` variable is a relative path, the path is considered to
>> be relative to the configuration file in which the include directive
>> was found.  See below for examples.
>> ---cut---
>> 
>> The commit that added IncludeIf (3efd0bedc6) does something with
>> relative path (just skimming, need to get to other things), but unlike
>> [Include] the docs don't explicitly mention what it's supposed to do
>> with that, and all the examples show absolute paths.
>> 
>> So whether this is a bug in the code or not it seems to definitely be
>> a doc bug, whatever it does with relative files should be in the docs.
> + 1

Re: Possible bug in includeIf / conditional includes

[Jeff King]

On Wed, May 10, 2017 at 09:48:05PM +0200, Ævar Arnfjörð Bjarmason wrote:

> In both cases the conditional is the same, but the path is relative
> v.s. absolute.
> 
> Raphael: Does the config get included if you cd to
> ~/Work/git-repos/oss/? From git-config(1):

That shouldn't matter for relative paths in the "path" value; they're
never based on the cwd.  E.g., with this setup:

  export HOME=$PWD
  git config -f .gitconfig foo.bar "base value"
  git config -f .gitconfig includeIf."gitdir:$PWD/repo/".path included
  git config -f included foo.bar "HOME include"
  git init -q repo && cd repo
  git config -f included foo.bar "working tree include"
  git config -f .git/included foo.bar "GIT_DIR include"
  git config foo.bar

Looking at foo.bar should always give you one of

  HOME include - if you are in the repo that matches the includeIf
  base value - if you are not

but never the "working tree" or "GIT_DIR" includes.

> ---cut---
> The included file is expanded immediately, as if its contents had been
> found at the location of the include directive. If the value of the
> `include.path` variable is a relative path, the path is considered to
> be relative to the configuration file in which the include directive
> was found.  See below for examples.
> ---cut---
> 
> The commit that added IncludeIf (3efd0bedc6) does something with
> relative path (just skimming, need to get to other things), but unlike
> [Include] the docs don't explicitly mention what it's supposed to do
> with that, and all the examples show absolute paths.
> 
> So whether this is a bug in the code or not it seems to definitely be
> a doc bug, whatever it does with relative files should be in the docs.

The includeIf variables should behave exactly as the documentation you
quoted above. The conditional impacts whether we respect the keys in
that section, but not the meaning of the keys.

I thought that was clear, but as somebody who was involved in the
development of both the original and the conditional includes, my
opinion probably doesn't count. I wouldn't mind a patch to make that
more explicit in the documentation.

-Peff

Re: Possible bug in includeIf / conditional includes

[Jeff King]

On Thu, May 11, 2017 at 02:26:16AM -0400, Jeff King wrote:

> > So whether this is a bug in the code or not it seems to definitely be
> > a doc bug, whatever it does with relative files should be in the docs.
> 
> The includeIf variables should behave exactly as the documentation you
> quoted above. The conditional impacts whether we respect the keys in
> that section, but not the meaning of the keys.
> 
> I thought that was clear, but as somebody who was involved in the
> development of both the original and the conditional includes, my
> opinion probably doesn't count. I wouldn't mind a patch to make that
> more explicit in the documentation.

I figured I'd just do this while we're thinking about it, since it
should be trivial. But it's actually there already:

  You can include a config file from another conditionally by setting a
  `includeIf.<condition>.path` variable to the name of the file to be
  included. The variable's value is treated the same way as
  `include.path`.

I'm open to suggestions to make that more obvious, though.

-Peff

Re: Possible bug in includeIf / conditional includes

[Ævar Arnfjörð Bjarmason]

On Thu, May 11, 2017 at 8:29 AM, Jeff King <peff@peff.net> wrote:
> On Thu, May 11, 2017 at 02:26:16AM -0400, Jeff King wrote:
>
>> > So whether this is a bug in the code or not it seems to definitely be
>> > a doc bug, whatever it does with relative files should be in the docs.
>>
>> The includeIf variables should behave exactly as the documentation you
>> quoted above. The conditional impacts whether we respect the keys in
>> that section, but not the meaning of the keys.
>>
>> I thought that was clear, but as somebody who was involved in the
>> development of both the original and the conditional includes, my
>> opinion probably doesn't count. I wouldn't mind a patch to make that
>> more explicit in the documentation.
>
> I figured I'd just do this while we're thinking about it, since it
> should be trivial. But it's actually there already:
>
>   You can include a config file from another conditionally by setting a
>   `includeIf.<condition>.path` variable to the name of the file to be
>   included. The variable's value is treated the same way as
>   `include.path`.
>
> I'm open to suggestions to make that more obvious, though.

Yes I started writing up a patch to make this better but once I
started doing that it wasn't really any better.

FWIW the reasons I didn't find this while quickly skimming the thing:

1. It says "The included file is expanded immediately, as if its
contents had been found at the location of the include directive.". At
first I thought this referred to glob expansion, not
s/expanded/interpolated/, the example section uses "expand" in the
context of pathnames, which caused the confusion.

Maybe this would make that less confusing by saying the same thing
without using the same phrasing to mean completely different things:

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 475e874d51..49855353c7 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -167,8 +167,8 @@ Example

        [include]
                path = /path/to/foo.inc ; include by absolute path
-               path = foo ; expand "foo" relative to the current file
-               path = ~/foo ; expand "foo" in your `$HOME` directory
+               path = foo ; find & include "foo" relative to the current file
+               path = ~/foo ; find & include "foo" in your `$HOME` directory

        ; include if $GIT_DIR is /path/to/foo/.git
        [includeIf "gitdir:/path/to/foo/.git"]

2. There is no example of such expansion for IncludeIf, prose should
always be backed up by examples for the lazy reader when possible:

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 475e874d51..fc4b87cd7e 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -173,6 +173,8 @@ Example
        ; include if $GIT_DIR is /path/to/foo/.git
        [includeIf "gitdir:/path/to/foo/.git"]
                path = /path/to/foo.inc
+               path = foo ; find & include "foo" relative to the current file
+               path = ~/foo ; find & include "foo" in your `$HOME` directory

        ; include for all repositories inside /path/to/group
        [includeIf "gitdir:/path/to/group/"]

3. When I read reference docs I almost never start at the beginning &
read it all the way through, in this case I thought I could help
someone out by maybe answering a question on the ML quickly, so I went
to the examples section, found no example (fixed above), then searched
for "relative" or "path" in my pager and the only results were for the
"Includes" section that has a paragraph that's only talking about
"include.path".

Of course we say "same rules apply" below, but I managed not to spot
that. Maybe this:

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 475e874d51..b35d9a7b80 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -95,8 +95,11 @@ Conditional includes

 You can include a config file from another conditionally by setting a
 `includeIf.<condition>.path` variable to the name of the file to be
-included. The variable's value is treated the same way as
-`include.path`. `includeIf.<condition>.path` can be given multiple times.
+included.
+
+If variable's value is a relative path it's treated the same way as
+`include.path`. `includeIf.<condition>.path` can also be given
+multiple times.

 The condition starts with a keyword followed by a colon and some data
 whose format and meaning depends on the keyword. Supported keywords

Would make it easier to find. Here we say the same thing but include
both "path" & "relative".

Re: Possible bug in includeIf / conditional includes

[Jeff King]

On Thu, May 11, 2017 at 09:19:50AM +0200, Ævar Arnfjörð Bjarmason wrote:

> 1. It says "The included file is expanded immediately, as if its
> contents had been found at the location of the include directive.". At
> first I thought this referred to glob expansion, not
> s/expanded/interpolated/, the example section uses "expand" in the
> context of pathnames, which caused the confusion.

Perhaps it should say "The contents of the included file are expanded
immediately, as if they had been found at..."?

> Maybe this would make that less confusing by saying the same thing
> without using the same phrasing to mean completely different things:
> 
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 475e874d51..49855353c7 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -167,8 +167,8 @@ Example
> 
>         [include]
>                 path = /path/to/foo.inc ; include by absolute path
> -               path = foo ; expand "foo" relative to the current file
> -               path = ~/foo ; expand "foo" in your `$HOME` directory
> +               path = foo ; find & include "foo" relative to the current file
> +               path = ~/foo ; find & include "foo" in your `$HOME` directory

Yeah, probably makes sense to use a different word than "expand" here.

> 2. There is no example of such expansion for IncludeIf, prose should
> always be backed up by examples for the lazy reader when possible:
> 
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 475e874d51..fc4b87cd7e 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -173,6 +173,8 @@ Example
>         ; include if $GIT_DIR is /path/to/foo/.git
>         [includeIf "gitdir:/path/to/foo/.git"]
>                 path = /path/to/foo.inc
> +               path = foo ; find & include "foo" relative to the current file
> +               path = ~/foo ; find & include "foo" in your `$HOME` directory
> 
>         ; include for all repositories inside /path/to/group
>         [includeIf "gitdir:/path/to/group/"]

Yeah, makes sense.

> 3. When I read reference docs I almost never start at the beginning &
> read it all the way through, in this case I thought I could help
> someone out by maybe answering a question on the ML quickly, so I went
> to the examples section, found no example (fixed above), then searched
> for "relative" or "path" in my pager and the only results were for the
> "Includes" section that has a paragraph that's only talking about
> "include.path".

I do, too. And I'm all in favor of structuring things to accommodate
that flow. But at some point we have to assume the user actually reads
the documentation. :)

> Of course we say "same rules apply" below, but I managed not to spot
> that. Maybe this:
> 
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 475e874d51..b35d9a7b80 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -95,8 +95,11 @@ Conditional includes
> 
>  You can include a config file from another conditionally by setting a
>  `includeIf.<condition>.path` variable to the name of the file to be
> -included. The variable's value is treated the same way as
> -`include.path`. `includeIf.<condition>.path` can be given multiple times.
> +included.
> +
> +If variable's value is a relative path it's treated the same way as
> +`include.path`. `includeIf.<condition>.path` can also be given
> +multiple times.

I don't like this because it copies the rules for _one_ property to the
conditional section. What happens when you're looking for some other
property of include.path?

I think in your case you found the "relative" section in the earlier
paragraph, but there was no link there to the includeIf behavior. So
should that earlier paragraph just get the "includeIf.path behaves the
same way" mention?

I suspect that whole paragraph under Includes could be reworded to make
it clear that anything it is saying applies equally to include.$key and
includeIf.*.$key, and then that would future-proof us for other
modifications.

-Peff

Re: Possible bug in includeIf / conditional includes

[Ævar Arnfjörð Bjarmason]

On Thu, May 11, 2017 at 9:42 AM, Jeff King <peff@peff.net> wrote:
> On Thu, May 11, 2017 at 09:19:50AM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> 1. It says "The included file is expanded immediately, as if its
>> contents had been found at the location of the include directive.". At
>> first I thought this referred to glob expansion, not
>> s/expanded/interpolated/, the example section uses "expand" in the
>> context of pathnames, which caused the confusion.
>
> Perhaps it should say "The contents of the included file are expanded
> immediately, as if they had been found at..."?
>
>> Maybe this would make that less confusing by saying the same thing
>> without using the same phrasing to mean completely different things:
>>
>> diff --git a/Documentation/config.txt b/Documentation/config.txt
>> index 475e874d51..49855353c7 100644
>> --- a/Documentation/config.txt
>> +++ b/Documentation/config.txt
>> @@ -167,8 +167,8 @@ Example
>>
>>         [include]
>>                 path = /path/to/foo.inc ; include by absolute path
>> -               path = foo ; expand "foo" relative to the current file
>> -               path = ~/foo ; expand "foo" in your `$HOME` directory
>> +               path = foo ; find & include "foo" relative to the current file
>> +               path = ~/foo ; find & include "foo" in your `$HOME` directory
>
> Yeah, probably makes sense to use a different word than "expand" here.
>
>> 2. There is no example of such expansion for IncludeIf, prose should
>> always be backed up by examples for the lazy reader when possible:
>>
>> diff --git a/Documentation/config.txt b/Documentation/config.txt
>> index 475e874d51..fc4b87cd7e 100644
>> --- a/Documentation/config.txt
>> +++ b/Documentation/config.txt
>> @@ -173,6 +173,8 @@ Example
>>         ; include if $GIT_DIR is /path/to/foo/.git
>>         [includeIf "gitdir:/path/to/foo/.git"]
>>                 path = /path/to/foo.inc
>> +               path = foo ; find & include "foo" relative to the current file
>> +               path = ~/foo ; find & include "foo" in your `$HOME` directory
>>
>>         ; include for all repositories inside /path/to/group
>>         [includeIf "gitdir:/path/to/group/"]
>
> Yeah, makes sense.
>
>> 3. When I read reference docs I almost never start at the beginning &
>> read it all the way through, in this case I thought I could help
>> someone out by maybe answering a question on the ML quickly, so I went
>> to the examples section, found no example (fixed above), then searched
>> for "relative" or "path" in my pager and the only results were for the
>> "Includes" section that has a paragraph that's only talking about
>> "include.path".
>
> I do, too. And I'm all in favor of structuring things to accommodate
> that flow. But at some point we have to assume the user actually reads
> the documentation. :)
>
>> Of course we say "same rules apply" below, but I managed not to spot
>> that. Maybe this:
>>
>> diff --git a/Documentation/config.txt b/Documentation/config.txt
>> index 475e874d51..b35d9a7b80 100644
>> --- a/Documentation/config.txt
>> +++ b/Documentation/config.txt
>> @@ -95,8 +95,11 @@ Conditional includes
>>
>>  You can include a config file from another conditionally by setting a
>>  `includeIf.<condition>.path` variable to the name of the file to be
>> -included. The variable's value is treated the same way as
>> -`include.path`. `includeIf.<condition>.path` can be given multiple times.
>> +included.
>> +
>> +If variable's value is a relative path it's treated the same way as
>> +`include.path`. `includeIf.<condition>.path` can also be given
>> +multiple times.
>
> I don't like this because it copies the rules for _one_ property to the
> conditional section. What happens when you're looking for some other
> property of include.path?

Yeah, as I said once I wrote it up I found it wasn't really any
better, but just wanted to send an explanation for why I didn't find
it while I remembered, as a sort of case study.

> I think in your case you found the "relative" section in the earlier
> paragraph, but there was no link there to the includeIf behavior. So
> should that earlier paragraph just get the "includeIf.path behaves the
> same way" mention?
>
> I suspect that whole paragraph under Includes could be reworded to make
> it clear that anything it is saying applies equally to include.$key and
> includeIf.*.$key, and then that would future-proof us for other
> modifications.

Re: Possible bug in includeIf / conditional includes

[Jeff King]

On Thu, May 11, 2017 at 09:49:09AM +0200, Ævar Arnfjörð Bjarmason wrote:

> > I don't like this because it copies the rules for _one_ property to the
> > conditional section. What happens when you're looking for some other
> > property of include.path?
> 
> Yeah, as I said once I wrote it up I found it wasn't really any
> better, but just wanted to send an explanation for why I didn't find
> it while I remembered, as a sort of case study.
> [...]
> > I suspect that whole paragraph under Includes could be reworded to make
> > it clear that anything it is saying applies equally to include.$key and
> > includeIf.*.$key, and then that would future-proof us for other
> > modifications.

What about this:

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 475e874d5..e44ad21eb 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -80,13 +80,13 @@ Includes
 ~~~~~~~~
 
 You can include a config file from another by setting the special
-`include.path` variable to the name of the file to be included. The
-variable takes a pathname as its value, and is subject to tilde
-expansion. `include.path` can be given multiple times.
+`include.path` and `includeIf.*.path` variables to the name of the file
+to be included. The variable takes a pathname as its value, and is
+subject to tilde expansion. These variables can be given multiple times.
 
 The included file is expanded immediately, as if its contents had been
 found at the location of the include directive. If the value of the
-`include.path` variable is a relative path, the path is considered to
+variable is a relative path, the path is considered to
 be relative to the configuration file in which the include directive
 was found.  See below for examples.
 
@@ -95,8 +95,7 @@ Conditional includes
 
 You can include a config file from another conditionally by setting a
 `includeIf.<condition>.path` variable to the name of the file to be
-included. The variable's value is treated the same way as
-`include.path`. `includeIf.<condition>.path` can be given multiple times.
+included (see the "Includes" section above for more details).
 
 The condition starts with a keyword followed by a colon and some data
 whose format and meaning depends on the keyword. Supported keywords

[PATCH 0/4] doc improvements for config includes

[Jeff King]

On Thu, May 11, 2017 at 03:54:37AM -0400, Jeff King wrote:

> On Thu, May 11, 2017 at 09:49:09AM +0200, Ævar Arnfjörð Bjarmason wrote:
> 
> > > I don't like this because it copies the rules for _one_ property to the
> > > conditional section. What happens when you're looking for some other
> > > property of include.path?
> > 
> > Yeah, as I said once I wrote it up I found it wasn't really any
> > better, but just wanted to send an explanation for why I didn't find
> > it while I remembered, as a sort of case study.
> > [...]
> > > I suspect that whole paragraph under Includes could be reworded to make
> > > it clear that anything it is saying applies equally to include.$key and
> > > includeIf.*.$key, and then that would future-proof us for other
> > > modifications.
> 
> What about this:

I think this is the right path, but I actually ended up with an
introductory paragraph about the two sections. I hope it makes the same
point but is a bit less clunky.

Here's a series that I think covers all the bits discussed here, and a
few others.

  [1/4]: docs/config: clarify include/includeIf relationship
  [2/4]: docs/config: give a relative includeIf example
  [3/4]: docs/config: avoid the term "expand" for includes
  [4/4]: docs/config: consistify include.path examples

 Documentation/config.txt | 31 +++++++++++++++++++++----------
 1 file changed, 21 insertions(+), 10 deletions(-)

-Peff

[PATCH 1/4] docs/config: clarify include/includeIf relationship

[Jeff King]

The "includeIf" directives behave exactly like include ones,
except they only kick in when the conditional is true. That
was mentioned in the "conditional" section, but let's make
it more clear for the whole "includes" section, since people
don't necessarily read the documentation top to bottom.

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/config.txt | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 475e874d5..d5a453ed3 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -79,14 +79,20 @@ escape sequences) are invalid.
 Includes
 ~~~~~~~~
 
+The `include` and `includeIf` sections allow you include config
+directives from another source. These sections behave identically to
+each other with the exception that `includeIf` sections may be ignored
+if their condition does not evaluate to true; see "Conditional includes"
+below.
+
 You can include a config file from another by setting the special
-`include.path` variable to the name of the file to be included. The
-variable takes a pathname as its value, and is subject to tilde
-expansion. `include.path` can be given multiple times.
+`include.path` (or `includeIf.*.path`) variable to the name of the file
+to be included. The variable takes a pathname as its value, and is
+subject to tilde expansion. These variables can be given multiple times.
 
 The included file is expanded immediately, as if its contents had been
 found at the location of the include directive. If the value of the
-`include.path` variable is a relative path, the path is considered to
+variable is a relative path, the path is considered to
 be relative to the configuration file in which the include directive
 was found.  See below for examples.
 
@@ -95,8 +101,7 @@ Conditional includes
 
 You can include a config file from another conditionally by setting a
 `includeIf.<condition>.path` variable to the name of the file to be
-included. The variable's value is treated the same way as
-`include.path`. `includeIf.<condition>.path` can be given multiple times.
+included.
 
 The condition starts with a keyword followed by a colon and some data
 whose format and meaning depends on the keyword. Supported keywords
-- 
2.13.0.447.g4d26bc97c

Re: [PATCH 1/4] docs/config: clarify include/includeIf relationship

[Ramsay Jones]

On 11/05/17 10:10, Jeff King wrote:
> The "includeIf" directives behave exactly like include ones,
> except they only kick in when the conditional is true. That
> was mentioned in the "conditional" section, but let's make
> it more clear for the whole "includes" section, since people
> don't necessarily read the documentation top to bottom.
> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  Documentation/config.txt | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 475e874d5..d5a453ed3 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -79,14 +79,20 @@ escape sequences) are invalid.
>  Includes
>  ~~~~~~~~
>  
> +The `include` and `includeIf` sections allow you include config

s/you include/you to include/

ATB,
Ramsay Jones

Re: [PATCH 1/4] docs/config: clarify include/includeIf relationship

[Jeff King]

On Thu, May 11, 2017 at 05:47:28PM +0100, Ramsay Jones wrote:

> > diff --git a/Documentation/config.txt b/Documentation/config.txt
> > index 475e874d5..d5a453ed3 100644
> > --- a/Documentation/config.txt
> > +++ b/Documentation/config.txt
> > @@ -79,14 +79,20 @@ escape sequences) are invalid.
> >  Includes
> >  ~~~~~~~~
> >  
> > +The `include` and `includeIf` sections allow you include config
> 
> s/you include/you to include/

Oof, yeah. Too many revisions, not enough proofreading. I don't think
these otherwise need re-rolled, so hopefully Junio can just mark it up.

Thanks.

-Peff

[PATCH 2/4] docs/config: give a relative includeIf example

[Jeff King]

The changes in the previous commit hopefully clarify that
the evaluation of an include "path" variable is the same no
matter if it's in a conditional section or not. But since
this question came up on the list, let's add an example that
makes it obvious.

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/config.txt | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index d5a453ed3..929776954 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -187,6 +187,12 @@ Example
 	[includeIf "gitdir:~/to/group/"]
 		path = /path/to/foo.inc
 
+	; relative paths are always relative to the including
+	; file (if the condition is true); their location is not
+	; affected by the condition
+	[includeIf "gitdir:/path/to/group/"]
+		path = foo.inc
+
 Values
 ~~~~~~
 
-- 
2.13.0.447.g4d26bc97c

[PATCH 3/4] docs/config: avoid the term "expand" for includes

[Jeff King]

Using the word "expand" to refer to including the contents
of another config file isn't really accurate, since it's a
verbatim insertion. And it can cause confusion with the
expanding of the path itself via things like "~".

Let's clarify when we are referring to the contents versus
the filename, and use appropriate verbs in each case.

Signed-off-by: Jeff King <peff@peff.net>
---
I dropped the "& insert" from your suggestion, because I think it
clear in the context of the earlier "include by absolute path" (and I
think it reads better).

 Documentation/config.txt | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 929776954..70f79ac39 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -90,8 +90,8 @@ You can include a config file from another by setting the special
 to be included. The variable takes a pathname as its value, and is
 subject to tilde expansion. These variables can be given multiple times.
 
-The included file is expanded immediately, as if its contents had been
-found at the location of the include directive. If the value of the
+The contents of the included file are inserted immediately, as if they
+had been found at the location of the include directive. If the value of the
 variable is a relative path, the path is considered to
 be relative to the configuration file in which the include directive
 was found.  See below for examples.
@@ -172,8 +172,8 @@ Example
 
 	[include]
 		path = /path/to/foo.inc ; include by absolute path
-		path = foo ; expand "foo" relative to the current file
-		path = ~/foo ; expand "foo" in your `$HOME` directory
+		path = foo ; find "foo" relative to the current file
+		path = ~/foo ; find "foo" in your `$HOME` directory
 
 	; include if $GIT_DIR is /path/to/foo/.git
 	[includeIf "gitdir:/path/to/foo/.git"]
-- 
2.13.0.447.g4d26bc97c

[PATCH 4/4] docs/config: consistify include.path examples

[Jeff King]

Most of the include examples use "foo.inc", but some use
"foo". Since the string of examples are meant to show
variations and how they differ, it's a good idea to change
only one thing at a time. The filename differences are not
relevant to what we're trying to show.

Signed-off-by: Jeff King <peff@peff.net>
---
And yes, I just invented the word consistify. "make X consistent" hides
the meaning at the end, and I couldn't think of a more concise verb.
Prescriptivists beware. :)

 Documentation/config.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 70f79ac39..7b08df732 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -172,8 +172,8 @@ Example
 
 	[include]
 		path = /path/to/foo.inc ; include by absolute path
-		path = foo ; find "foo" relative to the current file
-		path = ~/foo ; find "foo" in your `$HOME` directory
+		path = foo.inc ; find "foo.inc" relative to the current file
+		path = ~/foo.inc ; find "foo.inc" in your `$HOME` directory
 
 	; include if $GIT_DIR is /path/to/foo/.git
 	[includeIf "gitdir:/path/to/foo/.git"]
-- 
2.13.0.447.g4d26bc97c

Re: [PATCH 4/4] docs/config: consistify include.path examples

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Most of the include examples use "foo.inc", but some use
> "foo". Since the string of examples are meant to show
> variations and how they differ, it's a good idea to change
> only one thing at a time. The filename differences are not
> relevant to what we're trying to show.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> And yes, I just invented the word consistify. "make X consistent" hides
> the meaning at the end, and I couldn't think of a more concise verb.

Yeah, "remove unnecessary variants from X" is the best I could do.
In any case, all four looked sensible.

Thanks.

Re: [PATCH 0/4] doc improvements for config includes

[Ævar Arnfjörð Bjarmason]

On Thu, May 11, 2017 at 11:09 AM, Jeff King <peff@peff.net> wrote:
> On Thu, May 11, 2017 at 03:54:37AM -0400, Jeff King wrote:
>
>> On Thu, May 11, 2017 at 09:49:09AM +0200, Ævar Arnfjörð Bjarmason wrote:
>>
>> > > I don't like this because it copies the rules for _one_ property to the
>> > > conditional section. What happens when you're looking for some other
>> > > property of include.path?
>> >
>> > Yeah, as I said once I wrote it up I found it wasn't really any
>> > better, but just wanted to send an explanation for why I didn't find
>> > it while I remembered, as a sort of case study.
>> > [...]
>> > > I suspect that whole paragraph under Includes could be reworded to make
>> > > it clear that anything it is saying applies equally to include.$key and
>> > > includeIf.*.$key, and then that would future-proof us for other
>> > > modifications.
>>
>> What about this:
>
> I think this is the right path, but I actually ended up with an
> introductory paragraph about the two sections. I hope it makes the same
> point but is a bit less clunky.
>
> Here's a series that I think covers all the bits discussed here, and a
> few others.
>
>   [1/4]: docs/config: clarify include/includeIf relationship
>   [2/4]: docs/config: give a relative includeIf example
>   [3/4]: docs/config: avoid the term "expand" for includes
>   [4/4]: docs/config: consistify include.path examples

This whole thing looks great to me. Thanks!

Reviewed-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>

>  Documentation/config.txt | 31 +++++++++++++++++++++----------
>  1 file changed, 21 insertions(+), 10 deletions(-)
>
> -Peff

Re: Possible bug in includeIf / conditional includes

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, May 11, 2017 at 09:19:50AM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> 1. It says "The included file is expanded immediately, as if its
>> contents had been found at the location of the include directive.". At
>> first I thought this referred to glob expansion, not
>> s/expanded/interpolated/, the example section uses "expand" in the
>> context of pathnames, which caused the confusion.
>
> Perhaps it should say "The contents of the included file are expanded
> immediately, as if they had been found at..."?

Or s/expanded/inserted/, perhaps?  The word "expand" does not quite
click to me in this context.  Just like Ævar, I associate the word
with an act of replacing some template-with-blank with the result of
blanks-in-the-template-filled.

Regarding 2. and 3., I agree with your comments.

Thanks.

Re: Possible bug in includeIf / conditional includes

[Jeff King]

On Thu, May 11, 2017 at 12:54:19AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > On Thu, May 11, 2017 at 09:19:50AM +0200, Ævar Arnfjörð Bjarmason wrote:
> >
> >> 1. It says "The included file is expanded immediately, as if its
> >> contents had been found at the location of the include directive.". At
> >> first I thought this referred to glob expansion, not
> >> s/expanded/interpolated/, the example section uses "expand" in the
> >> context of pathnames, which caused the confusion.
> >
> > Perhaps it should say "The contents of the included file are expanded
> > immediately, as if they had been found at..."?
> 
> Or s/expanded/inserted/, perhaps?  The word "expand" does not quite
> click to me in this context.  Just like Ævar, I associate the word
> with an act of replacing some template-with-blank with the result of
> blanks-in-the-template-filled.

Yeah, that is much better. I think "expand" here originally meant "read
the contents of", but when we talk about the contents already there is
nothing left to expand.

I agree "inserted" is probably the right word (or even just "read" or
"parsed" or something).

-Peff