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

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

[NGUYEN Huynh Khoi Nguyen]

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
---
 builtin/config.c |   31 ++++++++++++++++++++++++++++---
 config.c         |   15 ++++++++++++++-
 2 files changed, 42 insertions(+), 4 deletions(-)

diff --git a/builtin/config.c b/builtin/config.c
index 33c8820..dc890d5 100644
--- 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);
+			gitconfig_file = fopen(gitconfig_path, "r");
+			config_file = fopen(config_path, "r");
+
+			if (gitconfig_file==NULL && config_file!=NULL)
+				global = xstrdup(mkpath("%s/.config/git/config", home));
+			else
+				global = xstrdup(mkpath("%s/.gitconfig", home));
+		}
 		if (git_config_system())
 			system_wide = git_etc_gitconfig();
 	}
@@ -381,7 +393,20 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 	if (use_global_config) {
 		char *home = getenv("HOME");
 		if (home) {
-			char *user_config = xstrdup(mkpath("%s/.gitconfig", home));
+			char gitconfig_path[PATH_MAX], config_path[PATH_MAX];
+			FILE *gitconfig_file, *config_file;
+			char *user_config;
+
+			sprintf(gitconfig_path, "%s/.gitconfig", home);
+			sprintf(config_path, "%s/.config/git/config", home);
+			gitconfig_file = fopen(gitconfig_path, "r");
+			config_file = fopen(config_path, "r");
+
+			if (gitconfig_file==NULL && config_file!=NULL)
+				user_config = xstrdup(mkpath("%s/.config/git/config", home));
+			else
+				user_config = xstrdup(mkpath("%s/.gitconfig", home));
+
 			given_config_file = user_config;
 		} else {
 			die("$HOME not set");
diff --git a/config.c b/config.c
index eeee986..998dbbc 100644
--- a/config.c
+++ b/config.c
@@ -962,7 +962,20 @@ int git_config_early(config_fn_t fn, void *data, const char *repo_config)
 	home = getenv("HOME");
 	if (home) {
 		char buf[PATH_MAX];
-		char *user_config = mksnpath(buf, sizeof(buf), "%s/.gitconfig", home);
+		char gitconfig_path[PATH_MAX], config_path[PATH_MAX];
+		FILE *gitconfig_file, *config_file;
+		char *user_config;
+
+		sprintf(gitconfig_path, "%s/.gitconfig", home);
+		sprintf(config_path, "%s/.config/git/config", home);
+		gitconfig_file = fopen(gitconfig_path, "r");
+		config_file = fopen(config_path, "r");
+
+		if (gitconfig_file==NULL && config_file!=NULL)
+			user_config = mksnpath(buf, sizeof(buf), "%s/.config/git/config", home);
+		else
+			user_config = mksnpath(buf, sizeof(buf), "%s/.gitconfig", home);
+
 		if (!access(user_config, R_OK)) {
 			ret += git_config_from_file(fn, user_config, data);
 			found += 1;
-- 
1.7.0.4

[PATCH 2/2] Test File Name: t1306-second-config-file.sh

[NGUYEN Huynh Khoi Nguyen]

There are 4 tests:
test1 and test2: read tests
test1: ~/.config/git/config exists and ~/.gitconfig doesn't
git reads ~/.config/git/config
test2: ~/.config/git/config exists and ~/.gitconfig exists
git reads ~/.gitconfig
test3 and test4: write tests
test3: ~/.config/git/config exists and ~/.gitconfig doesn't
git writes in ~/.config/git/config
test4: ~/.config/git/config exists and ~/.gitconfig exists
git writes in ~/.gitconfig
---
 t/t1306-second-config-file.sh |   47 +++++++++++++++++++++++++++++++++++++++++
 1 files changed, 47 insertions(+), 0 deletions(-)
 create mode 100755 t/t1306-second-config-file.sh

diff --git a/t/t1306-second-config-file.sh b/t/t1306-second-config-file.sh
new file mode 100755
index 0000000..5406456
--- /dev/null
+++ b/t/t1306-second-config-file.sh
@@ -0,0 +1,47 @@
+#!/bin/sh
+#
+# Copyright (c) 2012 Valentin Duperray, Lucien Kong, Franck Jonas,
+#		     Thomas Nguy, Khoi Nguyen
+#		     Grenoble INP Ensimag
+#
+
+test_description='~/.config/git/config instead of ~/.gitconfig'
+
+. ./test-lib.sh
+
+test_expect_success 'read: ~/.config/git/config exists and ~/.gitconfig doesn'\''t' '
+	mkdir .config &&
+	mkdir .config/git &&
+	echo "[user]" >.config/git/config &&
+	echo "	name = read_config" >>.config/git/config &&
+	echo read_config >expect &&
+	git config --global --get user.name >output &&
+	test_cmp expect output
+'
+
+test_expect_success 'read: ~/.config/git/config exists and ~/.gitconfig exists' '
+	>.gitconfig &&
+	echo "[user]" >.gitconfig &&
+	echo "	name = read_gitconfig" >>.gitconfig &&
+	echo read_gitconfig >expect &&
+	git config --global --get user.name >output &&
+	test_cmp expect output
+'
+
+test_expect_success 'write: ~/.config/git/config exists and ~/.gitconfig doesn'\''t' '
+	rm .gitconfig &&
+	git config --global user.name "write_config" &&
+	echo "[user]" >expect &&
+	echo "	name = write_config" >>expect &&
+	test_cmp expect .config/git/config
+'
+
+test_expect_success 'write: ~/.config/git/config exists and ~/.gitconfig exists' '
+	>.gitconfig &&
+	git config --global user.name "write_gitconfig" &&
+	echo "[user]" >expect &&
+	echo "	name = write_gitconfig" >>expect &&
+	test_cmp expect .gitconfig
+'
+
+test_done
-- 
1.7.0.4

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

[Jeff King]

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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

Isn't it actually much worse than that?

If you read from .gitconfig and also from the new location, but update
only the new location, people who use two versions of git will be in a
very confusing situation.  Randomly, some of their updates are always in
effect, and others only appear sometimes, and after wasting a lot of time
and hair scratching their heads, they will realize that writing with old
versions of Git will store values to a place visible to both versions,
while writing with new versions will store values to a place visible only
to new versions.

I'd rather see it ignore the new location as long as ~/.gitconfig exists
(and if only the new location exists, read from and write to it), and have
users make a conscious decision to transition.  That is:

 - If ~/.gitconfig exists, do not do anything new.  Just exercise the
   original code.  For these users, ~/.config/ does _not_ exist as far as
   Git is concerned.

 - (optional) If ~/.gitconfig exists, offer _moving_ it to the new
   location after telling the user to make sure that the user will never
   use older version of git again, and move it if the user agrees.

 - Otherwise, read from and write to the new location.

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

[Jeff King]

On Fri, May 25, 2012 at 02:25:38PM -0700, Junio C Hamano wrote:

> > 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.
> 
> Isn't it actually much worse than that?
> 
> If you read from .gitconfig and also from the new location, but update
> only the new location, people who use two versions of git will be in a
> very confusing situation.  Randomly, some of their updates are always in
> effect, and others only appear sometimes, and after wasting a lot of time
> and hair scratching their heads, they will realize that writing with old
> versions of Git will store values to a place visible to both versions,
> while writing with new versions will store values to a place visible only
> to new versions.

That's true, but...

> I'd rather see it ignore the new location as long as ~/.gitconfig exists
> (and if only the new location exists, read from and write to it), and have
> users make a conscious decision to transition.  That is:
> 
>  - If ~/.gitconfig exists, do not do anything new.  Just exercise the
>    original code.  For these users, ~/.config/ does _not_ exist as far as
>    Git is concerned.
> 
>  - (optional) If ~/.gitconfig exists, offer _moving_ it to the new
>    location after telling the user to make sure that the user will never
>    use older version of git again, and move it if the user agrees.
> 
>  - Otherwise, read from and write to the new location.

That doesn't solve all problems with multiple versions, though. For
example, this sequence:

  1. User consciously moves to new location, moving ~/.gitconfig to
     ~/.config/git/config (or perhaps they do not do so consciously, but
     do not have a ~/.gitconfig at all, and run "git config --global"
     with the new version.

  2. User runs "git config --global" with an old version of git, which
     writes to ~/.gitconfig.

After step 1, old versions of git will not respect the user's config at
all. This is unavoidable; the old version does not know about the new
location.

But after step 2, _all_ versions of git have stopped respecting the new
location (because ~/.gitconfig takes precedence). Whereas if we read
from everywhere, then it is broken only in older versions (which are
broken anyway).

So I consider it the lesser of two evils. The rule is much simpler: "old
versions of git do not know about this new location". Which is
unavoidable, and easier to explain than "Old versions of git do not know
about this location. New versions do, but will sometimes ignore
depending on whether this other file exists, which might have been
created by an old version".

However, let's take a step back for a minute. I think the real issue is
writing to the XDG location without the user knowing about it. So a
better transition plan would be:

  1. Start reading from the XDG location in addition to the old
     location. Always write to the old location.

  2. Wait N time units until everybody reasonable has a version that
     does (1).

  3. Start writing to the XDG location by default. Keep reading from the
     old version for compatibility.

People who want to start using the new location after step 1 are free to
do so; they just shouldn't expect git to write to it, and they should
accept the obvious caveat that older versions of git will not understand
it. An optional addendum is that we could start writing to the XDG
location after step 1 only if it exists, which implies that the user has
decided it's OK to do so (which is still a guess; they might have wanted
to split their config intentionally).

-Peff

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

[Nguyen Thai Ngoc Duy]

On Sat, May 26, 2012 at 4:44 AM, Jeff King <peff@peff.net> wrote:
>> I'd rather see it ignore the new location as long as ~/.gitconfig exists
>> (and if only the new location exists, read from and write to it), and have
>> users make a conscious decision to transition.  That is:
>>
>>  - If ~/.gitconfig exists, do not do anything new.  Just exercise the
>>    original code.  For these users, ~/.config/ does _not_ exist as far as
>>    Git is concerned.
>>
>>  - (optional) If ~/.gitconfig exists, offer _moving_ it to the new
>>    location after telling the user to make sure that the user will never
>>    use older version of git again, and move it if the user agrees.
>>
>>  - Otherwise, read from and write to the new location.
>
> That doesn't solve all problems with multiple versions, though. For
> example, this sequence:
>
>  1. User consciously moves to new location, moving ~/.gitconfig to
>     ~/.config/git/config (or perhaps they do not do so consciously, but
>     do not have a ~/.gitconfig at all, and run "git config --global"
>     with the new version.
>
>  2. User runs "git config --global" with an old version of git, which
>     writes to ~/.gitconfig.
>
> After step 1, old versions of git will not respect the user's config at
> all. This is unavoidable; the old version does not know about the new
> location.
>
> But after step 2, _all_ versions of git have stopped respecting the new
> location (because ~/.gitconfig takes precedence). Whereas if we read
> from everywhere, then it is broken only in older versions (which are
> broken anyway).
>
> So I consider it the lesser of two evils. The rule is much simpler: "old
> versions of git do not know about this new location". Which is
> unavoidable, and easier to explain than "Old versions of git do not know
> about this location. New versions do, but will sometimes ignore
> depending on whether this other file exists, which might have been
> created by an old version".

We could amend Junio's version a bit:

 - if both versions exist, warn loudly (optionally refuse to work) and
suggest to symlink .gitconfig to .config/git/config


> However, let's take a step back for a minute. I think the real issue is
> writing to the XDG location without the user knowing about it. So a
> better transition plan would be:
>
>  1. Start reading from the XDG location in addition to the old
>     location. Always write to the old location.
>
>  2. Wait N time units until everybody reasonable has a version that
>     does (1).
>
>  3. Start writing to the XDG location by default. Keep reading from the
>     old version for compatibility.

Hang on.. this "by default" is only for Linux, or for every other OS too?


> People who want to start using the new location after step 1 are free to
> do so; they just shouldn't expect git to write to it, and they should
> accept the obvious caveat that older versions of git will not understand
> it. An optional addendum is that we could start writing to the XDG
> location after step 1 only if it exists, which implies that the user has
> decided it's OK to do so (which is still a guess; they might have wanted
> to split their config intentionally).
-- 
Duy

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

[Jeff King]

On Sat, May 26, 2012 at 05:15:13PM +0700, Nguyen Thai Ngoc Duy wrote:

> > So I consider it the lesser of two evils. The rule is much simpler: "old
> > versions of git do not know about this new location". Which is
> > unavoidable, and easier to explain than "Old versions of git do not know
> > about this location. New versions do, but will sometimes ignore
> > depending on whether this other file exists, which might have been
> > created by an old version".
> 
> We could amend Junio's version a bit:
> 
>  - if both versions exist, warn loudly (optionally refuse to work) and
> suggest to symlink .gitconfig to .config/git/config

Yeah, that might help. At the same time, it is not necessarily an error
condition (e.g., if I track my ~/.config directory, but want to put
one-offs in ~/.gitconfig. On the other hand, you can set that up
manually with include.path = ~/.some-not-tracked-file, so maybe it is
not a use case worth worrying about).

> >  1. Start reading from the XDG location in addition to the old
> >     location. Always write to the old location.
> >
> >  2. Wait N time units until everybody reasonable has a version that
> >     does (1).
> >
> >  3. Start writing to the XDG location by default. Keep reading from the
> >     old version for compatibility.
> 
> Hang on.. this "by default" is only for Linux, or for every other OS too?

Sorry, I overstated a bit. It would be _safe_ to do step 3 like that
after waiting N time units, but we could also do something much less
drastic (like only writing to it when it exists, or when ~/.gitconfig
does not exist, or whatever). And no, I wouldn't think following XDG on
non-Linux machines would make much sense if no other programs on that
platform do so (I don't know what OS X is like, but I can imagine nobody
uses XDG paths on Windows).

-Peff

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

[David Aguilar]

On Sat, May 26, 2012 at 2:54 PM, Jeff King <peff@peff.net> wrote:
> On Sat, May 26, 2012 at 05:15:13PM +0700, Nguyen Thai Ngoc Duy wrote:
>
>> > So I consider it the lesser of two evils. The rule is much simpler: "old
>> > versions of git do not know about this new location". Which is
>> > unavoidable, and easier to explain than "Old versions of git do not know
>> > about this location. New versions do, but will sometimes ignore
>> > depending on whether this other file exists, which might have been
>> > created by an old version".
>>
>> We could amend Junio's version a bit:
>>
>>  - if both versions exist, warn loudly (optionally refuse to work) and
>> suggest to symlink .gitconfig to .config/git/config
>
> Yeah, that might help. At the same time, it is not necessarily an error
> condition (e.g., if I track my ~/.config directory, but want to put
> one-offs in ~/.gitconfig. On the other hand, you can set that up
> manually with include.path = ~/.some-not-tracked-file, so maybe it is
> not a use case worth worrying about).
>
>> >  1. Start reading from the XDG location in addition to the old
>> >     location. Always write to the old location.
>> >
>> >  2. Wait N time units until everybody reasonable has a version that
>> >     does (1).
>> >
>> >  3. Start writing to the XDG location by default. Keep reading from the
>> >     old version for compatibility.
>>
>> Hang on.. this "by default" is only for Linux, or for every other OS too?
>
> Sorry, I overstated a bit. It would be _safe_ to do step 3 like that
> after waiting N time units, but we could also do something much less
> drastic (like only writing to it when it exists, or when ~/.gitconfig
> does not exist, or whatever). And no, I wouldn't think following XDG on
> non-Linux machines would make much sense if no other programs on that
> platform do so (I don't know what OS X is like, but I can imagine nobody
> uses XDG paths on Windows).

I completely disagree about the OS X thing.  We have shared home
directories across OS X and Linux.  There are many OS X apps that do
respect XDG because they use Qt.  Git configuration is core to the
system; I think we should avoid fragmentation.

Real-word scenario: At disney animation studio (and probably pixar
too) OS X is (thankfully) a 2nd-class citizen to Linux.  We would very
much appreciate the ability to have the OS X git behave exactly like
the Linux version, even if that means a Makefile switch.  IMO it
should do so by default, though; there's little to gain otherwise.

I would even suggest that windows should do the same.  .dotfiles are
not a regular windows thing in the first place.  It's the simplest and
easiest-to-explain thing to do, too.  Otherwise it means more
platform-specific documentation.  Less documentation is better
documentation (especially since no one reads it ;p)

So I think having OS X git follow XDG makes a lot of sense.  I don't
know of any alternatives short of adding something like an OS
X-specific System Preferences pane or some insane OS X `defaults`
.plist-integrated thing ;-)
-- 
David

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

[Matthieu Moy]

Junio C Hamano <gitster@pobox.com> writes:

> If you read from .gitconfig and also from the new location, but update
> only the new location,

That's not exactly the proposal. First, I agree that the first questions
to be answered is what happens on reading. About writing, I don't feel
very strongly about it, but I think it's reasonable to write to the new
location if the old location doesn't exist, and the new one does. This
way, ~/.gitconfig haters will create their ~/.config/git/config, and
won't be bothered with the former.

Obviously, trying too hard to write to the new location would harm old
versions users, so for example, it would be unreasonable to write to the
new location unconditionally.

> people who use two versions of git will be in a very confusing
> situation. Randomly, some of their updates are always in effect,

Only if they created manually the new file. People unaware of the change
won't be affected at all.

Since most people don't read the docs, most people will be
unaffected ;-).

>  - If ~/.gitconfig exists, do not do anything new.  Just exercise the
>    original code.  For these users, ~/.config/ does _not_ exist as far as
>    Git is concerned.

As Jeff already pointed out, this would be very confusing for a user
having the new file, since running once "git config --global" with an
old version would shadow the whole configuration by creating an almost
empty ~/.gitconfig file.

We clearly want to read both. I'm not sure which should take precedence
when the same variable is defined in both.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

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

[jaseem abid]

On Sat, May 26, 2012 at 1:17 AM, NGUYEN Huynh Khoi Nguyen
<nguyenhu@ensimag.imag.fr> 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


If a new config file gets introduced at `~/.config/git/config`, what
will be the new order of config file precedence?

`/etc/gitconfig` > `~/.gitconfig` > `~/.config/git/config`  > `.git/config` or
`/etc/gitconfig` >  `~/.config/git/config` > `~/.gitconfig` > `.git/config` ?

What will be the new flag to access it? I mean anything new like
--system / --global going to be introduced?

-- 
Jaseem Abid
http://jaseemabid.github.com

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

[Matthieu Moy]

NGUYEN Huynh Khoi Nguyen <nguyenhu@ensimag.imag.fr> writes:

> +			sprintf(config_path, "%s/.config/git/config", home);

It's not terribly important, but if we are to use something that looks
like XDG, I'd rather have a real support for it. ~/.config/ is the
default, but the spec
http://standards.freedesktop.org/basedir-spec/basedir-spec-latest.html
also define $XDG_CONFIG_HOME that may override the default and we may
want to support $XDG_CONFIG_DIRS too.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

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

[Jeff King]

On Sat, May 26, 2012 at 10:53:27AM +0200, Matthieu Moy wrote:

> NGUYEN Huynh Khoi Nguyen <nguyenhu@ensimag.imag.fr> writes:
> 
> > +			sprintf(config_path, "%s/.config/git/config", home);
> 
> It's not terribly important, but if we are to use something that looks
> like XDG, I'd rather have a real support for it. ~/.config/ is the
> default, but the spec
> http://standards.freedesktop.org/basedir-spec/basedir-spec-latest.html
> also define $XDG_CONFIG_HOME that may override the default and we may
> want to support $XDG_CONFIG_DIRS too.

I agree. This series is much less appealing as "put the config in some
other place" and much more appealing as "conform to an existing standard
about dot-file locations". And if we are going to follow the standard,
then we should do all parts of it.

-Peff