* git silently ignores include directive with single quotes @ 2018-09-08 18:58 Stas Bekman 2018-09-08 19:30 ` Martin Ågren 2018-09-08 21:22 ` Jeff King 0 siblings, 2 replies; 37+ messages in thread From: Stas Bekman @ 2018-09-08 18:58 UTC (permalink / raw) To: git Hi, One of the windows users discovered this bug, and I was able to reproduce it on linux. We are using a custom content filter configuration REPO/.gitconfig which needs to be enabled inside REPO/.git/config: This works: [include] path = ../.gitconfig This doesn’t: [include] path = '../.gitconfig' Notice the single quotes around the filename. When this is the case git silently (!) ignores the custom configuration, which is clearly a bug. I found the easiest to debug this is by using: git config --list --show-origin In the former case it shows the custom config, in the latter it does not. Yet, git gives no indication of any errors, not even with GIT_TRACE and other debug vars. The original problem cropped up due to using: git config --local include.path '../.gitconfig' which on linux stripped the single quotes, but on some windows git bash emulation it kept them. What am I suggesting is that git: (1) should complain if it encounters an invalid configuration and not silently ignore it. It took quite some effort and time to figure the culprit. (2) probably allow the quoted location of the file, but it's much less important, as it's easy to rectify once git gives user #1 I don't have the details about the windows user setup, but I was able to reproduce this bug with git version 2.17.1 on linux. Thank you. -- ________________________________________________ Stas Bekman <'))))>< <'))))>< https://stasosphere.com https://chestofbooks.com https://experientialsexlab.com https://stason.org https://stasosphere.com/experience-life/my-books ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: git silently ignores include directive with single quotes 2018-09-08 18:58 git silently ignores include directive with single quotes Stas Bekman @ 2018-09-08 19:30 ` Martin Ågren 2018-09-08 19:44 ` Stas Bekman ` (2 more replies) 2018-09-08 21:22 ` Jeff King 1 sibling, 3 replies; 37+ messages in thread From: Martin Ågren @ 2018-09-08 19:30 UTC (permalink / raw) To: stas; +Cc: Git Mailing List, Jeff King Hi Stas On Sat, 8 Sep 2018 at 21:00, Stas Bekman <stas@stason.org> wrote: > [include] > path = '../.gitconfig' > > Notice the single quotes around the filename. When this is the case git > silently (!) ignores the custom configuration, which is clearly a bug. Thanks for reporting and describing out your expectations and what you observed. Actually, there is a test explicitly testing that 'missing include files are ignored'. I couldn't find a motivation for this in 9b25a0b52e (config: add include directive, 2012-02-06). > The original problem cropped up due to using: > > git config --local include.path '../.gitconfig' > > which on linux stripped the single quotes, but on some windows git bash > emulation it kept them. Huh, I wouldn't have expected them to be kept. You learn something new every day... > What am I suggesting is that git: > > (1) should complain if it encounters an invalid configuration and not > silently ignore it. It took quite some effort and time to figure the > culprit. Sounds reasonable to me, but I might be missing something. I'm cc-ing the original author. Maybe he can recall why he made sure it silently ignores missing files. > (2) probably allow the quoted location of the file, but it's much less > important, as it's easy to rectify once git gives user #1 I don't think this will work. Allowing quoting for just this one item, or for all? Any and all quoting or just at the first and last character? What about those config items where quotes might legitimately occur, i.e., we'd need some escaping? Actually, something like '.gitconfig' *with* *those* *quotes* is a valid filename on my machine. Thank you for reporting. Martin ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: git silently ignores include directive with single quotes 2018-09-08 19:30 ` Martin Ågren @ 2018-09-08 19:44 ` Stas Bekman 2018-09-08 19:53 ` Stas Bekman 2018-09-08 19:54 ` Ævar Arnfjörð Bjarmason 2 siblings, 0 replies; 37+ messages in thread From: Stas Bekman @ 2018-09-08 19:44 UTC (permalink / raw) To: Martin Ågren; +Cc: Git Mailing List, Jeff King On 2018-09-08 12:30 PM, Martin Ågren wrote: > Actually, there is a test explicitly testing that 'missing include files > are ignored'. I couldn't find a motivation for this in 9b25a0b52e > (config: add include directive, 2012-02-06). Thank you for the follow up, Martin. And discovering that it is by design. I suppose this could have been done to optimize run-time performance. But there must be a way for a user to validate their custom configuration. So perhaps there should be a specific directive to do so? One could argue that: git config --list --show-origin does exactly that. Except it should probably also indicate that some configuration file or parts of were ignored - and clearly indicate the exact nature of the problem. In which case it'd be sufficient. >> (2) probably allow the quoted location of the file, but it's much less >> important, as it's easy to rectify once git gives user #1 > > I don't think this will work. Allowing quoting for just this one item, > or for all? Any and all quoting or just at the first and last character? > What about those config items where quotes might legitimately occur, > i.e., we'd need some escaping? Actually, something like '.gitconfig' > *with* *those* *quotes* is a valid filename on my machine. Let's ignore this sub-issue for now. If we can get git to report when something is mis-configured, this issue can then be easily resolved. -- ________________________________________________ Stas Bekman <'))))>< <'))))>< https://stasosphere.com https://chestofbooks.com https://experientialsexlab.com https://stason.org https://stasosphere.com/experience-life/my-books ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: git silently ignores include directive with single quotes 2018-09-08 19:30 ` Martin Ågren 2018-09-08 19:44 ` Stas Bekman @ 2018-09-08 19:53 ` Stas Bekman 2018-09-08 20:02 ` Ævar Arnfjörð Bjarmason 2018-09-08 19:54 ` Ævar Arnfjörð Bjarmason 2 siblings, 1 reply; 37+ messages in thread From: Stas Bekman @ 2018-09-08 19:53 UTC (permalink / raw) To: Martin Ågren; +Cc: Git Mailing List, Jeff King On 2018-09-08 12:30 PM, Martin Ågren wrote: > Hi Stas > > On Sat, 8 Sep 2018 at 21:00, Stas Bekman <stas@stason.org> wrote: >> [include] >> path = '../.gitconfig' > Actually, there is a test explicitly testing that 'missing include files > are ignored'. I couldn't find a motivation for this in 9b25a0b52e > (config: add include directive, 2012-02-06). And also to stress out, that the file is not missing. At least in the world of unix, in particular its many shells, - command line arguments "xyz", 'xyz', xyz are often deemed to be the same if there are no spaces in the word. So that's why it took us a lot of trial and error to even consider the quotes in '../.gitconfig' as a problem. While git deems it different, to me: path = '../.gitconfig' path = "../.gitconfig" path = ../.gitconfig appear to be the "same". So git needs to have a way to say otherwise. I realize I am going back to the issue of quoting here, after suggesting to ignore it. So to clarify I'm bringing it up only in the context of wanting git to tell the user what it wants, and not necessarily asking to support all the possible ways one could quote a filepath. -- ________________________________________________ Stas Bekman <'))))>< <'))))>< https://stasosphere.com https://chestofbooks.com https://experientialsexlab.com https://stason.org https://stasosphere.com/experience-life/my-books ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: git silently ignores include directive with single quotes 2018-09-08 19:53 ` Stas Bekman @ 2018-09-08 20:02 ` Ævar Arnfjörð Bjarmason 2018-09-08 20:13 ` Stas Bekman 0 siblings, 1 reply; 37+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2018-09-08 20:02 UTC (permalink / raw) To: Stas Bekman; +Cc: Martin Ågren, Git Mailing List, Jeff King On Sat, Sep 08 2018, Stas Bekman wrote: > On 2018-09-08 12:30 PM, Martin Ågren wrote: >> Hi Stas >> >> On Sat, 8 Sep 2018 at 21:00, Stas Bekman <stas@stason.org> wrote: >>> [include] >>> path = '../.gitconfig' > >> Actually, there is a test explicitly testing that 'missing include files >> are ignored'. I couldn't find a motivation for this in 9b25a0b52e >> (config: add include directive, 2012-02-06). > > And also to stress out, that the file is not missing. At least in the > world of unix, in particular its many shells, - command line arguments > "xyz", 'xyz', xyz are often deemed to be the same if there are no spaces > in the word. So that's why it took us a lot of trial and error to even > consider the quotes in '../.gitconfig' as a problem. While git deems it > different, to me: > > path = '../.gitconfig' > path = "../.gitconfig" > path = ../.gitconfig > > appear to be the "same". So git needs to have a way to say otherwise. > > I realize I am going back to the issue of quoting here, after suggesting > to ignore it. So to clarify I'm bringing it up only in the context of > wanting git to tell the user what it wants, and not necessarily asking > to support all the possible ways one could quote a filepath. Aside from other issues here, in the "wold of unix" (not that we only use the git config syntax on those sort of systems) you can't assume that just because some quoting construct works in the shell, that it works the same way in some random config format. If you look in your /etc/ you'll find plenty of config formats where you can't use single, double and no quotes interchangeably, so I don't see what hte confusion is with that particular aspect of this. Although as I mentioned in <87bm97rcih.fsf@evledraar.gmail.com> the fact that we ignore missing includes definitely needs to be documented, but that our quoting constructs in our config format behave like they do in POSIX shells I see as a non-issue. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: git silently ignores include directive with single quotes 2018-09-08 20:02 ` Ævar Arnfjörð Bjarmason @ 2018-09-08 20:13 ` Stas Bekman 2018-09-08 20:28 ` Ævar Arnfjörð Bjarmason 2018-09-09 2:51 ` Paul Smith 0 siblings, 2 replies; 37+ messages in thread From: Stas Bekman @ 2018-09-08 20:13 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: Martin Ågren, Git Mailing List, Jeff King On 2018-09-08 01:02 PM, Ævar Arnfjörð Bjarmason wrote: > Aside from other issues here, in the "wold of unix" (not that we only > use the git config syntax on those sort of systems) you can't assume > that just because some quoting construct works in the shell, that it > works the same way in some random config format. If you look in your > /etc/ you'll find plenty of config formats where you can't use single, > double and no quotes interchangeably, so I don't see what hte confusion > is with that particular aspect of this. > > Although as I mentioned in <87bm97rcih.fsf@evledraar.gmail.com> the fact > that we ignore missing includes definitely needs to be documented, but > that our quoting constructs in our config format behave like they do in > POSIX shells I see as a non-issue. I agree that I should make no such assumptions. Thank you. But it is a cross-platform problem. I remind that the original problem came from a simple command: git config --local include.path '../.gitconfig' Which on linux removed the quotes and all was fine, and on windows the same command kept the quotes and the user was tearing his hair out trying to understand why the custom config was ignored. So you can say, don't use the quotes in first place. But what if you have: git config --local include.path 'somewhere on the system/.gitconfig' you have to use single or double quotes inside the shell to keep it as a single argument, yet on some windows set ups it'll result in git ignoring this configuration directive, as the quotes will end up in git config file. I'd say at the very least 'git config' could have an option --verify-path or something similar and for it to validate that the path is there exactly as it adds it to .git/config at the time of running this command to help the user debug the situation. Of course this won't help if .git/config is modified manually. But it's a step towards supporting users. I hope this clarifies the situation. -- ________________________________________________ Stas Bekman <'))))>< <'))))>< https://stasosphere.com https://chestofbooks.com https://experientialsexlab.com https://stason.org https://stasosphere.com/experience-life/my-books ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: git silently ignores include directive with single quotes 2018-09-08 20:13 ` Stas Bekman @ 2018-09-08 20:28 ` Ævar Arnfjörð Bjarmason 2018-09-08 20:58 ` Stas Bekman 2018-09-09 2:51 ` Paul Smith 1 sibling, 1 reply; 37+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2018-09-08 20:28 UTC (permalink / raw) To: Stas Bekman Cc: Martin Ågren, Git Mailing List, Jeff King, Johannes Schindelin On Sat, Sep 08 2018, Stas Bekman wrote: > On 2018-09-08 01:02 PM, Ævar Arnfjörð Bjarmason wrote: > >> Aside from other issues here, in the "wold of unix" (not that we only >> use the git config syntax on those sort of systems) you can't assume >> that just because some quoting construct works in the shell, that it >> works the same way in some random config format. If you look in your >> /etc/ you'll find plenty of config formats where you can't use single, >> double and no quotes interchangeably, so I don't see what hte confusion >> is with that particular aspect of this. >> >> Although as I mentioned in <87bm97rcih.fsf@evledraar.gmail.com> the fact >> that we ignore missing includes definitely needs to be documented, but >> that our quoting constructs in our config format behave like they do in >> POSIX shells I see as a non-issue. > > I agree that I should make no such assumptions. Thank you. But it is a > cross-platform problem. I remind that the original problem came from a > simple command: > > git config --local include.path '../.gitconfig' > > Which on linux removed the quotes and all was fine, and on windows the > same command kept the quotes and the user was tearing his hair out > trying to understand why the custom config was ignored. > > So you can say, don't use the quotes in first place. But what if you have: > > git config --local include.path 'somewhere on the system/.gitconfig' > > you have to use single or double quotes inside the shell to keep it as a > single argument, yet on some windows set ups it'll result in git > ignoring this configuration directive, as the quotes will end up in git > config file. > > I'd say at the very least 'git config' could have an option > --verify-path or something similar and for it to validate that the path > is there exactly as it adds it to .git/config at the time of running > this command to help the user debug the situation. Of course this won't > help if .git/config is modified manually. But it's a step towards > supporting users. > > I hope this clarifies the situation. Yeah, some version of this is sensible. There's at least a doc patch in here somewhere, if not some "warn if missing" mode. So don't take any of this as minimizing that aspect of your bug report. *But* There's just no way that "git" the tool can somehow in a sane way rescue you from knowing the quoting rules of the shell on your system, which differ wildly between the likes of Windows and Linux. We guarantee that if you pass us the string "foo" it'll work the same (for the purposes of config syntax, and most other things on all systems). We can't guarantee that just because on one system/shell "foo" means the same as 'foo' when you type it into the terminal, but others it doesn't that we'll treat it the same way, it's ultimately up to you to know the quoting rules of your system shell. On linux/bash I can also do "git config foo.bar <(some-command)", and there's some systems where that'll be passed in as though (on linux/bash) we'd passed in: git config foo.bar "<(some-command)" What are we supposed to do with that? In particular in the case where "foo.bar" is supposed to point to a valid filename, and "<(some-command)" *is* a valid filename? ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: git silently ignores include directive with single quotes 2018-09-08 20:28 ` Ævar Arnfjörð Bjarmason @ 2018-09-08 20:58 ` Stas Bekman 0 siblings, 0 replies; 37+ messages in thread From: Stas Bekman @ 2018-09-08 20:58 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: Martin Ågren, Git Mailing List, Jeff King, Johannes Schindelin On 2018-09-08 01:28 PM, Ævar Arnfjörð Bjarmason wrote: [...] > Yeah, some version of this is sensible. There's at least a doc patch in > here somewhere, if not some "warn if missing" mode. > > So don't take any of this as minimizing that aspect of your bug report. > > *But* > > There's just no way that "git" the tool can somehow in a sane way rescue > you from knowing the quoting rules of the shell on your system, which > differ wildly between the likes of Windows and Linux. I understand. All your explanations are perfectly reasonable, Ævar. Thank you. Yet, there needs to be some way for a user to know that git ignored something if their configuration doesn't work as expected. 1) I suggest this is done via: git config --list --show-origin where the new addition would be to also show configuration parts that are not active and indicating why it is so. So for example currently I get on a valid configuration setup and having git/../.gitconfig in place the following output: [...] file:/home/stas/.gitconfig mergetool.prompt=false [...] file:.git/config include.path=../.gitconfig [...] file:.git/../.gitconfig filter.fastai-nbstripout-code.clean=tools/fastai-nbstripout [...] Now, if include.path=../.gitconfig is there and file:.git/../.gitconfig is not found, it will indicate that in some way that stands out for the user. Perhaps: [...] file:/home/stas/.gitconfig mergetool.prompt=false [...] file:.git/config include.path=../.gitconfig [...] file:.git/../.gitconfig FILE NOT FOUND! Ignored configuration [...] So that would allow things to work as before, but now we have a way to debug user-side configuration. And of course hoping that the docs would indicate that method for debugging configuration problems. I hope this is a reasonable suggestion that doesn't require any modification on the users' part who rely on this silent ignoring "feature", yet lending to a configuration debug feature. 2) And a secondary suggestion I mentioned earlier is to also have a flag for git config to validate the path as it is being configured: git config --local include.path '../.gitconfig' --validate-path so that on shells that deal with quoting differently, than what git expects, this git command will fail saying: error: can't find file:.git/'../.gitconfig' or at the very least give a warning if we don't want it be fatal. Though I see no problem with it being fatal if a user uses a special flag. I made this second suggestion since it will help users to detect the problem early on. Before they need to search for another debug solution such as the first one suggested in this email. 3) Finally, it'd be useful to have GIT_TRACE=1 state that so and so include path wasn't found and was ignored during various 'git whatever' commands. I am open to any or all of these solutions, or alternative suggestions of course. Thank you. -- ________________________________________________ Stas Bekman <'))))>< <'))))>< https://stasosphere.com https://chestofbooks.com https://experientialsexlab.com https://stason.org https://stasosphere.com/experience-life/my-books ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: git silently ignores include directive with single quotes 2018-09-08 20:13 ` Stas Bekman 2018-09-08 20:28 ` Ævar Arnfjörð Bjarmason @ 2018-09-09 2:51 ` Paul Smith 2018-09-09 2:57 ` Stas Bekman 1 sibling, 1 reply; 37+ messages in thread From: Paul Smith @ 2018-09-09 2:51 UTC (permalink / raw) To: Stas Bekman, Ævar Arnfjörð Bjarmason Cc: Martin Ågren, Git Mailing List, Jeff King On Sat, 2018-09-08 at 13:13 -0700, Stas Bekman wrote: > I remind that the original problem came from a simple command: > > git config --local include.path '../.gitconfig' > > Which on linux removed the quotes and all was fine, and on windows > the same command kept the quotes and the user was tearing his hair > out trying to understand why the custom config was ignored. I'm quite sure that the user was not using the Git Bash shell when they entered this command, but instead using command.com or powershell or some variant of that. If you use Git Bash as your shell then quotes will be handled like any POSIX shell and the above will do what (we all) expect. If you use command.com and powershell and use single quotes, then the single quotes will be put into the config file as you observed, because those shells don't deal with single quotes. You could use double-quotes, which ARE handled by command.com and powershell; in that case they would be stripped out and would not appear in the config. If we were designing from scratch maybe using something like GNU make's "include" vs "sinclude" (silent include--this is another name for the already mentioned "-include") would work; maybe "path" and "spath" or something. But to make it work right you really want the default behavior to be "warn if the file is not found" and have the special behavior be "quiet if the file is not found" otherwise it doesn't really help beginners to avoid errors. And that's a backward- compatibility problem. To my mind, adding extra "check this" options isn't very useful either: these kinds of warnings need to be on by default to be effective. The beginners, who need them, aren't going to remember to add extra options to enable more checking. What I personally think would be more useful would be some sort of "verbose parsing" option to git config, that would parse the configuration just as a normal Git command would and show diagnostic output as the entire config is parsed: for each action line the config file name and line number, and the operation performed (and any message about it) would be printed. This could be useful in a variety of situations, for instance to discover conflicts between local, global, and system configuration, easily see where settings are coming from, etc. And as part of this output, when an include file was not present or we didn't have permissions or whatever, an appropriate error message would be generated. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: git silently ignores include directive with single quotes 2018-09-09 2:51 ` Paul Smith @ 2018-09-09 2:57 ` Stas Bekman 0 siblings, 0 replies; 37+ messages in thread From: Stas Bekman @ 2018-09-09 2:57 UTC (permalink / raw) To: paul, Ævar Arnfjörð Bjarmason Cc: Martin Ågren, Git Mailing List, Jeff King On 2018-09-08 07:51 PM, Paul Smith wrote: [...] > What I personally think would be more useful would be some sort of > "verbose parsing" option to git config, that would parse the > configuration just as a normal Git command would and show diagnostic > output as the entire config is parsed: for each action line the config > file name and line number, and the operation performed (and any message > about it) would be printed. This could be useful in a variety of > situations, for instance to discover conflicts between local, global, > and system configuration, easily see where settings are coming from, > etc. > > And as part of this output, when an include file was not present or we > didn't have permissions or whatever, an appropriate error message would > be generated. I was thinking along the same lines, Paul - i.e. no need to change anything in the config syntax, but to provide better diagnostics. I quote below what I suggested in an earlier email, but I like Paul's idea even better as it'd be useful to many situations and not just the one that started this thread. > 1) I suggest this is done via: > > git config --list --show-origin > > where the new addition would be to also show configuration parts that > are not active and indicating why it is so. > > So for example currently I get on a valid configuration setup and having > git/../.gitconfig in place the following output: > > [...] > file:/home/stas/.gitconfig mergetool.prompt=false > [...] > file:.git/config include.path=../.gitconfig > [...] > file:.git/../.gitconfig > filter.fastai-nbstripout-code.clean=tools/fastai-nbstripout > [...] > > Now, if include.path=../.gitconfig is there and file:.git/../.gitconfig > is not found, it will indicate that in some way that stands out for the > user. Perhaps: > > [...] > file:/home/stas/.gitconfig mergetool.prompt=false > [...] > file:.git/config include.path=../.gitconfig > [...] > file:.git/../.gitconfig FILE NOT FOUND! Ignored configuration > [...] > > So that would allow things to work as before, but now we have a way to > debug user-side configuration. And of course hoping that the docs would > indicate that method for debugging configuration problems. > > I hope this is a reasonable suggestion that doesn't require any > modification on the users' part who rely on this silent ignoring > "feature", yet lending to a configuration debug feature. -- ________________________________________________ Stas Bekman <'))))>< <'))))>< https://stasosphere.com https://chestofbooks.com https://experientialsexlab.com https://stason.org https://stasosphere.com/experience-life/my-books ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: git silently ignores include directive with single quotes 2018-09-08 19:30 ` Martin Ågren 2018-09-08 19:44 ` Stas Bekman 2018-09-08 19:53 ` Stas Bekman @ 2018-09-08 19:54 ` Ævar Arnfjörð Bjarmason 2018-09-08 20:04 ` Stas Bekman 2018-09-08 21:14 ` Jeff King 2 siblings, 2 replies; 37+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2018-09-08 19:54 UTC (permalink / raw) To: Martin Ågren; +Cc: stas, Git Mailing List, Jeff King On Sat, Sep 08 2018, Martin Ågren wrote: > Hi Stas > > On Sat, 8 Sep 2018 at 21:00, Stas Bekman <stas@stason.org> wrote: >> [include] >> path = '../.gitconfig' >> >> Notice the single quotes around the filename. When this is the case git >> silently (!) ignores the custom configuration, which is clearly a bug. > > Thanks for reporting and describing out your expectations and what you > observed. > > Actually, there is a test explicitly testing that 'missing include files > are ignored'. I couldn't find a motivation for this in 9b25a0b52e > (config: add include directive, 2012-02-06). > >> The original problem cropped up due to using: >> >> git config --local include.path '../.gitconfig' >> >> which on linux stripped the single quotes, but on some windows git bash >> emulation it kept them. > > Huh, I wouldn't have expected them to be kept. You learn something > new every day... > >> What am I suggesting is that git: >> >> (1) should complain if it encounters an invalid configuration and not >> silently ignore it. It took quite some effort and time to figure the >> culprit. > > Sounds reasonable to me, but I might be missing something. I'm cc-ing > the original author. Maybe he can recall why he made sure it silently > ignores missing files. > >> (2) probably allow the quoted location of the file, but it's much less >> important, as it's easy to rectify once git gives user #1 > > I don't think this will work. Allowing quoting for just this one item, > or for all? Any and all quoting or just at the first and last character? > What about those config items where quotes might legitimately occur, > i.e., we'd need some escaping? Actually, something like '.gitconfig' > *with* *those* *quotes* is a valid filename on my machine. The reason missing includes are ignored is that the way this is expected to be used is e.g.: [include] path ~/.gitconfig.work Where .gitconfig.work is some configuration you're going to drop into place on your $dayjob servers, but not on your personal machine, even though you sync the same ~/.gitconfig everywhere. A lot of people who use includes rely on this, but I see from this thread this should be better documented. If we were to make nonexisting files an error, we'd need something like an extension of the includeIf syntax added in 3efd0bedc6 ("config: add conditional include", 2017-03-01) 3efd0bedc6 ("config: add conditional include", 2017-03-01). I.e.: [includeIfcond "test -e ~/.gitconfig.work"] path = ~/.gitconfig.work Or something like that, this is getting increasingly harder to shove into the *.ini config syntax. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: git silently ignores include directive with single quotes 2018-09-08 19:54 ` Ævar Arnfjörð Bjarmason @ 2018-09-08 20:04 ` Stas Bekman 2018-09-08 20:16 ` Ævar Arnfjörð Bjarmason 2018-09-08 21:14 ` Jeff King 1 sibling, 1 reply; 37+ messages in thread From: Stas Bekman @ 2018-09-08 20:04 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason, Martin Ågren Cc: Git Mailing List, Jeff King On 2018-09-08 12:54 PM, Ævar Arnfjörð Bjarmason wrote: > > On Sat, Sep 08 2018, Martin Ågren wrote: > >> Hi Stas >> >> On Sat, 8 Sep 2018 at 21:00, Stas Bekman <stas@stason.org> wrote: >>> [include] >>> path = '../.gitconfig' >>> >>> Notice the single quotes around the filename. When this is the case git >>> silently (!) ignores the custom configuration, which is clearly a bug. >> >> Thanks for reporting and describing out your expectations and what you >> observed. >> >> Actually, there is a test explicitly testing that 'missing include files >> are ignored'. I couldn't find a motivation for this in 9b25a0b52e >> (config: add include directive, 2012-02-06). >> >>> The original problem cropped up due to using: >>> >>> git config --local include.path '../.gitconfig' >>> >>> which on linux stripped the single quotes, but on some windows git bash >>> emulation it kept them. >> >> Huh, I wouldn't have expected them to be kept. You learn something >> new every day... >> >>> What am I suggesting is that git: >>> >>> (1) should complain if it encounters an invalid configuration and not >>> silently ignore it. It took quite some effort and time to figure the >>> culprit. >> >> Sounds reasonable to me, but I might be missing something. I'm cc-ing >> the original author. Maybe he can recall why he made sure it silently >> ignores missing files. >> >>> (2) probably allow the quoted location of the file, but it's much less >>> important, as it's easy to rectify once git gives user #1 >> >> I don't think this will work. Allowing quoting for just this one item, >> or for all? Any and all quoting or just at the first and last character? >> What about those config items where quotes might legitimately occur, >> i.e., we'd need some escaping? Actually, something like '.gitconfig' >> *with* *those* *quotes* is a valid filename on my machine. > > The reason missing includes are ignored is that the way this is expected > to be used is e.g.: > > [include] > path ~/.gitconfig.work > > Where .gitconfig.work is some configuration you're going to drop into > place on your $dayjob servers, but not on your personal machine, even > though you sync the same ~/.gitconfig everywhere. Thank you for clarifying why this is done silently, Ævar. It makes sense then. > If we were to make nonexisting files an error, we'd need something like > an extension of the includeIf syntax added in 3efd0bedc6 ("config: add > conditional include", 2017-03-01) 3efd0bedc6 ("config: add conditional > include", 2017-03-01). I.e.: > > [includeIfcond "test -e ~/.gitconfig.work"] > path = ~/.gitconfig.work > > Or something like that, this is getting increasingly harder to shove > into the *.ini config syntax. This suggestion won't solve the real problem. The real problem is that git can't find '.gitconfig' even though it's there, due to single quotes around the filepath. So the suggested check will still ignore the configuration even if it's there. This also leads me to think what if the include path has spaces in it? path = ~/somewhere on my system/.gitconfig.work most people would assume quotes are needed around the filepath. -- ________________________________________________ Stas Bekman <'))))>< <'))))>< https://stasosphere.com https://chestofbooks.com https://experientialsexlab.com https://stason.org https://stasosphere.com/experience-life/my-books ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: git silently ignores include directive with single quotes 2018-09-08 20:04 ` Stas Bekman @ 2018-09-08 20:16 ` Ævar Arnfjörð Bjarmason 0 siblings, 0 replies; 37+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2018-09-08 20:16 UTC (permalink / raw) To: Stas Bekman; +Cc: Martin Ågren, Git Mailing List, Jeff King On Sat, Sep 08 2018, Stas Bekman wrote: > On 2018-09-08 12:54 PM, Ævar Arnfjörð Bjarmason wrote: >> >> On Sat, Sep 08 2018, Martin Ågren wrote: >> >>> Hi Stas >>> >>> On Sat, 8 Sep 2018 at 21:00, Stas Bekman <stas@stason.org> wrote: >>>> [include] >>>> path = '../.gitconfig' >>>> >>>> Notice the single quotes around the filename. When this is the case git >>>> silently (!) ignores the custom configuration, which is clearly a bug. >>> >>> Thanks for reporting and describing out your expectations and what you >>> observed. >>> >>> Actually, there is a test explicitly testing that 'missing include files >>> are ignored'. I couldn't find a motivation for this in 9b25a0b52e >>> (config: add include directive, 2012-02-06). >>> >>>> The original problem cropped up due to using: >>>> >>>> git config --local include.path '../.gitconfig' >>>> >>>> which on linux stripped the single quotes, but on some windows git bash >>>> emulation it kept them. >>> >>> Huh, I wouldn't have expected them to be kept. You learn something >>> new every day... >>> >>>> What am I suggesting is that git: >>>> >>>> (1) should complain if it encounters an invalid configuration and not >>>> silently ignore it. It took quite some effort and time to figure the >>>> culprit. >>> >>> Sounds reasonable to me, but I might be missing something. I'm cc-ing >>> the original author. Maybe he can recall why he made sure it silently >>> ignores missing files. >>> >>>> (2) probably allow the quoted location of the file, but it's much less >>>> important, as it's easy to rectify once git gives user #1 >>> >>> I don't think this will work. Allowing quoting for just this one item, >>> or for all? Any and all quoting or just at the first and last character? >>> What about those config items where quotes might legitimately occur, >>> i.e., we'd need some escaping? Actually, something like '.gitconfig' >>> *with* *those* *quotes* is a valid filename on my machine. >> >> The reason missing includes are ignored is that the way this is expected >> to be used is e.g.: >> >> [include] >> path ~/.gitconfig.work >> >> Where .gitconfig.work is some configuration you're going to drop into >> place on your $dayjob servers, but not on your personal machine, even >> though you sync the same ~/.gitconfig everywhere. > > Thank you for clarifying why this is done silently, Ævar. It makes sense > then. > >> If we were to make nonexisting files an error, we'd need something like >> an extension of the includeIf syntax added in 3efd0bedc6 ("config: add >> conditional include", 2017-03-01) 3efd0bedc6 ("config: add conditional >> include", 2017-03-01). I.e.: >> >> [includeIfcond "test -e ~/.gitconfig.work"] >> path = ~/.gitconfig.work >> >> Or something like that, this is getting increasingly harder to shove >> into the *.ini config syntax. > > This suggestion won't solve the real problem. The real problem is that > git can't find '.gitconfig' even though it's there, due to single quotes > around the filepath. So the suggested check will still ignore the > configuration even if it's there. ...because that's not how the *.ini syntax works. That means to look up a file called '.gitconfig', as opposed to .gitconfig, ie. one that actually starts with a single quote. On POSIX systems filenames can include all bytes except \0, so we need some way to include those. I've just created a 'foo' file (i.e. one that has a 5-chararcer name, including single quotes), and including it via git's config works, as opposed to the filename foo (i.e. the three-character version). I can see how this is confusing, but we can't have some way to have this "ignore missing" feature and warn about stuff like 'foo' v.s. "foo" v.s. foo without carrying some list of quoting constructs deemed to be confusing, and forbidding includes from files that look like that. > This also leads me to think what if the include path has spaces in it? > > path = ~/somewhere on my system/.gitconfig.work > > most people would assume quotes are needed around the filepath. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: git silently ignores include directive with single quotes 2018-09-08 19:54 ` Ævar Arnfjörð Bjarmason 2018-09-08 20:04 ` Stas Bekman @ 2018-09-08 21:14 ` Jeff King 2018-09-08 22:10 ` Ramsay Jones 2018-09-08 22:32 ` git silently ignores include directive with single quotes Ævar Arnfjörð Bjarmason 1 sibling, 2 replies; 37+ messages in thread From: Jeff King @ 2018-09-08 21:14 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: Martin Ågren, stas, Git Mailing List On Sat, Sep 08, 2018 at 09:54:14PM +0200, Ævar Arnfjörð Bjarmason wrote: > The reason missing includes are ignored is that the way this is expected > to be used is e.g.: > > [include] > path ~/.gitconfig.work > > Where .gitconfig.work is some configuration you're going to drop into > place on your $dayjob servers, but not on your personal machine, even > though you sync the same ~/.gitconfig everywhere. > > A lot of people who use includes rely on this, but I see from this > thread this should be better documented. Right, this was an intentional choice at the time the feature was added, to support this kind of feature. I'd note also that it mirrors other misspelled keys. E.g.: [include] psth = whatever will also not generate an error. This is also intentional, for two reasons: 1. Git's config format has always been designed to carry extra keys used by third-party scripts and porcelain. So we don't actually know the complete set of valid keys. (Though you could make an argument that git-core could stake out include.* as its own). 2. It makes using multiple git versions easier in some ways (though also harder in others). A config key that isn't known to the current version will be quietly ignored. Of course those things mean that true spelling mistakes are harder to catch as such, because Git doesn't know that's what they are. And here I'm talking config _keys_, not values. So I'm just explaining the philosophical thinking that led to the "missing file is a silent noop". It doesn't _have_ to behave the same. That said, it _does_ behave the same and people are likely depending on it at this point. So if we introduce a warning, for example, there needs to be some way to suppress it. Probably: [include] warnOnMissing = false path = ... would be enough (with the default being "true"). You could even do: [include] warnOnMissing = false path = one warnOnMissing = true path = two to treat two includes differently (though I'm not sure why you would want to). > If we were to make nonexisting files an error, we'd need something like > an extension of the includeIf syntax added in 3efd0bedc6 ("config: add > conditional include", 2017-03-01) 3efd0bedc6 ("config: add conditional > include", 2017-03-01). I.e.: > > [includeIfcond "test -e ~/.gitconfig.work"] > path = ~/.gitconfig.work > > Or something like that, this is getting increasingly harder to shove > into the *.ini config syntax. I think it would be simpler to just introduce a new key that's a variant of "path". Like: [include] maybePath = ~/.gitconfig.work Though if it really is just a warning, the "warnOnMissing" above would make that unnecessary (and it also scales better if we have to end up adding more behavior tweaks in the future). -Peff ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: git silently ignores include directive with single quotes 2018-09-08 21:14 ` Jeff King @ 2018-09-08 22:10 ` Ramsay Jones 2018-09-09 2:33 ` Jeff King 2018-09-08 22:32 ` git silently ignores include directive with single quotes Ævar Arnfjörð Bjarmason 1 sibling, 1 reply; 37+ messages in thread From: Ramsay Jones @ 2018-09-08 22:10 UTC (permalink / raw) To: Jeff King, Ævar Arnfjörð Bjarmason Cc: Martin Ågren, stas, Git Mailing List On 08/09/18 22:14, Jeff King wrote: > On Sat, Sep 08, 2018 at 09:54:14PM +0200, Ævar Arnfjörð Bjarmason wrote: > >> The reason missing includes are ignored is that the way this is expected >> to be used is e.g.: >> >> [include] >> path ~/.gitconfig.work >> >> Where .gitconfig.work is some configuration you're going to drop into >> place on your $dayjob servers, but not on your personal machine, even >> though you sync the same ~/.gitconfig everywhere. >> >> A lot of people who use includes rely on this, but I see from this >> thread this should be better documented. > > Right, this was an intentional choice at the time the feature was added, > to support this kind of feature. I'd note also that it mirrors other > misspelled keys. E.g.: > > [include] > psth = whatever > [snip] > That said, it _does_ behave the same and people are likely depending on > it at this point. So if we introduce a warning, for example, there needs > to be some way to suppress it. > > Probably: > > [include] > warnOnMissing = false > path = ... I was going to suggest, inspired by Makefile syntax, that [-include] would not complain if the file was missing ... except, of course, it's too late for that! ;-) I suppose [+include] could complain if the file is missing instead, ... dunno. ATB, Ramsay Jones ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: git silently ignores include directive with single quotes 2018-09-08 22:10 ` Ramsay Jones @ 2018-09-09 2:33 ` Jeff King 2018-09-11 20:36 ` Junio C Hamano 0 siblings, 1 reply; 37+ messages in thread From: Jeff King @ 2018-09-09 2:33 UTC (permalink / raw) To: Ramsay Jones Cc: Ævar Arnfjörð Bjarmason, Martin Ågren, stas, Git Mailing List On Sat, Sep 08, 2018 at 11:10:44PM +0100, Ramsay Jones wrote: > > Probably: > > > > [include] > > warnOnMissing = false > > path = ... > > I was going to suggest, inspired by Makefile syntax, that > [-include] would not complain if the file was missing ... > except, of course, it's too late for that! ;-) > > I suppose [+include] could complain if the file is missing > instead, ... dunno. I think that's syntactically invalid. At any rate, there are clearly three options for setting a bit: 1. In the section header (+include, or Ævar's includeIf suggestion). 2. In another key (which looks pretty clean, but does introduce ordering constraints). 3. In the key name (maybePath or similar). I don't have a huge preference between them. -Peff ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: git silently ignores include directive with single quotes 2018-09-09 2:33 ` Jeff King @ 2018-09-11 20:36 ` Junio C Hamano 2018-09-11 20:57 ` Jeff King 0 siblings, 1 reply; 37+ messages in thread From: Junio C Hamano @ 2018-09-11 20:36 UTC (permalink / raw) To: Jeff King Cc: Ramsay Jones, Ævar Arnfjörð Bjarmason, Martin Ågren, stas, Git Mailing List Jeff King <peff@peff.net> writes: > I think that's syntactically invalid. At any rate, there are clearly > three options for setting a bit: > > 1. In the section header (+include, or Ævar's includeIf suggestion). > > 2. In another key (which looks pretty clean, but does introduce > ordering constraints). > > 3. In the key name (maybePath or similar). > > I don't have a huge preference between them. What's the longer term goal for the endgame? Is it acceptable that include.path will stay to be "optional include" for compatibility with users' existing configuration files, and include.requiredpath or similar gets introduced to allow people who want to get warned? Or do we want the usual multi-step deprecation dance where the first phase introduces include.maybepath and include.path starts warning against missing one, encouraging it to be rewritten to maybepath? I have mild preference against #2, as I suspect that the ordering constraints makes it harder to understand to end users. Between #1 and #3, there wouldn't be much difference, whether the endgame is "add a stricter variant that is opt in" or "migrate to a stricter default". ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: git silently ignores include directive with single quotes 2018-09-11 20:36 ` Junio C Hamano @ 2018-09-11 20:57 ` Jeff King 2018-09-23 22:48 ` Stas Bekman 0 siblings, 1 reply; 37+ messages in thread From: Jeff King @ 2018-09-11 20:57 UTC (permalink / raw) To: Junio C Hamano Cc: Ramsay Jones, Ævar Arnfjörð Bjarmason, Martin Ågren, stas, Git Mailing List On Tue, Sep 11, 2018 at 01:36:01PM -0700, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > > I think that's syntactically invalid. At any rate, there are clearly > > three options for setting a bit: > > > > 1. In the section header (+include, or Ævar's includeIf suggestion). > > > > 2. In another key (which looks pretty clean, but does introduce > > ordering constraints). > > > > 3. In the key name (maybePath or similar). > > > > I don't have a huge preference between them. > > What's the longer term goal for the endgame? Is it acceptable that > include.path will stay to be "optional include" for compatibility > with users' existing configuration files, and include.requiredpath > or similar gets introduced to allow people who want to get warned? > Or do we want the usual multi-step deprecation dance where the first > phase introduces include.maybepath and include.path starts warning > against missing one, encouraging it to be rewritten to maybepath? I don't see much point in introducing include.requiredPath. It might be useful for people who want to be extra-careful with their includes, but it would not really help users who simply made a spelling error and didn't know how to debug it. So switching the default for include.path and providing an escape hatch seems like the more useful path (if we indeed want to do one of these; adding better debugging like GIT_TRACE_CONFIG is yet another option). As far as deprecation, it depends on what the new behavior is. If it is simply that include.path will generate a warning on a missing file, I don't think there is any point in a multi-step dance. The endgame is a warning, which is no different than the deprecated-stage behavior. :) If the endgame is to die(), then I'd agree that there should be a warning in the middle. Between all those things I mentioned (or simply leaving it as-is), I really don't have a strong feeling. I hoped people who did would generate a patch to give something concrete to review. > I have mild preference against #2, as I suspect that the ordering > constraints makes it harder to understand to end users. Between #1 > and #3, there wouldn't be much difference, whether the endgame is > "add a stricter variant that is opt in" or "migrate to a stricter > default". The thing that #2 buys you is that multiple such bits could be combined. If we imagine that later there is another choice to make in interpreting include.path with two options, "foo" and "bar", then we would be stuck with: include.maybeFooPath include.maybeBarPath include.fooPath include.barPath and of course it only gets worse with a third one. Whereas with independent options, you can do: [include] warnOnMissing = false otherPreference = bar path = ... I dunno. The combinatorics might not be too bad if we document the required order, and actually code the parsing side like: if (!skip_prefix(key, "maybe", &key)) warn_on_missing = 0; if (!skip_prefix(key, "foo", &key)) other_pref = "foo"; ...and so on It's kind of hacky, but it does encode the bits into the name. As long as they remain bits, and not, say, arbitrary strings. -Peff ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: git silently ignores include directive with single quotes 2018-09-11 20:57 ` Jeff King @ 2018-09-23 22:48 ` Stas Bekman 2018-09-24 21:08 ` Ævar Arnfjörð Bjarmason ` (2 more replies) 0 siblings, 3 replies; 37+ messages in thread From: Stas Bekman @ 2018-09-23 22:48 UTC (permalink / raw) To: Git Mailing List Apologies for I don't know how this project manages issues, so I'm not sure whether it is my responsibility to make sure this issue gets resolved, or do you have some tracking mechanism where you have it registered? There is no rush, I'm asking because the discussion about this issue has suddenly dropped about 2 weeks ago, hence my ping. Thank you. -- ________________________________________________ Stas Bekman <'))))>< <'))))>< https://stasosphere.com https://chestofbooks.com https://experientialsexlab.com https://stason.org https://stasosphere.com/experience-life/my-books ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: git silently ignores include directive with single quotes 2018-09-23 22:48 ` Stas Bekman @ 2018-09-24 21:08 ` Ævar Arnfjörð Bjarmason 2018-09-24 23:20 ` Stas Bekman 2018-09-24 22:24 ` [PATCH 0/1] " Philip Oakley 2018-09-24 22:24 ` [PATCH 1/1] config doc: highlight the name=value syntax Philip Oakley 2 siblings, 1 reply; 37+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2018-09-24 21:08 UTC (permalink / raw) To: Stas Bekman; +Cc: Git Mailing List On Sun, Sep 23 2018, Stas Bekman wrote: > Apologies for I don't know how this project manages issues, so I'm not > sure whether it is my responsibility to make sure this issue gets > resolved, or do you have some tracking mechanism where you have it > registered? There is no rush, I'm asking because the discussion about > this issue has suddenly dropped about 2 weeks ago, hence my ping. Posting to this mailing list is generally how it's done, see https://github.com/git/git/blame/v2.19.0/README.md#L30-L37 Git's a project worked on by a bunch of people who're either doing it as a hobby, or are otherwise busy chasing stuff they're planning to work on. That doesn't mean the issue you reported doesn't matter, just that realistically we have thousands of issues big and small at any given time, and any new reported issue competes with those. There's always too much to do, and too little time to do it. Personally, I'm interested enough in this to muse about how it could be fixed / what sort of general issues it exposes, but not enough to tackle it myself, the general silence for a couple of weeks means a lot of people share that sentiment (or care even less). That doesn't mean this issue doesn't matter, or that it couldn't be addressed in some way. I just wanted to try to give you some fair & realistic summary of what's going on. The best way to fix stuff in git that you can't interest others in is to do it yourself. Take a look at Documentation/SubmittingPatches in the git.git repository for how to do that. In particular, starting by clarifying the docs around this as I suggested upthread might be a good and easy start to your first contribution to git! I hope that helps. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: git silently ignores include directive with single quotes 2018-09-24 21:08 ` Ævar Arnfjörð Bjarmason @ 2018-09-24 23:20 ` Stas Bekman 0 siblings, 0 replies; 37+ messages in thread From: Stas Bekman @ 2018-09-24 23:20 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason; +Cc: Git Mailing List On 2018-09-24 02:08 PM, Ævar Arnfjörð Bjarmason wrote: [...] > Posting to this mailing list is generally how it's done Thank you, Ævar, for clarifying that there is no issue tracker for the git project. > The best way to fix stuff in git that you can't interest others in is to > do it yourself. Take a look at Documentation/SubmittingPatches in the > git.git repository for how to do that. Based on the initial rich discussion this post created I had a feeling that there was a lot of interest. But you're correct, it's easier to share one's thought and patches take a lot more effort and time. > In particular, starting by clarifying the docs around this as I > suggested upthread might be a good and easy start to your first > contribution to git! That's an excellent idea. Philip started the process and hopefully it will lead to better documentation of the issue. Thanks again. -- ________________________________________________ Stas Bekman <'))))>< <'))))>< https://stasosphere.com https://chestofbooks.com https://experientialsexlab.com https://stason.org https://stasosphere.com/experience-life/my-books ^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH 0/1] Re: git silently ignores include directive with single quotes 2018-09-23 22:48 ` Stas Bekman 2018-09-24 21:08 ` Ævar Arnfjörð Bjarmason @ 2018-09-24 22:24 ` Philip Oakley 2018-09-24 23:05 ` Stas Bekman 2018-09-24 22:24 ` [PATCH 1/1] config doc: highlight the name=value syntax Philip Oakley 2 siblings, 1 reply; 37+ messages in thread From: Philip Oakley @ 2018-09-24 22:24 UTC (permalink / raw) To: Git Mailing List Cc: Philip Oakley, Ævar Arnfjörð Bjarmason, Junio C Hamano, Jeff King, Martin Ågren, stas Rather than attaching the problem with code, I decided to simply update the config file documentation. As the userbase expands the documentation will need to be more comprehensive about exclusions and omissions, along with better highlighting for core areas. I would be useful if Stas could comment on whether these changes would have assisted in debugging the faulty config file. Philip Oakley (1): config doc: highlight the name=value syntax Documentation/config.txt | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) -- 2.17.1.windows.2 ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 0/1] Re: git silently ignores include directive with single quotes 2018-09-24 22:24 ` [PATCH 0/1] " Philip Oakley @ 2018-09-24 23:05 ` Stas Bekman 0 siblings, 0 replies; 37+ messages in thread From: Stas Bekman @ 2018-09-24 23:05 UTC (permalink / raw) To: Philip Oakley, Git Mailing List Cc: Ævar Arnfjörð Bjarmason, Junio C Hamano, Jeff King, Martin Ågren On 2018-09-24 03:24 PM, Philip Oakley wrote: > Rather than attaching the problem with code, I decided to simply update > the config file documentation. > > As the userbase expands the documentation will need to be more comprehensive > about exclusions and omissions, along with better highlighting for core > areas. > > I would be useful if Stas could comment on whether these changes would > have assisted in debugging the faulty config file. Thank you for writing this doc patch, Philip. The documentation improvement would be most useful in conjunction with a an improved debugging/tracing facility. So that a user can see what git is seeing. Once a user sees that their configuration is broken then they can peruse the improved documentation to find why it is broken. Without the debugging ability, the docs would help but it'll be a much longer journey, since words like: "Single quotes are not special and form part of the variable's value." aren't necessarily going to stand out as an indicator of a potential problem, when you won't think twice that quotes could even be a suspect, even though the docs say so explicitly. A trace saying: "./.git/'.gitconfig'" is not found would speak volumes and be self-documenting. In lieu of that, the docs would be need to have more examples. Here are the potential expansions to the patch you shared: 1. "Single quotes are not special and form part of the variable's value. For example, if the configuration includes: include = '.gitconfig' then git will look for "'.gitconfig'", single quotes included. Also note that it'll look for the file relative to "REPO/.git/", hence it'll look for "REPO/.git/'.gitconfig'", which is most likely incorrect, since you can't check in files under "REPO/.git/". The correct configuration for including "REPO/.gitconfig" is: include = ../.gitconfig 2. Same with: "Both the `include` and `includeIf` sections implicitly apply an 'if found' condition to the given path names." To a user this would be a difficult statement to make sense of. An example would fix that: "Both the `include` and `includeIf` sections implicitly apply an 'if found' condition to the given path names. For example, if the configuration includes: include = ../.gitconfig and git finds "REPO/.gitconfig", it will include its configuration. If git can't find it, it will silently ignore this include statement until this file appears. It has been designed this way to allow for optional user-specific configuration facilities." Thank you. -- ________________________________________________ Stas Bekman <'))))>< <'))))>< https://stasosphere.com https://chestofbooks.com https://experientialsexlab.com https://stason.org https://stasosphere.com/experience-life/my-books ^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH 1/1] config doc: highlight the name=value syntax 2018-09-23 22:48 ` Stas Bekman 2018-09-24 21:08 ` Ævar Arnfjörð Bjarmason 2018-09-24 22:24 ` [PATCH 0/1] " Philip Oakley @ 2018-09-24 22:24 ` Philip Oakley 2018-09-25 22:03 ` Junio C Hamano 2 siblings, 1 reply; 37+ messages in thread From: Philip Oakley @ 2018-09-24 22:24 UTC (permalink / raw) To: Git Mailing List Cc: Philip Oakley, Ævar Arnfjörð Bjarmason, Junio C Hamano, Jeff King, Martin Ågren, stas Stas Bekman reported [1] that Git config was not accepting single quotes around a filename as may have been expected by shell users. Highlight the 'name = value' syntax with its own heading. Clarify that single quotes are not special here. Also point to this paragraph in the 'include' section regarding pathnames. In addition clarify that missing include file paths are not an error, but rather an implicit 'if found' for include files. [1] https://public-inbox.org/git/ca2b192e-1722-092e-2c54-d79d21a66ba2@stason.org/ Reported-by: Stas Bekman <stas@stason.org> Signed-off-by: Philip Oakley <philipoakley@iee.org> --- Documentation/config.txt | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/Documentation/config.txt b/Documentation/config.txt index 1264d91fa3..b65fd6138d 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -19,8 +19,8 @@ characters and `-`, and must start with an alphabetic character. Some variables may appear multiple times; we say then that the variable is multivalued. -Syntax -~~~~~~ +Config file Syntax +~~~~~~~~~~~~~~~~~~ The syntax is fairly flexible and permissive; whitespaces are mostly ignored. The '#' and ';' characters begin comments to the end of line, @@ -56,6 +56,9 @@ syntax, the subsection name is converted to lower-case and is also compared case sensitively. These subsection names follow the same restrictions as section names. +Variable name/value syntax +^^^^^^^^^^^^^^^^^^^^^^^^^^ + All the other lines (and the remainder of the line after the section header) are recognized as setting variables, in the form 'name = value' (or just 'name', which is a short-hand to say that @@ -69,7 +72,8 @@ stripped. Leading whitespaces after 'name =', the remainder of the line after the first comment character '#' or ';', and trailing whitespaces of the line are discarded unless they are enclosed in double quotes. Internal whitespaces within the value are retained -verbatim. +verbatim. Single quotes are not special and form part of the +variable's value. Inside double quotes, double quote `"` and backslash `\` characters must be escaped: use `\"` for `"` and `\\` for `\`. @@ -89,10 +93,14 @@ each other with the exception that `includeIf` sections may be ignored if their condition does not evaluate to true; see "Conditional includes" below. +Both the `include` and `includeIf` sections implicitly apply an 'if found' +condition to the given path names. + You can include a config file from another by setting the special `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. +subject to tilde expansion and the value syntax detailed above. +These variables can be given multiple times. 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 -- 2.17.1.windows.2 ^ permalink raw reply related [flat|nested] 37+ messages in thread
* Re: [PATCH 1/1] config doc: highlight the name=value syntax 2018-09-24 22:24 ` [PATCH 1/1] config doc: highlight the name=value syntax Philip Oakley @ 2018-09-25 22:03 ` Junio C Hamano 0 siblings, 0 replies; 37+ messages in thread From: Junio C Hamano @ 2018-09-25 22:03 UTC (permalink / raw) To: Philip Oakley Cc: Git Mailing List, Ævar Arnfjörð Bjarmason, Jeff King, Martin Ågren, stas Philip Oakley <philipoakley@iee.org> writes: > +Variable name/value syntax > +^^^^^^^^^^^^^^^^^^^^^^^^^^ > + > All the other lines (and the remainder of the line after the section > header) are recognized as setting variables, in the form > 'name = value' (or just 'name', which is a short-hand to say that > @@ -69,7 +72,8 @@ stripped. Leading whitespaces after 'name =', the remainder of the > line after the first comment character '#' or ';', and trailing > whitespaces of the line are discarded unless they are enclosed in > double quotes. Internal whitespaces within the value are retained > -verbatim. > +verbatim. Single quotes are not special and form part of the > +variable's value. > > Inside double quotes, double quote `"` and backslash `\` characters > must be escaped: use `\"` for `"` and `\\` for `\`. Hmph. This feels a bit backwards. The original paragraph is horrible in that there is no clear mention that a pair of dq can be used to quote (which primarily is useful if your value have leading or trailing whitespaces); the closest hint is "enclosed in double quotes" we see in the pre-context. The added sentence singles out sq but it is unclear why it is necessary to call out that it is not special---the readers can legitimately wonder if backquotes are special or not and why. I wonder if this is easier to understand: diff --git a/Documentation/config.txt b/Documentation/config.txt index ad0f4510c3..5eebd539df 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -61,12 +61,16 @@ the variable is the boolean "true"). The variable names are case-insensitive, allow only alphanumeric characters and `-`, and must start with an alphabetic character. +The value part can have segments that are enclosed in a pair of +double quotes (note: other kinds of quoting character pairs are not +special)--the double quotes are stripped from the value. + A line that defines a value can be continued to the next line by ending it with a `\`; the backquote and the end-of-line are stripped. Leading whitespaces after 'name =', the remainder of the line after the first comment character '#' or ';', and trailing -whitespaces of the line are discarded unless they are enclosed in -double quotes. Internal whitespaces within the value are retained +whitespaces of the line are discarded. +Internal whitespaces within the value are retained verbatim. Inside double quotes, double quote `"` and backslash `\` characters > @@ -89,10 +93,14 @@ each other with the exception that `includeIf` sections may be ignored > if their condition does not evaluate to true; see "Conditional includes" > below. > > +Both the `include` and `includeIf` sections implicitly apply an 'if found' > +condition to the given path names. > + Mentioning that missing target file is not an error is definitely an improvement. I've never viewed it as applying "if found" condition myself, but it is not wrong per-se to do so, I would think. > You can include a config file from another by setting the special > `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. > +subject to tilde expansion and the value syntax detailed above. > +These variables can be given multiple times. I have a mild suspicion that this adds negative value. Singling out that "[include] path = ..." follows the usual value syntax makes the readers wonder if there are some "[section] variable = ..." that does not follow the value syntax that they have to be aware of and careful about. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: git silently ignores include directive with single quotes 2018-09-08 21:14 ` Jeff King 2018-09-08 22:10 ` Ramsay Jones @ 2018-09-08 22:32 ` Ævar Arnfjörð Bjarmason 2018-09-09 2:29 ` Jeff King 1 sibling, 1 reply; 37+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2018-09-08 22:32 UTC (permalink / raw) To: Jeff King; +Cc: Martin Ågren, stas, Git Mailing List On Sat, Sep 08 2018, Jeff King wrote: > On Sat, Sep 08, 2018 at 09:54:14PM +0200, Ævar Arnfjörð Bjarmason wrote: > >> The reason missing includes are ignored is that the way this is expected >> to be used is e.g.: >> >> [include] >> path ~/.gitconfig.work >> >> Where .gitconfig.work is some configuration you're going to drop into >> place on your $dayjob servers, but not on your personal machine, even >> though you sync the same ~/.gitconfig everywhere. >> >> A lot of people who use includes rely on this, but I see from this >> thread this should be better documented. > > Right, this was an intentional choice at the time the feature was added, > to support this kind of feature. I'd note also that it mirrors other > misspelled keys. E.g.: > > [include] > psth = whatever > > will also not generate an error. This is also intentional, for two > reasons: > > 1. Git's config format has always been designed to carry extra keys > used by third-party scripts and porcelain. So we don't actually > know the complete set of valid keys. (Though you could make an > argument that git-core could stake out include.* as its own). > > 2. It makes using multiple git versions easier in some ways (though > also harder in others). A config key that isn't known to the > current version will be quietly ignored. > > Of course those things mean that true spelling mistakes are harder to > catch as such, because Git doesn't know that's what they are. And here > I'm talking config _keys_, not values. So I'm just explaining the > philosophical thinking that led to the "missing file is a silent noop". > It doesn't _have_ to behave the same. > > That said, it _does_ behave the same and people are likely depending on > it at this point. So if we introduce a warning, for example, there needs > to be some way to suppress it. > > Probably: > > [include] > warnOnMissing = false > path = ... > > would be enough (with the default being "true"). > > You could even do: > > [include] > warnOnMissing = false > path = one > warnOnMissing = true > path = two > > to treat two includes differently (though I'm not sure why you would > want to). I think this is introducing a brand new caveat into our *.ini syntax, i.e. that we're sensitive to the order in which we're parsing *different* keys. I.e. we already had the concept that some keys override existing sets (e.g. user.name), but not that a x.y=foo controls the behavior of a subsequent a.b=bar, or the other way around. This also makes programmatic (via "git config") editing of the config hard, we'd need to introduce something like: git config -f ~/.gitconfig a.b bar git config -f ~/.gitconfig --before a.b x.y foo To set a.b=bar before x.y=foo, or --after or whatever. >> If we were to make nonexisting files an error, we'd need something like >> an extension of the includeIf syntax added in 3efd0bedc6 ("config: add >> conditional include", 2017-03-01) 3efd0bedc6 ("config: add conditional >> include", 2017-03-01). I.e.: >> >> [includeIfcond "test -e ~/.gitconfig.work"] >> path = ~/.gitconfig.work >> >> Or something like that, this is getting increasingly harder to shove >> into the *.ini config syntax. > > I think it would be simpler to just introduce a new key that's a variant > of "path". Like: > > [include] > maybePath = ~/.gitconfig.work > > Though if it really is just a warning, the "warnOnMissing" above would > make that unnecessary (and it also scales better if we have to end up > adding more behavior tweaks in the future). Yeah, we could do that, and it wouldn't break the model described above, We can make that work, but this would be nasty. E.g. are we going to treat EACCES and ENOENT the same way in this construct? ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: git silently ignores include directive with single quotes 2018-09-08 22:32 ` git silently ignores include directive with single quotes Ævar Arnfjörð Bjarmason @ 2018-09-09 2:29 ` Jeff King 0 siblings, 0 replies; 37+ messages in thread From: Jeff King @ 2018-09-09 2:29 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: Martin Ågren, stas, Git Mailing List On Sun, Sep 09, 2018 at 12:32:57AM +0200, Ævar Arnfjörð Bjarmason wrote: > > You could even do: > > > > [include] > > warnOnMissing = false > > path = one > > warnOnMissing = true > > path = two > > > > to treat two includes differently (though I'm not sure why you would > > want to). > > I think this is introducing a brand new caveat into our *.ini syntax, > i.e. that we're sensitive to the order in which we're parsing > *different* keys. > > I.e. we already had the concept that some keys override existing sets > (e.g. user.name), but not that a x.y=foo controls the behavior of a > subsequent a.b=bar, or the other way around. This already exists. For example: echo '[foo]bar = inc' >config.inc echo '[foo]bar = main' >config.main echo '[include]path = config.inc' >>config.main git config -f config.main --includes foo.bar Arriving at that answer requires expanding the include's contents at the exact same spot in the file. As far as I know this is the only case, though, so include.path really is special. And this would be expanding that specialness to other things in include.*. But I'm not sure that is really that big a deal. That warnOnMissing isn't meant to be just a normal key. It's an order-sensitive directive for further parsing, just like include.path is. Either the parser understands includes (and has to handle these) or it doesn't. So I'm not worried about any burden on the parsing side, but... > This also makes programmatic (via "git config") editing of the config > hard, we'd need to introduce something like: > > git config -f ~/.gitconfig a.b bar > git config -f ~/.gitconfig --before a.b x.y foo > > To set a.b=bar before x.y=foo, or --after or whatever. Yes, I don't think "git config include.warnOnMissing true" would be very useful, because it would generally be added after any includes you have, and therefore not affect them. I think this is generally an issue with include.path, too. My assumption has been that anybody at the level of using includes is probably going to be hand-editing anyway. But if include.* is special on the parsing side, I don't know that it is that bad to make it special on the writing side, too. I.e., to recognize "git config include.warnOnMissing true" and always add it at the head of any existing include block. It certainly _feels_ hacky, but I think it would behave sensibly and predictably. And it would just work, as opposed to requiring something like "--before", which would be quite a subtle gotcha for somebody to forget to use. > Yeah, we could do that, and it wouldn't break the model described above, > We can make that work, but this would be nasty. E.g. are we going to > treat EACCES and ENOENT the same way in this construct? I don't have a strong opinion (after all, I already wrote the behavior I thought was reasonable long ago ;) ). So I think it would be up to somebody to propose. We do already report and die on EACCES (and basically any other error except ENOENT). So if we did treat them both as a warning, that would be a weakening for EACCES. -Peff ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: git silently ignores include directive with single quotes 2018-09-08 18:58 git silently ignores include directive with single quotes Stas Bekman 2018-09-08 19:30 ` Martin Ågren @ 2018-09-08 21:22 ` Jeff King 2018-09-08 22:49 ` Stas Bekman 2018-09-10 17:04 ` Junio C Hamano 1 sibling, 2 replies; 37+ messages in thread From: Jeff King @ 2018-09-08 21:22 UTC (permalink / raw) To: Stas Bekman; +Cc: git On Sat, Sep 08, 2018 at 11:58:47AM -0700, Stas Bekman wrote: > This doesn’t: > > [include] > path = '../.gitconfig' So I think it's been covered elsewhere that single quotes aren't a thing in git's config format. I will say that this was actually a minor surprise to me, after a decade of working with the format. ;) I don't know if it's worth changing now or not It would be backwards-incompatible, but I wonder if we could do it in a sane way. E.g., with a rule like: - if the first non-whitespace character of the value is a single-quote, assume the value is quoted and apply normal shell rules (i.e., no backslash escapes until the ending single-quote) - otherwise, single-quotes are not special at all That would allow things like: [diff "foo"] textconv = some_shell_hackery 'with quotes' | foo to continue working, but make: [some] path = 'this has "double quotes" in it!' do what the user probably intended. It would be a regression for anybody who literally has a value that starts with a single-quote, but that seems like it would be pretty rare. Or I dunno, maybe people do it on Windows to try to protect path-names that get interpreted by the shell. > The original problem cropped up due to using: > > git config --local include.path '../.gitconfig' > > which on linux stripped the single quotes, but on some windows git bash > emulation it kept them. That sounds like a bug in git bash, if it is not treating single quotes in the usual shell way. But I'd also expect such a bug to cause loads of problems in all of the shell scripts. Are you sure it wasn't cmd.exe or some other interpreter? -Peff ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: git silently ignores include directive with single quotes 2018-09-08 21:22 ` Jeff King @ 2018-09-08 22:49 ` Stas Bekman 2018-09-09 2:30 ` Jeff King 2018-09-10 17:04 ` Junio C Hamano 1 sibling, 1 reply; 37+ messages in thread From: Stas Bekman @ 2018-09-08 22:49 UTC (permalink / raw) To: Jeff King; +Cc: git On 2018-09-08 02:22 PM, Jeff King wrote: [...] >> The original problem cropped up due to using: >> >> git config --local include.path '../.gitconfig' >> >> which on linux stripped the single quotes, but on some windows git bash >> emulation it kept them. > > That sounds like a bug in git bash, if it is not treating single quotes > in the usual shell way. But I'd also expect such a bug to cause loads of > problems in all of the shell scripts. Are you sure it wasn't cmd.exe or > some other interpreter? I don't know, Jeff. I think the user said it was first anaconda shell. And then the user tried gitforwindows with same results. I don't know MSwindows at all. But it doesn't matter at the end of the day, since we can't cover all possible unix shell emulations out there. What matters is that there is a way to flag the misconfiguration, either by default, or through a special check - some ideas I suggested in my previous email, but surely you have a much better insight of how to deal with that. Thank you. -- ________________________________________________ Stas Bekman <'))))>< <'))))>< https://stasosphere.com https://chestofbooks.com https://experientialsexlab.com https://stason.org https://stasosphere.com/experience-life/my-books ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: git silently ignores include directive with single quotes 2018-09-08 22:49 ` Stas Bekman @ 2018-09-09 2:30 ` Jeff King 0 siblings, 0 replies; 37+ messages in thread From: Jeff King @ 2018-09-09 2:30 UTC (permalink / raw) To: Stas Bekman; +Cc: git On Sat, Sep 08, 2018 at 03:49:01PM -0700, Stas Bekman wrote: > On 2018-09-08 02:22 PM, Jeff King wrote: > [...] > >> The original problem cropped up due to using: > >> > >> git config --local include.path '../.gitconfig' > >> > >> which on linux stripped the single quotes, but on some windows git bash > >> emulation it kept them. > > > > That sounds like a bug in git bash, if it is not treating single quotes > > in the usual shell way. But I'd also expect such a bug to cause loads of > > problems in all of the shell scripts. Are you sure it wasn't cmd.exe or > > some other interpreter? > > I don't know, Jeff. I think the user said it was first anaconda shell. > And then the user tried gitforwindows with same results. I don't know > MSwindows at all. > > But it doesn't matter at the end of the day, since we can't cover all > possible unix shell emulations out there. What matters is that there is > a way to flag the misconfiguration, either by default, or through a > special check - some ideas I suggested in my previous email, but surely > you have a much better insight of how to deal with that. Yes, I agree this is pretty orthogonal to the config discussion. I was just wondering if there was a separate bug to look into. I'm willing to shrug and say "it was probably user error" until we see more definite details. Thanks. -Peff ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: git silently ignores include directive with single quotes 2018-09-08 21:22 ` Jeff King 2018-09-08 22:49 ` Stas Bekman @ 2018-09-10 17:04 ` Junio C Hamano 2018-09-10 17:14 ` Jonathan Nieder 1 sibling, 1 reply; 37+ messages in thread From: Junio C Hamano @ 2018-09-10 17:04 UTC (permalink / raw) To: Jeff King; +Cc: Stas Bekman, git Jeff King <peff@peff.net> writes: > On Sat, Sep 08, 2018 at 11:58:47AM -0700, Stas Bekman wrote: > >> This doesn’t: >> >> [include] >> path = '../.gitconfig' > > So I think it's been covered elsewhere that single quotes aren't a thing > in git's config format. I will say that this was actually a minor > surprise to me, after a decade of working with the format. ;) > > I don't know if it's worth changing now or not It would be > backwards-incompatible, but I wonder if we could do it in a sane way. > E.g., with a rule like: > > - if the first non-whitespace character of the value is a > single-quote, assume the value is quoted and apply normal shell > rules (i.e., no backslash escapes until the ending single-quote) > > - otherwise, single-quotes are not special at all At least the rule would not force those with ' in the middle of their family names to surround the user.name with extra double quotes, and it would probably be a good and safe practical solution. Being safe "by magic" tend to become hard to explain, but in this case the magic part is probably still simple enough. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: git silently ignores include directive with single quotes 2018-09-10 17:04 ` Junio C Hamano @ 2018-09-10 17:14 ` Jonathan Nieder 2018-09-10 18:30 ` Junio C Hamano 0 siblings, 1 reply; 37+ messages in thread From: Jonathan Nieder @ 2018-09-10 17:14 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jeff King, Stas Bekman, git Hi, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: >> So I think it's been covered elsewhere that single quotes aren't a thing >> in git's config format. I will say that this was actually a minor >> surprise to me, after a decade of working with the format. ;) >> >> I don't know if it's worth changing now or not It would be >> backwards-incompatible, but I wonder if we could do it in a sane way. >> E.g., with a rule like: >> >> - if the first non-whitespace character of the value is a >> single-quote, assume the value is quoted and apply normal shell >> rules (i.e., no backslash escapes until the ending single-quote) >> >> - otherwise, single-quotes are not special at all > > At least the rule would not force those with ' in the middle of > their family names to surround the user.name with extra double > quotes, and it would probably be a good and safe practical solution. > Being safe "by magic" tend to become hard to explain, but in this > case the magic part is probably still simple enough. Given that today, git config foo.bar "'baz'" produces [foo] bar = 'baz' I don't think this would be safe to do. Since the underlying problem is that the latter syntax is confusing, I wonder if we can do the following: 1. Treat single-quote as worth quoting in config.c::write_pair (line 2516). This would already help with the original issue, since the config would say [foo] bar = \'baz\' allowing a quick diagnosis. 2. (optional) Warn if a value is surrounded in single-quotes, encouraging using backslash to disambiguate. 3. (optional) Error out if a value is surrounded in single-quotes, encouraging using double-quote or backslash, depending on the user's intention. 4. (optional) Start treating wrapping single-quotes specially somehow. I think step 1 is a good idea, but I'm not convinced about any of the later steps. I also agree with the comments upthread about wanting a way to do a '[include] path' that errors out if the file doesn't exist, and maybe even starting a transition to repurpose standard [include] path to do that. Thanks, Jonathan ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: git silently ignores include directive with single quotes 2018-09-10 17:14 ` Jonathan Nieder @ 2018-09-10 18:30 ` Junio C Hamano 2018-09-10 18:35 ` Jonathan Nieder 0 siblings, 1 reply; 37+ messages in thread From: Junio C Hamano @ 2018-09-10 18:30 UTC (permalink / raw) To: Jonathan Nieder; +Cc: Jeff King, Stas Bekman, git Jonathan Nieder <jrnieder@gmail.com> writes: > 1. Treat single-quote as worth quoting in config.c::write_pair (line > 2516). This would already help with the original issue, since the > config would say > > [foo] > bar = \'baz\' > > allowing a quick diagnosis. I am mildly against this, as long as you feel that all the remaining steps need to be marked with "(optional)", because this will give readers an impression that somehow single-quote is special. If we do not intend to make it special at all, we shouldn't. If we do commit to make it special, then this is a very sensible first step that is backward compatible, of course, and I find that the following steps form a reasonable transition plan, if we intend to follow all the way through. > 2. (optional) Warn if a value is surrounded in single-quotes, > encouraging using backslash to disambiguate. > > 3. (optional) Error out if a value is surrounded in single-quotes, > encouraging using double-quote or backslash, depending on the > user's intention. > > 4. (optional) Start treating wrapping single-quotes specially > somehow. > > I think step 1 is a good idea, but I'm not convinced about any of the > later steps. > > I also agree with the comments upthread about wanting a way to do a > '[include] path' that errors out if the file doesn't exist, and maybe > even starting a transition to repurpose standard [include] path to do > that. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: git silently ignores include directive with single quotes 2018-09-10 18:30 ` Junio C Hamano @ 2018-09-10 18:35 ` Jonathan Nieder 2018-09-10 19:17 ` Junio C Hamano 0 siblings, 1 reply; 37+ messages in thread From: Jonathan Nieder @ 2018-09-10 18:35 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jeff King, Stas Bekman, git Junio C Hamano wrote: > Jonathan Nieder <jrnieder@gmail.com> writes: >> 1. Treat single-quote as worth quoting in config.c::write_pair (line >> 2516). This would already help with the original issue, since the >> config would say >> >> [foo] >> bar = \'baz\' >> >> allowing a quick diagnosis. > > I am mildly against this, as long as you feel that all the remaining > steps need to be marked with "(optional)", because this will give > readers an impression that somehow single-quote is special. If we > do not intend to make it special at all, we shouldn't. That's fair, especially because it would be inconsistent with shell command language, where single-quote inside double quotes is not special: $ printf '%s\n' "\'" \' (I realize that backslash means something different in Git config; I'm just saying it would be another source of cognitive dissonance.) Updated proposal: 1. Treat strings starting or ending with single-quote as worth quoting in config.c::write_pair (line 3269). This would already help with the original issue, since the config would say [foo] bar = "'baz'" allowing a quick diagnosis. 2. (optional) Warn if a value is surrounded in single-quotes, encouraging using surrounding double-quotes to disambiguate. 3. (optional) Error out if a value is surrounded in single-quotes, encouraging replacing with or surrounding with double-quote, depending on the user's intention. 4. (optional) Start treating wrapping single-quotes specially somehow. As before, I think step 1 is a good idea, but I'm not convinced about any of the later steps. Thanks, Jonathan ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: git silently ignores include directive with single quotes 2018-09-10 18:35 ` Jonathan Nieder @ 2018-09-10 19:17 ` Junio C Hamano 2018-09-10 19:52 ` Stas Bekman 0 siblings, 1 reply; 37+ messages in thread From: Junio C Hamano @ 2018-09-10 19:17 UTC (permalink / raw) To: Jonathan Nieder; +Cc: Jeff King, Stas Bekman, git Jonathan Nieder <jrnieder@gmail.com> writes: > Updated proposal: > > 1. Treat strings starting or ending with single-quote as worth > quoting in config.c::write_pair (line 3269). This would already > help with the original issue, since the config would say > > [foo] > bar = "'baz'" > > allowing a quick diagnosis. This does not change anything essential from Git's point of view since the previous round. But I changed my mind anyway ;-) Earlier I said "if we do not intend to make sq special, we shouldn't do #1", and it still is a good direction to go. But making sq special does not have to be making sq a character that quotes. A character (or a character sequence) that is *not* quoting can still be special---for example, we can say certain character or a character sequence *must* be quoted, and make sq such a character. That is, even if we stop at your step 3. or step 2., and did not go to step 4. (which I think is a bad idea for little gain), we are already treating sq as a special character, and step 1. above is a reasonable way to start the transition to that better world. The reason why it is a better world that have "must be quoted, even though they are not quoting characters themselves" is solely because that would avoid confusion for those who are not familiar with the file format, even when we stop at step #2. In addition to a single quote a the beginning of the value, I think two or more SP deserve to be such a "must be quoted" sequence, i.e. instead of producing this result, which we see with today's Git: $ git config a.test0 "'foo" $ git config a.test1 "foo bar" ;# two spaces $ grep -A2 '\[a\]' config [a] test0 = 'foo test1 = foo bar we'd produce $ grep -A2 '\[a\]' config [a] test0 = "'foo" test1 = "foo bar" but we can still interpret what we have historically written the same way. I do not know if step #3 is a good idea, and I do not think step #2 is particularly a good stopping point. Step #1 is probably slightly a better stopping point if the aim is to avoid user confusion than step #2. > 2. (optional) Warn if a value is surrounded in single-quotes, > encouraging using surrounding double-quotes to disambiguate. > > 3. (optional) Error out if a value is surrounded in single-quotes, > encouraging replacing with or surrounding with double-quote, > depending on the user's intention. > > 4. (optional) Start treating wrapping single-quotes specially > somehow. > > As before, I think step 1 is a good idea, but I'm not convinced about > any of the later steps. > > Thanks, > Jonathan ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: git silently ignores include directive with single quotes 2018-09-10 19:17 ` Junio C Hamano @ 2018-09-10 19:52 ` Stas Bekman 2018-09-10 20:10 ` Junio C Hamano 0 siblings, 1 reply; 37+ messages in thread From: Stas Bekman @ 2018-09-10 19:52 UTC (permalink / raw) To: Junio C Hamano, Jonathan Nieder; +Cc: Jeff King, git To add another report of a similar problem, of silent skipping and not of filepath quoting, I found this one: https://stackoverflow.com/questions/31203634/git-clean-filter-python-script-on-windows/52264440#52264440 The user created .gitconfig and added to .git/config: [include] path = .gitconfig Not realizing that the two were not in the same folder. And probably assuming that .git/config was referring to the root of repository, and not relative to .git/, which is a reasonable assumption. Of course he had no way of resolving this as git wasn't telling him where it wasn't finding the file. i.e. Can't find: ~/myrepo/.git/.gitconfig which would have instantly told him where the problem was. -- ________________________________________________ Stas Bekman <'))))>< <'))))>< https://stasosphere.com https://chestofbooks.com https://experientialsexlab.com https://stason.org https://stasosphere.com/experience-life/my-books ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: git silently ignores include directive with single quotes 2018-09-10 19:52 ` Stas Bekman @ 2018-09-10 20:10 ` Junio C Hamano 0 siblings, 0 replies; 37+ messages in thread From: Junio C Hamano @ 2018-09-10 20:10 UTC (permalink / raw) To: Stas Bekman; +Cc: Jonathan Nieder, Jeff King, git Stas Bekman <stas@stason.org> writes: > [include] > path = .gitconfig > > Not realizing that the two were not in the same folder. And probably > assuming that .git/config was referring to the root of repository, and > not relative to .git/, which is a reasonable assumption. > > Of course he had no way of resolving this as git wasn't telling him > where it wasn't finding the file. i.e. > > Can't find: ~/myrepo/.git/.gitconfig > > which would have instantly told him where the problem was. Yeah, I think Peff's idea of introducing a variant of include.path that reports missing file would make sense for such a use case. The code needs to turn a relative path to an absolute one by taking the value as path relative to the including configuration file, so we should already have the path to use in the error reporting, if we were to go that route. ^ permalink raw reply [flat|nested] 37+ messages in thread
end of thread, other threads:[~2018-09-25 22:03 UTC | newest] Thread overview: 37+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-09-08 18:58 git silently ignores include directive with single quotes Stas Bekman 2018-09-08 19:30 ` Martin Ågren 2018-09-08 19:44 ` Stas Bekman 2018-09-08 19:53 ` Stas Bekman 2018-09-08 20:02 ` Ævar Arnfjörð Bjarmason 2018-09-08 20:13 ` Stas Bekman 2018-09-08 20:28 ` Ævar Arnfjörð Bjarmason 2018-09-08 20:58 ` Stas Bekman 2018-09-09 2:51 ` Paul Smith 2018-09-09 2:57 ` Stas Bekman 2018-09-08 19:54 ` Ævar Arnfjörð Bjarmason 2018-09-08 20:04 ` Stas Bekman 2018-09-08 20:16 ` Ævar Arnfjörð Bjarmason 2018-09-08 21:14 ` Jeff King 2018-09-08 22:10 ` Ramsay Jones 2018-09-09 2:33 ` Jeff King 2018-09-11 20:36 ` Junio C Hamano 2018-09-11 20:57 ` Jeff King 2018-09-23 22:48 ` Stas Bekman 2018-09-24 21:08 ` Ævar Arnfjörð Bjarmason 2018-09-24 23:20 ` Stas Bekman 2018-09-24 22:24 ` [PATCH 0/1] " Philip Oakley 2018-09-24 23:05 ` Stas Bekman 2018-09-24 22:24 ` [PATCH 1/1] config doc: highlight the name=value syntax Philip Oakley 2018-09-25 22:03 ` Junio C Hamano 2018-09-08 22:32 ` git silently ignores include directive with single quotes Ævar Arnfjörð Bjarmason 2018-09-09 2:29 ` Jeff King 2018-09-08 21:22 ` Jeff King 2018-09-08 22:49 ` Stas Bekman 2018-09-09 2:30 ` Jeff King 2018-09-10 17:04 ` Junio C Hamano 2018-09-10 17:14 ` Jonathan Nieder 2018-09-10 18:30 ` Junio C Hamano 2018-09-10 18:35 ` Jonathan Nieder 2018-09-10 19:17 ` Junio C Hamano 2018-09-10 19:52 ` Stas Bekman 2018-09-10 20:10 ` Junio C Hamano
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).