sync results

sync results

[Ingo Schwarze]

Hi Kristaps,

i'm currently syncing from bsd.lv to openbsd.org.
The code looks good, and in the output, i see many improvements,
so i will now commit the sync.


However, there are also a three tiny regressions:

cvs(1) contains this code:

.TP 1i
\fB?\fP \fIfile\fP
\fIfile\fP is in your working directory, but does not correspond to
anything in the source repository, and is not in the list of files
for \fBcvs\fP to ignore (see the description of the \fB\-I\fP option).
.PP
.RS .5i
.sp
Use the
.B \-A
option to reset any sticky tags, dates, or
.B \-k
options.  (If you get a working copy of a file by using one of the

Now doubtless, .PP .RS .sp in a row is horrible.
However, with groff and old mandoc, that gave one blank line.
Now there are two.


lynx(1) contains this code:

.TP
.B \-cookie_file\fR=\fIFILENAME
specifies a file to use to read cookies.

In groff and old mandoc, the indent of "specifies" relative to "-cookie"
was 7, now it is 5.


nsd-xfer(8) contains this code:

.SH "OPTIONS"
.LP 
.TP
.B \-4
Only send to IPv4 addresses.

In groff and old mandoc, there is no blank line after OPTIONS,
now there is one.


Note this is the first time that i managed to use the whole OpenBSD
base tree (3725 distinct pages) for doing regression checks after a
merge, and the above is all i found.

Yours,
  Ingo
--
 To unsubscribe send an email to tech+unsubscribe@mdocml.bsd.lv

Re: sync results

[Kristaps Dzonsons]

Ingo,

I'm not able to reproduce some of these.

> i'm currently syncing from bsd.lv to openbsd.org.
> The code looks good, and in the output, i see many improvements,
> so i will now commit the sync.
>
>
> However, there are also a three tiny regressions:
>
> cvs(1) contains this code:
>
> .TP 1i
> \fB?\fP \fIfile\fP
> \fIfile\fP is in your working directory, but does not correspond to
> anything in the source repository, and is not in the list of files
> for \fBcvs\fP to ignore (see the description of the \fB\-I\fP option).
> .PP
> .RS .5i
> .sp
> Use the
> .B \-A
> option to reset any sticky tags, dates, or
> .B \-k
> options.  (If you get a working copy of a file by using one of the
>
> Now doubtless, .PP .RS .sp in a row is horrible.
> However, with groff and old mandoc, that gave one blank line.
> Now there are two.

This is not the case with my machine (groff 1.19.2).

The following:

.TH FOO 1
.SH NAME
foo - bar
.SH DESCRIPTION
hi.
.PP
.RS .5i
.sp
Use the

Has two spaces between the "hi" and "Use the" with both groff and mandoc 
(using nroff -mandoc and groff -Tascii -mandoc).

> lynx(1) contains this code:
>
> .TP
> .B \-cookie_file\fR=\fIFILENAME
> specifies a file to use to read cookies.
>
> In groff and old mandoc, the indent of "specifies" relative to "-cookie"
> was 7, now it is 5.

Are you sure?  I just ran this code and had both mandoc and groff align 
"specifies" under the underscore, i.e., 7 spaces.

> nsd-xfer(8) contains this code:
>
> .SH "OPTIONS"
> .LP
> .TP
> .B \-4
> Only send to IPv4 addresses.
>
> In groff and old mandoc, there is no blank line after OPTIONS,
> now there is one.

Ouch, this is indeed a bug: the syntax tree appears to be messed up by 
the removal of the `LP'.  I'll look into it...

Thanks,

Kristaps
--
 To unsubscribe send an email to tech+unsubscribe@mdocml.bsd.lv

Re: sync results

[Kristaps Dzonsons]

[-- Attachment #1: Type: text/plain, Size: 826 bytes --]

>> nsd-xfer(8) contains this code:
>>
>> .SH "OPTIONS"
>> .LP
>> .TP
>> .B \-4
>> Only send to IPv4 addresses.
>>
>> In groff and old mandoc, there is no blank line after OPTIONS,
>> now there is one.
>
> Ouch, this is indeed a bug: the syntax tree appears to be messed up by
> the removal of the `LP'. I'll look into it...

Ingo,

Please check if the enclosed patch fixes this problem without fallout. 
Basically, the "next node append type" for -man was automatically being 
set to NEXT, clobbering the value set during node deletion in the 
post-validator.  This was causing the `LP' to be removed and the next 
node, the `TP', being assigned under the block of `SH'.  Ew.  I 
eye-balled it over a selection of -man and ran it over my regular tests 
for failure.  This is quite an old bug, it turns out.

Thanks,

Kristaps

[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 1043 bytes --]

Index: man_macro.c
===================================================================
RCS file: /usr/vhosts/mdocml.bsd.lv/cvs/mdocml/man_macro.c,v
retrieving revision 1.63
diff -u -r1.63 man_macro.c
--- man_macro.c	18 Jun 2011 17:36:52 -0000	1.63
+++ man_macro.c	5 Jul 2011 14:01:28 -0000
@@ -120,6 +120,9 @@
 
 	assert(to);
 
+	assert(MAN_ROOT != m->last->type);
+	m->next = MAN_NEXT_SIBLING;
+
 	/* LINTED */
 	while (m->last != to) {
 		/*
@@ -140,9 +143,6 @@
 	if ( ! man_valid_post(m))
 		return(0);
 
-	m->next = MAN_ROOT == m->last->type ? 
-		MAN_NEXT_CHILD : MAN_NEXT_SIBLING;
-
 	return(1);
 }
 
@@ -428,6 +428,9 @@
 		assert( ! (MAN_SCOPED & man_macros[tok].flags));
 		m->flags |= MAN_ILINE;
 	}
+
+	assert(MAN_ROOT != m->last->type);
+	m->next = MAN_NEXT_SIBLING;
 	
 	/*
 	 * Rewind our element scope.  Note that when TH is pruned, we'll
@@ -452,9 +455,6 @@
 
 	if (m->last->type != MAN_ROOT && ! man_valid_post(m))
 		return(0);
-
-	m->next = MAN_ROOT == m->last->type ?
-		MAN_NEXT_CHILD : MAN_NEXT_SIBLING;
 
 	return(1);
 }

Re: sync results

[Ingo Schwarze]

Hi Kristaps,

Kristaps Dzonsons wrote on Tue, Jul 05, 2011 at 04:06:44PM +0200:
> Kristaps wrote:
>> Ingo wrote:

>>> nsd-xfer(8) contains this code:
>>>
>>> .SH "OPTIONS"
>>> .LP
>>> .TP
>>> .B \-4
>>> Only send to IPv4 addresses.
>>>
>>> In groff and old mandoc, there is no blank line after OPTIONS,
>>> now there is one.

>> Ouch, this is indeed a bug: the syntax tree appears to be messed up by
>> the removal of the `LP'. I'll look into it...

> Please check if the enclosed patch fixes this problem

Yes, it does.

> without fallout.

I don't see any new regressions.

> Basically, the "next node append type" for -man was
> automatically being set to NEXT, clobbering the value set during
> node deletion in the post-validator.  This was causing the `LP' to
> be removed and the next node, the `TP', being assigned under the
> block of `SH'.  Ew.  I eye-balled it over a selection of -man and
> ran it over my regular tests for failure.  This is quite an old bug,
> it turns out.

Somehow, it must have been uncovered by recent changes elsewhere,
or it wouldn't have shown up as an regression.  Anyway,
investigating the details why it remained hidden looks like
a waste of time.

So, ok schwarze@ and committed to OpenBSD.

Thanks,
  Ingo


> Index: man_macro.c
> ===================================================================
> RCS file: /usr/vhosts/mdocml.bsd.lv/cvs/mdocml/man_macro.c,v
> retrieving revision 1.63
> diff -u -r1.63 man_macro.c
> --- man_macro.c	18 Jun 2011 17:36:52 -0000	1.63
> +++ man_macro.c	5 Jul 2011 14:01:28 -0000
> @@ -120,6 +120,9 @@
>  
>  	assert(to);
>  
> +	assert(MAN_ROOT != m->last->type);
> +	m->next = MAN_NEXT_SIBLING;
> +
>  	/* LINTED */
>  	while (m->last != to) {
>  		/*
> @@ -140,9 +143,6 @@
>  	if ( ! man_valid_post(m))
>  		return(0);
>  
> -	m->next = MAN_ROOT == m->last->type ? 
> -		MAN_NEXT_CHILD : MAN_NEXT_SIBLING;
> -
>  	return(1);
>  }
>  
> @@ -428,6 +428,9 @@
>  		assert( ! (MAN_SCOPED & man_macros[tok].flags));
>  		m->flags |= MAN_ILINE;
>  	}
> +
> +	assert(MAN_ROOT != m->last->type);
> +	m->next = MAN_NEXT_SIBLING;
>  	
>  	/*
>  	 * Rewind our element scope.  Note that when TH is pruned, we'll
> @@ -452,9 +455,6 @@
>  
>  	if (m->last->type != MAN_ROOT && ! man_valid_post(m))
>  		return(0);
> -
> -	m->next = MAN_ROOT == m->last->type ?
> -		MAN_NEXT_CHILD : MAN_NEXT_SIBLING;
>  
>  	return(1);
>  }
--
 To unsubscribe send an email to tech+unsubscribe@mdocml.bsd.lv

Re: sync results

[Ingo Schwarze]

Hi Kristaps,

Kristaps Dzonsons wrote on Tue, Jul 05, 2011 at 02:43:38PM +0200:
> Ingo Schwarze wrote:

> I'm not able to reproduce some of these.

>>i'm currently syncing from bsd.lv to openbsd.org.
>>The code looks good, and in the output, i see many improvements,
>>so i will now commit the sync.
>>
>>
>>However, there are also a three tiny regressions:
>>
>>cvs(1) contains this code:
>>
>>.TP 1i
>>\fB?\fP \fIfile\fP
>>\fIfile\fP is in your working directory, but does not correspond to
>>anything in the source repository, and is not in the list of files
>>for \fBcvs\fP to ignore (see the description of the \fB\-I\fP option).
>>.PP
>>.RS .5i
>>.sp
>>Use the
>>.B \-A
>>option to reset any sticky tags, dates, or
>>.B \-k
>>options.  (If you get a working copy of a file by using one of the
>>
>>Now doubtless, .PP .RS .sp in a row is horrible.
>>However, with groff and old mandoc, that gave one blank line.
>>Now there are two.

> This is not the case with my machine (groff 1.19.2).
> 
> The following:
> 
> .TH FOO 1
> .SH NAME
> foo - bar
> .SH DESCRIPTION
> hi.
> .PP
> .RS .5i
> .sp
> Use the
> 
> Has two spaces between the "hi" and "Use the" with both groff and
> mandoc (using nroff -mandoc and groff -Tascii -mandoc).

That looks like a bug in groff-1.19 that has been fixed in 1.21.
With groff-1.21 and your test code, i get one blank line between
"hi" and "Use the", which makes more sense given the general
tendency of roff to compress multiple blank lines to one.

>> lynx(1) contains this code:
>>
>> .TP
>> .B \-cookie_file\fR=\fIFILENAME
>> specifies a file to use to read cookies.
>>
>> In groff and old mandoc, the indent of "specifies" relative to "-cookie"
>> was 7, now it is 5.

> Are you sure?  I just ran this code and had both mandoc and groff
> align "specifies" under the underscore, i.e., 7 spaces.

Yes, with both OpenBSD and bsd.lv mandoc, i get the "specifies"
aligned under the "i" in "-cookie_file".  I'm just noticing
that the change in alignment happens about two dozen lines above,
below "-cmd_script".  The "comment beginning with..." is still
aligned at 7 (below the 'r'), the "exit" is aligned at 5 (below
the 's').

Yours,
  Ingo
--
 To unsubscribe send an email to tech+unsubscribe@mdocml.bsd.lv

Re: sync results

[Kristaps Dzonsons]

[-- Attachment #1: Type: text/plain, Size: 2853 bytes --]

On 06/07/2011 08:03, Ingo Schwarze wrote:
> Hi Kristaps,
>
> Kristaps Dzonsons wrote on Tue, Jul 05, 2011 at 02:43:38PM +0200:
>> Ingo Schwarze wrote:
>
>> I'm not able to reproduce some of these.
>
>>> i'm currently syncing from bsd.lv to openbsd.org.
>>> The code looks good, and in the output, i see many improvements,
>>> so i will now commit the sync.
>>>
>>>
>>> However, there are also a three tiny regressions:
>>>
>>> cvs(1) contains this code:
>>>
>>> .TP 1i
>>> \fB?\fP \fIfile\fP
>>> \fIfile\fP is in your working directory, but does not correspond to
>>> anything in the source repository, and is not in the list of files
>>> for \fBcvs\fP to ignore (see the description of the \fB\-I\fP option).
>>> .PP
>>> .RS .5i
>>> .sp
>>> Use the
>>> .B \-A
>>> option to reset any sticky tags, dates, or
>>> .B \-k
>>> options.  (If you get a working copy of a file by using one of the
>>>
>>> Now doubtless, .PP .RS .sp in a row is horrible.
>>> However, with groff and old mandoc, that gave one blank line.
>>> Now there are two.
>
>> This is not the case with my machine (groff 1.19.2).
>>
>> The following:
>>
>> .TH FOO 1
>> .SH NAME
>> foo - bar
>> .SH DESCRIPTION
>> hi.
>> .PP
>> .RS .5i
>> .sp
>> Use the
>>
>> Has two spaces between the "hi" and "Use the" with both groff and
>> mandoc (using nroff -mandoc and groff -Tascii -mandoc).
>
> That looks like a bug in groff-1.19 that has been fixed in 1.21.
> With groff-1.21 and your test code, i get one blank line between
> "hi" and "Use the", which makes more sense given the general
> tendency of roff to compress multiple blank lines to one.
>
>>> lynx(1) contains this code:
>>>
>>> .TP
>>> .B \-cookie_file\fR=\fIFILENAME
>>> specifies a file to use to read cookies.
>>>
>>> In groff and old mandoc, the indent of "specifies" relative to "-cookie"
>>> was 7, now it is 5.
>
>> Are you sure?  I just ran this code and had both mandoc and groff
>> align "specifies" under the underscore, i.e., 7 spaces.
>
> Yes, with both OpenBSD and bsd.lv mandoc, i get the "specifies"
> aligned under the "i" in "-cookie_file".  I'm just noticing
> that the change in alignment happens about two dozen lines above,
> below "-cmd_script".  The "comment beginning with..." is still
> aligned at 7 (below the 'r'), the "exit" is aligned at 5 (below
> the 's').

Ingo, this was a bit tricky.

It seems I've neglected that `RS' saves the current default-left-margin, 
restoring it on output.  The enclosed patch introduces a stack of `RS' 
default-left-margins.  The stack overflows neatly: if there are too many 
nested `RS' scopes, it stops nesting but keeps track of what the stack 
would be.  When the stack drops below overflow, nesting is reenabled.  I 
set the current max stack to 64.

This should clean up output a great deal when the `RS' is used with 
embedded paragraphs.

Thoughts?

Kristaps

[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 4415 bytes --]

Index: man_term.c
===================================================================
RCS file: /usr/vhosts/mdocml.bsd.lv/cvs/mdocml/man_term.c,v
retrieving revision 1.113
diff -u -r1.113 man_term.c
--- man_term.c	3 Jul 2011 22:57:32 -0000	1.113
+++ man_term.c	7 Jul 2011 10:27:14 -0000
@@ -33,29 +33,19 @@
 #include "term.h"
 #include "main.h"
 
-#define	INDENT		  7
-#define	HALFINDENT	  3
+#define	INDENT		  7 /* fixed-width char full-indent */
+#define	HALFINDENT	  3 /* fixed-width char half-indent */
+#define	MAXMARGINS	  64 /* maximum number of indented scopes */
 
 /* FIXME: have PD set the default vspace width. */
 
 struct	mtermp {
 	int		  fl;
 #define	MANT_LITERAL	 (1 << 0)
-	/* 
-	 * Default amount to indent the left margin after leading text
-	 * has been printed (e.g., `HP' left-indent, `TP' and `IP' body
-	 * indent).  This needs to be saved because `HP' and so on, if
-	 * not having a specified value, must default.
-	 *
-	 * Note that this is the indentation AFTER the left offset, so
-	 * the total offset is usually offset + lmargin.
-	 */
-	size_t		  lmargin;
-	/*
-	 * The default offset, i.e., the amount between any text and the
-	 * page boundary.
-	 */
-	size_t		  offset;
+	size_t		  lmargin[MAXMARGINS]; /* margins (incl. visible page) */
+	int		  lmargincur; /* index of current margin */
+	int		  lmarginsz; /* actual number of nested margins */
+	size_t		  offset; /* default offset to visible page */
 };
 
 #define	DECL_ARGS 	  struct termp *p, \
@@ -164,8 +154,9 @@
 	term_begin(p, print_man_head, print_man_foot, m);
 	p->flags |= TERMP_NOSPACE;
 
-	mt.fl = 0;
-	mt.lmargin = term_len(p, INDENT);
+	memset(&mt, 0, sizeof(struct mtermp));
+
+	mt.lmargin[mt.lmargincur] = term_len(p, INDENT);
 	mt.offset = term_len(p, INDENT);
 
 	if (n->child)
@@ -456,7 +447,7 @@
 		return(0);
 	}
 
-	len = mt->lmargin;
+	len = mt->lmargin[mt->lmargincur];
 	ival = -1;
 
 	/* Calculate offset. */
@@ -472,7 +463,7 @@
 	p->rmargin = mt->offset + len;
 
 	if (ival >= 0)
-		mt->lmargin = (size_t)ival;
+		mt->lmargin[mt->lmargincur] = (size_t)ival;
 
 	return(1);
 }
@@ -507,7 +498,7 @@
 
 	switch (n->type) {
 	case (MAN_BLOCK):
-		mt->lmargin = term_len(p, INDENT);
+		mt->lmargin[mt->lmargincur] = term_len(p, INDENT);
 		print_bvspace(p, n);
 		break;
 	default:
@@ -542,7 +533,7 @@
 		return(1);
 	}
 
-	len = mt->lmargin;
+	len = mt->lmargin[mt->lmargincur];
 	ival = -1;
 
 	/* Calculate the offset from the optional second argument. */
@@ -563,7 +554,7 @@
 			break;
 
 		/* Set the saved left-margin. */
-		mt->lmargin = (size_t)ival;
+		mt->lmargin[mt->lmargincur] = (size_t)ival;
 
 		savelit = MANT_LITERAL & mt->fl;
 		mt->fl &= ~MANT_LITERAL;
@@ -631,18 +622,15 @@
 		return(1);
 	}
 
-	len = (size_t)mt->lmargin;
+	len = (size_t)mt->lmargin[mt->lmargincur];
 	ival = -1;
 
 	/* Calculate offset. */
 
-	if (NULL != (nn = n->parent->head->child)) {
-		while (nn && MAN_TEXT != nn->type)
-			nn = nn->next;
-		if (nn && nn->next)
+	if (NULL != (nn = n->parent->head->child))
+		if (nn->parent->line == nn->line)
 			if ((ival = a2width(p, nn->string)) >= 0)
 				len = (size_t)ival;
-	}
 
 	switch (n->type) {
 	case (MAN_HEAD):
@@ -663,9 +651,8 @@
 
 		if (savelit)
 			mt->fl |= MANT_LITERAL;
-
 		if (ival >= 0)
-			mt->lmargin = (size_t)ival;
+			mt->lmargin[mt->lmargincur] = (size_t)ival;
 
 		return(0);
 	case (MAN_BODY):
@@ -710,7 +697,7 @@
 	switch (n->type) {
 	case (MAN_BLOCK):
 		mt->fl &= ~MANT_LITERAL;
-		mt->lmargin = term_len(p, INDENT);
+		mt->lmargin[mt->lmargincur] = term_len(p, INDENT);
 		mt->offset = term_len(p, INDENT);
 		/* If following a prior empty `SS', no vspace. */
 		if (n->prev && MAN_SS == n->prev->tok)
@@ -761,7 +748,7 @@
 	switch (n->type) {
 	case (MAN_BLOCK):
 		mt->fl &= ~MANT_LITERAL;
-		mt->lmargin = term_len(p, INDENT);
+		mt->lmargin[mt->lmargincur] = term_len(p, INDENT);
 		mt->offset = term_len(p, INDENT);
 		/* If following a prior empty `SH', no vspace. */
 		if (n->prev && MAN_SH == n->prev->tok)
@@ -830,6 +817,10 @@
 	mt->offset += sz;
 	p->offset = mt->offset;
 
+	if (++mt->lmarginsz < MAXMARGINS)
+		mt->lmargincur = mt->lmarginsz;
+
+	mt->lmargin[mt->lmargincur] = mt->lmargin[mt->lmargincur - 1];
 	return(1);
 }
 
@@ -858,6 +849,9 @@
 
 	mt->offset = mt->offset < sz ?  0 : mt->offset - sz;
 	p->offset = mt->offset;
+
+	if (--mt->lmarginsz < MAXMARGINS)
+		mt->lmargincur = mt->lmarginsz;
 }
 
 static void

Re: sync results

[Ingo Schwarze]

Hi Kristaps,

Kristaps Dzonsons wrote on Thu, Jul 07, 2011 at 12:27:27PM +0200:

> It seems I've neglected that `RS' saves the current
> default-left-margin, restoring it on output.  The enclosed patch
> introduces a stack of `RS' default-left-margins.  The stack
> overflows neatly: if there are too many nested `RS' scopes, it stops
> nesting but keeps track of what the stack would be.  When the stack
> drops below overflow, nesting is reenabled.  I set the current max
> stack to 64.
> 
> This should clean up output a great deal when the `RS' is used with
> embedded paragraphs.

It looks reasonable, does not cause regressions,
and fixes lynx(1) indentation.

So i have committed it to OpenBSD.

Thanks,
  Ingo
--
 To unsubscribe send an email to tech+unsubscribe@mdocml.bsd.lv