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
=== modified file 'bzrlib/config.py'
--- bzrlib/config.py 2010-05-07 06:35:34 +0000
+++ bzrlib/config.py 2010-05-09 13:52:27 +0000
@@ -147,6 +147,7 @@
147 """A configuration policy - what username, editor, gpg needs etc."""147 """A configuration policy - what username, editor, gpg needs etc."""
148148
149 def __init__(self):149 def __init__(self):
150 ensure_config_dir_exists()
150 super(Config, self).__init__()151 super(Config, self).__init__()
151152
152 def get_editor(self):153 def get_editor(self):
@@ -819,15 +820,29 @@
819 if not os.path.isdir(parent_dir):820 if not os.path.isdir(parent_dir):
820 trace.mutter('creating config parent directory: %r', parent_dir)821 trace.mutter('creating config parent directory: %r', parent_dir)
821 os.mkdir(parent_dir)822 os.mkdir(parent_dir)
823 legacy_config_path = ''
824 else:
825 legacy_config_path = _nonxdg_config_dir()
822 trace.mutter('creating config directory: %r', path)826 trace.mutter('creating config directory: %r', path)
823 os.mkdir(path)827 os.mkdir(path)
824 osutils.copy_ownership_from_path(path)828 osutils.copy_ownership_from_path(path)
825829
830 if os.path.isdir(legacy_config_path):
831 trace.mutter('copying legacy config from: %r', legacy_config_path)
832 osutils.copy_tree(legacy_config_path, path)
833 user_notice = os.path.join(legacy_config_path,
834 'NOTICE: DATA OBSOLETE')
835 while os.path.exists(user_notice):
836 user_notice = '_'.join((user_notice, 'ALT'))
837 with open(user_notice, 'w') as file:
838 file.write('This directory is no longer used by bazaar. '
839 'The data contained was transferred to %s.' % path)
840
826841
827def config_dir():842def config_dir():
828 """Return per-user configuration directory.843 """Return per-user configuration directory.
829844
830 By default this is ~/.bazaar/845 By default this is $XDG_CONFIG_HOME/bazaar/
831846
832 TODO: Global option --config-dir to override this.847 TODO: Global option --config-dir to override this.
833 """848 """
@@ -844,8 +859,20 @@
844 else:859 else:
845 # cygwin, linux, and darwin all have a $HOME directory860 # cygwin, linux, and darwin all have a $HOME directory
846 if base is None:861 if base is None:
847 base = os.path.expanduser("~")862 base = os.environ.get('XDG_CONFIG_HOME', None)
848 return osutils.pathjoin(base, ".bazaar")863 if base is None:
864 base = osutils.pathjoin(os.path.expanduser("~"), ".config")
865 return osutils.pathjoin(base, "bazaar")
866
867
868def _nonxdg_config_dir():
869 # This returns the legacy path of a config directory as it would
870 # have been created before the xdg base directory specification
871 # was implemented.
872 base = os.environ.get('BZR_HOME', None)
873 if base is None:
874 base = os.path.expanduser("~")
875 return osutils.pathjoin(base, ".bazaar")
849876
850877
851def config_filename():878def config_filename():