From cd0a1cf3620289e7dffbd86e5c58d14318d5582b Mon Sep 17 00:00:00 2001 From: "mgiuca@chromium.org" Date: Mon, 22 Feb 2016 00:40:33 +0000 Subject: [PATCH] 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 | 31 ++++++++++++-- man/html/git-hyper-blame.html | 44 +++++++++---------- man/man1/git-hyper-blame.1 | 48 +++++++++------------ man/src/git-hyper-blame.txt | 22 ++++++---- tests/git_hyper_blame_test.py | 79 +++++++++++++++++++++++++++++++++-- 5 files changed, 158 insertions(+), 66 deletions(-) diff --git a/git_hyper_blame.py b/git_hyper_blame.py index 5a7daa0cf..b9965a7a6 100755 --- a/git_hyper_blame.py +++ b/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). - stderr.write('fatal: unknown revision \'%s\'.\n' % c) - return e.returncode + # Custom warning string (the message from git-rev-parse is inappropriate). + stderr.write('warning: unknown revision \'%s\'.\n' % c) return hyper_blame(ignored, filename, args.revision, out=stdout, err=stderr) diff --git a/man/html/git-hyper-blame.html b/man/html/git-hyper-blame.html index 62f7cc82a..c123d81b0 100644 --- a/man/html/git-hyper-blame.html +++ b/man/html/git-hyper-blame.html @@ -755,7 +755,8 @@ git-hyper-blame(1) Manual Page

SYNOPSIS

-
git hyper-blame [-i <rev> [-i <rev> …]] [<rev>] [--] <file>
+
git hyper-blame [-i <rev> [-i <rev> …]] [--ignore-file=<file>]
+                [--no-default-ignores] [<rev>] [--] <file>
@@ -773,6 +774,9 @@ touched a given line.

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.

@@ -787,6 +791,23 @@ start from.

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. +

+
@@ -833,25 +854,6 @@ 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

git-blame(1)

@@ -869,7 +871,7 @@ from
diff --git a/man/man1/git-hyper-blame.1 b/man/man1/git-hyper-blame.1 index e92990ead..ef7d80a6b 100644 --- a/man/man1/git-hyper-blame.1 +++ b/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 -.\" 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 [\-i \&...]] [] [\-\-] +\fIgit hyper\-blame\fR [\-i [\-i \&...]] [\-\-ignore\-file=] + [\-\-no\-default\-ignores] [] [\-\-] .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 with the revision that last modified each line\&. Optional specifies the revision of 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 .RS 4 A revision to ignore\&. Can be specified as many times as needed\&. .RE +.PP +\-\-ignore\-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) diff --git a/man/src/git-hyper-blame.txt b/man/src/git-hyper-blame.txt index 8bbe7f706..890b6649a 100644 --- a/man/src/git-hyper-blame.txt +++ b/man/src/git-hyper-blame.txt @@ -9,7 +9,8 @@ include::_git-hyper-blame_desc.helper.txt[] SYNOPSIS -------- [verse] -'git hyper-blame' [-i [-i ...]] [] [--] +'git hyper-blame' [-i [-i ...]] [--ignore-file=] + [--no-default-ignores] [] [--] DESCRIPTION ----------- @@ -27,12 +28,23 @@ Follows the normal `blame` syntax: annotates `` with the revision that last modified each line. Optional `` specifies the revision of `` 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 :: A revision to ignore. Can be specified as many times as needed. +--ignore-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] diff --git a/tests/git_hyper_blame_test.py b/tests/git_hyper_blame_test.py index 692287d1b..1ac396edb 100755 --- a/tests/git_hyper_blame_test.py +++ b/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('', stdout.getvalue()) - self.assertEqual('fatal: unknown revision \'xxxx\'.\n', stderr.getvalue()) + self.assertEqual(0, retval) + self.assertEqual(expected_output, stdout.getvalue().rstrip().split('\n')) + 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 = """