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

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

>Jeff King <peff@peff.net>
>
> 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