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

[Jeff King <peff@peff.net>]

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.

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