git hyper-blame crash ignoring blink rename CL |
|||
Issue descriptionChrome Version: ea2b51465a (r463155) Depot tools version: 7b2ca416e4 OS: Linux What steps will reproduce the problem? (1) git hyper-blame -i 1c4d759e44 ea2b51465a -- third_party/WebKit/Source/modules/installedapp/NavigatorInstalledApp.cpp What is the expected result? Blames NavigatorInstalledApp ignoring the Blink Rename CL (r463139) What happens instead? Crashes: $ git hyper-blame -i 1c4d759e44 ea2b51465a -- third_party/WebKit/Source/modules/installedapp/NavigatorInstalledApp.cpp Traceback (most recent call last): File "depot_tools/git_hyper_blame.py", line 391, in <module> sys.exit(main(sys.argv[1:], stdout=less_input)) File "depot_tools/git_hyper_blame.py", line 385, in main return hyper_blame(ignored, filename, args.revision, out=stdout, err=stderr) File "depot_tools/git_hyper_blame.py", line 311, in hyper_blame assert 1 <= lineno_previous <= len(parent_blame) AssertionError This urgently needs to be fixed so that we can add r463139 to .git-blame-ignore-revs.
,
Apr 10 2017
,
Apr 10 2017
A more direct repro: git hyper-blame --no-default-ignores -i 1c8e1a7719 -i 1c4d759e44 ea2b51465a -- third_party/WebKit/Source/modules/installedapp/NavigatorInstalledApp.cpp (But due to Issue 709835 , you need to edit git_hyper_blame.py adding "action='store_true'" to the add_argument call for --no-default-ignores.) In other words, this is caused by ignoring precisely these two commits on NavigatorInstalledApp.cpp: 1c8e1a7719 (Reformat blink; r422256) 1c4d759e44 (The Blink Rename; r463139) (I can't tell how widespread this issue is; it just happened on the first file I tried.)
,
Apr 10 2017
I found and fixed the problem.
Minimal test case (line numbers are not part of the file):
Version A:
1) red
2) blue
Version B:
1) red
2) green
Version C:
1) X
2) Y
3) red
4) green
Version D:
1) X
2) Y
3) red
4) yellow
$ git hyper-blame -i B -i D D <file>
Expected output:
C (... 1) X
C (... 2) Y
A (... 3) red
A (... 4*) yellow
Actual result: AssertionError.
The problem is due to both B and D being ignored, while C is in between, adding two new lines to the top of the file. The line in question ("blue"/"green"/"yellow") is Line 2 in A,B and Line 4 in C,D.
The algorithm starts with a git blame of D, which says Line 4 was changed by D (ignored).
Therefore, blame C, which says Line 4 was changed by B (also ignored).
At this point, it is supposed to set lineno_previous to 2 (the line "green" in B), but instead it remains at 4 (the position of that line in C). Then it hits this assert:
assert 1 <= lineno_previous <= len(parent_blame)
(parent_blame is B, which has 2 lines, so this assertion fails).
The fix is to change lineno_previous to newline.lineno_then in the construction of the new BlameLine in the inner loop of the hyper_blame function.
Assessment: This bug was always possible, but I hadn't seen it until now. I think it's much easier to hit with more commits affecting the same lines, so it will be much more likely now that we have two major CLs that touch practically all of Blink. Therefore, this should be fixed before r463139 is added to .git-blame-ignore-revs in the Chromium repo.
,
Apr 10 2017
Code review: https://chromium-review.googlesource.com/c/472648/
,
Apr 10 2017
,
Apr 10 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/tools/depot_tools/+/2cd3c14ef72a306bfacbb0dc961d61c238a85244 commit 2cd3c14ef72a306bfacbb0dc961d61c238a85244 Author: Matt Giuca <mgiuca@chromium.org> Date: Mon Apr 10 14:09:20 2017 git_hyper_blame: Fix wrong line number on lines changed many times. This could, in extreme cases, result in a crash due to the wrong line number being out of bounds of the file line count. BUG= 709831 Change-Id: I08ec75362d49c4a72e7ee9fd489d5f9baa6d31bd Reviewed-on: https://chromium-review.googlesource.com/472648 Reviewed-by: Nico Weber <thakis@chromium.org> Reviewed-by: Jochen Eisinger <jochen@chromium.org> Commit-Queue: Nico Weber <thakis@chromium.org> [modify] https://crrev.com/2cd3c14ef72a306bfacbb0dc961d61c238a85244/git_hyper_blame.py [modify] https://crrev.com/2cd3c14ef72a306bfacbb0dc961d61c238a85244/tests/git_hyper_blame_test.py
,
Apr 10 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/tools/depot_tools/+/2cd3c14ef72a306bfacbb0dc961d61c238a85244 commit 2cd3c14ef72a306bfacbb0dc961d61c238a85244 Author: Matt Giuca <mgiuca@chromium.org> Date: Mon Apr 10 14:09:20 2017 git_hyper_blame: Fix wrong line number on lines changed many times. This could, in extreme cases, result in a crash due to the wrong line number being out of bounds of the file line count. BUG= 709831 Change-Id: I08ec75362d49c4a72e7ee9fd489d5f9baa6d31bd Reviewed-on: https://chromium-review.googlesource.com/472648 Reviewed-by: Nico Weber <thakis@chromium.org> Reviewed-by: Jochen Eisinger <jochen@chromium.org> Commit-Queue: Nico Weber <thakis@chromium.org> [modify] https://crrev.com/2cd3c14ef72a306bfacbb0dc961d61c238a85244/git_hyper_blame.py [modify] https://crrev.com/2cd3c14ef72a306bfacbb0dc961d61c238a85244/tests/git_hyper_blame_test.py
,
Apr 11 2017
|
|||
►
Sign in to add a comment |
|||
Comment 1 by mgiuca@chromium.org
, Apr 10 2017