Merge lp:~nmb/bzr/xdgconfigdir into lp:bzr

[Neil Martinsen-Burrell]

Support configuration files located in $XDG_CONFIG_HOME/bazaar

As discussed on the email list, this reads configuration files in $XDG_CONFIG_HOME/bazaar if that directory exists, but has no user-visible warnings. It also doesn't change the location of the plugins directory.

[John A Meinel (jameinel)]

16 if not os.path.isdir(path):
17 - if sys.platform == 'win32':
18 - parent_dir = os.path.dirname(path)
19 - if not os.path.isdir(parent_dir):
20 - trace.mutter('creating config parent directory: %r', parent_dir)
21 - os.mkdir(parent_dir)
22 + parent_dir = os.path.dirname(path)
23 + if not os.path.isdir(parent_dir):
24 + trace.mutter('creating config parent directory: %r', parent_dir)
25 + os.mkdir(parent_dir)

^- I think this breaks it for Windows. The path is "$APPDATA\Bazaar\2.0\" so we have 2 levels of directories to create, and you removed one of them.

A possible fix is to have it use "os.makedirs()" instead.

review: Needs Fixing

[Neil Martinsen-Burrell (nmb)]

I put the ensure_config_dir code back as it was, since the XDG_CONFIG_HOME stuff is now optional we don't have to worry about creating two levels of directories on Linux as well (which was the original intent of my code above).

[Andrew Bennetts (spiv)]

It looks like you've addressed John's review nicely.

In doc/developers/xdg_config_spec.txt there's a typo:

> +%APPDATA%/Bazaar/2.0 and on Mac OS X, the directory is ~/.bazaaar. With the

You don't need quite so many “a”s in bazaar :)

> Note that the following descriptions do not apply
> +to Windows or Mac OS X which can and should use their own native configuration
> +locations.

True... although is ~/.bazaar the native location for Mac OS X? I'm not sure what
is expected on OS X, and improving the OS X experience is out of scope for this
patch, but this doc shouldn't be misleading about how good we are currently.

It would be nice to have tests for this, but I guess it's awkward to do. Hmm, I
suppose it's possible by overriding the environment variables for $HOME and/or
$XDG_CONFIG_HOME. TestConfigPath in bzrlib/tests/test_config.py is probably a
good starting point.

The only other comment I have is this is worth highlighting in What's New in 2.3
as well.

I think this should have some simple tests added before we merge it, so I'm voting
Needs Fixing, but basically this is looking pretty good.

review: Needs Fixing

[Neil Martinsen-Burrell (nmb)]

Andrew's comments addressed with two tests in bzrlib/tests/test_config.py and better documentation. I tried to clarify our current behavior WRT Windows and Mac OS X in doc/developers/xdg_config_spec.txt

[Andrew Bennetts (spiv)]

Looks good, thanks! I'm going to make some very minor tweaks to the tests (give a reason to TestNotApplicable, move some duplicated test code into setUp, say “BZR_HOME” rather than “this” in the comment, add docstrings to test methods), then I'll send it to PQM.

review: Approve

[Andrew Bennetts (spiv)]

Submitted to PQM via <https://code.launchpad.net/~spiv/bzr/xdgconfigdir/+merge/42069>

=== modified file 'bzrlib/config.py'
--- bzrlib/config.py	2010-11-17 15:51:10 +0000
+++ bzrlib/config.py	2010-11-28 06:16:46 +0000
@@ -1123,7 +1123,9 @@
 def config_dir():
     """Return per-user configuration directory.
 
-    By default this is ~/.bazaar/
+    By default this is %APPDATA%/bazaar/2.0 on Windows, ~/.bazaar on Mac OS X
+    and Linux.  On Linux, if there is a $XDG_CONFIG_HOME/bazaar directory,
+    that will be used instead.
 
     TODO: Global option --config-dir to override this.
     """
@@ -1137,8 +1139,23 @@
             raise errors.BzrError('You must have one of BZR_HOME, APPDATA,'
                                   ' or HOME set')
         return osutils.pathjoin(base, 'bazaar', '2.0')
+    elif sys.platform == 'darwin':
+        if base is None:
+            # this takes into account $HOME
+            base = os.path.expanduser("~")
+        return osutils.pathjoin(base, '.bazaar')
     else:
         if base is None:
+
+            xdg_dir = os.environ.get('XDG_CONFIG_HOME', None)
+            if xdg_dir is None:
+                xdg_dir = osutils.pathjoin(os.path.expanduser("~"), ".config")
+            xdg_dir = osutils.pathjoin(xdg_dir, 'bazaar')
+            if osutils.isdir(xdg_dir):
+                trace.mutter(
+                    "Using configuration in XDG directory %s." % xdg_dir)
+                return xdg_dir
+
             base = os.path.expanduser("~")
         return osutils.pathjoin(base, ".bazaar")
 
 
=== modified file 'bzrlib/tests/test_config.py'
--- bzrlib/tests/test_config.py	2010-11-17 15:52:03 +0000
+++ bzrlib/tests/test_config.py	2010-11-28 06:16:46 +0000
@@ -432,6 +432,36 @@
             '/home/bogus/.cache')
 
 
