Closed Bug 1507923 Opened 6 years ago Closed 6 years ago

Add a mechanism to skip over certain commits in the blame view

Categories

(Webtools :: Searchfox, enhancement)

Product:
Webtools
Component:
Searchfox
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: kats, Assigned: kats)

References

(Depends on 2 open bugs)

Details

The upcoming mass-clang-format of the mozilla-central codebase is going to basically "break blame" in searchfox in that the blame for most lines will point to this one massive changeset. We should add some way to skip over this changeset. There's some discussion about adding a .git-blame-ignore-revs file to the tree, or adding the "# skip-blame" string to the commit message (or both) - whatever gets decided, we should try and support that in searchfox to skip over commits that are not useful for blame purposes.
Should we do this at the blame repo level (skipping generating commits in the blame mirror?) or at runtime? I guess at runtime is easier if we want to support something like the .git-blame-ignore-revs file without rebuilding the blame repo.
Yeah I was thinking at runtime. It irks me a little though that at best this will be approximate, there's no way to do it so that it's 100% accurate because the line number can change during one these commits.

Also in terms of UI I was thinking that the little blame tooltip that appears when you hover over the left bar would still list the "skip-blame" but it would *also* list what we compute to be the previous change to that line. That way we're not totally hiding the fact that there was a "skip-blame" change but we're also not letting it destroy the usefulness of the blame.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #4)
> Also in terms of UI I was thinking that the little blame tooltip that
> appears when you hover over the left bar would still list the "skip-blame"
> but it would *also* list what we compute to be the previous change to that
> line. That way we're not totally hiding the fact that there was a
> "skip-blame" change but we're also not letting it destroy the usefulness of
> the blame.

That sounds good to me.

Whatever mechanism we use, it'll be good to use something that is extensible enough that we can also include older reformatting work, like Poiru's stuff patches in bug 995730 and other places.
I've given this some more thought after poking around the code, and I think my original idea is still reasonable. The places we'll need to do changes:

- Somewhere around [1] we'll want to do a lookup to see if `rev` is a "skippable changeset" and if so, do what hyper-blame does to find some reasonable previous changesets for that line. Note that we might have to go back through multiple skippable changesets here, and we'll end up with a list of revisions (maybe cap it at 5 or something). And then we want to use that list of changesets instead of a single rev to to do the color flipflop, and put that list of changesets into the data-blame of the generated HTML.
- At [2] we want to accept the list of changesets instead of just a single one, and pass them (as a comma-separated list probably) to the commit-info XHR request at [3]. We could do multiple XHR requests but it seems silly.
- In the commit-info handler at [4] handle a request for multiple changesets and return all the data in the json blob.
- And then at [5] we want to accept the new json blob with all the data and make a mega-popup with multiple entries as I described in comment 4.

I'll try and implement this sometime this week, will need to carve out some time for it.

