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 9:42 AM, Jeff King <peff@xxxxxxxx> 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.




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