Added git hyper-blame, a tool that skips unwanted commits in git blame.

Added git hyper-blame, a tool that skips unwanted commits in git blame.

Currently, the script requires you to pass the unwanted commits on the
command line, but eventually, you could configure it with a file
(checked into the repo) that provides a fixed set of commits to always
skip (such as commits that do a huge amount of renaming and nothing
else).

BUG=574290
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=298544

[Matt Giuca, 2016-01-05 03:11:27]

Description was changed from

==========
Added git_hyper_blame, a tool that skips unwanted commits in git blame.

Currently, the script requires you to pass the unwanted commits on the
command line, but eventually, you could configure it with a file
(checked into the repo) that provides a fixed set of commits to always
skip (such as commits that do a huge amount of renaming and nothing
else).

This works but is very rough around the edges and hasn't been properly
tested at all.

DO NOT SUBMIT. PROOF OF CONCEPT ONLY.

BUG=574290
==========

to

==========
Added git_hyper_blame, a tool that skips unwanted commits in git blame.

Currently, the script requires you to pass the unwanted commits on the
command line, but eventually, you could configure it with a file
(checked into the repo) that provides a fixed set of commits to always
skip (such as commits that do a huge amount of renaming and nothing
else).

This works but is very rough around the edges and hasn't been properly
tested at all.

DO NOT SUBMIT. PROOF OF CONCEPT ONLY.

NOTE TO REVIEWERS: Please discuss at a conceptual level but DO NOT
REVIEW THE CODE. I know it has a bunch of style and design issues which
I can clean up if we decide it is worthwhile going ahead.

BUG=574290
==========

[Matt Giuca, 2016-01-25 04:41:33]

Description was changed from

==========
Added git_hyper_blame, a tool that skips unwanted commits in git blame.

Currently, the script requires you to pass the unwanted commits on the
command line, but eventually, you could configure it with a file
(checked into the repo) that provides a fixed set of commits to always
skip (such as commits that do a huge amount of renaming and nothing
else).

This works but is very rough around the edges and hasn't been properly
tested at all.

DO NOT SUBMIT. PROOF OF CONCEPT ONLY.

NOTE TO REVIEWERS: Please discuss at a conceptual level but DO NOT
REVIEW THE CODE. I know it has a bunch of style and design issues which
I can clean up if we decide it is worthwhile going ahead.

BUG=574290
==========

to

==========
Added git_hyper_blame, a tool that skips unwanted commits in git blame.

Currently, the script requires you to pass the unwanted commits on the
command line, but eventually, you could configure it with a file
(checked into the repo) that provides a fixed set of commits to always
skip (such as commits that do a huge amount of renaming and nothing
else).

BUG=574290
==========

[Matt Giuca, 2016-01-25 04:56:20]

Description was changed from

==========
Added git_hyper_blame, a tool that skips unwanted commits in git blame.

Currently, the script requires you to pass the unwanted commits on the
command line, but eventually, you could configure it with a file
(checked into the repo) that provides a fixed set of commits to always
skip (such as commits that do a huge amount of renaming and nothing
else).

BUG=574290
==========

to

==========
Added git hyper-blame, a tool that skips unwanted commits in git blame.

Currently, the script requires you to pass the unwanted commits on the
command line, but eventually, you could configure it with a file
(checked into the repo) that provides a fixed set of commits to always
skip (such as commits that do a huge amount of renaming and nothing
else).

BUG=574290
==========

[Matt Giuca, 2016-01-28 05:58:50]

mgiuca@chromium.org changed reviewers:
+ iannucci@chromium.org

[Matt Giuca, 2016-01-28 05:58:51]

Hi Robert,

I don't know who the best reviewer is for that, but you are Mr. Git so I'm
starting with you. Please forward if there is a better person.

This adds a tool requested on blink-dev [1] to view a git blame while bypassing
certain commits.

