Thanks man, much appreciated

Change 226234 had a related patch set uploaded (by MZMcBride):
Don't match Phabricator task IDs inside URLs

https://gerrit.wikimedia.org/r/226234

@chasemp: Could you backport https://gerrit-review.googlesource.com/#/c/68377/ to our Gerrit installation?

In T75997#1469791, @MZMcBride wrote:

@chasemp: Could you backport https://gerrit-review.googlesource.com/#/c/68377/ to our Gerrit installation?

From my point of view, it's not worth it.
Although the change itself is simple, there have been quite some changes between WMF version of its-base (the relevant project) and upstream.
WMF is talking since some time about upgrading gerrit.
If gerrit got upgraded, one could bump the plugin for free ... and the gerrit upgrade would solve many other issues too.

Also, misleadingly, for secure.phabricator.com:
https://gerrit.wikimedia.org/r/#/c/186957/14/pywikibot/comms/rcstream.py

In T75997#1470183, @QChris wrote:

In T75997#1469791, @MZMcBride wrote:

@chasemp: Could you backport https://gerrit-review.googlesource.com/#/c/68377/ to our Gerrit installation?

From my point of view, it's not worth it.
Although the change itself is simple, there have been quite some changes between WMF version of its-base (the relevant project) and upstream.
WMF is talking since some time about upgrading gerrit.
If gerrit got upgraded, one could bump the plugin for free ... and the gerrit upgrade would solve many other issues too.

I have really no insight in gerrit in that way @demon or @greg would make this call

Change 256663 had a related patch set uploaded (by Thiemo Mättig (WMDE)):
Avoid breaking full phabricator URLs

https://gerrit.wikimedia.org/r/256663

Change 226234 abandoned by Ricordisamoa:
Don't match Phabricator task IDs inside URLs

