Re: Possible bug in includeIf / conditional includes

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

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