git hyper-blame: Added automatically ignoring revs from a file.

Added --ignore-file argument, so you can specify ignored commits in a
file rather than as raw command-line arguments. Also, automatically
searches for a file called .git-blame-ignore-revs, which is
automatically used as an ignore list by default.

Also, specifying an unknown revision (either on the command line or in a
file) now generates a warning, not an error.

Notes on some decisions:
- The file is called .git-blame-ignore-revs (not mentioning hyper-blame)
  because we may use the same list in tools other than hyper-blame in
  the future.
- We look at the *currently checked out* version of
  .git-blame-ignore-revs (not the version at the specified revision) for
  consistency with .git-ignore. Because we only expect revisions to be
  added (not deleted), it should be fine to use an ignore list from a
  newer version than the revision being blamed.
- We considered using git notes for the ignore list so that you could
  add a revision to the ignore list without needing a follow-up CL.
  However, there are some problems with this approach. git notes is not
  automatically synced with git clone/pull. Also the Chromium infra
  tools (Reitveld, CQ) are not set up to allow modification of git
  notes, nor are changes to git notes subject to OWNERS checks. Using a
  regular file ensures all users synced to a particular revision are
  using the same ignore list.

BUG=574290

Review URL: https://codereview.chromium.org/1697423004

git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/depot_tools@298897 0039d316-1c4b-4281-b951-d872f2087c98

git_hyper_blame.py

@@ -22,6 +22,9 @@ import git_dates
 logging.getLogger().setLevel(logging.INFO)
 
 
+DEFAULT_IGNORE_FILE_NAME = '.git-blame-ignore-revs'
+
+
 class Commit(object):
   """Info about a commit."""
   def __init__(self, commithash):
@@ -323,12 +326,25 @@ def hyper_blame(ignored, filename, revision='HEAD', out=sys.stdout,
 
   return 0
 
+
+def parse_ignore_file(ignore_file):
+  for line in ignore_file:
+    line = line.split('#', 1)[0].strip()
+    if line:
+      yield line
+
+
 def main(args, stdout=sys.stdout, stderr=sys.stderr):
   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',
+                      help='a file containing a list of revisions to ignore')
+  parser.add_argument('--no-default-ignores', dest='no_default_ignores',
+                      help='Do not ignore commits from .git-blame-ignore-revs.')
   parser.add_argument('revision', nargs='?', default='HEAD', metavar='REVISION',
                       help='revision to look at')
   parser.add_argument('filename', metavar='FILE', help='filename to blame')
@@ -349,14 +365,21 @@ def main(args, stdout=sys.stdout, stderr=sys.stderr):
   filename = os.path.normpath(filename)
   filename = os.path.normcase(filename)
 
+  ignored_list = list(args.ignored)
+  if not args.no_default_ignores and os.path.exists(DEFAULT_IGNORE_FILE_NAME):
+    with open(DEFAULT_IGNORE_FILE_NAME) as ignore_file:
+      ignored_list.extend(parse_ignore_file(ignore_file))
+
+  if args.ignore_file:
+    ignored_list.extend(parse_ignore_file(args.ignore_file))
+
   ignored = set()
-  for c in args.ignored:
+  for c in ignored_list:
     try:
       ignored.add(git_common.hash_one(c))
     except subprocess2.CalledProcessError as e:
-      # Custom error message (the message from git-rev-parse is inappropriate).
+      # Custom warning string (the message from git-rev-parse is inappropriate).
-      stderr.write('fatal: unknown revision \'%s\'.\n' % c)
+      stderr.write('warning: unknown revision \'%s\'.\n' % c)
-      return e.returncode
 
   return hyper_blame(ignored, filename, args.revision, out=stdout, err=stderr)
 

man/html/git-hyper-blame.html

@@ -755,7 +755,8 @@ git-hyper-blame(1) Manual Page
 <h2 id="_synopsis">SYNOPSIS</h2>
 <div class="sectionbody">
 <div class="verseblock">