Reason:
:(

https://gerrit.wikimedia.org/r/226234

I tried to link to T119209#1912745 from https://gerrit.wikimedia.org/r/#/c/254440/ and caused weirdness in the escaping code. Basically, I tried to work around the lack of fix for T76459: Allow links to phabricator comments from gerrit messages. and failed.

The way gerrit matches links is very limited, there is no straightforward way to fix it.

@mmodell: If you think that Thiemo's patch does not resolve this task, please explain in the review.

Now that Gerrit has been upgraded to version 2.12.2, can we fix this task?

@MZMcBride hi, yes I'm working on a fix and I think I found a good fix. I'm uploading a patch now.

It is based on @thiemowmde suggestion here https://gerrit.wikimedia.org/r/#/c/242237/2/modules/gerrit/templates/gerrit.config.erb

and worked reliably and didn't break anything from my testing.

Testing on http://gerrit-test.wmflabs.org/

Change 302129 had a related patch set uploaded (by Paladox):
Gerrit: Avoid breaking full phabricator URLs

https://gerrit.wikimedia.org/r/302129

Change 302129 merged by Dzahn:
Gerrit: Avoid breaking full phabricator URLs

https://gerrit.wikimedia.org/r/302129

This should now be partially fixed. The only way it breaks now is if you link to a comment which I have a patch for/

Change 302229 had a related patch set uploaded (by Paladox):
Gerrit: Support having phab commits as links

https://gerrit.wikimedia.org/r/302229

@MZMcBride what do you think of https://gerrit.wikimedia.org/r/#/c/302229/

Change 302229 merged by Dzahn:
Gerrit: Support linking to phabricator comments

https://gerrit.wikimedia.org/r/302229

I merged Paladox' change https://gerrit.wikimedia.org/r/#/c/302229/16 and just linked from there to
T75997#2518198 directly.

T75997#2518198 works, but https://phabricator.wikimedia.org/T75997#2518198 continues to fail at https://gerrit.wikimedia.org/r/302229. Should I re-open this task or file a new task?

Maybe re open this task please. But not much people should notice unless they link to phabricator comments

In T75997#2556111, @MZMcBride wrote:

T75997#2518198 works, but https://phabricator.wikimedia.org/T75997#2518198 continues to fail at https://gerrit.wikimedia.org/r/302229. Should I re-open this task or file a new task?

As title and description says, the task was about full links, so in fact the task shouldn't have been closed as it hasn't been fixed.

As suggested for a long, long time now (see https://gerrit.wikimedia.org/r/256663) you should change (?!\") to (?![#\"]) or even (?![#\"<]). I strongly believe this will solve most, if not all of the remaining issues.

It will break most things actually. It fixes full links but then breaks the short links. As I have tested that.

What exactly does "break" mean? How does the addition of a # character break inputs that do not contain this character?

The current regular expression (which replaces "\\bT(\\d+)(#\\d+)?\\b(?!\")" with https://phabricator.wikimedia.org/T$1$2) destroys some links because the HTML <a href="https://phabricator.wikimedia.org/T1#2">…</a> contains two possible substrings that seem to fit: But the obvious pick T1#2 is not used any more (finally!) because it is followed by a double quote, which is excluded by the regular expression. So it picks T1 and still destroys the existing HTML.

Exactly this is fixed by replacing (?!\") with (?![#\"]).

It will then still destroy HTML like <a href="…">https://phabricator.wikimedia.org/T1#2</a> and possibly make the whole situation worse because the final HTML Tidy step can not work this out any more. Which is what (?![#\"<]) will fix.

This may lead to a situation where some specific T… numbers in specific contexts are not replaced any more. Which is, in my humble opinion, acceptable. What I find unacceptable is messed up HTML.

Over at https://gerrit.wikimedia.org/r/288462, Krinkle used "%54" instead of "T" as a workaround. Example: https://phabricator.wikimedia.org/%54135170#2532152. Hackish, but effective!

@thiemowmde hi yes that's what I mean some plain T 1 for example links will break, I found they broke in the comment box but not in commit msg.

I'm wondering is there a way we can do it your way but also fix it so plain T 1 work please.

(Remove space between T and 1)

I can test any proposed solutions, since I have a test instance. But we doint want to be breaking things when it only breaks if you link to the full link with a comment Id.

Again: What does "break" mean? What exactly do you input? What exactly do you get?

@thiemowmde

You said this

This may lead to a situation where some specific T… numbers in specific contexts are not replaced any more. Which is, in my humble opinion, acceptable. What I find unacceptable is messed up HTML.

Which is true.

Is there something wrong with the question I'm now asking for the third time? Can you please, please, please explain your test scenario, what exactly you entered (please copy and paste it), what you expected and what erroneous output you got instead (again, please copy the resulting HTML source and paste it here)? Thank you very much.

@thiemowmde I have told you.

When you use your change, it causes the T1 links for example to not work in the comment box but work in commit msg. IE T1 will no longer link, whereas the full link will.

See http://gerrit-test.wmflabs.org/gerrit/#/c/16/ please

Oh wait, it seems to work now since upgrading to gerrit 2.12.4.

Change 306413 had a related patch set (by Paladox) published:
Fix phabricator expanding links

https://gerrit.wikimedia.org/r/306413

Again, what exactly do you enter in the comment box? Only the two characters T1 and nothing else? Other characters or words before or after? What happens if the comment contains spaces before and after the ticket number, e.g. xx T1 xx? Newlines?

So under which exact circumstances does it stop working when you change nothing but what I suggested above (and still strongly suggest): Expand (?!\") to (?![#\"<]). No other change.

On http://gerrit-test.wmflabs.org/gerrit/#/c/16/ I do see two comments with ticket numbers that are not part of a pasted http://… link, one on PS5 and one on PS28, and they are all blue. Which of these turn black with the proposed change?

@Paladox: Noone can look over your shoulder. So as long as you do not provide an absolutely clear list of numbered steps and avoid any paraphrasing and vagueness, things will remain unclear...

@thiemowmde hi, ok done I deployed your change on the test server, please take a look at http://gerrit-test.wmflabs.org/gerrit/#/c/16/ (look at the comment box scroll to bottom and expand comment with the plain T numbers please to see the problem.

also sorry for late reply and deploy.

the commit msg works but in the comment box it dosent please could you recheck since I deployed the change you wanted please?

it could be because plain T numbers doint include the < so they are ignored but the full links work because they include <

we should go with the patch I created since it may not be perfect but at least it makes an improvement. it makes the links look nice instead of horrible and expanded. we can also improve it further.

I agree that https://gerrit.wikimedia.org/r/306413 should be merged as it is because it is already an improvement.

@Paladox, your change on the test system helped a lot, thank you very much. Note that one of the comments contains a plain T1 in the middle of a sentence, and this one is linked. The ones that are not linked are directly followed by </p> in the HTML source, which explains everything.

Please replace (?![#\"<]) with (?![#\"]|</a>) on the test system.

@thiemowmde no thank you since your working on a fix and I test it :)

@thiemowmde I get this error
fatal: bad config file line 121 in /var/www/html/review_site/etc/gerrit

That line is

match = "\\bT(\\d+)(#\\d+)?\\b(?![#\"]|<\/a>)"

I have nothing in the logs to say what piece is failing, only the line that is causing the failures. Maybe it is because the regex is invalid or incorrect?

I fixed it by doing

match = "\\bT(\\d+)(#\\d+)?\\b(?![#\"]|<\\/a>)"

per @thiemowmde I am now doing

match = "\\bT(\\d+)(#\\d+)?\\b(?![#\"]|</a>)"

and works :) just waiting for patch to be merged. I have tested it and works. See http://gerrit-test.wmflabs.org/gerrit/#/c/16/ please.

@thiemowmde also tested and said it looked good :)

@MZMcBride this change https://gerrit.wikimedia.org/r/#/c/306413/ done by @thiemowmde should now fix

https://phabricator.wikimedia.org/T75997#2518198

see http://gerrit-test.wmflabs.org/gerrit/#/c/16/ please :)

Change 306413 merged by Dzahn:
Gerrit: Fix phabricator expanding links

https://gerrit.wikimedia.org/r/306413

:)

Woo! Thank you all for the hard work here. :-)

Change 256663 merged by Dzahn:
Gerit: Rewrite outdated comment about Gerrit-Phabricator linking

https://gerrit.wikimedia.org/r/256663