From: Jeff King <peff@peff•net>
To: Eric Sunshine <sunshine@sunshineco•com>
Cc: 0xAX <kuleshovmail@gmail•com>,
"git@vger•kernel.org" <git@vger•kernel.org>,
Alex Kuleshov <kuleshovmail@gmial•com>
Subject: Re: [PATCH 1/1] git-config: git-config --list fixed when GIT_CONFIG value starts with ~/
Date: Fri, 14 Nov 2014 14:30:49 -0500 [thread overview]
Message-ID: <20141114193049.GB10860@peff.net> (raw)
In-Reply-To: <CAPig+cTpUyfKYj4VTK1AT-ga6UvupJrERsTHWTEzNP-Ogc4ujQ@mail.gmail.com>
On Fri, Nov 14, 2014 at 02:19:41PM -0500, Eric Sunshine wrote:
> On Fri, Nov 14, 2014 at 1:29 PM, 0xAX <kuleshovmail@gmail•com> 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.)
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.
> > builtin/config.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/builtin/config.c b/builtin/config.c
> > index 7bba516..df1bee0 100644
> > --- a/builtin/config.c
> > +++ b/builtin/config.c
> > @@ -540,6 +540,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)
> >
> > if (actions == ACTION_LIST) {
> > check_argc(argc, 0, 0);
> > + const char* newpath = expand_user_path(given_config_source.file);
> > + given_config_source.file = newpath;
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
next prev parent reply other threads:[~2014-11-14 19:30 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-11-14 18:29 [PATCH 1/1] git-config: git-config --list fixed when GIT_CONFIG value starts with ~/ 0xAX
2014-11-14 19:19 ` Eric Sunshine
2014-11-14 19:30 ` Jeff King [this message]
2014-11-14 19:38 ` Alex Kuleshov
2014-11-14 20:04 ` Jeff King
2014-11-14 20:10 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20141114193049.GB10860@peff.net \
--to=peff@peff$(echo .)net \
--cc=git@vger$(echo .)kernel.org \
--cc=kuleshovmail@gmail$(echo .)com \
--cc=kuleshovmail@gmial$(echo .)com \
--cc=sunshine@sunshineco$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox