{"thread":{"id":"37967","subject":"[PATCH 1/1] git-config: git-config --list fixed when GIT_CONFIG value starts with ~/","startedAt":"2014-11-14T18:29:20Z","lastAt":"2014-11-14T20:10:16Z","messageCount":6,"participants":["0xAX","Eric Sunshine","Jeff King","Alex Kuleshov","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"251892","messageId":"1415989760-20259-1-git-send-email-kuleshovmail@gmail.com","threadId":"37967","inReplyTo":null,"subject":"[PATCH 1/1] git-config: git-config --list fixed when GIT_CONFIG value starts with ~/","fromName":"0xAX","fromEmail":"kuleshovmail@gmail.com","sentAt":"2014-11-14T18:29:20Z","receivedAt":"2014-11-14T18:29:20Z","isPatch":true,"sender":{"key":"kuleshovmail@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2699235?v=4"},"body":"When we execute git config --list and $GIT_CONFIG value starts with home\nprefix - ~/ it produces folowing error - fatal: unable to read config\nfile '~/.gitconfig': No such file or directory. This patch fixed it with\nexpand_user_path for configuration file path before git-config --list\ncall.\n\nSigned-off-by: Alex Kuleshov \nSigned-off-by: 0xAX \n---\n builtin/config.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 7bba516..df1bee0 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -540,6 +540,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \n \tif (actions == ACTION_LIST) {\n \t\tcheck_argc(argc, 0, 0);\n+\t\tconst char* newpath = expand_user_path(given_config_source.file);\n+\t\tgiven_config_source.file = newpath;\n \t\tif (git_config_with_options(show_all_config, NULL,\n \t\t\t\t\t &given_config_source,\n \t\t\t\t\t respect_includes) < 0) {\n-- \n2.1.3.17.g7fa1365.dirty\n"},{"id":"251894","messageId":"CAPig+cTpUyfKYj4VTK1AT-ga6UvupJrERsTHWTEzNP-Ogc4ujQ@mail.gmail.com","threadId":"37967","inReplyTo":"1415989760-20259-1-git-send-email-kuleshovmail@gmail.com","subject":"Re: [PATCH 1/1] git-config: git-config --list fixed when GIT_CONFIG value starts with ~/","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-11-14T19:19:41Z","receivedAt":"2014-11-14T19:19:41Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Nov 14, 2014 at 1:29 PM, 0xAX wrote:\n> When we execute git config --list and $GIT_CONFIG value starts with home\n> prefix - ~/ it produces folowing error - fatal: unable to read config\n> file '~/.gitconfig': No such file or directory. This patch fixed it with\n> expand_user_path for configuration file path before git-config --list\n> call.\n\nIs this special case really warranted? Elsewhere, GIT_CONFIG does not\nget this sort of special treatment. Moreover, it appears that no other\nGIT_* environment variable is subject to such special treatment. (And,\ngenerally speaking, on Unix, it is generally assumed that a path\nassigned to an environment variable is to be used as-is.)\n\n> Signed-off-by: Alex Kuleshov \n\ns/gmial/gmail/\n\n> Signed-off-by: 0xAX \n> ---\n> builtin/config.c | 2 ++\n> 1 file changed, 2 insertions(+)\n>\n> diff --git a/builtin/config.c b/builtin/config.c\n> index 7bba516..df1bee0 100644\n> --- a/builtin/config.c\n> +++ b/builtin/config.c\n> @@ -540,6 +540,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n>\n> if (actions == ACTION_LIST) {\n> check_argc(argc, 0, 0);\n> + const char* newpath = expand_user_path(given_config_source.file);\n> + given_config_source.file = newpath;\n\nA few issues:\n\n(1) Style: s/char* /char */\n\n(2) Avoid declaration (of 'newpath') after statement.\n\n(3) You can drop 'newpath' altogether and just assign the result of\nexpand_user_path() directly to given_config_source.file.\n\nThis code is potentially leaking the old value of\ngiven_config_source.file, and (later) the new value, however, as\ngiven_config_source.file is already being leaked elsewhere, it\nprobably does not make the situation much worse.\n\n> if (git_config_with_options(show_all_config, NULL,\n> &given_config_source,\n> respect_includes) < 0) {\n> --\n> 2.1.3.17.g7fa1365.dirty\n"},{"id":"251898","messageId":"20141114193049.GB10860@peff.net","threadId":"37967","inReplyTo":"CAPig+cTpUyfKYj4VTK1AT-ga6UvupJrERsTHWTEzNP-Ogc4ujQ@mail.gmail.com","subject":"Re: [PATCH 1/1] git-config: git-config --list fixed when GIT_CONFIG value starts with ~/","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-11-14T19:30:49Z","receivedAt":"2014-11-14T19:30:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 14, 2014 at 02:19:41PM -0500, Eric Sunshine wrote:\n\n> On Fri, Nov 14, 2014 at 1:29 PM, 0xAX wrote:\n> > When we execute git config --list and $GIT_CONFIG value starts with home\n> > prefix - ~/ it produces folowing error - fatal: unable to read config\n> > file '~/.gitconfig': No such file or directory. This patch fixed it with\n> > expand_user_path for configuration file path before git-config --list\n> > call.\n> \n> Is this special case really warranted? Elsewhere, GIT_CONFIG does not\n> get this sort of special treatment. Moreover, it appears that no other\n> GIT_* environment variable is subject to such special treatment. (And,\n> generally speaking, on Unix, it is generally assumed that a path\n> assigned to an environment variable is to be used as-is.)\n\nYeah, I'd agree it is a little unexpected to expand here. The \"~\" is\nmostly a shell thing, and doing:\n\n GIT_CONFIG=~/.gitconfig git config --list\n\nfrom the shell generally works, because the shell will expand the \"~\"\nbefore it even hits git. If you're not using a shell to set the\nvariable, you probably should be pre-expanding it yourself.\n\nNote that this code path affects \"git config --file=~/.gitconfig\", too.\nAt least there it would be a little bit useful because the shell will\nnot expand for you, but it still feels a bit unconventional to me.\n\n> > builtin/config.c | 2 ++\n> > 1 file changed, 2 insertions(+)\n> >\n> > diff --git a/builtin/config.c b/builtin/config.c\n> > index 7bba516..df1bee0 100644\n> > --- a/builtin/config.c\n> > +++ b/builtin/config.c\n> > @@ -540,6 +540,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n> >\n> > if (actions == ACTION_LIST) {\n> > check_argc(argc, 0, 0);\n> > + const char* newpath = expand_user_path(given_config_source.file);\n> > + given_config_source.file = newpath;\n\nIf we _were_ going to do such an expansion, this is absolutely the wrong\nplace for it. It works only for the \"--list\" action; if we are going to\nexpand it, we would want to do so everywhere. And we do not even know if\ngiven_config_source.file is non-NULL here (we could be reading from\nstdin, or a blob). Fortunately expand_user_path will pass through a NULL\nwithout segfaulting.\n\nProbably the right place would be the if/else chain around\nbuiltin/config.c:514, where we convert a relative path into an absolute\none. But I'm not convinced it's a good thing to be doing in the first\nplace.\n\n-Peff\n"},{"id":"251900","messageId":"87vbmh8syq.fsf@gmail.com","threadId":"37967","inReplyTo":"20141114193049.GB10860@peff.net","subject":"Re: [PATCH 1/1] git-config: git-config --list fixed when GIT_CONFIG value starts with ~/","fromName":"Alex Kuleshov","fromEmail":"kuleshovmail@gmail.com","sentAt":"2014-11-14T19:38:36Z","receivedAt":"2014-11-14T19:38:36Z","isPatch":true,"sender":{"key":"kuleshovmail@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2699235?v=4"},"body":"\nHello Eric and Jeff,\n\n>Eric Sunshine\n>A few issues:\n>\n>(1) Style: s/char* /char */\n>\n>(2) Avoid declaration (of 'newpath') after statement.\n>\n>(3) You can drop 'newpath' altogether and just assign the result of\n>expand_user_path() directly to given_config_source.file.\n>\n>This code is potentially leaking the old value of\n>given_config_source.file, and (later) the new value, however, as\n>given_config_source.file is already being leaked elsewhere, it\n>probably does not make the situation much worse.\n\nIt is my first patch to git's code base, so many thanks for your\nfeedback, i'll fix these issues if there will be need in this patch.\n\n>Jeff King \n>\n> Yeah, I'd agree it is a little unexpected to expand here. The \"~\" is\n> mostly a shell thing, and doing:\n>\n> GIT_CONFIG=~/.gitconfig git config --list\n>\n> from the shell generally works, because the shell will expand the \"~\"\n> before it even hits git. If you're not using a shell to set the\n> variable, you probably should be pre-expanding it yourself.\n\nYes, you're right here, but i put GIT_CONFIG=~/.gitconfig to my .bashrc\nand it doesn't work so.\n\n>\n> Note that this code path affects \"git config --file=~/.gitconfig\", too.\n> At least there it would be a little bit useful because the shell will\n> not expand for you, but it still feels a bit unconventional to me.\n>\n>> > builtin/config.c | 2 ++\n>> > 1 file changed, 2 insertions(+)\n>> >\n>> > diff --git a/builtin/config.c b/builtin/config.c\n>> > index 7bba516..df1bee0 100644\n>> > --- a/builtin/config.c\n>> > +++ b/builtin/config.c\n>> > @@ -540,6 +540,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n>> >\n>> > if (actions == ACTION_LIST) {\n>> > check_argc(argc, 0, 0);\n>> > + const char* newpath = expand_user_path(given_config_source.file);\n>> > + given_config_source.file = newpath;\n>\n> If we _were_ going to do such an expansion, this is absolutely the wrong\n> place for it. It works only for the \"--list\" action; if we are going to\n> expand it, we would want to do so everywhere. And we do not even know if\n> given_config_source.file is non-NULL here (we could be reading from\n> stdin, or a blob). Fortunately expand_user_path will pass through a NULL\n> without segfaulting.\n>\n> Probably the right place would be the if/else chain around\n> builtin/config.c:514, where we convert a relative path into an absolute\n> one. But I'm not convinced it's a good thing to be doing in the first\n> place.\n>\n> -Peff\n\nWhat if we'll put path expanding right after getting value of file path,\nafter given_config_source.file = getenv(CONFIG_ENVIRONMENT); at 451?\n\nThank you.\n\n--\nBest regards.\n0xAX\n"},{"id":"251904","messageId":"20141114200419.GB11581@peff.net","threadId":"37967","inReplyTo":"87vbmh8syq.fsf@gmail.com","subject":"Re: [PATCH 1/1] git-config: git-config --list fixed when GIT_CONFIG value starts with ~/","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-11-14T20:04:20Z","receivedAt":"2014-11-14T20:04:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Nov 15, 2014 at 01:38:36AM +0600, Alex Kuleshov wrote:\n\n> > Yeah, I'd agree it is a little unexpected to expand here. The \"~\" is\n> > mostly a shell thing, and doing:\n> >\n> > GIT_CONFIG=~/.gitconfig git config --list\n> >\n> > from the shell generally works, because the shell will expand the \"~\"\n> > before it even hits git. If you're not using a shell to set the\n> > variable, you probably should be pre-expanding it yourself.\n> \n> Yes, you're right here, but i put GIT_CONFIG=~/.gitconfig to my .bashrc\n> and it doesn't work so.\n\nWeird. It seems to work fine for me (though I admit I only did a pretty\ncursory test).\n\n> > Probably the right place would be the if/else chain around\n> > builtin/config.c:514, where we convert a relative path into an absolute\n> > one. But I'm not convinced it's a good thing to be doing in the first\n> > place.\n> >\n> What if we'll put path expanding right after getting value of file path,\n> after given_config_source.file = getenv(CONFIG_ENVIRONMENT); at 451?\n\nThat is a good place to put it if you want to impact $GIT_CONFIG but not\n\"--file\". I am not sure if that is sensible. But then, I am not sure\nthat I am convinced that we should be making any change at all.\n\n-Peff\n"},{"id":"251906","messageId":"xmqqk32x35nb.fsf@gitster.dls.corp.google.com","threadId":"37967","inReplyTo":"1415989760-20259-1-git-send-email-kuleshovmail@gmail.com","subject":"Re: [PATCH 1/1] git-config: git-config --list fixed when GIT_CONFIG value starts with ~/","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-14T20:10:16Z","receivedAt":"2014-11-14T20:10:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"0xAX writes:\n\n> When we execute git config --list and $GIT_CONFIG value starts with home\n> prefix - ~/ it produces folowing error - fatal: unable to read config\n> file '~/.gitconfig': No such file or directory. This patch fixed it with\n> expand_user_path for configuration file path before git-config --list\n> call.\n\nExpanding tilde ~ in environment variables is what you let your\nshell do when you assign it. This is not limited to Git:\n\n $ FOO=~/.bashrc\n $ BAR='~/.bashrc'\n $ head -n 1 \"$FOO\"\n # Bourne Again SHell init file.\n $ head -n 1 \"$BAR\"\n head: cannot open ‘~/.bashrc’ for reading: No such file rdirectory\n\n $ echo \"FOO=$FOO\" \"BAR=$BAR\"\n FOO=/home/gitster/.bashrc BAR=~/.bashrc\n\nThe patch does not fix anything and not necessary.\n"}]}