Bazaar

Merge lp:~mwhudson/bzr/bytestring-environment-variables into lp:bzr

  1. bytestring-environment-variables
  2. Merge into bzr.dev
Proposed by Michael Hudson-Doyle
Status: Merged
Merged at revision: not available
Proposed branch: lp:~mwhudson/bzr/bytestring-environment-variables
Merge into: lp:bzr
Diff against target: 48 lines (+15/-2)
3 files modified
NEWS (+2/-0)
bzrlib/tests/__init__.py (+5/-2)
bzrlib/tests/test_selftest.py (+8/-0)
To merge this branch: bzr merge lp:~mwhudson/bzr/bytestring-environment-variables
Reviewer Review Type Date Requested Status
Martin Pool Approve
Robert Collins (community) Approve
Review via email: mp+15136@code.launchpad.net

This proposal supersedes a proposal from 2009-11-21.

To post a comment you must log in.
Revision history for this message
Michael Hudson-Doyle (mwhudson) wrote : Posted in a previous version of this proposal

As mentioned in the bug report, $BZR_HOME and $HOME are unicode strings in tests now, and this upsets some things (and I think it's just true, good or bad, that environment variables are bytestrings). This is one possible fix; I'm not sure if it's a good or bad one.

Revision history for this message
Robert Collins (lifeless) wrote : Posted in a previous version of this proposal

I think its fine to make the environment strings be fs encoded; however changing in memory objects is a different question.

review: Needs Fixing
Revision history for this message
Michael Hudson-Doyle (mwhudson) wrote :

Agree with Robert that changing only the environment variables makes sense.

Revision history for this message
Robert Collins (lifeless) wrote :

Looks good. A test would be nicer if you have time to add one. A second review is needed too.

I'll be your pilot here.

review: Approve
Revision history for this message
Martin Pool (mbp) wrote :

Please mention the bug in NEWS, and please also mention it in the tests, otherwise people might wonder what the point of the test is.

review: Approve
Revision history for this message
Martin Packman (gz) wrote :

Intended to comment on this at the time, but got lost with the resubmit.

This is a sub-optimal fix because:
1) sys.getfilesystemencoding may return None which will throw an exception on the encode.
2) The environment really is unicode on windows, even though the Python 2.* os.environ goes down the bytestring road.
3) The encoding will be "mbcs" on windows, which is dangerous to use with encode as it's lossy in that direction.

Revision history for this message
Michael Hudson-Doyle (mwhudson) wrote :

I'm not at all sure what a fix to address those concerns would look like, so someone else will have to do it I guess.

Revision history for this message
John A Meinel (jameinel) wrote :

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

Martin [gz] wrote:
> Intended to comment on this at the time, but got lost with the resubmit.
>
> This is a sub-optimal fix because:
> 1) sys.getfilesystemencoding may return None which will throw an exception on the encode.
> 2) The environment really is unicode on windows, even though the Python 2.* os.environ goes down the bytestring road.
> 3) The encoding will be "mbcs" on windows, which is dangerous to use with encode as it's lossy in that direction.

Except... the test suite really doesn't do that crazy of things to
environment variables.

I don't think Python uses the unicode get environment functions anyway.

If this was something that was part of runtime code, then I would worry
more. As it stands, I think Michael's fix is appropriate and sufficient.

John
=:->

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

iEYEARECAAYFAksoEwcACgkQJdeBCYSNAAOI/gCeM875PA84mzGkAxj60kDs/2/z
BMkAoJTUVzhqfN5DzT/Ph0X9rZifJZzA
=OvvI
-----END PGP SIGNATURE-----

Revision history for this message
Martin Packman (gz) wrote :

I mentioned because I have a branch that this change broke, and needed to rewrite it to address 1 and 2 (3 is more a general point and indeed not that relevant in a testing-only scenario).

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'NEWS'
2--- NEWS 2009-11-20 16:42:28 +0000
3+++ NEWS 2009-11-24 02:28:12 +0000
4@@ -60,6 +60,8 @@
5 Testing
6 *******
7
8+* TestCaseWithMemoryTransport no longer sets $HOME and $BZR_HOME to
9+ unicode strings. (Michael Hudson, #464174)
10
11 bzr 2.0.3 (not released yet)
12 ############################
13
14=== modified file 'bzrlib/tests/__init__.py'
15--- bzrlib/tests/__init__.py 2009-11-18 16:10:18 +0000
16+++ bzrlib/tests/__init__.py 2009-11-24 02:28:12 +0000
17@@ -2475,8 +2475,11 @@
18 return branchbuilder.BranchBuilder(branch=branch)
19
20 def overrideEnvironmentForTesting(self):
21- os.environ['HOME'] = self.test_home_dir
22- os.environ['BZR_HOME'] = self.test_home_dir
23+ test_home_dir = self.test_home_dir
24+ if isinstance(test_home_dir, unicode):
25+ test_home_dir = test_home_dir.encode(sys.getfilesystemencoding())
26+ os.environ['HOME'] = test_home_dir
27+ os.environ['BZR_HOME'] = test_home_dir
28
29 def setUp(self):
30 super(TestCaseWithMemoryTransport, self).setUp()
31
32=== modified file 'bzrlib/tests/test_selftest.py'
33--- bzrlib/tests/test_selftest.py 2009-11-08 04:44:23 +0000
34+++ bzrlib/tests/test_selftest.py 2009-11-24 02:28:12 +0000
35@@ -516,6 +516,14 @@
36 cwd = osutils.getcwd()
37 self.assertIsSameRealPath(self.test_dir, cwd)
38
39+ def test_BZR_HOME_and_HOME_are_bytestrings(self):
40+ """The $BZR_HOME and $HOME environment variables should not be unicode.
41+
42+ See https://bugs.launchpad.net/bzr/+bug/464174
43+ """
44+ self.assertIsInstance(os.environ['BZR_HOME'], str)
45+ self.assertIsInstance(os.environ['HOME'], str)
46+
47 def test_make_branch_and_memory_tree(self):
48 """In TestCaseWithMemoryTransport we should not make the branch on disk.
49