roffit: fix special characters and broken links #37

jhauga · 2023-07-08T00:22:49Z

Pull Summary

Tested this with files matching issues this pull request resolves and with repos that have an extensive amount of files roffit can convert to html. Made two supporting repos with test results. They are:

File Test - roffit-test
- ctrl+click - RESULTS - File Test
- ctrl+click - roffit-test repository
Repo Test - roffit-full-test
- ctrl+click - RESULTS - Repo Test
- ctrl+click - roffit-full-test repository

The two tests used more or less the same procedure, and focused on:

Files with differences
Build time

The two support repos have full details of the procedure, but in short the steps were:

Extract data and ready test elements
Clone source (bagder) and pull (jhauga) roffit repo.
Run roffit and time each runs' build time.
Determine files with differences.
Calculate stats for files with difference and the build times.
Output results to html file.

bagder

I don't see any actual roffit changes here? Does it only add tests? How can it then solve any problems?

Also, this has to be possible to test with much less data added. 23K of data is too excessive.

bagder · 2023-07-11T09:44:25Z

I approve of extending the tests to test for more problems, but I think this PR goes about it the wrong way.

A first take would be to extend the existing test case with more text for the problems you have identified. Not adding thousands of lines of new files.

roffit is not curl specific, it converts man pages to html so it has no knowledge of or special handling for curl related issues.

jhauga

Reviewed with comments on Makefile, fixBrokenLink, fixSpecialCharacters, and roffit files.

bagder · 2023-07-14T20:38:35Z

Sorry, but this PR does not fix roffit as described. It just fixes two minor static HTML texts.

jhauga · 2023-07-14T20:46:31Z

Sorry, but this PR does not fix roffit as described. It just fixes two minor static HTML texts.

That's fine. Thank you for considering it. Adding two bash scripts was the only way I was able to resolve issue 36. Learning more about perl is on my todo list, so if the issue is still up in several months I may try again.

Regardless, this has been a very enjoyable and rewarding process!

bagder · 2023-07-14T20:51:59Z

Separate scripts cannot solve this issue. roffit is the program that runs that converts nroff files, so you need to fix that file. Having separate shell scripts is not a solution to the problem!

jhauga · 2023-07-14T21:07:06Z

Separate scripts cannot solve this issue. roffit is the program that runs that converts nroff files, so you need to fix that file. Having separate shell scripts is not a solution to the problem!

Ok. Closing this for now and will reopen when able to implement in roffit.

bagder

This is totally unacceptable code. Make it clean pure perl and do not generate temp files.

jhauga

lines 259 to 285 - perl changes

bagder · 2023-07-19T22:31:24Z

roffit

