bugGNU roff - Bugs: bug #59442, [PATCH] groff.cpp: move soelim...

 
 

bug #59442: [PATCH] groff.cpp: move soelim before preconv in constructed pipeline

Submitter:  Bjarni Ingi Gislason <bjarniig>
Submitted:  Tue 10 Nov 2020 05:26:03 PM UTC
   
 
Category:  General Severity:  1 - Wish
Item Group:  Feature change Status:  Need Info
Privacy:  Public Assigned to:  gbranden
Open/Closed:  Open Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Wed 03 Jan 2024 02:25:27 AM UTC, comment #6: 

comment #5:

> It occurs to me that I hadn't actually written up this idea with these
> particulars before.  This should probably be a new ticket.


Now bug #65108.

Dave <barx>
Group Member
Tue 02 Jan 2024 10:03:34 PM UTC, comment #5: 

comment #4:

> comment #1:
> > 1. What happens if a .so(urced) file has a non-ASCII character
> > in its filename?
>
> This seems more likely to work with the proposed change than without
> it.  Currently, with preconv preceding soelim, preconv will change the
> line ".so naïve_file" into ".so na\[u00EF]ve_file", guaranteed to
> fail.  If soelim comes first, it at least has a chance of succeeding.


Another thing I want to do is specialize the formatter's logic when
handling file name arguments given to requests.

Presently, GNU troff calls the same internal function to gather an
argument that is a file name as it does to gather a *roff identifier.

Maybe that made sense in 1990, but it doesn't today.  File names can
contain spaces and non-ASCII characters (in whatever encoding the file
system happens to support).

Since these arguments used mainly as-is, handed off to standard C
library functions like `fopen()`, I don't anticipate many problems here
(O Fortuna, seize my hostage).  The only exception to that I can think
of off the top of my head is the value of the `.F` register, which
interpolates a file name.  We will need some way to represent this such
things as output.  At first blush, it seems to me that we can
interpolate spaces as-is (if you want the argument quoted, do that
yourself in context), and any unprintable non-Basic Latin bytes in
groff's \[u00xx] notation.

I say "\[u00xx]" instead of "\[uXXXX]" because we have no way of knowing
what the file system's character encoding is.  Might be ISO 8859-1,
UTF-8, UTF-16BE/LE, or something else entirely.

What would be affected by this:

Requests:

cf
fp (when invoked with a 3rd argument)
hpf
hpfa
lf (when invoked with a 2nd argument)
mso
msoquiet
nx (when invoked with a 2nd argument)
open
opena
psbb
so
soquiet
trf

Escape sequences:

\O5 (but since this is mainly used internally to manage temporary files
    by grohtml, maybe lazily postponing this in hope that my Grand Plan
    to revise grohtml to no longer use a dedicated preprocessor is a
    better idea)


Registers:

.F

It occurs to me that I hadn't actually written up this idea with these
particulars before.  This should probably be a new ticket.

And probably step 1 would be a simple refactor to introduce file name
argument-gathering and -interpolating functions which initially behave
no differently than the status quote, but simply wrap existing logic for
identifier gathering and whatever one-off thing the `.F` interpolator
does.

Regards,
Branden

G. Branden Robinson <gbranden>
Group administrator
Tue 02 Jan 2024 08:02:32 PM UTC, comment #4: 

comment #1:

> 1. What happens if a .so(urced) file has a non-ASCII character
> in its filename?


This seems more likely to work with the proposed change than without it.  Currently, with preconv preceding soelim, preconv will change the line ".so naïve_file" into ".so na\[u00EF]ve_file", guaranteed to fail.  If soelim comes first, it at least has a chance of succeeding.

Dave <barx>
Group Member
Tue 29 Aug 2023 03:46:43 AM UTC, comment #3: 

comment #2:

> this may indeed be worth considering, but after release, not before.


That time has arrived!

The patch as posted still applies fine (even despite the "verbatim" tags being malformed, which can sometimes cause problems), but the original submission makes clear that the patch is incomplete as-as, pointing out:

> the documentation must then mention this fact in the
> explanation of the "-s" option.

Dave <barx>
Group Member
Wed 11 Nov 2020 11:38:28 AM UTC, comment #2: 

Quick answer without doing a full analysis: this may indeed be worth considering, but after release, not before.  This is not a bugfix but a fundamental change in behaviour, and as such, it could in principle cause serious regressions in existing documents.

Moving to Item Group "Preprocessor preconv" und Category "New feature" because in a nutshell, the idea here is to apply preconv to additional content.

Ingo Schwarze <schwarze>
Group Member
Wed 11 Nov 2020 07:37:27 AM UTC, comment #1: 

Bjari's point seems sound to me.  The whole point of soelim is to get preprocessors to run on `.so`ed files (`.mso`, `.pso`).

