[PATCH] more: fix control character display bug

[PATCH] more: fix control character display bug

[Sami Kerola]

This removes isprint(c) column++ from get_line() that made underlining,
bold, and other control characters to cause more display to wrap lines
incorrectly.  Easiest way to see the difference is to run following
before and after compiling this change.

PAGER=./more man man

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 text-utils/more.c | 59 +++++++++++++++++++++++++------------------------------
 1 file changed, 27 insertions(+), 32 deletions(-)

diff --git a/text-utils/more.c b/text-utils/more.c
index 94b0455..a8e9254 100644
--- a/text-utils/more.c
+++ b/text-utils/more.c
@@ -997,41 +997,36 @@ int get_line(register FILE *f, int *length)
 		} else if (c == EOF) {
 			*length = p - Line;
 			return (column);
-		} else {
+		}
 #ifdef HAVE_WIDECHAR
-			if (fold_opt && MB_CUR_MAX > 1) {
-				memset(mbc, '\0', MB_LEN_MAX);
-				mbc_pos = 0;
-				mbc[mbc_pos++] = c;
-				state_bak = state;
-
-				mblength = mbrtowc(&wc, mbc, mbc_pos, &state);
-				/* The value of mblength is always less than 2 here. */
-				switch (mblength) {
-				case (size_t)-2:
-					p--;
-					file_pos_bak = Ftell(f) - 1;
-					state = state_bak;
-					use_mbc_buffer_flag = 1;
-					break;
-
-				case (size_t)-1:
-					state = state_bak;
-					column++;
-					break;
-
-				default:
-					wc_width = wcwidth(wc);
-					if (wc_width > 0)
-						column += wc_width;
-				}
-			} else
-#endif	/* HAVE_WIDECHAR */
-			{
-				if (isprint(c))
-					column++;
+		else if (fold_opt && MB_CUR_MAX > 1) {
+			memset(mbc, '\0', MB_LEN_MAX);
+			mbc_pos = 0;
+			mbc[mbc_pos++] = c;
+			state_bak = state;
+
+			mblength = mbrtowc(&wc, mbc, mbc_pos, &state);
+			/* The value of mblength is always less than 2 here. */
+			switch (mblength) {
+			case (size_t)-2:
+				p--;
+				file_pos_bak = Ftell(f) - 1;
+				state = state_bak;
+				use_mbc_buffer_flag = 1;
+				break;
+
+			case (size_t)-1:
+				state = state_bak;
+				column++;
+				break;
+
+			default:
+				wc_width = wcwidth(wc);
+				if (wc_width > 0)
+					column += wc_width;
 			}
 		}
+#endif	/* HAVE_WIDECHAR */
 
 		if (column >= Mcol && fold_opt)
 			break;
-- 
2.2.2

Re: [PATCH] more: fix control character display bug

[Sami Kerola]

On 30 January 2015 at 09:54, Sami Kerola <kerolasa@iki.fi> wrote:
> This removes isprint(c) column++ from get_line() that made underlining,
> bold, and other control characters to cause more display to wrap lines
> incorrectly.  Easiest way to see the difference is to run following
> before and after compiling this change.
>
> PAGER=./more man man

Hold on. The change also alter press \n that earlier made more(1) to
more forward one line, but after the change only new line appears. I
will try to write better change sometime soon.

-- 
Sami Kerola
http://www.iki.fi/kerolasa/

Re: [PATCH] more: fix control character display bug

[Karel Zak]

On Fri, Jan 30, 2015 at 05:05:55PM +0100, Sami Kerola wrote:
> On 30 January 2015 at 09:54, Sami Kerola <kerolasa@iki.fi> wrote:
> > This removes isprint(c) column++ from get_line() that made underlining,
> > bold, and other control characters to cause more display to wrap lines
> > incorrectly.  Easiest way to see the difference is to run following
> > before and after compiling this change.
> >
> > PAGER=./more man man
> 
> Hold on. The change also alter press \n that earlier made more(1) to
> more forward one line, but after the change only new line appears. I
> will try to write better change sometime soon.

 I'm going to postpone this to provide time for better bugfix, I think
 it's better one old bug than another unknown hidden bug(s) :-)

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH] more: fix control character display bug

[Benno Schulenberg]

On Fri, Jan 30, 2015, at 09:54, Sami Kerola wrote:
> This removes isprint(c) column++ from get_line() that made underlining,
> bold, and other control characters to cause more display to wrap lines

Suggestion: s/more display/'more'/

> incorrectly.  Easiest way to see the difference is to run following
> before and after compiling this change.
> 
> PAGER=./more man man

Could you include in this description a snippet of the observed changes?

