# Possible bug in includeIf / conditional includes 22 messages from 2017-05-10 to 2017-05-12. Participants: raphael.stolt@gmail.com, Sebastian Schuberth, Ævar Arnfjörð Bjarmason, Raphael Stolt, Jeff King, Junio C Hamano, Ramsay Jones. Thread: https://gitlist.dev/t/45924 ## raphael.stolt@gmail.com, 2017-05-10 17:00 Subject: Possible bug in includeIf / conditional includes Message-ID: URL: https://gitlist.dev/e/C1ACB28C-F1B4-42A1-8135-797DEFD9F03C%40gmail.com ``` 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 ``` ## Sebastian Schuberth, 2017-05-10 18:58 Subject: Re: Possible bug in includeIf / conditional includes Message-ID: URL: https://gitlist.dev/e/b80c68fa-fe1e-3cf8-3c7e-527a4d941108%40gmail.com In-Reply-To: ``` 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 ``` ## Ævar Arnfjörð Bjarmason, 2017-05-10 19:48 Subject: Re: Possible bug in includeIf / conditional includes Message-ID: URL: https://gitlist.dev/e/CACBZZX7CgeW1OK%2Bg9Mbir62MMaCx4sHFZngUuWz_H241Sb355g%40mail.gmail.com In-Reply-To: ``` On Wed, May 10, 2017 at 8:58 PM, Sebastian Schuberth 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. ``` ## Raphael Stolt, 2017-05-10 21:21 Subject: Re: Possible bug in includeIf / conditional includes Message-ID: URL: https://gitlist.dev/e/CC77A0CE-88A5-4A3C-97AF-ED0180FE385B%40gmail.com In-Reply-To: ``` > > Am 10.05.2017 um 21:48 schrieb Ævar Arnfjörð Bjarmason : > > On Wed, May 10, 2017 at 8:58 PM, Sebastian Schuberth > 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 ``` ## Raphael Stolt, 2017-05-10 21:37 Subject: Re: Possible bug in includeIf / conditional includes Message-ID: URL: https://gitlist.dev/e/C2EAA2DF-B7F4-4011-BB0F-4F3BA9E96890%40gmail.com In-Reply-To: ``` > Am 10.05.2017 um 23:21 schrieb Raphael Stolt : > >> >> Am 10.05.2017 um 21:48 schrieb Ævar Arnfjörð Bjarmason : >> >> On Wed, May 10, 2017 at 8:58 PM, Sebastian Schuberth >> 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 ``` ## Jeff King, 2017-05-11 06:26 Subject: Re: Possible bug in includeIf / conditional includes Message-ID: <20170511062616.f7rg6f5cgrqeh7gf@sigill.intra.peff.net> URL: https://gitlist.dev/e/20170511062616.f7rg6f5cgrqeh7gf%40sigill.intra.peff.net In-Reply-To: ``` 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 ``` ## Jeff King, 2017-05-11 06:29 Subject: Re: Possible bug in includeIf / conditional includes Message-ID: <20170511062924.6euaynsbyocjcw3q@sigill.intra.peff.net> URL: https://gitlist.dev/e/20170511062924.6euaynsbyocjcw3q%40sigill.intra.peff.net In-Reply-To: <20170511062616.f7rg6f5cgrqeh7gf@sigill.intra.peff.net> ``` 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..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 ``` ## Ævar Arnfjörð Bjarmason, 2017-05-11 07:19 Subject: Re: Possible bug in includeIf / conditional includes Message-ID: URL: https://gitlist.dev/e/CACBZZX4ob04fG9ZZtvbdcqrYOKijoZohVdUCNyeayHZtdtNyxQ%40mail.gmail.com In-Reply-To: <20170511062924.6euaynsbyocjcw3q@sigill.intra.peff.net> ``` On Thu, May 11, 2017 at 8:29 AM, Jeff King 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..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..path` variable to the name of the file to be -included. The variable's value is treated the same way as -`include.path`. `includeIf..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..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". ``` ## Jeff King, 2017-05-11 07:42 Subject: Re: Possible bug in includeIf / conditional includes Message-ID: <20170511074257.xueofze3dv3xwrql@sigill.intra.peff.net> URL: https://gitlist.dev/e/20170511074257.xueofze3dv3xwrql%40sigill.intra.peff.net In-Reply-To: ``` 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..path` variable to the name of the file to be > -included. The variable's value is treated the same way as > -`include.path`. `includeIf..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..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 ``` ## Ævar Arnfjörð Bjarmason, 2017-05-11 07:49 Subject: Re: Possible bug in includeIf / conditional includes Message-ID: URL: https://gitlist.dev/e/CACBZZX6hpL0%3DZw0kvB-Ww95uGvuJ8129MTwkmv7DOEE1i66a_Q%40mail.gmail.com In-Reply-To: <20170511074257.xueofze3dv3xwrql@sigill.intra.peff.net> ``` On Thu, May 11, 2017 at 9:42 AM, Jeff King 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..path` variable to the name of the file to be >> -included. The variable's value is treated the same way as >> -`include.path`. `includeIf..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..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. ``` ## Junio C Hamano, 2017-05-11 07:54 Subject: Re: Possible bug in includeIf / conditional includes Message-ID: URL: https://gitlist.dev/e/xmqqbmqzbzh0.fsf%40gitster.mtv.corp.google.com In-Reply-To: <20170511074257.xueofze3dv3xwrql@sigill.intra.peff.net> ``` Jeff King 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. ``` ## Jeff King, 2017-05-11 07:54 Subject: Re: Possible bug in includeIf / conditional includes Message-ID: <20170511075437.yjxs6oit7ibe7rkq@sigill.intra.peff.net> URL: https://gitlist.dev/e/20170511075437.yjxs6oit7ibe7rkq%40sigill.intra.peff.net In-Reply-To: ``` 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..path` variable to the name of the file to be -included. The variable's value is treated the same way as -`include.path`. `includeIf..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 ``` ## Jeff King, 2017-05-11 07:56 Subject: Re: Possible bug in includeIf / conditional includes Message-ID: <20170511075624.572rujfaedzihpn4@sigill.intra.peff.net> URL: https://gitlist.dev/e/20170511075624.572rujfaedzihpn4%40sigill.intra.peff.net In-Reply-To: ``` On Thu, May 11, 2017 at 12:54:19AM -0700, Junio C Hamano wrote: > Jeff King 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 ``` ## Jeff King, 2017-05-11 09:09 Subject: [PATCH 0/4] doc improvements for config includes Message-ID: <20170511090905.tvkb4bk2ddjzjip4@sigill.intra.peff.net> URL: https://gitlist.dev/e/20170511090905.tvkb4bk2ddjzjip4%40sigill.intra.peff.net In-Reply-To: <20170511075437.yjxs6oit7ibe7rkq@sigill.intra.peff.net> ``` 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 ``` ## Jeff King, 2017-05-11 09:10 Subject: [PATCH 1/4] docs/config: clarify include/includeIf relationship Message-ID: <20170511091047.2dw52pnriulhjky3@sigill.intra.peff.net> URL: https://gitlist.dev/e/20170511091047.2dw52pnriulhjky3%40sigill.intra.peff.net In-Reply-To: <20170511090905.tvkb4bk2ddjzjip4@sigill.intra.peff.net> ``` 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 --- 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..path` variable to the name of the file to be -included. The variable's value is treated the same way as -`include.path`. `includeIf..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 ``` ## Jeff King, 2017-05-11 09:11 Subject: [PATCH 2/4] docs/config: give a relative includeIf example Message-ID: <20170511091106.cpk5emqtvp4cdeoi@sigill.intra.peff.net> URL: https://gitlist.dev/e/20170511091106.cpk5emqtvp4cdeoi%40sigill.intra.peff.net In-Reply-To: <20170511090905.tvkb4bk2ddjzjip4@sigill.intra.peff.net> ``` 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 --- 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 ``` ## Jeff King, 2017-05-11 09:13 Subject: [PATCH 3/4] docs/config: avoid the term "expand" for includes Message-ID: <20170511091304.ivauge72dncv65fs@sigill.intra.peff.net> URL: https://gitlist.dev/e/20170511091304.ivauge72dncv65fs%40sigill.intra.peff.net In-Reply-To: <20170511090905.tvkb4bk2ddjzjip4@sigill.intra.peff.net> ``` 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 --- 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 ``` ## Jeff King, 2017-05-11 09:14 Subject: [PATCH 4/4] docs/config: consistify include.path examples Message-ID: <20170511091430.jwpbyca3m32wterq@sigill.intra.peff.net> URL: https://gitlist.dev/e/20170511091430.jwpbyca3m32wterq%40sigill.intra.peff.net In-Reply-To: <20170511090905.tvkb4bk2ddjzjip4@sigill.intra.peff.net> ``` 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 --- 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 ``` ## Ævar Arnfjörð Bjarmason, 2017-05-11 09:29 Subject: Re: [PATCH 0/4] doc improvements for config includes Message-ID: URL: https://gitlist.dev/e/CACBZZX4BEOO%2BDeO-OzJzHT88Y2k7_ZmFRPJKJ9e6HMb9eMonZA%40mail.gmail.com In-Reply-To: <20170511090905.tvkb4bk2ddjzjip4@sigill.intra.peff.net> ``` On Thu, May 11, 2017 at 11:09 AM, Jeff King 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 > Documentation/config.txt | 31 +++++++++++++++++++++---------- > 1 file changed, 21 insertions(+), 10 deletions(-) > > -Peff ``` ## Junio C Hamano, 2017-05-11 09:41 Subject: Re: [PATCH 4/4] docs/config: consistify include.path examples Message-ID: URL: https://gitlist.dev/e/xmqqh90rafxk.fsf%40gitster.mtv.corp.google.com In-Reply-To: <20170511091430.jwpbyca3m32wterq@sigill.intra.peff.net> ``` Jeff King 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 > --- > 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. ``` ## Ramsay Jones, 2017-05-11 16:47 Subject: Re: [PATCH 1/4] docs/config: clarify include/includeIf relationship Message-ID: <0013fb73-12f2-a365-963e-ed517ea53dc7@ramsayjones.plus.com> URL: https://gitlist.dev/e/0013fb73-12f2-a365-963e-ed517ea53dc7%40ramsayjones.plus.com In-Reply-To: <20170511091047.2dw52pnriulhjky3@sigill.intra.peff.net> ``` 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 > --- > 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 ``` ## Jeff King, 2017-05-12 09:28 Subject: Re: [PATCH 1/4] docs/config: clarify include/includeIf relationship Message-ID: <20170512092825.wfxnnlk4qiced5ow@sigill.intra.peff.net> URL: https://gitlist.dev/e/20170512092825.wfxnnlk4qiced5ow%40sigill.intra.peff.net In-Reply-To: <0013fb73-12f2-a365-963e-ed517ea53dc7@ramsayjones.plus.com> ``` 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 ```