Reviewing: This CL is huge. I split off some complex (but non-critical) logic
into another CL (which is why there are TODOs everywhere about the line
numbering algorithm), to shrink the review size. I thought of splitting up the
git_dates and git_common.blame additions, but I think they make more sense in
the context of my main script, so I left it in. Note that a couple of hundred
lines of this CL are just generated HTML and man page files that you don't need
to look at. It is easiest to read the man page by downloading
git-hyper-blame.html.

Naming: So there has been some discussion over the name --- see
http://crbug.com/574290 (#8 and #10). I'm actually rather fond of hyper-blame
(it's "cute" I guess) so I stuck with it for the review. But let me know if you
think it's inappropriate and if you have a preference for any of the other names
suggested.

[1] https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/0utXspEG4o0

[iannucci, 2016-01-29 19:29:25]

generally lgtm, though I suspect that this algorithm may fall apart with any
sort of semi-complicated refactor :(. Some comments too

https://chromiumcodereview.appspot.com/1559943003/diff/230001/git_common.py
File git_common.py (right):

https://chromiumcodereview.appspot.com/1559943003/diff/230001/git_common.py#n...
git_common.py:286: if porcelain:  # pragma: no cover
why not covered?

https://chromiumcodereview.appspot.com/1559943003/diff/230001/git_common.py#n...
git_common.py:289: command.append(revision)
may want to consider -w and -M options

https://chromiumcodereview.appspot.com/1559943003/diff/230001/git_common.py#n...
git_common.py:559: def repo_root():  # pragma: no cover
?

https://chromiumcodereview.appspot.com/1559943003/diff/230001/git_common.py#n...
git_common.py:574: """
does this work on windows?

Generally stuff in depot_tools at least tries to work in windows too... I would
at least try to make linux features gracefully degrade. If running it with `git
bash` is sufficient, I would detect a non-bash environment, print an error to
that effect and exit.

https://chromiumcodereview.appspot.com/1559943003/diff/230001/git_hyper_blame.py
File git_hyper_blame.py (right):

https://chromiumcodereview.appspot.com/1559943003/diff/230001/git_hyper_blame...
git_hyper_blame.py:221: def main(args=None):  # pragma: no cover
no covers are sad :(

https://chromiumcodereview.appspot.com/1559943003/diff/230001/man/src/git-hyp...
File man/src/git-hyper-blame.txt (right):

https://chromiumcodereview.appspot.com/1559943003/diff/230001/man/src/git-hyp...
man/src/git-hyper-blame.txt:28: start from.
You know, it would be useful if we could use a git-notes ref on the origin repo
to mark certain commits as skippable by default so that folks don't have to
accumulate a list of to-be-skipped revisions.

https://chromiumcodereview.appspot.com/1559943003/diff/230001/man/src/git-hyp...
man/src/git-hyper-blame.txt:64: list of "big change" commits that are ignored by
hyper-blame by default.
er, right :)

I would definitely use git-notes though. Otherwise you have to double-commit.

[Matt Giuca, 2016-02-01 03:32:23]

Patchset #14 (id:250001) has been deleted

[Matt Giuca, 2016-02-01 03:51:06]

PTAL.

On 2016/01/29 19:29:25, iannucci wrote:
> generally lgtm, though I suspect that this algorithm may fall apart with any
> sort of semi-complicated refactor :(.

In its current state, yes. The next CL
(https://codereview.chromium.org/1629253002/) adds line motion detection and
seems to be pretty good. (Nico tested it on a very large rename on Blink; see
bug.)

That will only be good at tracking renames and reformats though (anything where
the general line content remains the same *and in the same order*, but may move
around, grow and shrink, etc). It is a non-goal of hyper-blame that we can skip
over more serious refactors, and such commits should not be added to the include
list. (Note: I make sure to handle any situation, and the tests cover that, but
it may not end up blaming a sensible line in that case.)

PS. What were your thoughts on naming? Looks like on the bug the consensus is
moving towards "git xblame" which kind of invokes the idea of an X-ray.

https://chromiumcodereview.appspot.com/1559943003/diff/230001/git_common.py
File git_common.py (right):

https://chromiumcodereview.appspot.com/1559943003/diff/230001/git_common.py#n...
git_common.py:286: if porcelain:  # pragma: no cover
On 2016/01/29 19:29:25, iannucci wrote:
> why not covered?

Testing porcelain was a lot of work, and covered implicitly by hyper-blame
tests.

Well, I've added porcelain tests.

https://chromiumcodereview.appspot.com/1559943003/diff/230001/git_common.py#n...
git_common.py:289: command.append(revision)
On 2016/01/29 19:29:25, iannucci wrote:
> may want to consider -w and -M options

I think we could consider that in a future version. I don't think we want that
to be the default for hyper-blame (which has the specific goal of ignoring
certain commits, not necessarily bypassing other arbitrary changes).

https://chromiumcodereview.appspot.com/1559943003/diff/230001/git_common.py#n...
git_common.py:559: def repo_root():  # pragma: no cover
On 2016/01/29 19:29:25, iannucci wrote:
> ?

I didn't think it would be easy to test this since it's in a random directory.
But it was kind of easy. Done.

https://chromiumcodereview.appspot.com/1559943003/diff/230001/git_common.py#n...
git_common.py:574: """
On 2016/01/29 19:29:25, iannucci wrote:
> does this work on windows?
> 
> Generally stuff in depot_tools at least tries to work in windows too... I
would
> at least try to make linux features gracefully degrade. If running it with
`git
> bash` is sufficient, I would detect a non-bash environment, print an error to
> that effect and exit.

Tested on Windows; works perfectly, except for an IOError if you quit less
before getting to the end. Added a catch for that.

Tested on both MinGW and cmd, for both regular terminal output and piping to a
file. I'm a bit surprised that less worked from cmd, given that typing "less"
doesn't work on the command line. But it seems to be fine.

Tests almost completely fail, but that's also true of git_common_test so I think
there is just something bad about the testing environment on Windows and
probably not worth looking into.

https://chromiumcodereview.appspot.com/1559943003/diff/230001/git_hyper_blame.py
File git_hyper_blame.py (right):

https://chromiumcodereview.appspot.com/1559943003/diff/230001/git_hyper_blame...
git_hyper_blame.py:221: def main(args=None):  # pragma: no cover
On 2016/01/29 19:29:25, iannucci wrote:
> no covers are sad :(

I didn't think main was worth testing. But, I added comprehensive tests for
main, and found some issues too! ^_^

Note: I added a catch for CalledProcessError below. Previously you would get a
traceback if you passed a non-existent revision to -i.

https://chromiumcodereview.appspot.com/1559943003/diff/230001/man/src/git-hyp...
File man/src/git-hyper-blame.txt (right):

https://chromiumcodereview.appspot.com/1559943003/diff/230001/man/src/git-hyp...
man/src/git-hyper-blame.txt:28: start from.
On 2016/01/29 19:29:25, iannucci wrote:
> You know, it would be useful if we could use a git-notes ref on the origin
repo
> to mark certain commits as skippable by default so that folks don't have to
> accumulate a list of to-be-skipped revisions.

See below.

https://chromiumcodereview.appspot.com/1559943003/diff/230001/man/src/git-hyp...
man/src/git-hyper-blame.txt:64: list of "big change" commits that are ignored by
hyper-blame by default.
On 2016/01/29 19:29:25, iannucci wrote:
> er, right :)
> 
> I would definitely use git-notes though. Otherwise you have to double-commit.

If you can crack the SHA-1 algorithm then you should be able to land a single
commit that refers to itself in a file :)

Jokes aside, I'll have a look at git notes. My initial thoughts are that it
seems a bit anti-git to have an object that's conceptually part of the checkout
that isn't actually in version control, or rather it's in some kind of parallel
version control that isn't in lock-step with the main codebase. Even though a
hyper-blame ignore list is not a "core" part of a Chrome checkout, it would
still be nice if it was versioned properly.

I don't think the double-commit is a big problem, given that we shouldn't be
adding commits to the ignore list very often.

But in any case, that should not block this CL since we would definitely want to
implement the ignore list in a future CL.

[Matt Giuca, 2016-02-03 00:53:25]

Hi Robert, PTAL. Thanks.

(Not sure if you were expecting me to land with your LG, but I think the changes
you asked me to make are significant enough to warrant further review.)

[iannucci, 2016-02-03 01:14:51]

lgtm, I'm fine with hyperblame or any other name that folks feel strongly about.
git aliases are a thing too :)

