Repository navigation
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #454 +/- ##
==========================================
+ Coverage 78.93% 79.06% +0.12%
==========================================
Files 5 5
Lines 3865 3883 +18
Branches 1294 1300 +6
==========================================
+ Hits 3051 3070 +19
+ Misses 297 296 -1
Partials 517 517 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Looks good, thanks you! |
hryhoriiK97
left a comment
hryhoriiK97
left a comment
There was a problem hiding this comment.
Tested this branch against all the cases from #449 and everything still works, full test suite passes here too.
One thing I noticed, and it's actually a leftover from #449, not from this PR: the host scanner's rollback step runs even when it consumed nothing, so an empty host after http:// counts as one component. Master already links http://?q=1 and http://.com; with the new tail scan it also links http:///path, http://:3000 and http://[.
A one-line guard on the rollback (&& off != scan_beg) fixes it and keeps the suite green, without touching any of the tail logic here. Happy to send it as a small follow-up PR once this is merged, or you can fold it in, whichever you prefer.
Otherwise looks good to me.
|
@hryhoriiK97 Good catch. Thx to both of you. |
The segmented path/query/fragment allowlists reject or truncate ordinary URLs containing colons, repeated semicolons, redirect URLs and percent escapes. A numeric port after a host is also rejected, and unmatched opening parentheses prevent the canonical GFM www example from linking. Scan the tail continuously with md4c's Unicode decoding and resolved-mark checks, then trim terminal punctuation and excess closing parentheses. Reuse ISUNICODEWHITESPACE_ so the ASCII-only build remains supported. Keep the existing host, preceding-boundary and e-mail rules. No dependency or public API is added. Host-colon tails remain permissive rather than validating numeric ports; document this policy and its overlap with #449. Extend the existing permissive-autolink specification with 12 examples covering paths, queries, fragments, parentheses, Unicode, terminal punctuation and Markdown boundaries. Nine examples fail before the fix. The full Debug/Release suites and focused sanitizer tests pass locally. Downstream report: https://bugs.documentfoundation.org/show_bug.cgi?id=169429 Downstream patch: https://gerrit.libreoffice.org/c/core/+/212109 Related proposal: #449 Signed-off-by: Emanuel Schorsch <[email protected]> Assisted by GPT 6.1 Sol High
Since the URL tail runs until whitespace (#454) and only excess ')' is trimmed at its end, a URL enclosed in square brackets or braces swallows the closing one: "[http://example.com/x]" links "http://example.com/x]", where 0.6.0 kept the ']' outside the link. Apply the same rule to ']' and '}' as to ')': only excess closing ones at the end of the tail are excluded, so "https://example.org/a[b]c" is still linked whole.
There was a conflict between PR #449 and #452.
The first added support specifically for port numbers and allowed one-component hostnames. The other one generalizes/relaxes the syntax in a more substantive way but was not as strict vis-a-vis the port numbers.
This is a merge of both (or strictly speaking #452 rebased on top of master witch now includes #449; plus removal/fix of invalidated test cases). I'll ask authors of both PRs for kind review/testing in your environment and approval of this.
cc @hryhoriiK97 @emschorsch
(Also note even after merging this I'll keep this in my mind with a little question mark, as the permissive autolinks are always a dance between false positives and false negatives and only some time will show whether some false positives are reported; certainly do not expect some official release with this in a short time period.)