Re: Possible bug in includeIf / conditional includes

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

 



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



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