git-hyper-blame: Simplify code and tests.

Remove default value from optional arguments, since they are always passed.

In particular, the default value for `out` was a text stream (sys.stdout),
but the value passed to `out` is always a binary stream, which is confusing.
Get rid of the `err` argument, since it is always sys.stderr, and only used
for testing.

Change-Id: Ia289e9a97b968a0c802fc2f419397c1e494f713c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/1986064
Reviewed-by: Anthony Polito <apolito@google.com>
Commit-Queue: Edward Lesmes <ehmaldonado@chromium.org>
Auto-Submit: Edward Lesmes <ehmaldonado@chromium.org>

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

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))

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 = """