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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html