https://codereview.chromium.org/1559943003/diff/290001/git_hyper_blame.py
File git_hyper_blame.py (right):

https://codereview.chromium.org/1559943003/diff/290001/git_hyper_blame.py#new...
git_hyper_blame.py:226: def main(args=None, stdout=sys.stdout,
stderr=sys.stderr):
make args mandatory

https://codereview.chromium.org/1559943003/diff/290001/git_hyper_blame.py#new...
git_hyper_blame.py:228: args = sys.argv[1:]
then delete

https://codereview.chromium.org/1559943003/diff/290001/git_hyper_blame.py#new...
git_hyper_blame.py:269: sys.exit(main(stdout=less_input))
then pass sys.argv[1:] here

[Matt Giuca, 2016-02-03 07:27:58]

https://codereview.chromium.org/1559943003/diff/290001/git_hyper_blame.py
File git_hyper_blame.py (right):

https://codereview.chromium.org/1559943003/diff/290001/git_hyper_blame.py#new...
git_hyper_blame.py:226: def main(args=None, stdout=sys.stdout,
stderr=sys.stderr):
On 2016/02/03 01:14:51, iannucci wrote:
> make args mandatory

I'm following the convention established by Guido in 2003:
https://www.artima.com/weblogs/viewpost.jsp?thread=4829

