git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Eric Sunshine <sunshine@sunshineco.com>
To: 0xAX <kuleshovmail@gmail.com>
Cc: "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:19:41 -0500	[thread overview]
Message-ID: <CAPig+cTpUyfKYj4VTK1AT-ga6UvupJrERsTHWTEzNP-Ogc4ujQ@mail.gmail.com> (raw)
In-Reply-To: <1415989760-20259-1-git-send-email-kuleshovmail@gmail.com>

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

> Signed-off-by: Alex Kuleshov <kuleshovmail@gmial.com>

s/gmial/gmail/

> Signed-off-by: 0xAX <kuleshovmail@gmail.com>
> ---
>  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;

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

  reply	other threads:[~2014-11-14 19:19 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 [this message]
2014-11-14 19:30   ` Jeff King
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=CAPig+cTpUyfKYj4VTK1AT-ga6UvupJrERsTHWTEzNP-Ogc4ujQ@mail.gmail.com \
    --to=sunshine@sunshineco.com \
    --cc=git@vger.kernel.org \
    --cc=kuleshovmail@gmail.com \
    --cc=kuleshovmail@gmial.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
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;
as well as URLs for NNTP newsgroup(s).