diff --git a/git_common.py b/git_common.py index 0fa07e8e1..817f2c30d 100644 --- a/git_common.py +++ b/git_common.py @@ -683,6 +683,8 @@ def less(): # pragma: no cover Automatically checks if sys.stdout is a non-TTY stream. If so, it avoids running less and just yields sys.stdout. + + The returned PIPE is opened on binary mode. """ if not setup_color.IS_TTY: # On Python 3, sys.stdout doesn't accept bytes, and sys.stdout.buffer must diff --git a/git_hyper_blame.py b/git_hyper_blame.py index 44e3f8a03..bd5789a6f 100755 --- a/git_hyper_blame.py +++ b/git_hyper_blame.py @@ -95,12 +95,12 @@ def parse_blame(blameoutput): yield BlameLine(commit, context, lineno_then, lineno_now, False) -def print_table(table, align=None, out=sys.stdout): +def print_table(outbuf, table, align): """Print a 2D rectangular array, aligning columns with spaces. Args: - align: Optional string of 'l' and 'r', designating whether each column is - left- or right-aligned. Defaults to left aligned. + align: string of 'l' and 'r', designating whether each column is left- or + right-aligned. """ if len(table) == 0: return @@ -112,9 +112,6 @@ def print_table(table, align=None, out=sys.stdout): else: colwidths = [max(colwidths[i], len(x)) for i, x in enumerate(row)] - if align is None: # pragma: no cover - align = 'l' * len(colwidths) - for row in table: cells = [] for i, cell in enumerate(row): @@ -124,16 +121,15 @@ def print_table(table, align=None, out=sys.stdout): elif i < len(row) - 1: # Do not pad the final column if left-aligned. cell += padding - cell = cell.encode('utf-8', 'replace') - cells.append(cell) + cells.append(cell.encode('utf-8', 'replace')) try: - out.write(b' '.join(cells) + b'\n') + outbuf.write(b' '.join(cells) + b'\n') except IOError: # pragma: no cover # Can happen on Windows if the pipe is closed early. pass -def pretty_print(parsedblame, show_filenames=False, out=sys.stdout): +def pretty_print(outbuf, parsedblame, show_filenames=False): """Pretty-prints the output of parse_blame.""" table = [] for line in parsedblame: @@ -147,7 +143,7 @@ def pretty_print(parsedblame, show_filenames=False, out=sys.stdout): if show_filenames: row.insert(1, line.commit.filename) table.append(row) - print_table(table, align='llllrl' if show_filenames else 'lllrl', out=out) + print_table(outbuf, table, align='llllrl' if show_filenames else 'lllrl') def get_parsed_blame(filename, revision='HEAD'): @@ -259,8 +255,7 @@ def approx_lineno_across_revs(filename, newfilename, revision, newrevision, return lineno + cumulative_offset -def hyper_blame(ignored, filename, revision='HEAD', out=sys.stdout, - err=sys.stderr): +def hyper_blame(outbuf, ignored, filename, revision): # Map from commit to parsed blame from that commit. blame_from = {} @@ -275,7 +270,7 @@ def hyper_blame(ignored, filename, revision='HEAD', out=sys.stdout, try: parsed = cache_blame_from(filename, git_common.hash_one(revision)) except subprocess2.CalledProcessError as e: - err.write(e.stderr.decode()) + sys.stderr.write(e.stderr.decode()) return e.returncode new_parsed = [] @@ -325,7 +320,7 @@ def hyper_blame(ignored, filename, revision='HEAD', out=sys.stdout, new_parsed.append(line) - pretty_print(new_parsed, show_filenames=show_filenames, out=out) + pretty_print(outbuf, new_parsed, show_filenames=show_filenames) return 0 @@ -337,14 +332,13 @@ def parse_ignore_file(ignore_file): yield line -def main(args, stdout=sys.stdout, stderr=sys.stderr): +def main(args, outbuf): parser = argparse.ArgumentParser( prog='git hyper-blame', description='git blame with support for ignoring certain commits.') parser.add_argument('-i', metavar='REVISION', action='append', dest='ignored', default=[], help='a revision to ignore') - parser.add_argument('--ignore-file', metavar='FILE', - type=argparse.FileType('r'), dest='ignore_file', + parser.add_argument('--ignore-file', metavar='FILE', dest='ignore_file', help='a file containing a list of revisions to ignore') parser.add_argument('--no-default-ignores', dest='no_default_ignores', action='store_true', @@ -357,7 +351,7 @@ def main(args, stdout=sys.stdout, stderr=sys.stderr): try: repo_root = git_common.repo_root() except subprocess2.CalledProcessError as e: - stderr.write(e.stderr.decode()) + sys.stderr.write(e.stderr.decode()) return e.returncode # Make filename relative to the repository root, and cd to the root dir (so @@ -375,7 +369,8 @@ def main(args, stdout=sys.stdout, stderr=sys.stderr): ignored_list.extend(parse_ignore_file(ignore_file)) if args.ignore_file: - ignored_list.extend(parse_ignore_file(args.ignore_file)) + with open(args.ignore_file) as ignore_file: + ignored_list.extend(parse_ignore_file(ignore_file)) ignored = set() for c in ignored_list: @@ -383,12 +378,12 @@ def main(args, stdout=sys.stdout, stderr=sys.stderr): ignored.add(git_common.hash_one(c)) except subprocess2.CalledProcessError as e: # Custom warning string (the message from git-rev-parse is inappropriate). - stderr.write('warning: unknown revision \'%s\'.\n' % c) + sys.stderr.write('warning: unknown revision \'%s\'.\n' % c) - return hyper_blame(ignored, filename, args.revision, out=stdout, err=stderr) + return hyper_blame(outbuf, ignored, filename, args.revision) if __name__ == '__main__': # pragma: no cover setup_color.init() with git_common.less() as less_input: - sys.exit(main(sys.argv[1:], stdout=less_input)) + sys.exit(main(sys.argv[1:], less_input)) diff --git a/tests/git_hyper_blame_test.py b/tests/git_hyper_blame_test.py index 695ba6c97..80843ed80 100755 --- a/tests/git_hyper_blame_test.py +++ b/tests/git_hyper_blame_test.py @@ -17,8 +17,10 @@ import unittest from io import BytesIO if sys.version_info.major == 2: from StringIO import StringIO + import mock else: from io import StringIO + from unittest import mock DEPOT_TOOLS_ROOT = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) sys.path.insert(0, DEPOT_TOOLS_ROOT) @@ -38,13 +40,16 @@ class GitHyperBlameTestBase(git_test_utils.GitRepoReadOnlyTestBase): import git_hyper_blame cls.git_hyper_blame = git_hyper_blame + def setUp(self): + mock.patch('sys.stderr', StringIO()).start() + self.addCleanup(mock.patch.stopall) + def run_hyperblame(self, ignored, filename, revision): - stdout = BytesIO() - stderr = StringIO() + outbuf = BytesIO() ignored = [self.repo[c] for c in ignored] - retval = self.repo.run(self.git_hyper_blame.hyper_blame, ignored, filename, - revision=revision, out=stdout, err=stderr) - return retval, stdout.getvalue().rstrip().split(b'\n') + retval = self.repo.run( + self.git_hyper_blame.hyper_blame, outbuf, ignored, filename, revision) + return retval, outbuf.getvalue().rstrip().split(b'\n') def blame_line(self, commit_name, rest, author=None, filename=None): """Generate a blame line from a commit. @@ -96,27 +101,24 @@ class GitHyperBlameMainTest(GitHyperBlameTestBase): """Tests the main function (simple end-to-end test with no ignores).""" expected_output = [self.blame_line('C', '1) line 1.1'), self.blame_line('B', '2) line 2.1')] - stdout = BytesIO() - stderr = StringIO() - retval = self.repo.run(self.git_hyper_blame.main, - args=['tag_C', 'some/files/file'], stdout=stdout, - stderr=stderr) + outbuf = BytesIO() + retval = self.repo.run( + self.git_hyper_blame.main, ['tag_C', 'some/files/file'], outbuf) self.assertEqual(0, retval) - self.assertEqual(expected_output, stdout.getvalue().rstrip().split(b'\n')) - self.assertEqual('', stderr.getvalue()) + self.assertEqual(expected_output, outbuf.getvalue().rstrip().split(b'\n')) + self.assertEqual('', sys.stderr.getvalue()) def testIgnoreSimple(self): """Tests the main function (simple end-to-end test with ignores).""" expected_output = [self.blame_line('C', ' 1) line 1.1'), self.blame_line('A', '2*) line 2.1')] - stdout = BytesIO() - stderr = StringIO() - retval = self.repo.run(self.git_hyper_blame.main, - args=['-i', 'tag_B', 'tag_C', 'some/files/file'], - stdout=stdout, stderr=stderr) + outbuf = BytesIO() + retval = self.repo.run( + self.git_hyper_blame.main, ['-i', 'tag_B', 'tag_C', 'some/files/file'], + outbuf) self.assertEqual(0, retval) - self.assertEqual(expected_output, stdout.getvalue().rstrip().split(b'\n')) - self.assertEqual('', stderr.getvalue()) + self.assertEqual(expected_output, outbuf.getvalue().rstrip().split(b'\n')) + self.assertEqual('', sys.stderr.getvalue()) def testBadRepo(self): """Tests the main function (not in a repo).""" @@ -125,46 +127,43 @@ class GitHyperBlameMainTest(GitHyperBlameTestBase): tempdir = tempfile.mkdtemp(suffix='_nogit', prefix='git_repo') try: os.chdir(tempdir) - stdout = BytesIO() - stderr = StringIO() + outbuf = BytesIO() retval = self.git_hyper_blame.main( - args=['-i', 'tag_B', 'tag_C', 'some/files/file'], stdout=stdout, - stderr=stderr) + ['-i', 'tag_B', 'tag_C', 'some/files/file'], outbuf) finally: shutil.rmtree(tempdir) os.chdir(curdir) self.assertNotEqual(0, retval) - self.assertEqual(b'', stdout.getvalue()) + self.assertEqual(b'', outbuf.getvalue()) r = re.compile('^fatal: Not a git repository', re.I) - self.assertRegexpMatches(stderr.getvalue(), r) + self.assertRegexpMatches(sys.stderr.getvalue(), r) def testBadFilename(self): """Tests the main function (bad filename).""" - stdout = BytesIO() - stderr = StringIO() - retval = self.repo.run(self.git_hyper_blame.main, - args=['-i', 'tag_B', 'tag_C', 'some/files/xxxx'], - stdout=stdout, stderr=stderr) + outbuf = BytesIO() + retval = self.repo.run( + self.git_hyper_blame.main, ['-i', 'tag_B', 'tag_C', 'some/files/xxxx'], + outbuf) self.assertNotEqual(0, retval) - self.assertEqual(b'', stdout.getvalue()) + self.assertEqual(b'', outbuf.getvalue()) # TODO(mgiuca): This test used to test the exact string, but it broke due to # an upstream bug in git-blame. For now, just check the start of the string. # A patch has been sent upstream; when it rolls out we can revert back to # the original test logic. self.assertTrue( - stderr.getvalue().startswith('fatal: no such path some/files/xxxx in ')) + sys.stderr.getvalue().startswith( + 'fatal: no such path some/files/xxxx in ')) def testBadRevision(self): """Tests the main function (bad revision to blame from).""" - stdout = BytesIO() - stderr = StringIO() - retval = self.repo.run(self.git_hyper_blame.main, - args=['-i', 'tag_B', 'xxxx', 'some/files/file'], - stdout=stdout, stderr=stderr) + outbuf = BytesIO() + retval = self.repo.run( + self.git_hyper_blame.main, ['-i', 'tag_B', 'xxxx', 'some/files/file'], + outbuf) self.assertNotEqual(0, retval) - self.assertEqual(b'', stdout.getvalue()) - self.assertRegexpMatches(stderr.getvalue(), + self.assertEqual(b'', outbuf.getvalue()) + self.assertRegexpMatches(sys.stderr.getvalue(), '^fatal: ambiguous argument \'xxxx\': unknown ' 'revision or path not in the working tree.') @@ -172,21 +171,20 @@ class GitHyperBlameMainTest(GitHyperBlameTestBase): """Tests the main function (bad revision passed to -i).""" expected_output = [self.blame_line('C', '1) line 1.1'), self.blame_line('B', '2) line 2.1')] - stdout = BytesIO() - stderr = StringIO() - retval = self.repo.run(self.git_hyper_blame.main, - args=['-i', 'xxxx', 'tag_C', 'some/files/file'], - stdout=stdout, stderr=stderr) + outbuf = BytesIO() + retval = self.repo.run( + self.git_hyper_blame.main, ['-i', 'xxxx', 'tag_C', 'some/files/file'], + outbuf) self.assertEqual(0, retval) - self.assertEqual(expected_output, stdout.getvalue().rstrip().split(b'\n')) - self.assertEqual('warning: unknown revision \'xxxx\'.\n', stderr.getvalue()) + self.assertEqual(expected_output, outbuf.getvalue().rstrip().split(b'\n')) + self.assertEqual( + 'warning: unknown revision \'xxxx\'.\n', sys.stderr.getvalue()) def testIgnoreFile(self): """Tests passing the ignore list in a file.""" expected_output = [self.blame_line('C', ' 1) line 1.1'), self.blame_line('A', '2*) line 2.1')] - stdout = BytesIO() - stderr = StringIO() + outbuf = BytesIO() with tempfile.NamedTemporaryFile(mode='w+', prefix='ignore') as ignore_file: ignore_file.write('# Line comments are allowed.\n') @@ -195,14 +193,15 @@ class GitHyperBlameMainTest(GitHyperBlameTestBase): # A revision that is not in the repo (should be ignored). ignore_file.write('xxxx\n') ignore_file.flush() - retval = self.repo.run(self.git_hyper_blame.main, - args=['--ignore-file', ignore_file.name, 'tag_C', - 'some/files/file'], - stdout=stdout, stderr=stderr) + retval = self.repo.run( + self.git_hyper_blame.main, + ['--ignore-file', ignore_file.name, 'tag_C', 'some/files/file'], + outbuf) self.assertEqual(0, retval) - self.assertEqual(expected_output, stdout.getvalue().rstrip().split(b'\n')) - self.assertEqual('warning: unknown revision \'xxxx\'.\n', stderr.getvalue()) + self.assertEqual(expected_output, outbuf.getvalue().rstrip().split(b'\n')) + self.assertEqual( + 'warning: unknown revision \'xxxx\'.\n', sys.stderr.getvalue()) def testDefaultIgnoreFile(self): """Tests automatically using a default ignore list.""" @@ -212,30 +211,26 @@ class GitHyperBlameMainTest(GitHyperBlameTestBase): expected_output = [self.blame_line('A', '1*) line 1.1'), self.blame_line('B', ' 2) line 2.1')] - stdout = BytesIO() - stderr = StringIO() + outbuf = BytesIO() - retval = self.repo.run(self.git_hyper_blame.main, - args=['tag_D', 'some/files/file'], - stdout=stdout, stderr=stderr) + retval = self.repo.run( + self.git_hyper_blame.main, ['tag_D', 'some/files/file'], outbuf) self.assertEqual(0, retval) - self.assertEqual(expected_output, stdout.getvalue().rstrip().split(b'\n')) - self.assertEqual('', stderr.getvalue()) + self.assertEqual(expected_output, outbuf.getvalue().rstrip().split(b'\n')) + self.assertEqual('', sys.stderr.getvalue()) # Test blame from a different revision. Despite the default ignore file # *not* being committed at that revision, it should still be picked up # because D is currently checked out. - stdout = BytesIO() - stderr = StringIO() + outbuf = BytesIO() - retval = self.repo.run(self.git_hyper_blame.main, - args=['tag_C', 'some/files/file'], - stdout=stdout, stderr=stderr) + retval = self.repo.run( + self.git_hyper_blame.main, ['tag_C', 'some/files/file'], outbuf) self.assertEqual(0, retval) - self.assertEqual(expected_output, stdout.getvalue().rstrip().split(b'\n')) - self.assertEqual('', stderr.getvalue()) + self.assertEqual(expected_output, outbuf.getvalue().rstrip().split(b'\n')) + self.assertEqual('', sys.stderr.getvalue()) def testNoDefaultIgnores(self): """Tests the --no-default-ignores switch.""" @@ -245,17 +240,15 @@ class GitHyperBlameMainTest(GitHyperBlameTestBase): expected_output = [self.blame_line('C', '1) line 1.1'), self.blame_line('B', '2) line 2.1')] - stdout = BytesIO() - stderr = StringIO() + outbuf = BytesIO() retval = self.repo.run( self.git_hyper_blame.main, - args=['tag_D', 'some/files/file', '--no-default-ignores'], - stdout=stdout, stderr=stderr) + ['tag_D', 'some/files/file', '--no-default-ignores'], outbuf) self.assertEqual(0, retval) - self.assertEqual(expected_output, stdout.getvalue().rstrip().split(b'\n')) - self.assertEqual('', stderr.getvalue()) + self.assertEqual(expected_output, outbuf.getvalue().rstrip().split(b'\n')) + self.assertEqual('', sys.stderr.getvalue()) class GitHyperBlameSimpleTest(GitHyperBlameTestBase): REPO_SCHEMA = """