1285657 - Rewrite MXR links in comments to DXR

Status: RESOLVED WONTFIX

(bugzilla.mozilla.org :: General, task)

Description

[Lawrence Mandel [:lmandel] (use needinfo)]

MXR has been decommissioned. We have a lot of links to MXR in Bugzilla comments. DXR handles most MXR search URLs. Can we rewrite all of the MXR URLs in comments to DXR? ie. s/mxr/dxr/

Comment 1

[Boris Zbarsky [:bzbarsky]]

s/mxr/dxr/ won't quite work because of dxr's different (worse) syntax for which lines to highlight.  Some munging of the URLs will be needed at least for that.

Comment 2

[Dão Gottwald [:dao]]

(In reply to Boris Zbarsky [:bz] from comment #1)
> s/mxr/dxr/ won't quite work because of dxr's different (worse) syntax for
> which lines to highlight.  Some munging of the URLs will be needed at least
> for that.

Or we could fix DXR to use MXR's syntax if that's better.

Comment 3

[Boris Zbarsky [:bzbarsky]]

I had tried to get dxr folks to use mxr's syntax in the past and failed.  Others are welcome to try too, of course.

The key part is that in dxr the set of lines to highlight is a query param, not a fragment identifier, so it's possible to specify it independently of "where should I scroll" information.

Comment 4

[Dylan Hardison [:dylan] (he/him)]

(In reply to Boris Zbarsky [:bz] from comment #3)
> I had tried to get dxr folks to use mxr's syntax in the past and failed. 
> Others are welcome to try too, of course.
> 
> The key part is that in dxr the set of lines to highlight is a query param,
> not a fragment identifier, so it's possible to specify it independently of
> "where should I scroll" information.

this is the mark= syntax in the query string, correct?
http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsWindowDefs.h?rev=9104abe421a8&mark=23-41#23
for instance.

It looks like mark= is never repeated, so we can get by with a simple regex rather than parsing each URI.

Comment 5

[Dylan Hardison [:dylan] (he/him)]

Here are some more examples with mark= being a range:

https://mxr.mozilla.org/mozilla-central/source/dom/html/nsHTMLDocument.cpp?rev=cba6ff5bd32b&mark=1466-1469,1484-1484#1466
https://mxr.mozilla.org/mozilla-central/source/dom/html/nsHTMLDocument.cpp?rev=cba6ff5bd32b&mark=1667-1667,1712-1712#1665
https://mxr.mozilla.org/mozilla-central/source/dom/html/nsHTMLDocument.cpp?rev=cba6ff5bd32b&mark=2410-2410#2410

Comment 6

[Boris Zbarsky [:bzbarsky]]

Yes, this is the mark=N,NNN-MMM,MM,NN-MM syntax.  It takes a comma-separated list of things that are numbers or ranges.

Comment 7

[Sebastin Santy [:seban]]

Attachment: rev1

The revision and mark is not included, as I don't know exactly how is dxr url is composed of versus that of mxr ones.

Comment 8

[Masatoshi Kimura [:emk]]

(In reply to Sebastin Santy [:seban] from comment #7)
> The revision and mark is not included, as I don't know exactly how is dxr
> url is composed of versus that of mxr ones.

Then do not rewrite, please. Unrevisioned dxr URL is much worse than the "broken" mxr URL. The latter contains a sufficient information to rebuild the correct dxr URL. At least URL rewriter should support URLs that the MXR to DXR add-on[1] supports.

[1] https://addons.mozilla.org/en-US/firefox/addon/mxr-to-dxr-webextension/

Comment 9

[Sebastin Santy [:seban]]

(In reply to Masatoshi Kimura [:emk] from comment #8)
> (In reply to Sebastin Santy [:seban] from comment #7)
> > The revision and mark is not included, as I don't know exactly how is dxr
> > url is composed of versus that of mxr ones.
> 
> Then do not rewrite, please. Unrevisioned dxr URL is much worse than the
> "broken" mxr URL. The latter contains a sufficient information to rebuild
> the correct dxr URL. At least URL rewriter should support URLs that the MXR
> to DXR add-on[1] supports.
> 
> [1] https://addons.mozilla.org/en-US/firefox/addon/mxr-to-dxr-webextension/

I'll include the revision and mark parameters once I get the format of dxr.

Comment 10

[Sebastin Santy [:seban]]

I have already made a provision to accept the revision, in the patch, but have not included in the url.

Comment 11

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Doing rewriting of comment history seems like a bad idea; it breaks our historical record, and if we get things wrong, it's going to be very difficult to figure out the history later.  (And the chance of getting people to do additional bugzilla mass changes to fix the mistakes in the first one(s) seems likely to get lower over time.)

It seems far better to make mxr.mozilla.org links redirect appropriately, which is also the appropriate thing to do in general (see, e.g., https://www.w3.org/Provider/Style/URI.html from 1998).

Comment 12

[Sebastin Santy [:seban]]

(In reply to David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) from comment #11)
> Doing rewriting of comment history seems like a bad idea; it breaks our
> historical record, and if we get things wrong, it's going to be very
> difficult to figure out the history later.  (And the chance of getting
> people to do additional bugzilla mass changes to fix the mistakes in the
> first one(s) seems likely to get lower over time.)
> 
> It seems far better to make mxr.mozilla.org links redirect appropriately,
> which is also the appropriate thing to do in general (see, e.g.,
> https://www.w3.org/Provider/Style/URI.html from 1998).

Thanks for the feedback David.

Actually I am not rewriting the url. It is stored in the db as it is. It is just that when they are retrieved and displayed in the comments, they'll be shown as dxr.mozilla.org/... 

Technically speaking, what I have done is just match the mxr.mozilla.org with a regex and return a dxr.mozilla.org link (to be displayed in comments), taking the parameters from the regex.

Comment 13

[Dylan Hardison [:dylan] (he/him)]

Comment on attachment 8769500 [details] [review]
rev1

We need to take both the rev= and mark= parameters into account.

for mark, it should be added to the fragment.

Perhaps copying what the extension Gijs wrote is a good idea?
https://github.com/gijsk/mxr-to-dxr/blob/master/changealllinks.js

Given we have to muck with rev= and =mark paremeters, you might want to use the URI module: https://metacpan.org/pod/URI (and also https://metacpan.org/pod/URI::QueryParam)

use URI;
use URI::QueryParam;

my $uri = URI->new($uri);

my $mark = $uri->query_param_delete("mark");
$uri->fragment($mark);

etc

Comment 14

[Dylan Hardison [:dylan] (he/him)]

my $uri = URI->new($uri_string) that is.

Comment 15

[Dylan Hardison [:dylan] (he/him)]

(In reply to David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) from comment #11)
> Doing rewriting of comment history seems like a bad idea; it breaks our
> historical record, and if we get things wrong, it's going to be very
> difficult to figure out the history later.  (And the chance of getting
> people to do additional bugzilla mass changes to fix the mistakes in the
> first one(s) seems likely to get lower over time.)
I think this is reasonable short term solution. We're not changing any data, only how it is presented.
We have to do this with git.mozilla.org links already, for instance.

> It seems far better to make mxr.mozilla.org links redirect appropriately,
> which is also the appropriate thing to do in general (see, e.g.,
> https://www.w3.org/Provider/Style/URI.html from 1998).

I don't know the dxr code, or even if they're open to taking a pull request to handle mxr-style links.
I know that to do this dxr-side, you'd have to do it in the front end because fragments are not visible to service-side HTTP requests. You could probably do this with a static-content mxr.mozilla.org.

Comment 16

[Masatoshi Kimura [:emk]]

(In reply to David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) from comment #11)
> (see, e.g.,
> https://www.w3.org/Provider/Style/URI.html from 1998).

<offtopic>
I know this document has the following disclaimer:
> This is an outstanding reason, which applies for example to many W3C pages including
> this one: so do what I say, not what I do.

But the recent tendency of the W3C URI is almost a joke (e.g. https://heycam.github.io/webidl/ ).
</offtopic>

Comment 17

[Sebastin Santy [:seban]]

Comment on attachment 8769500 [details] [review]
rev1

Changed from regex to Perl URI. Includes the revision and mark.

Comment 18

[Dylan Hardison [:dylan] (he/him)]

Comment on attachment 8769500 [details] [review]
rev1

see pull request comments.

Comment 21

[Dylan Hardison [:dylan] (he/him)]

Comment on attachment 8769500 [details] [review]
rev1

talking with seban I think we'll have any kinks worked out by next week, and there's still a need for this even if mxr redirects itself.

Comment 22

[Dylan Hardison [:dylan] (he/him)]

Comment on attachment 8769500 [details] [review]
rev1

This does appear to work, but it's really messy. Either just using the URI splitting functions, or using the URI object would be good but this mix is really messy.

Comment 23

[Sebastin Santy [:seban]]

Yeah I also felt so, I'll go through it and try to make it on one side.

Comment 24

[:glob ✱]

Old request; no value in performing this work now.