[1] https://github.com/mozsearch/mozsearch/blob/8f00e7a8509bab61aa5400c20b74506fc0797ec9/tools/src/format.rs#L349
[2] https://github.com/mozsearch/mozsearch/blob/8f00e7a8509bab61aa5400c20b74506fc0797ec9/static/js/blame.js#L21
[3] https://github.com/mozsearch/mozsearch/blob/8f00e7a8509bab61aa5400c20b74506fc0797ec9/static/js/blame.js#L77
[4] https://github.com/mozsearch/mozsearch/blob/8f00e7a8509bab61aa5400c20b74506fc0797ec9/tools/src/blame.rs#L25
[5] https://github.com/mozsearch/mozsearch/blob/8f00e7a8509bab61aa5400c20b74506fc0797ec9/static/js/blame.js#L28
(In reply to Andrew McCreight [:mccr8] from comment #5)
> Whatever mechanism we use, it'll be good to use something that is extensible
> enough that we can also include older reformatting work, like Poiru's stuff
> patches in bug 995730 and other places.

Agreed. I've also thought that this "show multiple blame entries UI" would be useful for cases where the blame points to a backout changeset. I find it annoying when trying to find when a line was added, if I have to work my way through several bounce-reland cycles of a changeset.
I wasn't even thinking about the backout problem. That would be great if we could do something useful for that, as I also find that to be annoying.

(Another change set we want to hide is the dvander IonMonkey merge, which somehow managed to touch a large portion of all files in the tree, though maybe it is relevant for the JS tree...)
Started working on this last night. Almost done the first cut.
Assignee: nobody → kats
I have a "working" implementation, patches are at https://github.com/staktrace/mozsearch/commits/hyperblame

I've done some very basic testing and bugfixing. I'll need to do more heavy testing and probably add some caching to avoid repeatedly doing the same computations. Also my UI skillz are bad, so the UI is ugly. For now I'm just assuming there will be a .git-blame-ignore-revs file in the root of the tree with the right SHAs but that probably won't be the case since nobody cares about poor ole gecko-dev anymore. So we'll have to come up with some mechanism for getting the right SHAs. Or we can do it purely based on commit messages, or something.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #10)
For now I'm just
> assuming there will be a .git-blame-ignore-revs file in the root of the tree
> with the right SHAs but that probably won't be the case since nobody cares
> about poor ole gecko-dev anymore. So we'll have to come up with some
> mechanism for getting the right SHAs.

It looks like there is a .hg-annotate-ignore-revs file, and we can use the releng gecko-dev mapper to get the equivalent gecko-dev hashes. So that's doable. We can also scrape commit messages if desired although that'll likely slow things down a bit.
I did a dev run with my patches and hard-coded to treat the dom/media clang-formatting patch as the only "ignore" changeset. This line has a sample of the result: https://dev.searchfox.org/mozilla-central/source/dom/media/AudioCaptureStream.cpp#39

As mentioned it looks ugly. And it took a long time for the indexing to finish (just over 2 hours for the m-c output-file portion to finish, vs ~9 minutes normally). But I'm pretty sure I can speed it up significantly just by adding a small cache for the prev-blame data that gets computed.
I'm happy to give a shot at the front-end side of this if you want. What about showing the non-ignored changeset first, and then doing something like:

<details>
  <summary>N ignored changesets</summary>
  <!-- The ignored commits, maybe styled a bit different -->
</details>

FWIW I think searchfox is so good at jumping across blame that jumping over the reformat is not going to be a huge deal (for me personally), but hey, improvements are always nice :-)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #10)
> For now I'm just
> assuming there will be a .git-blame-ignore-revs file in the root of the tree
> with the right SHAs but that probably won't be the case since nobody cares
> about poor ole gecko-dev anymore.

False!  I'm planning to put both sha1's in that file soon.  :-)
Depends on: 1511501
Updated patches at https://github.com/staktrace/mozsearch/commits/hyperblame

Now has caching (took a surprising amount of fighting with the borrow checker), and runs in a reasonable amount of time, but probably could be optimized further. I did another dev push, this time including both the dom/media and full-tree rewrites as ignored commits.

UX still needs updating, but after that it's probably ready for review.
I tweaked the UX to what Emilio suggested in comment 13 - I didn't know about <details> before, pretty cool! :)

Applied the JS changes manually to the last dev push, so you can see it in action on dev.searchfox.org currently. Cleaned up the patches a bit, and pushed them up for review:

https://github.com/mozsearch/mozsearch/pull/170
So this is deployed now but:
(a) somebody (sheriffs?) cancelled the searchfox indexing jobs from this morning along with some other stuff, so the latest searchfox is still using dec 4 data. Which means the ignore-revs file doesn't have the full-tree reformat changeset, just the dom/media one. I've triggered the jobs on the latest m-c push and will rerun the indexing
(b) your browser will likely have cached blame data from yesterday and that's not compatible with the new searchfox JS code. So you might get undefined blame/errors until you clear the browser cache or wait for it to expire (expiry is ~24 hours from whenever you last visited that particular line of code).

I'll close this bug since the code is deployed and at least is working on the dom/media code.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.