-<pre class="content"><em>git hyper-blame</em> [-i &lt;rev&gt; [-i &lt;rev&gt; &#8230;]] [&lt;rev&gt;] [--] &lt;file&gt;</pre>
+<pre class="content"><em>git hyper-blame</em> [-i &lt;rev&gt; [-i &lt;rev&gt; &#8230;]] [--ignore-file=&lt;file&gt;]
+                [--no-default-ignores] [&lt;rev&gt;] [--] &lt;file&gt;</pre>
 <div class="attribution">
 </div></div>
 </div>
@@ -773,6 +774,9 @@ touched a given line.</p></div>
 <div class="paragraph"><p>Follows the normal <code>blame</code> syntax: annotates <code>&lt;file&gt;</code> with the revision that
 last modified each line. Optional <code>&lt;rev&gt;</code> specifies the revision of <code>&lt;file&gt;</code> to
 start from.</p></div>
+<div class="paragraph"><p>Automatically looks for a file called <code>.git-blame-ignore-revs</code> in the repository
+root directory. This file has the same syntax as the <code>--ignore-file</code> argument,
+and any commits mentioned in this file are added to the ignore list.</p></div>
 </div>
 </div>
 <div class="sect1">
@@ -787,6 +791,23 @@ start from.</p></div>
   A revision to ignore. Can be specified as many times as needed.
 </p>
 </dd>
+<dt class="hdlist1">
+--ignore-file=&lt;file&gt;
+</dt>
+<dd>
+<p>
+  A file containing a list of revisions to ignore. Can have comments beginning
+  with <code>#</code>.
+</p>
+</dd>
+<dt class="hdlist1">
+--no-default-ignores
+</dt>
+<dd>
+<p>
+  Do not ignore commits from the <code>.git-blame-ignore-revs</code> file.
+</p>
+</dd>
 </dl></div>
 </div>
 </div>
@@ -833,25 +854,6 @@ other more invasive changes.</p></div>
 </div>
 </div>
 <div class="sect1">
-<h2 id="_bugs">BUGS</h2>
-<div class="sectionbody">
-<div class="ulist"><ul>
-<li>
-<p>
-There is currently no way to pass the ignore list as a file.
-</p>
-</li>
-<li>
-<p>
-It should be possible for a git repository to configure an automatic list of
-  commits to ignore (like <code>.gitignore</code>), so that project owners can maintain a
-  list of "big change" commits that are ignored by hyper-blame by default.
-</p>
-</li>
-</ul></div>
-</div>
-</div>
-<div class="sect1">
 <h2 id="_see_also">SEE ALSO</h2>
 <div class="sectionbody">
 <div class="paragraph"><p><a href="git-blame.html">git-blame(1)</a></p></div>
@@ -869,7 +871,7 @@ from <a href="https://chromium.googlesource.com/chromium/tools/depot_tools.git">
 <div id="footnotes"><hr /></div>
 <div id="footer">
 <div id="footer-text">
-Last updated 2016-02-05 13:43:52 AEDT
+Last updated 2016-02-19 15:04:46 AEDT
 </div>
 </div>
 </body>

man/man1/git-hyper-blame.1

@@ -2,12 +2,12 @@
 .\"     Title: git-hyper-blame
 .\"    Author: [FIXME: author] [see http://docbook.sf.net/el/author]
 .\" Generator: DocBook XSL Stylesheets v1.78.1 <http://docbook.sf.net/>
-.\"      Date: 02/05/2016
+.\"      Date: 02/19/2016
 .\"    Manual: Chromium depot_tools Manual
-.\"    Source: depot_tools d2dbf32
+.\"    Source: depot_tools ba74a75
 .\"  Language: English
 .\"
-.TH "GIT\-HYPER\-BLAME" "1" "02/05/2016" "depot_tools d2dbf32" "Chromium depot_tools Manual"
+.TH "GIT\-HYPER\-BLAME" "1" "02/19/2016" "depot_tools ba74a75" "Chromium depot_tools Manual"
 .\" -----------------------------------------------------------------
 .\" * Define some portability stuff
 .\" -----------------------------------------------------------------
@@ -32,7 +32,8 @@ git-hyper-blame \- Like git blame, but with the ability to ignore or bypass cert
 .SH "SYNOPSIS"
 .sp
 .nf
-\fIgit hyper\-blame\fR [\-i <rev> [\-i <rev> \&...]] [<rev>] [\-\-] <file>
+\fIgit hyper\-blame\fR [\-i <rev> [\-i <rev> \&...]] [\-\-ignore\-file=<file>]
+                [\-\-no\-default\-ignores] [<rev>] [\-\-] <file>
 .fi
 .sp
 .SH "DESCRIPTION"
@@ -42,12 +43,27 @@ git hyper\-blame is like git blame but it can ignore or "look through" a given s
 This is useful if you have a commit that makes sweeping changes that are unlikely to be what you are looking for in a blame, such as mass reformatting or renaming\&. By adding these commits to the hyper\-blame ignore list, git hyper\-blame will look past these commits to find the previous commit that touched a given line\&.
 .sp
 Follows the normal blame syntax: annotates <file> with the revision that last modified each line\&. Optional <rev> specifies the revision of <file> to start from\&.
+.sp
+Automatically looks for a file called \&.git\-blame\-ignore\-revs in the repository root directory\&. This file has the same syntax as the \-\-ignore\-file argument, and any commits mentioned in this file are added to the ignore list\&.
 .SH "OPTIONS"
 .PP
 \-i <rev>
 .RS 4
 A revision to ignore\&. Can be specified as many times as needed\&.
 .RE
+.PP
+\-\-ignore\-file=<file>
+.RS 4
+A file containing a list of revisions to ignore\&. Can have comments beginning with
+#\&.
+.RE
+.PP
+\-\-no\-default\-ignores
+.RS 4
+Do not ignore commits from the
+\&.git\-blame\-ignore\-revs
+file\&.
+.RE
 .SH "EXAMPLE"
 .sp
 Let\(cqs run git blame on a file:
@@ -98,30 +114,6 @@ hyper\-blame places a * next to any line where it has skipped over an ignored co
 When a line skips over an ignored commit, a guess is made as to which commit previously modified that line, but it is not always clear where the line came from\&. If the ignored commit makes lots of changes in close proximity, in particular adding/removing/reordering lines, then the wrong authors may be blamed for nearby edits\&.
 .sp
 For this reason, hyper\-blame works best when the ignored commits are be limited to minor changes such as formatting and renaming, not refactoring or other more invasive changes\&.
-.SH "BUGS"
-.sp
-.RS 4
-.ie n \{\
-\h'-04'\(bu\h'+03'\c
-.\}
-.el \{\
-.sp -1
-.IP \(bu 2.3
-.\}
-There is currently no way to pass the ignore list as a file\&.
-.RE
-.sp
-.RS 4
-.ie n \{\
-\h'-04'\(bu\h'+03'\c
-.\}
-.el \{\
-.sp -1
-.IP \(bu 2.3
-.\}
-It should be possible for a git repository to configure an automatic list of commits to ignore (like
-\&.gitignore), so that project owners can maintain a list of "big change" commits that are ignored by hyper\-blame by default\&.
-.RE
 .SH "SEE ALSO"
 .sp
 \fBgit-blame\fR(1)

man/src/git-hyper-blame.txt

@@ -9,7 +9,8 @@ include::_git-hyper-blame_desc.helper.txt[]
 SYNOPSIS
 --------
 [verse]
-'git hyper-blame' [-i <rev> [-i <rev> ...]] [<rev>] [--] <file>
+'git hyper-blame' [-i <rev> [-i <rev> ...]] [--ignore-file=<file>]
+                [--no-default-ignores] [<rev>] [--] <file>
 
 DESCRIPTION
 -----------
@@ -27,12 +28,23 @@ Follows the normal `blame` syntax: annotates `<file>` with the revision that
 last modified each line. Optional `<rev>` specifies the revision of `<file>` to
 start from.
 
+Automatically looks for a file called `.git-blame-ignore-revs` in the repository
+root directory. This file has the same syntax as the `--ignore-file` argument,
+and any commits mentioned in this file are added to the ignore list.
+
 OPTIONS
 -------
 
 -i <rev>::
   A revision to ignore. Can be specified as many times as needed.
 
+--ignore-file=<file>::
+  A file containing a list of revisions to ignore. Can have comments beginning
+  with `#`.
+
+--no-default-ignores::
+  Do not ignore commits from the `.git-blame-ignore-revs` file.
+
 EXAMPLE
 -------
 
@@ -64,14 +76,6 @@ For this reason, `hyper-blame` works best when the ignored commits are be
 limited to minor changes such as formatting and renaming, not refactoring or
 other more invasive changes.
 
-BUGS
-----
-
-- There is currently no way to pass the ignore list as a file.
-- It should be possible for a git repository to configure an automatic list of
-  commits to ignore (like `.gitignore`), so that project owners can maintain a
-  list of "big change" commits that are ignored by hyper-blame by default.
-
 SEE ALSO
 --------
 linkgit:git-blame[1]

tests/git_hyper_blame_test.py

@@ -50,7 +50,7 @@ class GitHyperBlameTestBase(git_test_utils.GitRepoReadOnlyTestBase):
 
 class GitHyperBlameMainTest(GitHyperBlameTestBase):
   """End-to-end tests on a very simple repo."""
-  REPO_SCHEMA = "A B C"
+  REPO_SCHEMA = "A B C D"
 
   COMMIT_A = {
     'some/files/file': {'data': 'line 1\nline 2\n'},
@@ -64,6 +64,19 @@ class GitHyperBlameMainTest(GitHyperBlameTestBase):
     'some/files/file': {'data': 'line 1.1\nline 2.1\n'},
   }
 
+  COMMIT_D = {
+    # This file should be automatically considered for ignore.
+    '.git-blame-ignore-revs': {'data': 'tag_C'},
+    # This file should not be considered.
+    'some/files/.git-blame-ignore-revs': {'data': 'tag_B'},
+  }
+
+  def setUp(self):
+    super(GitHyperBlameMainTest, self).setUp()
+    # Most tests want to check out C (so the .git-blame-ignore-revs is not
+    # used).
+    self.repo.git('checkout', '-f', 'tag_C')
+
   def testBasicBlame(self):
     """Tests the main function (simple end-to-end test with no ignores)."""
     expected_output = [self.blame_line('C', '1) line 1.1'),
@@ -137,14 +150,72 @@ class GitHyperBlameMainTest(GitHyperBlameTestBase):
 
   def testBadIgnore(self):
     """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 = StringIO.StringIO()
     stderr = StringIO.StringIO()
     retval = self.repo.run(self.git_hyper_blame.main,
                            args=['-i', 'xxxx', 'tag_C', 'some/files/file'],
                            stdout=stdout, stderr=stderr)
-    self.assertNotEqual(0, retval)
+    self.assertEqual(0, retval)
-    self.assertEqual('', stdout.getvalue())
+    self.assertEqual(expected_output, stdout.getvalue().rstrip().split('\n'))
-    self.assertEqual('fatal: unknown revision \'xxxx\'.\n', stderr.getvalue())
+    self.assertEqual('warning: unknown revision \'xxxx\'.\n', 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 = StringIO.StringIO()
+    stderr = StringIO.StringIO()
+
+    with tempfile.NamedTemporaryFile(mode='w+', prefix='ignore') as ignore_file:
+      ignore_file.write('# Line comments are allowed.\n'.format(self.repo['B']))
+      ignore_file.write('\n')
+      ignore_file.write('{}\n'.format(self.repo['B']))
+      # 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)
+
+    self.assertEqual(0, retval)
+    self.assertEqual(expected_output, stdout.getvalue().rstrip().split('\n'))
+    self.assertEqual('warning: unknown revision \'xxxx\'.\n', stderr.getvalue())
+
+  def testDefaultIgnoreFile(self):
+    """Tests automatically using a default ignore list."""
+    # Check out revision D. We expect the script to use the default ignore list
+    # that is checked out, *not* the one committed at the given revision.
+    self.repo.git('checkout', '-f', 'tag_D')
+
+    expected_output = [self.blame_line('A', '1*) line 1.1'),
+                       self.blame_line('B', ' 2) line 2.1')]
+    stdout = StringIO.StringIO()
+    stderr = StringIO.StringIO()
+
+    retval = self.repo.run(self.git_hyper_blame.main,
+                           args=['tag_D', 'some/files/file'],
+                           stdout=stdout, stderr=stderr)
+
+    self.assertEqual(0, retval)
+    self.assertEqual(expected_output, stdout.getvalue().rstrip().split('\n'))
+    self.assertEqual('', 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 = StringIO.StringIO()
+    stderr = StringIO.StringIO()
+
+    retval = self.repo.run(self.git_hyper_blame.main,
+                           args=['tag_C', 'some/files/file'],
+                           stdout=stdout, stderr=stderr)
+
+    self.assertEqual(0, retval)
+    self.assertEqual(expected_output, stdout.getvalue().rstrip().split('\n'))
+    self.assertEqual('', stderr.getvalue())
 
 class GitHyperBlameSimpleTest(GitHyperBlameTestBase):
   REPO_SCHEMA = """