Re: [PATCH 1/1] git-config: git-config --list fixed when GIT_CONFIG value starts with ~/

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.

quoted

 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