diff --git a/reviewboard/diffviewer/tests.py b/reviewboard/diffviewer/tests.py index 1b86a9f831c94695ecf7c42425f38bbba6903ff3..042fe9c4a62ef470306ed2767ffd5634d813b9fd 100644 --- a/reviewboard/diffviewer/tests.py +++ b/reviewboard/diffviewer/tests.py @@ -454,10 +454,9 @@ class DiffParserTest(TestCase): self.assertEqual(files[0].delete_count, 4) def _get_file(self, *relative): - f = open(os.path.join(*tuple([self.PREFIX] + list(relative)))) - data = f.read() - f.close() - return data + path = os.path.join(*tuple([self.PREFIX] + list(relative))) + with open(path, 'rb') as f: + return f.read() def _test_move_detection(self, a, b, expected_i_moves, expected_r_moves): differ = MyersDiffer(a, b) diff --git a/reviewboard/hostingsvcs/tests.py b/reviewboard/hostingsvcs/tests.py index eb2d33e7d9b4f4f4def3248f423f1a67fe19ef2c..68735a49fddfa60289a29e7bc27ee2cb27b93882 100644 --- a/reviewboard/hostingsvcs/tests.py +++ b/reviewboard/hostingsvcs/tests.py @@ -234,7 +234,7 @@ class BeanstalkTests(ServiceTests): 'https://mydomain.beanstalkapp.com/api/repositories/' 'myrepo/blob?id=%s&name=path' % expected_revision) - return 'My data', {} + return b'My data', {} account = self._get_hosting_account() service = account.service @@ -252,6 +252,7 @@ class BeanstalkTests(ServiceTests): result = service.get_file(repository, '/path', revision, base_commit_id) self.assertTrue(service._http_get.called) + self.assertTrue(isinstance(result, bytes)) self.assertEqual(result, 'My data') def _test_get_file_exists(self, tool_name, revision, base_commit_id, diff --git a/reviewboard/scmtools/bzr.py b/reviewboard/scmtools/bzr.py index 7f23132758f89913578300fc8071c0a0b984ad28..8b371c7b5b752b9451deb0ccf3c0d5cf3d675298 100644 --- a/reviewboard/scmtools/bzr.py +++ b/reviewboard/scmtools/bzr.py @@ -118,9 +118,13 @@ class BZRTool(SCMTool): revtree = revisionspec.RevisionSpec.from_string(revspec).as_tree(branch) fileid = revtree.path2id(relpath) if fileid: - contents = revtree.get_file_text(fileid) + # XXX: get_file_text returns str, which isn't Python 3 + # safe. According to the internet they have no immediate + # plans to port to 3, so we may find it hard to support + # that combination. + contents = bytes(revtree.get_file_text(fileid)) else: - contents = "" + contents = b'' except BzrError as e: raise SCMError(e) finally: diff --git a/reviewboard/scmtools/clearcase.py b/reviewboard/scmtools/clearcase.py index 5f99df7cfdea0553e015e32acc08de4bd0436858..3e2edb1d2662cfe458876b7fc27b8dcd1cf85567 100644 --- a/reviewboard/scmtools/clearcase.py +++ b/reviewboard/scmtools/clearcase.py @@ -356,10 +356,8 @@ class ClearCaseDynamicViewClient(object): self.path = path def cat_file(self, filename, revision): - f = open(filename, 'r') - lines = f.readlines() - f.close() - return ''.join(lines) + with open(filename, 'rb') as f: + return f.read() def list_dir(self, path, revision): return ''.join([ @@ -393,9 +391,7 @@ class ClearCaseSnapshotViewClient(object): raise FileNotFoundError(extended_path, revision) try: - fp = open(temp.name, 'r') - data = fp.read() - fp.close() - return data + with open(temp.name, 'rb') as f: + return f.read() except: raise FileNotFoundError(extended_path, revision) diff --git a/reviewboard/scmtools/cvs.py b/reviewboard/scmtools/cvs.py index 4c32a84f97abe2f8273d9c528e33fbdcc62e7bea..4685fc789f49e76614f3550106c6e2893a80eaa1 100644 --- a/reviewboard/scmtools/cvs.py +++ b/reviewboard/scmtools/cvs.py @@ -284,7 +284,7 @@ class CVSClient(object): '-r', str(revision), '-p', filename], self.local_site_name) contents = p.stdout.read() - errmsg = p.stderr.read() + errmsg = unicode(p.stderr.read()) failure = p.wait() # Unfortunately, CVS is not consistent about exiting non-zero on @@ -304,9 +304,9 @@ class CVSClient(object): # So, if nothing is in errmsg, or errmsg has a specific recognized # message, call it FileNotFound. - if not errmsg or \ - errmsg.startswith('cvs checkout: cannot find module') or \ - errmsg.startswith('cvs checkout: could not read RCS file'): + if (not errmsg or + errmsg.startswith(u'cvs checkout: cannot find module') or + errmsg.startswith(u'cvs checkout: could not read RCS file')): self.cleanup() raise FileNotFoundError(filename, revision) @@ -315,8 +315,8 @@ class CVSClient(object): # # If the .cvspass file doesn't exist, CVS will return an error message # stating this. This is safe to ignore. - if (failure and not errmsg.startswith('==========')) and \ - not ".cvspass does not exist - creating new file" in errmsg: + if ((failure and not errmsg.startswith(u'==========')) and + not u'.cvspass does not exist - creating new file' in errmsg): self.cleanup() raise SCMError(errmsg) diff --git a/reviewboard/scmtools/git.py b/reviewboard/scmtools/git.py index a5edd9173686423cd16844601d98689ae6defa44..66eb87ccadccd45b320d2773bf9514067dcc4353 100644 --- a/reviewboard/scmtools/git.py +++ b/reviewboard/scmtools/git.py @@ -439,11 +439,11 @@ class GitClient(SCMClient): p = self._run_git(['--git-dir=%s' % self.git_dir, 'cat-file', option, commit]) contents = p.stdout.read() - errmsg = p.stderr.read() + errmsg = unicode(p.stderr.read()) failure = p.wait() if failure: - if errmsg.startswith("fatal: Not a valid object name"): + if errmsg.startswith(u"fatal: Not a valid object name"): raise FileNotFoundError(commit) else: raise SCMError(errmsg) diff --git a/reviewboard/scmtools/localfile.py b/reviewboard/scmtools/localfile.py index 9deb0b753bd800d760078fda87d4b5ae0b70dc77..900223c14dd7816bc3c2de25fdbdc46acf0490c4 100644 --- a/reviewboard/scmtools/localfile.py +++ b/reviewboard/scmtools/localfile.py @@ -17,10 +17,8 @@ class LocalFileTool(SCMTool): raise FileNotFoundError(path, revision) try: - fp = open(self.repopath + '/' + path, 'r') - data = fp.read() - fp.close() - return data + with open(self.repopath + '/' + path, 'rb') as f: + return f.read() except IOError as e: raise FileNotFoundError(path, revision, detail=str(e)) diff --git a/reviewboard/scmtools/mtn.py b/reviewboard/scmtools/mtn.py index fa39a024ae909b4c6d15448e20c689c8093321a4..c59fdbda85af57f2cb24757168bbc7545827863f 100644 --- a/reviewboard/scmtools/mtn.py +++ b/reviewboard/scmtools/mtn.py @@ -26,7 +26,7 @@ class MonotoneTool(SCMTool): def get_file(self, path, revision=None): # revision is actually the file id here... if not revision: - return "" + return b"" return self.client.get_file(revision) @@ -91,13 +91,13 @@ class MonotoneClient: close_fds=(os.name != 'nt')) out = p.stdout.read() - err = p.stderr.read() + err = unicode(p.stderr.read()) failure = p.wait() if not failure: return out - if "mtn: misuse: no file" in err: + if u"mtn: misuse: no file" in err: raise FileNotFoundError(fileid) else: raise SCMError(err) diff --git a/reviewboard/scmtools/plastic.py b/reviewboard/scmtools/plastic.py index c3645610bed38927302a36f7a047afa3a78a9315..b495cfe6e03039087460c2206e9c1ea137a2bc0a 100644 --- a/reviewboard/scmtools/plastic.py +++ b/reviewboard/scmtools/plastic.py @@ -91,11 +91,11 @@ class PlasticTool(SCMTool): logging.debug('Plastic: get_file %s revision %s' % (path, revision)) if revision == PRE_CREATION: - return '' + return b'' # Check for new files if revision == self.UNKNOWN_REV: - return '' + return b'' return self.client.get_file(path, revision) @@ -236,7 +236,7 @@ class PlasticClient(object): ['cm', 'cat', revision + '@' + repo, '--file=' + tmpfile], stderr=subprocess.PIPE, stdout=subprocess.PIPE, close_fds=(os.name != 'nt')) - errmsg = p.stderr.read() + errmsg = unicode(p.stderr.read()) failure = p.wait() if failure: @@ -245,7 +245,7 @@ class PlasticClient(object): raise SCMError(errmsg) - readtmp = open(tmpfile) + readtmp = open(tmpfile, 'rb') contents = readtmp.read() readtmp.close() os.unlink(tmpfile) diff --git a/reviewboard/scmtools/tests.py b/reviewboard/scmtools/tests.py index b80b6ad7f2136e95542c4c397074a0b381cb2d3e..518e2881f1a6958df2793b28ed70eecb3d294d21 100644 --- a/reviewboard/scmtools/tests.py +++ b/reviewboard/scmtools/tests.py @@ -174,7 +174,7 @@ class RepositoryTests(DjangoTestCase): """Testing Repository.get_file caches result""" def get_file(self, path, revision): num_calls['get_file'] += 1 - return 'file data' + return b'file data' num_calls = { 'get_file': 0, @@ -411,11 +411,13 @@ class CVSTests(SCMTestCase): def test_get_file(self): """Testing CVSTool.get_file""" expected = "test content\n" - file = 'test/testfile' + file = b'test/testfile' rev = Revision('1.1') badrev = Revision('2.1') - self.assertEqual(self.tool.get_file(file, rev), expected) + value = self.tool.get_file(file, rev) + self.assertTrue(isinstance(value, bytes)) + self.assertEqual(value, expected) self.assertEqual(self.tool.get_file(file + ",v", rev), expected) self.assertEqual(self.tool.get_file(self.tool.repopath + '/' + file + ",v", rev), expected) @@ -621,18 +623,20 @@ class SubversionTests(SCMTestCase): def test_get_file(self): """Testing SVNTool.get_file""" - expected = ('include ../tools/Makefile.base-vars\n' - 'NAME = misc-docs\n' - 'OUTNAME = svn-misc-docs\n' - 'INSTALL_DIR = $(DESTDIR)/usr/share/doc/subversion\n' - 'include ../tools/Makefile.base-rules\n') + expected = (b'include ../tools/Makefile.base-vars\n' + b'NAME = misc-docs\n' + b'OUTNAME = svn-misc-docs\n' + b'INSTALL_DIR = $(DESTDIR)/usr/share/doc/subversion\n' + b'include ../tools/Makefile.base-rules\n') # There are 3 versions of this test in order to get 100% coverage of # the svn module. rev = Revision('2') file = 'trunk/doc/misc-docs/Makefile' - self.assertEqual(self.tool.get_file(file, rev), expected) + value = self.tool.get_file(file, rev) + self.assertTrue(isinstance(value, bytes)) + self.assertEqual(value, expected) self.assertEqual(self.tool.get_file('/' + file, rev), expected) @@ -948,7 +952,7 @@ class PerforceTests(SCMTestCase): def test_get_file(self): """Testing PerforceTool.get_file""" file = self.tool.get_file('//depot/foo', PRE_CREATION) - self.assertEqual(file, '') + self.assertEqual(file, b'') file = self.tool.get_file('//public/perforce/api/python/P4Client/p4.py', 1) self.assertEqual(md5(file).hexdigest(), @@ -1368,7 +1372,9 @@ class MercurialTests(SCMTestCase): rev = Revision('661e5dd3c493') file = 'doc/readme' - self.assertEqual(self.tool.get_file(file, rev), 'Hello\n\ngoodbye\n') + value = self.tool.get_file(file, rev) + self.assertTrue(isinstance(value, bytes)) + self.assertEqual(value, b'Hello\n\ngoodbye\n') self.assertTrue(self.tool.file_exists('doc/readme')) self.assertTrue(not self.tool.file_exists('doc/readme2')) @@ -1393,7 +1399,7 @@ class MercurialTests(SCMTestCase): @online_only def test_https_repo(self): - """Testing HgTool.get_file with an HTTPS-based repository""" + """Testing HgTool.file_exists with an HTTPS-based repository""" repo = Repository(name='Test HG2', path='https://bitbucket.org/pypy/pypy', tool=Tool.objects.get(name='Mercurial')) @@ -1821,11 +1827,13 @@ class GitTests(SCMTestCase): def test_get_file(self): """Testing GitTool.get_file""" - self.assertEqual(self.tool.get_file("readme", PRE_CREATION), '') - self.assertEqual(self.tool.get_file("readme", "e965047"), 'Hello\n') - self.assertEqual(self.tool.get_file("readme", "d6613f5"), 'Hello there\n') + self.assertEqual(self.tool.get_file("readme", PRE_CREATION), b'') + self.assertTrue( + isinstance(self.tool.get_file("readme", "e965047"), bytes)) + self.assertEqual(self.tool.get_file("readme", "e965047"), b'Hello\n') + self.assertEqual(self.tool.get_file("readme", "d6613f5"), b'Hello there\n') - self.assertEqual(self.tool.get_file("readme"), 'Hello there\n') + self.assertEqual(self.tool.get_file("readme"), b'Hello there\n') self.assertRaises(SCMError, lambda: self.tool.get_file(""))