[bug #64639] [mom] odd output with #64421 reproducer

[G. Branden Robinson]

Update of bug #64639 (project groff):

  Status: In Progress => Fixed  
 Open/Closed:Open => Closed 
 Planned Release:None => 1.24.0 

___

Follow-up Comment #7:


commit 4905320fa26d8fbeb2b2da9bdd6c3dc8c85815e5
Author: Peter Schaffter 
Date:   Thu Sep 14 15:43:30 2023 -0400

[mom]: Version 2.6 released

Table of contents and internal color management revamped.




___

Reply to this item at:

  

___
Message sent via Savannah
https://savannah.gnu.org/

[bug #64639] [mom] odd output with #64421 reproducer

[G. Branden Robinson]

Update of bug #64639 (project groff):

  Status:   Confirmed => In Progress

___

Follow-up Comment #6:

Works for me!  Thanks as always for your swift attention to the report.

Updating status.


___

Reply to this item at:

  

___
Message sent via Savannah
https://savannah.gnu.org/

[bug #64639] [mom] odd output with #64421 reproducer

[Peter Schaffter]

Follow-up Comment #5, bug #64639 (project groff):


[comment #4 comment #4:]

> Nevertheless Bjarni seems to have found a footgun.  (Perhaps he has a big
gun...or big feet.)
> 
> Could we use more input validation in `DRH`?

Not necessary, really.  I preferred to use mom's COLOR macro in the graphical
objects macro, but it's not essential.  This patch to DRH should clear up any
concerns Bjarni envisages.  The same change will be applied to the remaining
graphical object macros if it passes muster.

--- om.tmac.working.copy.bak02  2023-09-08 21:01:23.606662237 -0400
+++ om.tmac 2023-09-11 13:58:17.269063693 -0400
@@ -1934,7 +1934,6 @@
 .ds BLACK   \m[black]
 .ds white   \m[white]
 .ds WHITE   \m[white]
-.ds default black
 \#
 \# =
 \#
@@ -2844,17 +2843,12 @@
 .ds $RL_WEIGHT \\$1
 .ds $RL_INDENT \\$2
 .ds $RL_LENGTH \\$3
-.ie !'\\$4'' .ds $RL_COLOR  \\$4
-.el \{\
-.   ie '\\n[.m]'' .ds $RL_COLOR \\*[default]
-.   el .ds $RL_COLOR \\n[.m]
-.\}
 .nr #SAVED_WEIGHT \\n[#RULE_WEIGHT]
 .nr #SAVED_WEIGHT_ADJ \\n[#RULE_WEIGHT_ADJ]
 .di NULL
 .   if \\n[#NUM_ARGS]>=1 .RULE_WEIGHT \\*[$RL_WEIGHT]
 .di
-.COLOR \\*[$RL_COLOR]
+.if !'\\$4'' .gcolor \\$4
 .ie \\n[#NUM_ARGS]=0 \{\
 .   ie \\n[#INDENT_ACTIVE] \{\
 .  nr #RESTORE_L_LENGTH \\n[.l]
@@ -2899,7 +2893,7 @@
 .   if \\n[#NOFILL_MODE]=3 .CENTER
 .   if \\n[#NOFILL_MODE]=5 .RIGHT
 .\}
-.gcolor
+.gcolor default
 .nr #RULE_WEIGHT \\n[#SAVED_WEIGHT]
 .nr #RULE_WEIGHT_ADJ \\n[#SAVED_WEIGHT_ADJ]
 .rr #SAVED_WEIGHT


___

Reply to this item at:

  

___
Message sent via Savannah
https://savannah.gnu.org/

[bug #64639] [mom] odd output with #64421 reproducer

[G. Branden Robinson]

Follow-up Comment #4, bug #64639 (project groff):

Aha, that makes sense.  Once again my guess at how _mom_ works was all wrong. 
:-O

I should have known, because the `COLOR` macro's definition has been stable
for something like 20 years.  If it was as griefed as my hypothesis had it,
many people would have noticed.

Nevertheless Bjarni seems to have found a footgun.  (Perhaps he has a big
gun...or big feet.)

Could we use more input validation in `DRH`?


___

Reply to this item at:

  

___
Message sent via Savannah
https://savannah.gnu.org/

[bug #64639] [mom] odd output with #64421 reproducer

[Peter Schaffter]

Follow-up Comment #3, bug #64639 (project groff):

[comment #2 comment #2:]
 
> The issue with the input in comment #1 appears to be that _mom_ assumes that
a color named "default" exists (for me, it certainly defines a string _named_
`default` with the _contents_ `default`).

cat  What puzzles me more is the `COLOR` macro itself.
> 
> The following patch seems to make it do what it is documented to do.

> diff --git a/contrib/mom/om.tmac b/contrib/mom/om.tmac
> index b0fd1ef55..039bcfe48 100644
> --- a/contrib/mom/om.tmac
> +++ b/contrib/mom/om.tmac
> @@ -1873,9 +1873,9 @@ end
>  .MAC COLOR END
>  .ie \\n[.u]=1 \{\
>  \c
> -\\*[\\$1]\c
> +\m[\\*[\\$1]]\c
>  .\}
> -.el \\*[\\$1]
> +.el \m[\\*[\\$1]]
>  .END
>  \#
>  \# NEWCOLOR

Colours in mom have to be "initialized" with NEWCOLOR or XCOLOR.  Either of
these creates a string of the form "\m[color]" allowing the user to change
colour with the string "\*[color]".  Because "\\*[\\$1]" expands to
"\m[color]" (arg1 is a pre-initialize color name), the patch expands to
"\m[\m[color]]", which is incorrect.


___

Reply to this item at:

  

___
Message sent via Savannah
https://savannah.gnu.org/

[bug #64639] [mom] odd output with #64421 reproducer

[G. Branden Robinson]

Update of bug #64639 (project groff):

  Status:None => Confirmed  
 Assigned to:None => PTPi   
 Summary: [mom] bug #64421 is a mom-bug => [mom] odd output
with #64421 reproducer

___

Follow-up Comment #2:

This LTO stuff is no longer relevant; I've pushed a fix for that.

The issue with the input in comment #1 appears to be that _mom_ assumes that a
color named "default" exists (for me, it certainly defines a string _named_
`default` with the _contents_ `default`).

Whether this is a deliberate aspect of its design, I am not sure.  

What puzzles me more is the `COLOR` macro itself.

The following patch seems to make it do what it is documented to do.


diff --git a/contrib/mom/om.tmac b/contrib/mom/om.tmac
index b0fd1ef55..039bcfe48 100644
--- a/contrib/mom/om.tmac
+++ b/contrib/mom/om.tmac
@@ -1873,9 +1873,9 @@ end
 .MAC COLOR END
 .ie \\n[.u]=1 \{\
 \c
-\\*[\\$1]\c
+\m[\\*[\\$1]]\c
 .\}
-.el \\*[\\$1]
+.el \m[\\*[\\$1]]
 .END
 \#
 \# NEWCOLOR


But I didn't check all of `COLOR`'s call sites, so I'm not sure this is a
kosher fix.

Assigning to Peter for evaluation. 


___

Reply to this item at:

  

___
Message sent via Savannah
https://savannah.gnu.org/