git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: Possible bug in includeIf / conditional includes

From
Jeff King <peff@peff.net>
Date
May 11, 2017, 07:42 UTC
Message-ID
<20170511074257.xueofze3dv3xwrql@sigill.intra.peff.net>
In-Reply-To
<CACBZZX4ob04fG9ZZtvbdcqrYOKijoZohVdUCNyeayHZtdtNyxQ@mail.gmail.com>
On Thu, May 11, 2017 at 09:19:50AM +0200, Ævar Arnfjörð Bjarmason wrote:
Show 5 quoted lines
> 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..."?

Show 15 quoted lines
> 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.
Show 16 quoted lines
> 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.
Show 7 quoted lines
> 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. :)

Show 18 quoted lines
> 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
Previous: Ævar Arnfjörð BjarmasonNext: Ævar Arnfjörð Bjarmason
Message 9 of 22 in “Possible bug in includeIf / conditional includes”
  1. raphael.stolt@gmail.comMay 10, 2017
  2. Sebastian SchuberthMay 10, 2017
  3. Ævar Arnfjörð BjarmasonMay 10, 2017
  4. Raphael StoltMay 10, 2017
  5. Raphael StoltMay 10, 2017
  6. Jeff KingMay 11, 2017
  7. Jeff KingMay 11, 2017
  8. Ævar Arnfjörð BjarmasonMay 11, 2017
  9. Jeff KingMay 11, 2017
  10. Ævar Arnfjörð BjarmasonMay 11, 2017
  11. Jeff KingMay 11, 2017
  12. 0/4 doc improvements for config includesJeff King, May 11, 2017
  13. 1/4 docs/config: clarify include/includeIf relationshipJeff King, May 11, 2017
  14. Ramsay JonesMay 11, 2017
  15. Jeff KingMay 12, 2017
  16. 2/4 docs/config: give a relative includeIf exampleJeff King, May 11, 2017
  17. 3/4 docs/config: avoid the term "expand" for includesJeff King, May 11, 2017
  18. 4/4 docs/config: consistify include.path examplesJeff King, May 11, 2017
  19. Junio C HamanoMay 11, 2017
  20. Ævar Arnfjörð BjarmasonMay 11, 2017
  21. Junio C HamanoMay 11, 2017
  22. Jeff KingMay 11, 2017

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.