git silently ignores include directive with single quotes

git silently ignores include directive with single quotes

[Stas Bekman]

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

Re: git silently ignores include directive with single quotes

[Martin Ågren]

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

Re: git silently ignores include directive with single quotes

[Stas Bekman]

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

Re: git silently ignores include directive with single quotes

[Stas Bekman]

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

Re: git silently ignores include directive with single quotes

[Ævar Arnfjörð Bjarmason]

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.

Re: git silently ignores include directive with single quotes

[Ævar Arnfjörð Bjarmason]

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.

Re: git silently ignores include directive with single quotes

[Stas Bekman]

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

Re: git silently ignores include directive with single quotes

[Stas Bekman]

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

Re: git silently ignores include directive with single quotes

[Ævar Arnfjörð Bjarmason]

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.

Re: git silently ignores include directive with single quotes

[Ævar Arnfjörð Bjarmason]

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?

Re: git silently ignores include directive with single quotes

[Stas Bekman]

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

Re: git silently ignores include directive with single quotes

[Jeff King]

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

Re: git silently ignores include directive with single quotes

[Jeff King]

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

Re: git silently ignores include directive with single quotes

[Ramsay Jones]

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

Re: git silently ignores include directive with single quotes

[Ævar Arnfjörð Bjarmason]

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?

Re: git silently ignores include directive with single quotes

[Stas Bekman]

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

Re: git silently ignores include directive with single quotes

[Jeff King]

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

Re: git silently ignores include directive with single quotes

[Jeff King]

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

Re: git silently ignores include directive with single quotes

[Jeff King]

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

Re: git silently ignores include directive with single quotes

[Paul Smith]

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.

Re: git silently ignores include directive with single quotes

[Stas Bekman]

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

Re: git silently ignores include directive with single quotes

[Junio C Hamano]

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.

Re: git silently ignores include directive with single quotes

[Jonathan Nieder]

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

Re: git silently ignores include directive with single quotes

[Junio C Hamano]

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.

Re: git silently ignores include directive with single quotes

[Jonathan Nieder]

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

Re: git silently ignores include directive with single quotes

[Junio C Hamano]

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

Re: git silently ignores include directive with single quotes

[Stas Bekman]

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

Re: git silently ignores include directive with single quotes

[Junio C Hamano]

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.

Re: git silently ignores include directive with single quotes

[Junio C Hamano]

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".

Re: git silently ignores include directive with single quotes

[Jeff King]

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

Re: git silently ignores include directive with single quotes

[Stas Bekman]

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

Re: git silently ignores include directive with single quotes

[Ævar Arnfjörð Bjarmason]

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.

[PATCH 0/1] Re: git silently ignores include directive with single quotes

[Philip Oakley]

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

[PATCH 1/1] config doc: highlight the name=value syntax

[Philip Oakley]

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

Re: [PATCH 0/1] Re: git silently ignores include directive with single quotes

[Stas Bekman]

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

Re: git silently ignores include directive with single quotes

[Stas Bekman]

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

Re: [PATCH 1/1] config doc: highlight the name=value syntax

[Junio C Hamano]

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.