@@ -256,6 +256,33 @@ sub linkfile {
            # convert https://, http:// and ftp:// URLs to <a href> links
            $field =~ s/(^|\W)((https|http|ftp):\/\/[a-z0-9\-._~%:\/?\#\[\]\@!\$&'()*+,;=]+)/$1<a href=\"$2\">$2<\/a>/gi;

+            # remove punctuation characters at end of url
+            $field =~ s/(href=\")([^\"]*)(&quot;)/$1$2/g;


This does not remove punctuation. What is this for?

" seemed to be a troublemaker so for following lines to work it had to be removed first in process.

bagder · 2023-07-19T22:32:21Z

roffit

@@ -256,6 +256,33 @@ sub linkfile {
            # convert https://, http:// and ftp:// URLs to <a href> links
            $field =~ s/(^|\W)((https|http|ftp):\/\/[a-z0-9\-._~%:\/?\#\[\]\@!\$&'()*+,;=]+)/$1<a href=\"$2\">$2<\/a>/gi;

+            # remove punctuation characters at end of url
+            $field =~ s/(href=\")([^\"]*)(&quot;)/$1$2/g;
+            $field =~ s/(href=\"(http:\/\/|https:\/\/|ftp:\/\/)\")*([,.;()*])*(\">)/$1$4/g;


Shouldn't this rather correct the pattern on line 257 to not include punctuation as terminating symbols?

Tried with variations of pattern. Either my corrections were flawed or the URLs need to be built first.

Shouldn't this rather correct the pattern on line 257 to not include punctuation as terminating symbols?

Kept getting unexpected results like <a href="https://curl">https://curl</a>.se/, or inline punctuations being removed.

roffit

bagder · 2023-07-19T22:37:18Z

roffit

+            # add anchor and anchor links to special character options
+            my $specialcharacters = "`~!@\$%^*()-_=+{};:\'\\|,.?";
+            $field =~ s/(<a name=)\"-\"(><\/a>)(<span class=\"nroffip\">-)([$specialcharacters]|[\[\]])(,|<\/span>)/$1\"-$4\"$2$3$4/g;
+            $field =~ s/(<a class=\"emphasis\") href=\"#-\">-([$specialcharacters]|[\[\]])(,|<\/a>)/$1 href=\"#-$2\">-$2/g;


I think these changes should rather be done as updates to text2name() and do_encode() instead of adding this extra stage of edits.

Unable to achieve in subroutines, either anchor links were removed, all anchors (name attribute) removed, or no effect.

I think these changes should rather be done as updates to text2name() and do_encode() instead of adding this extra stage of edits.

With text2name and do_encode new things break. Some examples:

name="socks5h://" (should be name="socks5h")

name="AUNDERSCORE" (shoud be name="A_UNDERSCORE")

name=""--any-option" (2 "" at start).

bagder · 2023-07-19T22:37:54Z

roffit

+                $field =~ s/(<a name=)\"-\"(><\/a>)(<span class=\"nroffip\">-)($htmlentity)(,|<\/span>)/$1\"-$4\"$2$3$4/g;
+                $field =~ s/&#35;/hash/ | $field =~ s/&amp;/ampersand/ | $field =~ s/&#39;/single-quote/ |
+                $field =~ s/&lt;/less-than/ | $field =~ s/&gt;/greater-than/;
+            }


I think these ones could also be included into that change.

Created subroutine field_anchor to handle repeating blocks. Required $field and $htmlentity to be defined globally.

roffit

bagder · 2023-07-19T22:43:05Z

testpage.1

+
+.IP "-?, --special-char"
+Options with special characters will be included in anchors.
+Such as this option with special character \fI\-?, \-\-special\-char\fP.


I don't see any code handling this new option. Plus, I don't think this needs to be managed by an option - I think it makes sense to improve the general handling of anchors and links so it should probably be used by default.

For the test build. Moved into description. Anyway options are named wrong (--mumbo and --jumbo), which gave me impression that it was only for build test purposes. Does not seem like an issue though (maybe good for someone to find who has never contributed on github also) .

testpage.1

jhauga · 2023-07-21T21:24:20Z

Regarding some suggested edits:

Unable to achieve fix with text2name and do_encode.
In short - new things break.

Quick takeaway ( where - (1) should be => (2) becomes ):
name="--any-option" => name=""--any-option" ("" at start)
name="ANY_NAMES" => name="ANYNAMES" (_ is removed)

For details see:

collapsed section

Detailed report (explains above):

Since curl.1 is a good use case for thoroughly testing a roffit
build I made two files, running "diff" command on them:

with current badger roffit
with (multiple) edited roffit

Below are sample outcomes from two instances of build, and then results
from using current edits.

do_encode --- example case removes ":

sub do_encode($) {
    return encode_entities(shift, q{<>&'#});
}

results in:
name=""--any-option" (2 "" at start)

text2name --- example case adds special characters to $text:

sub text2name { .....
   $text =~ s/[^a-zA-Z0-9-`~!@\$%^*()-_=+{};:\'\\|,.?]//g;
   ...}

results in:
name="socks5h://" (should be name="socks5h").
or
name="AUNDERSCORE" (shoud be name="A_UNDERSCORE").

Several variations of the above edits were done with similar (if not the same) outcome.

So in conclusion; either my approaches were flawed, or I need to study the roffit
file some more.

When "diff" ran using -- 2. with current pull roffit --

roffit built as expected, nothing new broken
Outputted difference resolved roffit #36 (didn't know that'd close it) and #42.

Edits

Reordered htmlentity variable to be same order as call in do_encode (excluding ").
Edited regular expression for special character options to fix a missed bug
that removed inline comma in option text. (i.e. "-? --opt" => "-?, --opt").
Removed condition in field_anchor subroutine to reduce line numbers,
moving line needed to line number 289.

This comment was marked as outdated.

Sign in to view

bagder mentioned this pull request Jul 8, 2023

curl.1 problems when rendered on website curl/curl#11381

Closed

4 tasks

bagder requested changes Jul 10, 2023

View reviewed changes

This comment was marked as outdated.

Sign in to view

jhauga changed the title ~~Special Characters and Broken Links~~ roffit: fix special characters and broken links Jul 13, 2023

jhauga commented Jul 13, 2023

View reviewed changes

jhauga requested a review from bagder July 14, 2023 20:17

jhauga closed this Jul 14, 2023

jhauga reopened this Jul 16, 2023

jhauga mentioned this pull request Jul 16, 2023

roffit: change <style> and </p> #40

Closed

bagder requested changes Jul 18, 2023

View reviewed changes

jhauga commented Jul 19, 2023

View reviewed changes

bagder reviewed Jul 19, 2023

View reviewed changes

jhauga requested a review from bagder July 21, 2023 21:38

jhauga marked this pull request as draft August 5, 2023 21:08

jhauga marked this pull request as ready for review August 26, 2023 23:27

jhauga marked this pull request as draft September 3, 2023 17:36

jhauga marked this pull request as ready for review September 4, 2023 01:03

jhauga closed this Sep 29, 2023

jhauga force-pushed the master branch from a2afb03 to 9310b6b Compare September 29, 2023 16:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

roffit: fix special characters and broken links #37

roffit: fix special characters and broken links #37

jhauga commented Jul 8, 2023 •

edited

Loading

This comment was marked as outdated.

bagder left a comment

This comment was marked as outdated.

bagder commented Jul 11, 2023

jhauga left a comment •

edited

Loading

bagder commented Jul 14, 2023

jhauga commented Jul 14, 2023

bagder commented Jul 14, 2023 •

edited

Loading

jhauga commented Jul 14, 2023 •

edited

Loading

bagder left a comment

jhauga left a comment

bagder Jul 19, 2023

jhauga Jul 20, 2023

bagder Jul 19, 2023

jhauga Jul 20, 2023

jhauga Jul 21, 2023

bagder Jul 19, 2023

jhauga Jul 20, 2023

jhauga Jul 21, 2023

bagder Jul 19, 2023

jhauga Jul 20, 2023

bagder Jul 19, 2023

jhauga Jul 20, 2023

jhauga commented Jul 21, 2023 •

edited

Loading

Detailed report (explains above):

When "diff" ran using -- 2. with current pull roffit --

roffit: fix special characters and broken links #37

roffit: fix special characters and broken links #37

Conversation

jhauga commented Jul 8, 2023 • edited Loading

Pull Summary

This comment was marked as outdated.

bagder left a comment

Choose a reason for hiding this comment

This comment was marked as outdated.

bagder commented Jul 11, 2023

jhauga left a comment • edited Loading

Choose a reason for hiding this comment

bagder commented Jul 14, 2023

jhauga commented Jul 14, 2023

bagder commented Jul 14, 2023 • edited Loading

jhauga commented Jul 14, 2023 • edited Loading

bagder left a comment

Choose a reason for hiding this comment

jhauga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhauga commented Jul 21, 2023 • edited Loading

Regarding some suggested edits:

For details see:

Detailed report (explains above):

When "diff" ran using -- 2. with current pull roffit --

Edits

jhauga commented Jul 8, 2023 •

edited

Loading

jhauga left a comment •

edited

Loading

bagder commented Jul 14, 2023 •

edited

Loading

jhauga commented Jul 14, 2023 •

edited

Loading

jhauga commented Jul 21, 2023 •

edited

Loading