+class TestXDGConfigDir(tests.TestCaseInTempDir):
+    # must be in temp dir because config tests for the existence of the bazaar
+    # subdirectory of $XDG_CONFIG_HOME
+
+    def setUp(self):
+        super(TestXDGConfigDir, self).setUp()
+        os.environ['HOME'] = self.test_home_dir
+
+    def test_xdg_config_dir_exists(self):
+        if sys.platform in ('darwin', 'win32'):
+            raise tests.TestNotApplicable
+        # this overrides everything we want to test so get rid of it
+        del os.environ['BZR_HOME'] 
+        newdir = osutils.pathjoin(self.test_home_dir, '.config', 'bazaar')
+        os.makedirs(newdir)
+        self.assertEqual(config.config_dir(), newdir)
+
+    def test_xdg_config_home(self):
+        if sys.platform in ('darwin', 'win32'):
+            raise tests.TestNotApplicable
+        # this overrides everything we want to test so get rid of it
+        del os.environ['BZR_HOME']
+
+        xdgconfigdir = osutils.pathjoin(self.test_home_dir, 'xdgconfig')
+        os.environ['XDG_CONFIG_HOME'] = xdgconfigdir
+        newdir = osutils.pathjoin(xdgconfigdir, 'bazaar')
+        os.makedirs(newdir)
+        self.assertEqual(config.config_dir(), newdir)
+
+
 class TestIniConfig(tests.TestCaseInTempDir):
 
     def make_config_parser(self, s):
 
=== added file 'doc/developers/xdg_config_spec.txt'
--- doc/developers/xdg_config_spec.txt	1970-01-01 00:00:00 +0000
+++ doc/developers/xdg_config_spec.txt	2010-11-28 06:16:46 +0000
@@ -0,0 +1,27 @@
+Transitioning Unix installs to the XDG Base Directory Specification
+===================================================================
+
+Currently, Bazaar stores its configuration files and plugins under the
+directory ~/.bazaar on unix installs.  On Windows, this is
+%APPDATA%/Bazaar/2.0 and on Mac OS X, the directory is ~/.bazaar.  With the
+XDG Base Directory specification
+(http://standards.freedesktop.org/basedir-spec/basedir-spec-latest.html), many
+Linux and Unix platforms have tried to centralize configuration files under a
+specific directory referred to as $XDG_CONFIG_HOME.  This has a default value
+of ~/.config.
+
+Bazaar would like to be a good Unix citizen by using these standard locations
+for configuration files.  As such, we should support that location, but not
+require it.  Note that the following descriptions do not apply
+to Windows or Mac OS X which should use their own native configuration
+locations.  (On Windows, we currently do this by working under %APPDATA%.  The
+Mac OS X equivalent would be ~/Library/Application Support/Bazaar but there is
+also cultural support for ~/.bazaar on that platform.)
+
+* If $XDG_CONFIG_HOME/bazaar exists, use the files there for configuration,
+  noting in the log that we are doing so.  This allows individuals who would
+  like to use the XDG specification to do so.
+* Due to a lack of consensus on where plugins should live under the XDG Base
+  Directory spec, continue to look for plugins in ~/.bazaar/plugins.  To
+  change this directory to something not under ~/.bazaar, use the environment
+  variable $BZR_PLUGIN_PATH.
 
=== modified file 'doc/en/release-notes/bzr-2.3.txt'
--- doc/en/release-notes/bzr-2.3.txt	2010-11-26 15:57:20 +0000
+++ doc/en/release-notes/bzr-2.3.txt	2010-11-28 06:16:46 +0000
@@ -39,6 +39,9 @@
   number of remaining conflicts. This provides a better feedback about the
   whole resolution process. (Vincent Ladeuil)
 
+* Read configuration files in $XDG_CONFIG_HOME/bazaar on Unix if there is
+  already a directory there.  (Neil Martinsen-Burrell, #195397)
+
 Bug Fixes
 *********
 
 
=== modified file 'doc/en/whats-new/whats-new-in-2.3.txt'
--- doc/en/whats-new/whats-new-in-2.3.txt	2010-11-22 03:35:24 +0000
+++ doc/en/whats-new/whats-new-in-2.3.txt	2010-11-28 06:16:46 +0000
@@ -38,6 +38,13 @@
   get the old behavior, one can use ``bzr tags --sort=alpha``.
   (Neil Martinsen-Burrell, #640760)
 
+* On platforms other than Windows and Mac OS X, Bazaar will use configuration
+  files that live in $XDG_CONFIG_HOME/bazaar if that directory exists.  This
+  allows interested individuals to conform to the XDG Base Directory
+  specification.  The plugin location has not changed and is still
+  ~/.bazaar/plugins.  To use a different directory for plugins, use the
+  environment variable BZR_PLUGIN_PATH.  (Neil Martinsen-Burrell, #195397)
+
 Launchpad integration
 *********************