But it's a minor thing, so Done.

https://codereview.chromium.org/1559943003/diff/290001/git_hyper_blame.py#new...
git_hyper_blame.py:228: args = sys.argv[1:]
On 2016/02/03 01:14:51, iannucci wrote:
> then delete

Done.

https://codereview.chromium.org/1559943003/diff/290001/git_hyper_blame.py#new...
git_hyper_blame.py:269: sys.exit(main(stdout=less_input))
On 2016/02/03 01:14:51, iannucci wrote:
> then pass sys.argv[1:] here

Done.

[Matt Giuca, 2016-02-03 07:58:36]

The CQ bit was checked by mgiuca@chromium.org

[Matt Giuca, 2016-02-03 07:58:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
iannucci@chromium.org
Link to the patchset: https://codereview.chromium.org/1559943003/#ps310001
(title: "Respond to review.")

[commit-bot: I haz the power, 2016-02-03 07:58:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1559943003/310001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1559943003/310001

[commit-bot: I haz the power, 2016-02-03 08:00:53]

Description was changed from

==========
Added git hyper-blame, a tool that skips unwanted commits in git blame.

Currently, the script requires you to pass the unwanted commits on the
command line, but eventually, you could configure it with a file
(checked into the repo) that provides a fixed set of commits to always
skip (such as commits that do a huge amount of renaming and nothing
else).

BUG=574290
==========

to

==========
Added git hyper-blame, a tool that skips unwanted commits in git blame.

Currently, the script requires you to pass the unwanted commits on the
command line, but eventually, you could configure it with a file
(checked into the repo) that provides a fixed set of commits to always
skip (such as commits that do a huge amount of renaming and nothing
else).

BUG=574290

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=298544
==========

[commit-bot: I haz the power, 2016-02-03 08:00:54]

Committed patchset #16 (id:310001) as
http://src.chromium.org/viewvc/chrome?view=rev&revision=298544