When we execute git config --list and $GIT_CONFIG value starts with home
prefix - ~/ it produces folowing error - fatal: unable to read config
file '~/.gitconfig': No such file or directory. This patch fixed it with
expand_user_path for configuration file path before git-config --list
call.
Signed-off-by: Alex Kuleshov <redacted>
Signed-off-by: 0xAX <redacted>
---
builtin/config.c | 2 ++
1 file changed, 2 insertions(+)
From: Eric Sunshine <hidden> Date: 2016-06-15 23:02:56
On Fri, Nov 14, 2014 at 1:29 PM, 0xAX [off-list ref] wrote:
When we execute git config --list and $GIT_CONFIG value starts with home
prefix - ~/ it produces folowing error - fatal: unable to read config
file '~/.gitconfig': No such file or directory. This patch fixed it with
expand_user_path for configuration file path before git-config --list
call.
Is this special case really warranted? Elsewhere, GIT_CONFIG does not
get this sort of special treatment. Moreover, it appears that no other
GIT_* environment variable is subject to such special treatment. (And,
generally speaking, on Unix, it is generally assumed that a path
assigned to an environment variable is to be used as-is.)
A few issues:
(1) Style: s/char* /char */
(2) Avoid declaration (of 'newpath') after statement.
(3) You can drop 'newpath' altogether and just assign the result of
expand_user_path() directly to given_config_source.file.
This code is potentially leaking the old value of
given_config_source.file, and (later) the new value, however, as
given_config_source.file is already being leaked elsewhere, it
probably does not make the situation much worse.
if (git_config_with_options(show_all_config, NULL,
&given_config_source,
respect_includes) < 0) {
--
2.1.3.17.g7fa1365.dirty
From: Jeff King <hidden> Date: 2016-06-15 23:02:56
On Fri, Nov 14, 2014 at 02:19:41PM -0500, Eric Sunshine wrote:
On Fri, Nov 14, 2014 at 1:29 PM, 0xAX [off-list ref] wrote:
quoted
When we execute git config --list and $GIT_CONFIG value starts with home
prefix - ~/ it produces folowing error - fatal: unable to read config
file '~/.gitconfig': No such file or directory. This patch fixed it with
expand_user_path for configuration file path before git-config --list
call.
Is this special case really warranted? Elsewhere, GIT_CONFIG does not
get this sort of special treatment. Moreover, it appears that no other
GIT_* environment variable is subject to such special treatment. (And,
generally speaking, on Unix, it is generally assumed that a path
assigned to an environment variable is to be used as-is.)
Yeah, I'd agree it is a little unexpected to expand here. The "~" is
mostly a shell thing, and doing:
GIT_CONFIG=~/.gitconfig git config --list
from the shell generally works, because the shell will expand the "~"
before it even hits git. If you're not using a shell to set the
variable, you probably should be pre-expanding it yourself.
Note that this code path affects "git config --file=~/.gitconfig", too.
At least there it would be a little bit useful because the shell will
not expand for you, but it still feels a bit unconventional to me.
If we _were_ going to do such an expansion, this is absolutely the wrong
place for it. It works only for the "--list" action; if we are going to
expand it, we would want to do so everywhere. And we do not even know if
given_config_source.file is non-NULL here (we could be reading from
stdin, or a blob). Fortunately expand_user_path will pass through a NULL
without segfaulting.
Probably the right place would be the if/else chain around
builtin/config.c:514, where we convert a relative path into an absolute
one. But I'm not convinced it's a good thing to be doing in the first
place.
-Peff
From: Alex Kuleshov <hidden> Date: 2016-06-15 23:02:56
Hello Eric and Jeff,
Eric Sunshine
A few issues:
(1) Style: s/char* /char */
(2) Avoid declaration (of 'newpath') after statement.
(3) You can drop 'newpath' altogether and just assign the result of
expand_user_path() directly to given_config_source.file.
This code is potentially leaking the old value of
given_config_source.file, and (later) the new value, however, as
given_config_source.file is already being leaked elsewhere, it
probably does not make the situation much worse.
It is my first patch to git's code base, so many thanks for your
feedback, i'll fix these issues if there will be need in this patch.
Jeff King [off-list ref]
Yeah, I'd agree it is a little unexpected to expand here. The "~" is
mostly a shell thing, and doing:
GIT_CONFIG=~/.gitconfig git config --list
from the shell generally works, because the shell will expand the "~"
before it even hits git. If you're not using a shell to set the
variable, you probably should be pre-expanding it yourself.
Yes, you're right here, but i put GIT_CONFIG=~/.gitconfig to my .bashrc
and it doesn't work so.
Note that this code path affects "git config --file=~/.gitconfig", too.
At least there it would be a little bit useful because the shell will
not expand for you, but it still feels a bit unconventional to me.
If we _were_ going to do such an expansion, this is absolutely the wrong
place for it. It works only for the "--list" action; if we are going to
expand it, we would want to do so everywhere. And we do not even know if
given_config_source.file is non-NULL here (we could be reading from
stdin, or a blob). Fortunately expand_user_path will pass through a NULL
without segfaulting.
Probably the right place would be the if/else chain around
builtin/config.c:514, where we convert a relative path into an absolute
one. But I'm not convinced it's a good thing to be doing in the first
place.
-Peff
What if we'll put path expanding right after getting value of file path,
after given_config_source.file = getenv(CONFIG_ENVIRONMENT); at 451?
Thank you.
--
Best regards.
0xAX
From: Jeff King <hidden> Date: 2016-06-15 23:02:56
On Sat, Nov 15, 2014 at 01:38:36AM +0600, Alex Kuleshov wrote:
quoted
Yeah, I'd agree it is a little unexpected to expand here. The "~" is
mostly a shell thing, and doing:
GIT_CONFIG=~/.gitconfig git config --list
from the shell generally works, because the shell will expand the "~"
before it even hits git. If you're not using a shell to set the
variable, you probably should be pre-expanding it yourself.
Yes, you're right here, but i put GIT_CONFIG=~/.gitconfig to my .bashrc
and it doesn't work so.
Weird. It seems to work fine for me (though I admit I only did a pretty
cursory test).
quoted
Probably the right place would be the if/else chain around
builtin/config.c:514, where we convert a relative path into an absolute
one. But I'm not convinced it's a good thing to be doing in the first
place.
What if we'll put path expanding right after getting value of file path,
after given_config_source.file = getenv(CONFIG_ENVIRONMENT); at 451?
That is a good place to put it if you want to impact $GIT_CONFIG but not
"--file". I am not sure if that is sensible. But then, I am not sure
that I am convinced that we should be making any change at all.
-Peff