Macro file comment loading "regression"

[pmatilai]

PR #1606 (commit 75275a8) changed macro file loading in a subtle way. Prior to that, the both macros in the following would be loaded. Now, anything coming after the comment gets silently discarded:

%xmacro1 foo
# %{
%xmacro2 bar

[pmatilai🎩︎lumikko rpm]$ ./rpm --showrc|grep xmac
-13: xmacro1 foo
[pmatilai🎩︎lumikko rpm]$

There are two ways to see this: either the contents of comment lines should be completely ignored, or this should emit an error about an unterminated macro. In spec, macros are expanded in comments too so the latter would be more consistent with that, but in macro files we're not expanding anything ever which suggests the former should be the right behavior. Thoughts? @mlschroe , others...

[mlschroe]

That also happens without the patch. I think you need

%xmacro1 foo
# %{

%xmacro2 bar

I.e. an empty line that used to terminate the body.

[mlschroe]

If you don't mind rdcl() reaching an even higher level of ugliness, an easy fix
is to add

        if (p == buf) {
            while (*p && isblank(*p))
                p++;
            if (*p != '%') {
                *q = '\0';              /* trim trailing \r, \n */
                break;
            }    
        }

right before the for (; p < q; p++) { loop.

This only does the "add continuation lines" magic for real macro definitions.

A different thing to consider is if we want to revert the "empty line ends macro definition"
change, just to make runaway macros not wreck havoc.

[pmatilai]

That also happens without the patch.

Oh, sorry. I thought I had a minimal reproducer but it was in a bit of a haste (https://bugzilla.redhat.com/show_bug.cgi?id=1953910 is the original case) so I missed the some further subtleties. It indeed relates to the empty newlines, the real reproducer being:

%xmacro1 foo
# %{
%xmacro2 bar

%xmacro3 zzz

...and whether xmacro3 gets defined depends on whether the patch is applied or not. And with this it all actually fits together in how the heck does that patch change the behavior in the original case 😅

[pmatilai]

If you don't mind rdcl() reaching an even higher level of ugliness, an easy fix is to add
[...]

Doesn't seem that bad to me, considering the company it's in. Perhaps a comment of the case would be in order. Care to submit a PR?

I rather like the new behavior otherwise.

[DemiMarie]

Personally, I would prefer to emit an error here.

[pmatilai]

No you wouldn't. Nobody expects contents of a #-commented line to affect anything coming after it, it's just absurd. It's absurd in specs too as indicated by the endless bugs and tickets filed on the behavior over the years, but at least there we have an excuse or two, and other options (%dnl).

[DemiMarie]

No you wouldn't. Nobody expects contents of a #-commented line to affect anything coming after it, it's just absurd. It's absurd in specs too as indicated by the endless bugs and tickets filed on the behavior over the years, but at least there we have an excuse or two, and other options (%dnl).

Silently discarding everything after the comment is certainly wrong, but I agree that comment lines should not be parsed.

[pmatilai]

@mlschroe , I went ahead and created a PR from this with a testcase, hope you don't mind.

[pmatilai]

Oh and BTW, thanks @mlschroe for the quick fix!