Re: [PATCH 1/2] Add possibility to store configuration in ~/.config/git/config file

From: Jeff King <peff@peff.net>
Date: May 25, 2012, 20:30 UTC
Message-ID: <20120525203056.GC4364@sigill.intra.peff.net>
In-Reply-To: <1337975239-17169-1-git-send-email-nguyenhu@ensibm.imag.fr>

On Fri, May 25, 2012 at 09:47:18PM +0200, NGUYEN Huynh Khoi Nguyen wrote:

> git will store its configuration in ~/.config/git/config file if this file
> exists and ~/.gitconfig file doesn't, otherwise git store its configuration
> in ~/.gitconfig as usual

What about reading? For maximum compatibility, we should always read from _both_ of them, and choose between them only when writing, no? It looks like your patch will only read from one or the other.

At first people will have only one or the other, but people using multiple versions of git, or people following already-written instructions on the web about modifying ~/.gitconfig could end up with both.

Show 14 quoted lines

> --- a/builtin/config.c
> +++ b/builtin/config.c
> @@ -171,8 +171,20 @@ static int get_value(const char *key_, const char *regex_)
>  	if (!local) {
>  		const char *home = getenv("HOME");
>  		local = repo_config = git_pathdup("config");
> -		if (home)
> -			global = xstrdup(mkpath("%s/.gitconfig", home));
> +		if (home) {
> +			char gitconfig_path[PATH_MAX], config_path[PATH_MAX];
> +			FILE *gitconfig_file, *config_file;
> +
> +			sprintf(gitconfig_path, "%s/.gitconfig", home);
> +			sprintf(config_path, "%s/.config/git/config", home);

These are both exploitable buffer overflows. Why not use mkpath?

> +			gitconfig_file = fopen(gitconfig_path, "r");
> +			config_file = fopen(config_path, "r");

So we open both files. It looks like in an attempt to see if they are readable. But:

  1. No need to go to that much work. You can just call access(R_OK).

  2. You never close the files, so you are leaking memory and file
     descriptors.

> +			if (gitconfig_file==NULL && config_file!=NULL)

Style. We put whitespace around comparison operators, and we usually don't refer to NULL specifically, like:

  if (!gitconfig_file && config_file)

So a simpler way to write this section would be something like:

  if (home) {
          const char *path = mkpath("%s/.config/git/config", home);

          if (!access(path, R_OK))
                  global = xstrdup(path);
          else
                  global = xstrdup(mkpath("%s/.gitconfig", home));
  }

But like I said earlier, I think this should really be reading from _both_, which is a different change altogether.

I won't go through the other two hunks individually, but my comments are similar.

-Peff