Merge lp://staging/~jelmer/bzr/cachedir into lp://staging/bzr

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 8/14/2011 5:45 PM, Jelmer Vernooij wrote:
> Jelmer Vernooij has proposed merging lp:~jelmer/bzr/cachedir into
> lp:bzr.
>
> Requested reviews: bzr-core (bzr-core)
>
> For more details, see:
> https://code.launchpad.net/~jelmer/bzr/cachedir/+merge/71486
>
> Add a cache_dir() function in bzrlib.config.
>
> The foreign branch plugins and bzr-search currently all duplicate
> this code in one form or another, so it would be nice to have it in
> bzrlib.
>
> There are a few things I'm not certain about:
>
> * using xdg.BaseDirectory versus just environment variables for the
> xdg stuff. The environment variables don't always seem to be set,
> even on xdg systems like Ubuntu, but does that justify another
> import?
>
> * Nothing in bzr relies on this during tests at the moment, but
> perhaps we'd like to override cache_dir during tests on Windows to
> avoid cluttering the users' actual cache directory.

I don't think we want a try/except ImportError in the middle of live
code. Because then the import gets tried on every call, and fails each
time. Can we pull it out somewhere? Possibly just a function call that
caches negative results.

+
+def cache_dir():
+ """Return the cache directory to use."""
+ if sys.platform in ("nt", "win32"):

sys.platform should always be "win32" I think it is os.name that is 'nt'.

And if we get a 'str' out of Windows, then it should be decoded with
"mbcs" not 'bzrlib.user_encoding'. user_encoding is deprecated anyway,
vs using "osutils.get_user_encoding()" but that is terminal encoding,
not filesystem encoding or file content encoding.

xdg_cache_dir seems to be a public function, so I'm not sure why you're
just removing it. (I really don't know who has been calling it.)

+ if cache_dir is None:
+ cache_dir = os.path.expanduser('~/.cache')

Shouldn't this somehow be a bzr specific cache directory?

+class TestXDGCacheDir(tests.TestCaseInTempDir):

You are using just "cache_dir()" as the function name, so the test class
should match.

+ def setUp(self):
+ if sys.platform in ('darwin', 'win32'):
+ raise tests.TestNotApplicable(
+ 'XDG cache dir not used on this platform')
+ super(TestXDGCacheDir, self).setUp()
+ self.overrideEnv('HOME', self.test_home_dir)
+ # BZR_HOME overrides everything we want to test so unset it.
+ self.overrideEnv('BZR_HOME', None)
+
+ def test_xdg_cache_dir_exists(self):
+ """When ~/.cache/bazaar exists, use it as the config dir."""
+ cachedir = osutils.pathjoin(self.test_home_dir, '.cache')
+ newdir = osutils.pathjoin(cachedir, 'bazaar')
+ try:
+ from xdg import BaseDirectory
+ except ImportError:
+ pass
+ else:
+ self.overrideAttr(BaseDirectory, "xdg_cache_home", cachedir)
+ os.makedirs(newdir)
+ self.assertEqual(config.cache_dir(), newdir)

I don't see how this test could pass if 'xdg' wasn't present, since you
don't set the cache location any other way. As such, it would seem that
the try/except in this location function is superfluous. And if you want
to t...

This looks like a good improvement overall, just are a few niggles in addition to the ones John has mentioned. It feels like cache_dir would be better suited to osutils than config, and possibly split into per-platform functions as there's not much sharing.

+ assert s is not None

There's a test in bt.test_source that fails if assert statements are used.

+ s = get_local_appdata_location()
...
+ if type(s) == str:

So, the bad api here isn't your fault, but adding type checks like this is the wrong way around it. The function needs fixing the return a consistent type.

+ cache_dir = s.encode(osutils._fs_enc)

This likely harmless in this specific case, but there's no guarantee a unicode path will be encodable to the filesystem encoding. Generally bzrlib treats paths as unicode, which is correct for windows and mac filesystems, having this api try and fit the path in a bytestring for the benefit of nix seems inconsistent.

+ if sys.platform in ("nt", "win32"):
...
+ else:

OSX is taking the XDG path on the `else:` here, which according to your tests is incorrect.

+ if type(cache_dir) == unicode:

Looks like an unreachable branch to me, all of the nix code deals in bytes only on Python 2.

@jelmer: ping

Are you still working on this ?

Yeah, I just need to find some time to finish it.

I'll move this to work in progress for now, but will try and unblock you by cleaning up the winutils side of things.