Bazaar

Merge lp:~jjed/bzr/xdg_basedir_compliance into lp:bzr

  1. xdg_basedir_compliance
  2. Merge into bzr.dev
Proposed by Jjed
Status: Work in progress
Proposed branch: lp:~jjed/bzr/xdg_basedir_compliance
Merge into: lp:bzr
Diff against target: 65 lines (+30/-3)
1 file modified
bzrlib/config.py (+30/-3)
To merge this branch: bzr merge lp:~jjed/bzr/xdg_basedir_compliance
Reviewer Review Type Date Requested Status
Robert Collins (community) Needs Resubmitting
Andrew Bennetts Needs Resubmitting
Review via email: mp+24954@code.launchpad.net

Description of the change

This branch moves the default bzr configuration path from $HOME/.bazaar to $XDG_CONFIG_HOME/bazaar.

The migration of old configuration files is silent. However, to obey the principle of least surprise, the old configuration files are left in place, and a file is written there to alert the user of the new, actively-used location.

To post a comment you must log in.
Revision history for this message
Andrew Bennetts (spiv) wrote :

Thanks for moving this bug along.

I read the linked bug (bug 195397), but didn't see any clear conclusion about what to do with plugins, and this branch doesn't seem to address them. I think they need to be addressed before this lands, because currently they are expected to exist in ~/.bazaar/plugins.

On to the code:

I'm not sure that calling ensure_config_dir_exists() in Config.__init__ is a good idea. Why is that necessary? In general it's nice to defer doing work until it is actually required. And I'm not sure it makes sense for BranchConfig to do this unconditionally.

> + while os.path.exists(user_notice):
> + user_notice = '_'.join((user_notice, 'ALT'))

This seems like overkill for a one-off transition. Is this just being extra careful, or do you expect this to be used? And it seems a shame to be so cautious and then leave a race between the exists check and the file open anyway.

> with open(user_notice, 'w') as file:

This is a Python 2.5-ism (although it needs a __future__ import), but bzr supports Python 2.4+. So update this to use try/finally.

Finally, the docs must be updated before this could be merged, so for the docs and the plugins issue I'm marking this Resubmit.

review: Needs Resubmitting
Reply
Revision history for this message
Robert Collins (lifeless) wrote :

I really don't like moving user edited files around without warning; even worse is leaving the old ones in place: we'll have to deal with questions like:

"I edited ~/.bazaar/bazaar.conf" and *it didn't work*. Help.

Certainly, your code is part of migrating, but I don't think we can or should merge a change like this until we've got all the ducks in a row: consensus on changing [perhaps mail the mailing list to discuss it], documentation updates ready to roll, some assessment about the impact on users in existing distribution releases (e.g. Lucid, supported for 3 years) if they run a release from our PPA and then switch back to their distro version - or worse if they are running on a NFS mounted home with different versions of bzr on two machines.

All in all I'm really not convinced that migrating is worth the impact on our users - we have a huge amount of documentation about where our config files are, that is out there in blog posts and so on. I appreciate the uniformity that the XDG specification offers, so I want to be clear that I'm not rejecting this patch overall, but I do think we need to defer it until these other issues are addressed.

Alternatively, you could make it ready-to-go, but not actually enabled, so that the branch is merged but has no impact until we're ready to migrate.

If you want to do that, please address Andrew's comments and then move the status of this proposal back to needs-review, or alternatively click 'resubmit' (after addressing Andrew's comments).

review: Needs Resubmitting
Reply

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'bzrlib/config.py'
2--- bzrlib/config.py 2010-05-07 06:35:34 +0000
3+++ bzrlib/config.py 2010-05-09 13:52:27 +0000
4@@ -147,6 +147,7 @@
5 """A configuration policy - what username, editor, gpg needs etc."""
6
7 def __init__(self):
8+ ensure_config_dir_exists()
9 super(Config, self).__init__()
10
11 def get_editor(self):
12@@ -819,15 +820,29 @@
13 if not os.path.isdir(parent_dir):
14 trace.mutter('creating config parent directory: %r', parent_dir)
15 os.mkdir(parent_dir)
16+ legacy_config_path = ''
17+ else:
18+ legacy_config_path = _nonxdg_config_dir()
19 trace.mutter('creating config directory: %r', path)
20 os.mkdir(path)
21 osutils.copy_ownership_from_path(path)
22
23+ if os.path.isdir(legacy_config_path):
24+ trace.mutter('copying legacy config from: %r', legacy_config_path)
25+ osutils.copy_tree(legacy_config_path, path)
26+ user_notice = os.path.join(legacy_config_path,
27+ 'NOTICE: DATA OBSOLETE')
28+ while os.path.exists(user_notice):
29+ user_notice = '_'.join((user_notice, 'ALT'))
30+ with open(user_notice, 'w') as file:
31+ file.write('This directory is no longer used by bazaar. '
32+ 'The data contained was transferred to %s.' % path)
33+
34
35 def config_dir():
36 """Return per-user configuration directory.
37
38- By default this is ~/.bazaar/
39+ By default this is $XDG_CONFIG_HOME/bazaar/
40
41 TODO: Global option --config-dir to override this.
42 """
43@@ -844,8 +859,20 @@
44 else:
45 # cygwin, linux, and darwin all have a $HOME directory
46 if base is None:
47- base = os.path.expanduser("~")
48- return osutils.pathjoin(base, ".bazaar")
49+ base = os.environ.get('XDG_CONFIG_HOME', None)
50+ if base is None:
51+ base = osutils.pathjoin(os.path.expanduser("~"), ".config")
52+ return osutils.pathjoin(base, "bazaar")
53+
54+
55+def _nonxdg_config_dir():
56+ # This returns the legacy path of a config directory as it would
57+ # have been created before the xdg base directory specification
58+ # was implemented.
59+ base = os.environ.get('BZR_HOME', None)
60+ if base is None:
61+ base = os.path.expanduser("~")
62+ return osutils.pathjoin(base, ".bazaar")
63
64
65 def config_filename():