patch, 2 partsTest File Name: t1306-second-config-file.sh

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

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

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

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.

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

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.

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

Jeff King <peff@peff.net> writes:

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.

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

That's true, but...

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

On Sat, May 26, 2012 at 4:44 AM, Jeff King <peff@peff.net> wrote:

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

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

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

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

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

On Sat, May 26, 2012 at 2:54 PM, Jeff King <peff@peff.net> wrote:

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

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

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.

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

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.

On Sat, May 26, 2012 at 1:17 AM, NGUYEN Huynh Khoi Nguyen <nguyenhu@ensimag.imag.fr> wrote:

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?

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

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.

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

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