Code review comment for lp://staging/~jelmer/bzr/cachedir

-----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 trap it, then you need a "ModuleAvailableFeature('xdg')".

Alternatively, the test needs to be updated to handle when xdg is not
actually available.

I'm a bit confused by this code:

+
+def cache_dir():
+ """Return the cache directory to use."""
....
+ try:
+ from xdg.BaseDirectory import xdg_cache_home
+ except ImportError:
+ cache_dir = os.environ.get('XDG_CACHE_DIR', None)
+ else:
+ cache_dir = os.path.join(xdg_cache_home, "bazaar")

^- so *if* python-xdg is available, then we use xdg_cache_home +
"bazaar". But if the environment variable is set, then we use
$XDG_CACHE_DIR without adding .bazaar.

+ if type(cache_dir) == unicode:
+ cache_dir = cache_dir.encode(osutils._fs_enc)
+
+ if cache_dir is None:
+ cache_dir = os.path.expanduser('~/.cache')
+

^- And if neither is set, we use "~/.cache", again *without* bazaar.

+ if not os.path.exists(cache_dir):
+ os.makedirs(cache_dir)

I have the feeling that we should be tacking on "bazaar" in all cases.
But I don't know the XDG_CACHE_DIR spec. Is it supposed to have the
custom value for each application? It seems like it would just set the
root for all caches, and applications should carve out a sub-namespace
for themselves.

 review: needsfixing

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk5I734ACgkQJdeBCYSNAAMeIgCg0rQGRkNR+tEMTAbxU3mSD3FV
29wAnAkOQfyyJEtHdYHWOigOnNzDJ32N
=s7j1
-----END PGP SIGNATURE-----