Also, it would be nice to see the patch split into two: one that changes
the logic, and one that adjusts the whitespace for the changed logic.
(Once the patch is in git, one can of course use 'git log -p -w' to see
just the changed logic among all the changed whitespace, but from just
a plain email this is a bit hard to do.  Or maybe you yourself could post
a -p -w diff as a follow-up to the full patch to show the essential changes.
Because it seems that also github is not able to strip whitespace changes
from diffs.)

Benno

-- 
http://www.fastmail.com - Same, same, but different...

[PATCH 2/2] more: fix get_line() indentation

[Sami Kerola]

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 text-utils/more.c | 72 +++++++++++++++++++++++++++----------------------------
 1 file changed, 35 insertions(+), 37 deletions(-)

diff --git a/text-utils/more.c b/text-utils/more.c
index 75426e9..15ebb91 100644
--- a/text-utils/more.c
+++ b/text-utils/more.c
@@ -997,47 +997,45 @@ int get_line(register FILE *f, int *length)
 		} else if (c == EOF) {
 			*length = p - Line;
 			return (column);
-		} else {
+		}
 #ifdef HAVE_WIDECHAR
-			if (fold_opt && MB_CUR_MAX > 1) {
-				memset(mbc, '\0', MB_LEN_MAX);
-				mbc_pos = 0;
-				mbc[mbc_pos++] = c;
-				state_bak = state;
-
-				mblength = mbrtowc(&wc, mbc, mbc_pos, &state);
-				/* The value of mblength is always less than 2 here. */
-				switch (mblength) {
-				case (size_t)-2:
-					p--;
-					file_pos_bak = Ftell(f) - 1;
-					state = state_bak;
-					use_mbc_buffer_flag = 1;
-					break;
-
-				case (size_t)-1:
-					state = state_bak;
-					column++;
-					break;
-
-				default:
-					wc_width = wcwidth(wc);
-					if (wc_width > 0)
-						column += wc_width;
-				}
-			} else
+		else if (fold_opt && MB_CUR_MAX > 1) {
+			memset(mbc, '\0', MB_LEN_MAX);
+			mbc_pos = 0;
+			mbc[mbc_pos++] = c;
+			state_bak = state;
+
+			mblength = mbrtowc(&wc, mbc, mbc_pos, &state);
+			/* The value of mblength is always less than 2 here. */
+			switch (mblength) {
+			case (size_t)-2:
+				p--;
+				file_pos_bak = Ftell(f) - 1;
+				state = state_bak;
+				use_mbc_buffer_flag = 1;
+				break;
+
+			case (size_t)-1:
+				state = state_bak;
+				column++;
+				break;
+
+			default:
+				wc_width = wcwidth(wc);
+				if (wc_width > 0)
+					column += wc_width;
+			}
+		}
 #endif	/* HAVE_WIDECHAR */
-			{
-				if (isprint(c))
-					column++;
-				else {
-					column -= 4;
-					if (column < 0)
-						column = 0;
-				}
+		else {
+			if (isprint(c))
+				column++;
+			else {
+				column -= 4;
+				if (column < 0)
+					column = 0;
 			}
 		}
-
 		if (column >= Mcol && fold_opt)
 			break;
 #ifdef HAVE_WIDECHAR
-- 
2.2.2

Re: [PATCH] more: fix control character display bug

[Karel Zak]

On Fri, Jan 30, 2015 at 09:54:10AM +0100, Sami Kerola wrote:
>  text-utils/more.c | 59 +++++++++++++++++++++++++------------------------------
>  1 file changed, 27 insertions(+), 32 deletions(-)

Does it make sense to apply this patch? I guess the issue is already
(also) fixed in you "more" branch at git hub, right?

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH] more: fix control character display bug

[Sami Kerola]

On 20 February 2015 at 10:29, Karel Zak <kzak@redhat.com> wrote:
> On Fri, Jan 30, 2015 at 09:54:10AM +0100, Sami Kerola wrote:
>>  text-utils/more.c | 59 +++++++++++++++++++++++++------------------------------
>>  1 file changed, 27 insertions(+), 32 deletions(-)
>
> Does it make sense to apply this patch? I guess the issue is already
> (also) fixed in you "more" branch at git hub, right?

Hi Karel

Please do not merge this change.

As you said I have quite some changes to more(1) in my git[1], and I
have intention to fix the issue before v2.27. The current change set
does not solve the problem,  but I will send it anyway during weekend
to flush my change buffer[2].

[1] https://github.com/kerolasa/lelux-utiliteetit/tree/more
[2] 70 changes in total. Sorry for review burden, I'm been buffering
since 2.26-rc1

-- 
Sami Kerola
http://www.iki.fi/kerolasa/