1673468 - searchfox blame missing a commit

Status: RESOLVED FIXED

(Webtools :: Searchfox, enhancement)

Description

[Scott [:thecount] Downe]

Came across this curiosity and wasn't able to explain it, so might be a bug.

The blame on this line here: https://searchfox.org/mozilla-central/source/browser/components/pocket/content/pktApi.jsm#727 return getMultipleTestOption("panelSignUp", { control: 1, v1: 0, v2: 0 });

Suggests that last time that line was changed was roughly 4 years ago in this bug https://bugzilla.mozilla.org/show_bug.cgi?id=1304142.

But I can confirm it was changed after that, roughly 2 years ago, here: https://phabricator.services.mozilla.com/D4151

Seems like searchfox is missing a bit of history for the blame on that line.

I also know that file has moved once https://bugzilla.mozilla.org/show_bug.cgi?id=1488813, and searchfox seems to know to maintain history from the old file location. Maybe something went wrong importing changes from the old file and the new? The line it is reporting as blame is pulling from the old file location, but the last change on it was actually from the current file location. The change it's reporting is also before phabricator, and the actual last time it was changed was with phabricator.

Comment 1

[Andrew Sutherland [:asuth] (he/him)]

If you expand the "1 ignored changesets" in the blame UI you'll find that Searchfox is trying to help avoid a reformatting change that happened. If you click on the "Show latest version without this line" link of that ignored changeset, you'll end up at https://searchfox.org/mozilla-central/rev/f9f59140398bc4d04d840e8217c04e0d7eafafb9/browser/components/pocket/content/pktApi.jsm#703 which suggests some kind of confusion about where that line maps to after compensating for the skipped blame. If you manually find the correct line in that revision, which is https://searchfox.org/mozilla-central/rev/f9f59140398bc4d04d840e8217c04e0d7eafafb9/browser/components/pocket/content/pktApi.jsm#681 and look at its blame, then that properly identifies https://hg.mozilla.org/mozilla-central/rev/aa6160e3fc5ca38918802ff2971233aa62ca2d22.

This is obviously not obvious, just elaborating on what's going on. I'm going to make this depend on the tracking bug for this general variety of problem.

Comment 2

[Botond Ballo [:botond]]

Seems similar to bug 1639451.

Comment 3

[Kelsey Gilbert [:jgilbert]]

https://searchfox.org/mozilla-central/rev/c3894603d85d2e45dbebf9697f1951ab998b22cb/gfx/gl/GLContext.cpp#756
Shows:

Bug 1262265 - Cleanup GLContext symbol init. - r=jrmuizel
Jeff Gilbert <[e-mail]>, Thu, 21 Apr 2016 16:32:18 -0700
Ignored:

Bug 1511181 - Reformat everything to the Google coding style r=ehsan a=clang-format
Sylvestre Ledru <[e-mail]>, Fri, 30 Nov 2018 11:46:48 +0100

If I do the naive "latest without this line", I don't see the line anymore, and same with "earliest with".
It's only if I go to 1511181's "latest without this line" option that the blame then correctly reads:

Bug 1429963 - Don't allow RBAB on Mesa for now. - r=lenzak
Jeff Gilbert <[e-mail]>, Wed, 17 Jan 2018 12:22:01 -0800

I would rather not ignore the commits if it causes wrong information, and it sounds like I kind of have to check them anyway if I want to be confident in what I'm reading.

The current approach risks confusion, which I worry that we can afford.
I am trying to fix a sec-bug and this came up during part of the investigation. Luckily it was obviously wrong this time, so merely confusing and not dangerous, but we really do need a resolution here that makes this reliable.

I've run into this before, but I can't find the previous bug like this that I contributed to.

Can we bump the priority here?

Comment 4

[Kartikaya Gupta (email:[e-mail])]

(In reply to Kelsey Gilbert [:jgilbert] (previously Jeff) from comment #3)

Can we bump the priority here?

Sure! But searchfox is not officially staffed so it won't make much difference :/

That being said I'm inclined to remove the current whitespace-skipping code because it feels like we're stuck in a local maxima and maybe shoving us out of the local maxima will inspire somebody to do it better.

Comment 5

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #4)

(In reply to Kelsey Gilbert [:jgilbert] (previously Jeff) from comment #3)

Can we bump the priority here?

Sure! But searchfox is not officially staffed so it won't make much difference :/

To expand on :kats' comment a little; searchfox has primarily operated (intentionally) as a side-project of platform engineers who discuss their level of involvement with their managers. It's obvious that the current equilibrium isn't great, as it's primarily just me doing feature development since :kats left Mozilla (although still making some very major contributions since leaving! :), so I've been trying to get a new searchfox learn/hack week that would ideally help those who attend also discuss ongoing involvement on searchfox at a firefox engineering level with their managers. Unfortunately that didn't work, so I've run the meta-question up my direct management chain.

I would definitely encourage you to surface the limitations of searchfox and how it impacts your work up your own management chain and whether you're potentially interested in contributing to searchfox yourself. I think it definitely helps to make it clear that there are direct concrete things that can be done that will help reduce known, existing manual labor that potentially has to be re-done over and over and that would be expedited via enhancements to searchfox.

I think the blame experience of searchfox is quite important and significant and we do see problems like this coming up again and again (hence :kats' comment about removing the feature). I've been primarily focused on semantic analysis issues and while I am very interested in the potential of bug 1517978 in particular as a way to overhaul things, it's definitely not something I realistically expect to get to this year (and has its own logistical issues, ex its comment 7). (And just to be clear, :kats has done incredible work in improving the blame process both at a "how good is the blame?" level as well as the logistics by porting it to rust, making it run faster, make it easier to run, etc. So significant progress has been made, but as :kats says, we have somewhat hit a local maximum with non-semantically aware line-based diff, which broadly continues to be the state of the art.)

Comment 6

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #4)

That being said I'm inclined to remove the current whitespace-skipping code because it feels like we're stuck in a local maxima and maybe shoving us out of the local maxima will inspire somebody to do it better.

I generally agree with this both on the "encourage more searchfox contribution in the org" and "it's not entirely obvious what's going on from the UX and people are definitely running into the edge cases in a way is counterproductive" axes. (Note that I again want to emphasize that searchfox intentionally exists in a weird space as a "commons" resource and this fact is not inherently obvious; my rationale here is about trying to improve the equilibrium around searchfox development, recognizing that searchfox definitely benefits from direct platform engineering expertise/experience and that moving it into a separate part of the org isn't necessarily a panacea.)

That said, maybe we could keep the feature but flip the positions in the blame box? Like, right now it's [magic result, ignored result] and we could instead make it [ignored result, magic result] where the "1 ignored changeset" flavor text could then become "Best guess at where this line came from but could be wrong:". Experts could then look at the diff hunk and determine whether that seems like it's the right guess or not, but would hopefully be appropriately warned.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

(In reply to Andrew Sutherland [:asuth] (he/him) from comment #6)

That said, maybe we could keep the feature but flip the positions in the blame box?

I think my preference is to remove the feature entirely. Flipping the positions would leave us in roughly the same local maximum and I don't think would really inspire/torment users enough to drive a better implementation.

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Attachment: GitHub Pull Request

Comment 9

[Kartikaya Gupta (email:[e-mail])]

This is rolled out now, so I'll mark this fixed.

Comment 16

[Kelsey Gilbert [:jgilbert]]

I just wanted to say, thanks for all the work on SearchFox, and I sympathize with the bandwidth/staffing issues.