I'm trying to think of cases where this would break things where they aren't already broken (we have no test cases here, of course...yet).

Questions to ponder, regarding both current behavior and what happens if we accept the proposed patch.

1. What happens if a .so(urced) file has a non-ASCII character in its filename?  soelim(1) does not seem to speak to this question. Maybe because it is assuming preconv(1) has already been run, or, more likely I think, the issue was not considered when the page was written, since it long predates preconv itself.

2. What about EBCDIC hosts?  Even plain ASCII code points are complete disarranged in EBCDIC.  soelim uses standard C library functions for character comparison and handling (see src/preproc/soelim/soelim.cpp:do_file()).  There are no EBCDIC worries today because preconv has already busted the input down to 7-bit chars.  What about after this patch?  We can't assume that's been done, so chars with the 8th bit set might be in the input.  But I'm not seeing any problems; anything that soelim doesn't care about as documented in its man page, it passes through without alteration, amounting to `putchar(getc());`.  And the chars that soelim DOES care about should not be spuriously matched in a UTF-8 sequence, because all continuation bytes in a UTF-8 sequence have the high bit set.  soelim will, therefore, not spurious match them and munge them.

Can anyone else think of any objections?

G. Branden Robinson <gbranden>
Group administrator
Tue 10 Nov 2020 05:26:03 PM UTC, original submission:  

src/roff/groff/groff.cpp: correct the order of preprocessors in the
pipeline.

  The "soelim" command must be the first in the pipeline to incorporate
 all sourced files into the input file,
 before it is sent to the next command.

  Change the order from "preconv, soelim, refer" to
"soelim, preconv, refer".

  As "preconv" can't (currently) figure out the encoding,
 if it gets its input from standard input,
 "groff" must be called with the option "-K <encoding>" or
 "-D <encoding>", and not just with "-k",
 if the encoding is not the default one.

  This means that the documentation must then mention this fact in the
explanation of the "-s" option.
.verbatim+
Signed-off-by: Bjarni Ingi Gislason <bjarniig@rhi.hi.is>
---
 src/roff/groff/groff.cpp | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/src/roff/groff/groff.cpp b/src/roff/groff/groff.cpp
index f568ac02..cd61dbd0 100644
--- a/src/roff/groff/groff.cpp
+++ b/src/roff/groff/groff.cpp
@@ -53,9 +53,11 @@ extern "C" {
 
 // grap, chem, and ideal must come before pic;
 // tbl must come before eqn
-const int PRECONV_INDEX = 0;
-const int SOELIM_INDEX = PRECONV_INDEX + 1;
-const int REFER_INDEX = SOELIM_INDEX + 1;
+// soelim must come first to complete the input file for the later
+// commands in the pipeline
+const int SOELIM_INDEX = 0;
+const int PRECONV_INDEX = SOELIM_INDEX +1;
+const int REFER_INDEX = PRECONV_INDEX + 1;
 const int GRAP_INDEX = REFER_INDEX + 1;
 const int CHEM_INDEX = GRAP_INDEX + 1;
 const int IDEAL_INDEX = CHEM_INDEX + 1;
--
2.28.0

-verbatim-

Bjarni Ingi Gislason <bjarniig>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

No files currently attached

 

Depends on the following items: None found

Items that depend on this one

Digest:
   bug dependencies.

 

Carbon-Copy List
  • -email is unavailable- added by barx (Posted a comment)
  • -email is unavailable- added by schwarze (Posted a comment)
  • -email is unavailable- added by gbranden (Posted a comment)
  • -email is unavailable- added by bjarniig (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

    Follow 12 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2024-01-02 gbranden Dependencies- bugs #65099 is dependent
    2024-01-02 gbranden Summary[PATCH] groff.cpp: move soelim before preconv in constructed pipeline [PATCH] groff.cpp: move soelim before preconv in constructed pipeline
    2022-06-10 gbranden CategoryPreprocessor preconv General
        Severity3 - Normal 1 - Wish
    2020-11-11 schwarze CategoryCore Preprocessor preconv
        Item GroupIncorrect behaviour Feature change
    2020-11-11 gbranden Summary[PATCH] groff.cpp: correct the order of preprocessors in the pipeline [PATCH] groff.cpp: move soelim before preconv in constructed pipeline
    2020-11-11 gbranden Item GroupFeature change Incorrect behaviour
        StatusNone Need Info
        Assigned toNone gbranden
    2020-11-10 barx Carbon-CopyRemoved 93119 -
    2020-11-10 barx Summarygroff.cpp: correct the order of preprocessors in the pipeline [PATCH] groff.cpp: correct the order of preprocessors in the pipeline

    Back to the top

    Powered by Savane 3.12.
    Corresponding source code