Update of bug #67372 (group groff):
Status: Need Info => Confirmed
Assigned to: schwarze => gbranden
Planned Release: None => 1.24.0
_______________________________________________________
Follow-up Comment #12:
[comment #11 comment #11:]
> Here's a proof-of-concept patch, fixing up only the `\A` escape sequence.
Your patch is absolutely not OK.
In fact, it significantly exacerbates the regression.
Here is what i tested.
1. I replaced s/want_att_compat/compatible_flag/ in your patch.
2. I added the resulting modified patch to the OpenBSD port.
3. The patch, configure, build, fake, and package stages all went through
with no suspicious output.
4. I installed the modified port.
5. Nothing changes in the rest of the test suite, which is good as far as
that goes.
But while the roff/esc/A1 test exposes two regressions in \A handling in
1.23,
with \A otherwise still in working order, with your patch
1. These two regressions become significantly worse:
With your patch, groff now prints
string expansion: 0xwordxu
register expansion: 0121u
instead of the expected
string expansion: 1u
register expansion: 1u
which is much worse than without the patch because it now leaks \A
arguments to the output
rather than only losing a single suffix character.
2. The scope of the regression expands massively.
In addition to not fixing the problem (and making it worse),
the \A sequence is almost entirely broken with the patch.
With one exception (the final "unterminated sequence" test),
the patch causes every single one of the "Argument delimiter" tests to
fail very badly:
most of them switching value from 1 to 0 *and* leaking arguments to the
output.
3. The regression of the first test, the \A\r test, is particularly dire.
That test now causes this output (observe the almost blank line with the
"u", and the ruined section title):
u
Argument delimitersword
unsupported \r: 0
instead of the expected
Argument delimiters
unsupported \r: 1u
I have no idea why you are bringing compat mode into the discussion.
This regression has absolutely nothing to do with compat mode.
You cannot be suggesting that manual pages should be rendered in compat mode,
or that the mandoc test suite should run in compat mode?
I have never used compat mode for anything and i think its very existence is a
terrible idea.
Software always changes, and the idea that this continuous evolution can be
captured by a single bit of information
is delusional. Which point in time does compat mode target for
compatibility?
So i had to look up how to even enable compat mode because i never used or
tested it...
Putting ".cp 1" as the first line into the test file changes absolutely
nothing.
Neither does setting the -C command line option.
Putting ".cp 1" after the mdoc(7) preamble (right before the ".Sh NAME" line
results in this printout on standard output, *instead* of the NAME header:
$] .tm Usage: .Sh section_name ... (#0.c]) c-macro-name Sh.
and then two blank lines,
but all parts of the (exacerbated) regression still occur with no changes.
Putting ".cp 1" after the ".Ss Argument delimiters" section header
only prevents the *new* regressions that your patch causes,
but the *original* regression that this ticket is about still occurs.
To summarize, your patch is ineffective for the purposes of this ticket
and causes new regressions that are even worse.
I think your development methodology is not quite right.
It started off well enough:
1. A regression is reported.
2. You identified the commit that caused it (thanks for that!).
(I did not yet check whether reverting that commit, or the relevent
part(s) of it, indeed fixes the regression.)
3. That commit was *not* intended to change behaviour.
Indeed, the commit message says "refactor".
At this point, the logical next step is to _just fix the bloody regression_
by reverting the defective commit, or the defective parts of it,
then consider what can be done to reduce the probability of future
refactoring commits causing regressions in a similar way.
Instead you chose to
4. Research historic behaviour ab initio.
5. The result was (at least for \w; i don't think discussing \b for nroff(1)
makes much sense,
and i didn't check what's going on for \o yet since the roff/esc/o test
does not indicate any regression)
that \w behaviour has been consistent from AT&T all the way through to
groff-1.22.4,
which impressively confirms what we already knew after steps 2 & 3,
i.e. that there is a regression.
Now instead of fixing the regression, you take off on a breathtaking tangent
about an unrelated topic,
namely, compatibility mode. That doesn't make sense to me.
Sure, i'm not saying we cannot discuss compatibility mode (even though i
consider compat mode
mostly a waste of time and am not particularly keen on even considering it at
all),
but when a regression is found, normal development methodology is:
1. Fix the regression by reverting the defective change, or the defective
part of it.
2. Consider which safeguards are needed to prevent repeat incidents,
in particular in case a regression not only got committed, but released.
3. Only after that can other changes to the code in question be considered,
and if those other changes change behaviour (like the patch you posted
here does)
that needs a full rationale and a discussion completely separate from the
regression fix.
When a regression is found, it is certainly not acceptable to pile more
commits on top
without fixing the regression first, and it is methodologically also not
acceptable
to discuss other changes in the context of the regression fix.
Needless to say, i'll also try to develop a regression fix, simply because i
need such a fix
in order to become able to update to groff-1.23.0 in OpenBSD. And i hope your
bisecting will
help with developing that fix, so thanks again for identifiying the defective
commit.
But can we please focus this ticket exclusively on the regression fix and
discuss any other matters elsewhere?
Reacting to regressions by piling more changes on top instead of just
reverting and learning from the mishap
is *the* most effective way to make software unstable, accumulate bugs, and
lose control of what one is doing.
_______________________________________________________
Reply to this item at:
<https://savannah.gnu.org/bugs/?67372>
_______________________________________________
Message sent via Savannah
https://savannah.gnu.org/
signature.asc
Description: PGP signature
