NetBSD Problem Report #53682
From www@NetBSD.org Thu Oct 25 00:39:07 2018
Return-Path: <www@NetBSD.org>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
(Client CN "mail.NetBSD.org", Issuer "mail.NetBSD.org CA" (not verified))
by mollari.NetBSD.org (Postfix) with ESMTPS id 985C07A1B9
for <gnats-bugs@gnats.NetBSD.org>; Thu, 25 Oct 2018 00:39:07 +0000 (UTC)
Message-Id: <20181025003906.487DC7A202@mollari.NetBSD.org>
Date: Thu, 25 Oct 2018 00:39:06 +0000 (UTC)
From: jordan@cockroachlabs.com
Reply-To: jordan@cockroachlabs.com
To: gnats-bugs@NetBSD.org
Subject: libedit writes padding, not newline, to terminal when restoring a multi-line history entry
X-Send-Pr-Version: www-1.0
>Number: 53682
>Category: lib
>Synopsis: libedit writes padding, not newline, to terminal when restoring a multi-line history entry
>Confidential: no
>Severity: serious
>Priority: high
>Responsible: lib-bug-people
>State: open
>Class: sw-bug
>Submitter-Id: net
>Arrival-Date: Thu Oct 25 00:40:00 +0000 2018
>Last-Modified: Sat Nov 24 12:20:01 +0000 2018
>Originator: Jordan Lewis
>Release: latest
>Organization:
Cockroach Labs
>Environment:
Darwin Termato.local 17.4.0 Darwin Kernel Version 17.4.0: Sun Dec 17 09:19:54 PST 2017; root:xnu-4570.41.2~1/RELEASE_X86_64 x86_64
>Description:
The libedit library mishandles the printing of restored, multi-line history entries. Instead of printing a newline after each line within the history entry, it writes spaces until the edge of the terminal.
This leads to unreadable output when copy-pasting the result of libedit-written output, since all newlines in the restored command are deleted and replaced with padded spaces proportional to the size of the terminal.
See https://github.com/cockroachdb/cockroach/issues/31843 for more details.
>How-To-Repeat:
Compile and run the short test program at the end of this message, press up and enter, then copy-paste the output to a text editor to see what happened.
ACTUAL OUTPUT:
demo (press up to see bug)> this history line has an embedded newline
Got input: this history line has an
embedded newline
demo (press up to see bug)>
EXPECTED OUTPUT:
demo (press up to see bug)> this history line has an
embedded newline
Got input: this history line has an
embedded newline
demo (press up to see bug)>
Note that the actual output has no actual newline - just padded spaces
#include <stdio.h>
#include <stdlib.h>
#include <editline/readline.h>
int main(int argc, char** argv) {
add_history("this history line has an\nembedded newline");
while (1) {
char* input = readline("demo (press up to see bug)> ");
add_history(input);
printf("Got input: %s\n", input);
/* free input */
free(input);
}
return 0;
}
>Fix:
No known fix. I took a look at the code, especially refresh.c and the re__copy_and_pad function that I suspect is going wrong, but I couldn't understand what was going on well enough to fix it.
>Audit-Trail:
From: Leonardo Taccari <leot@NetBSD.org>
To: gnats-bugs@NetBSD.org
Cc:
Subject: Re: lib/53682: libedit writes padding, not newline, to terminal when restoring a multi-line history entry
Date: Thu, 25 Oct 2018 09:36:43 +0200
Hello Jordan,
jordan@cockroachlabs.com writes:
> >Number: 53682
> >Category: lib
> >Synopsis: libedit writes padding, not newline, to terminal when r=
estoring a multi-line history entry
> >Confidential: no
> >Severity: serious
> >Priority: high
> >Responsible: lib-bug-people
> >State: open
> >Class: sw-bug
> >Submitter-Id: net
> >Arrival-Date: Thu Oct 25 00:40:00 +0000 2018
> >Originator: Jordan Lewis
> >Release: latest
> [...]
> #include <stdio.h>
> #include <stdlib.h>
> #include <editline/readline.h>
>
> int main(int argc, char** argv) {
> add_history("this history line has an\nembedded newline");
> while (1) {
> char* input =3D readline("demo (press up to see bug)> ");
> add_history(input);
> printf("Got input: %s\n", input);
> /* free input */
> free(input);
> }
> return 0;
> }
> [...]
On NetBSD-current - after adjusting the include from <editline/readline.h>
to <readline/readline.h> it seems that running that there is a
"\n".
On which NetBSD version this happens?
If it's not on NetBSD and there are any $NetBSD$ RCS Id in the
editline source code and headers or similar please share them: it
can help to possible bisect the problem.
Thank you!
From: Raphael 'kena' Poss <knz@thaumogen.net>
To: netbsd-bugs@netbsd.org
Cc: gnats-bugs@NetBSD.org, Jordan Lewis <jordan@cockroachlabs.com>
Subject: Re: lib/53682: libedit writes padding, not newline, to terminal when
restoring a multi-line history entry
Date: Thu, 25 Oct 2018 15:13:46 +0200
This repros on the copy included in FreeBSD 12, RCS IDs included below.
It also repros on the copy included in Debian's own libedit package, RCS
IDs included below.
Please point us to the netbsd patch(es) where this was resolved.
FreeBSD:
chared.c:__RCSID("$NetBSD: chared.c,v 1.49 2016/02/24 14:29:21 christos
Exp $");
chartype.c:__RCSID("$NetBSD: chartype.c,v 1.23 2016/02/28 23:02:24
christos Exp $");
common.c:__RCSID("$NetBSD: common.c,v 1.40 2016/03/02 19:24:20 christos
Exp $");
el.c:__RCSID("$NetBSD: el.c,v 1.83 2016/02/24 17:13:22 christos Exp $");
eln.c:__RCSID("$NetBSD: eln.c,v 1.28 2016/02/28 23:02:24 christos Exp $");
emacs.c:__RCSID("$NetBSD: emacs.c,v 1.32 2016/02/16 22:53:14 christos
Exp $");
filecomplete.c:__RCSID("$NetBSD: filecomplete.c,v 1.40 2016/02/17
19:47:49 christos Exp $");
hist.c:__RCSID("$NetBSD: hist.c,v 1.24 2016/02/16 22:53:14 christos Exp $");
history.c:__RCSID("$NetBSD: history.c,v 1.52 2016/02/17 19:47:49
christos Exp $");
keymacro.c:__RCSID("$NetBSD: keymacro.c,v 1.14 2016/02/24 14:25:38
christos Exp $");
map.c:__RCSID("$NetBSD: map.c,v 1.43 2016/02/17 19:47:49 christos Exp $");
parse.c:__RCSID("$NetBSD: parse.c,v 1.35 2016/02/17 19:47:49 christos
Exp $");
prompt.c:__RCSID("$NetBSD: prompt.c,v 1.23 2016/02/16 15:53:48 christos
Exp $");
read.c:__RCSID("$NetBSD: read.c,v 1.86 2016/03/02 19:24:20 christos Exp $");
readline.c:__RCSID("$NetBSD: readline.c,v 1.126 2016/02/24 17:13:22
christos Exp $");
refresh.c:__RCSID("$NetBSD: refresh.c,v 1.45 2016/03/02 19:24:20
christos Exp $");
search.c:__RCSID("$NetBSD: search.c,v 1.39 2016/02/24 14:25:38 christos
Exp $");
sig.c:__RCSID("$NetBSD: sig.c,v 1.24 2016/02/16 19:08:41 christos Exp $");
terminal.c:__RCSID("$NetBSD: terminal.c,v 1.24 2016/03/22 01:38:17
christos Exp $");
tokenizer.c:__RCSID("$NetBSD: tokenizer.c,v 1.24 2016/02/17 19:47:49
christos Exp $");
tty.c:__RCSID("$NetBSD: tty.c,v 1.59 2016/03/22 01:34:32 christos Exp $");
vi.c:__RCSID("$NetBSD: vi.c,v 1.55 2016/03/02 19:24:20 christos Exp $");
Debian:
chared.c:__RCSID("$NetBSD: chared.c,v 1.56 2016/05/22 19:44:26 christos
Exp $");
chartype.c:__RCSID("$NetBSD: chartype.c,v 1.31 2017/01/09 02:54:18
christos Exp $");
common.c:__RCSID("$NetBSD: common.c,v 1.47 2016/05/22 19:44:26 christos
Exp $");
el.c:__RCSID("$NetBSD: el.c,v 1.92 2016/05/22 19:44:26 christos Exp $");
eln.c:__RCSID("$NetBSD: eln.c,v 1.34 2016/05/09 21:37:34 christos Exp $");
emacs.c:__RCSID("$NetBSD: emacs.c,v 1.36 2016/05/09 21:46:56 christos
Exp $");
filecomplete.c:__RCSID("$NetBSD: filecomplete.c,v 1.44 2016/10/31
17:46:32 abhinav Exp $");
hist.c:__RCSID("$NetBSD: hist.c,v 1.32 2017/03/05 19:23:58 christos Exp $");
history.c:__RCSID("$NetBSD: history.c,v 1.57 2016/04/11 18:56:31
christos Exp $");
keymacro.c:__RCSID("$NetBSD: keymacro.c,v 1.23 2016/05/24 15:00:45
christos Exp $");
map.c:__RCSID("$NetBSD: map.c,v 1.51 2016/05/09 21:46:56 christos Exp $");
parse.c:__RCSID("$NetBSD: parse.c,v 1.40 2016/05/09 21:46:56 christos
Exp $");
prompt.c:__RCSID("$NetBSD: prompt.c,v 1.26 2016/05/09 21:46:56 christos
Exp $");
read.c:__RCSID("$NetBSD: read.c,v 1.102 2016/12/11 15:47:06 christos Exp
$");
readline.c:__RCSID("$NetBSD: readline.c,v 1.140 2017/01/09 03:09:05
christos Exp $");
refresh.c:__RCSID("$NetBSD: refresh.c,v 1.51 2016/05/09 21:46:56
christos Exp $");
search.c:__RCSID("$NetBSD: search.c,v 1.47 2016/05/09 21:46:56 christos
Exp $");
sig.c:__RCSID("$NetBSD: sig.c,v 1.26 2016/05/09 21:46:56 christos Exp $");
strlcat.c:__RCSID("$NetBSD: strlcat.c,v 1.4 2013/01/23 07:57:27 matt Exp
$");
strlcpy.c:__RCSID("$NetBSD: strlcpy.c,v 1.3 2007/06/04 18:19:27 christos
Exp $");
terminal.c:__RCSID("$NetBSD: terminal.c,v 1.32 2016/05/09 21:46:56
christos Exp $");
tokenizer.c:__RCSID("$NetBSD: tokenizer.c,v 1.28 2016/04/11 18:56:31
christos Exp $");
tty.c:__RCSID("$NetBSD: tty.c,v 1.65 2016/05/09 21:46:56 christos Exp $");
unvis.c:__RCSID("$NetBSD: unvis.c,v 1.44 2014/09/26 15:43:36 roy Exp $");
vi.c:__RCSID("$NetBSD: vi.c,v 1.62 2016/05/09 21:46:56 christos Exp $");
vis.c:__RCSID("$NetBSD: vis.c,v 1.72 2017/02/12 22:37:49 christos Exp $");
wcsdup.c:__RCSID("$NetBSD: wcsdup.c,v 1.3 2008/05/26 13:17:48 haad Exp $");
--
Raphael 'kena' Poss
From: Nikhil Benesch <nikhil.benesch@gmail.com>
To: gnats-bugs@netbsd.org
Cc:
Subject: Re: lib/53682
Date: Tue, 20 Nov 2018 16:47:03 -0500
Attached is a patch for PR 53682. I based the patch on
https://www.thrysoee.dk/editline/, not the NetBSD source tree, so the
filename will need to be adjusted. I've reviewed the recent changes to
refresh.c in the NetBSD tree, however, and I expect the patch to
otherwise apply cleanly.
For reference, this is the RCS header from my version of refresh.c:
/* $NetBSD: refresh.c,v 1.54 2017/06/30 20:26:52 kre Exp $ */
I should note that I am rather perplexed by what the code I am removing
was trying to accomplish. It was introduced in [0] to improve handling
of the rightmost column on terminals with the automargin (am) termcap,
and appears to have been copied from tcsh. Most of that patch seems
sensible. For example, terminal_overwrite was taught to account for
automargin when writing the rightmost character.
What I don't understand is why terminal_move_to_line was given an
entirely new implementation on terminals with automargin. Namely,
instead of outputting DO or a newline, like it would on non-automargin
terminals, it moves to the last character on the line and overwrites
that character with whatever was already there, relying on automargin
to move the cursor to the next line as a side effect. In the common
case where the line does not take up the full width of the terminal,
the rightmost character is a padding character (a space) that was not
actually output; outputting it here causes my terminal emulator to fill
up the entire line with spaces, resulting in the behavior that Jordan
observed.
This patch removes this special case for terminal_move_to_line. This
prevents the space padding on my terminal emulator and hasn't caused any
other aberrations, even when writing to the rightmost column.
If there is some edge case here that I'm not seeing, I'd be happy to
find an alternative workaround that does not result in space padding,
provided someone can point me to a terminal emulator that exhibits a
problem with this patch.
[0]: https://github.com/NetBSD/src/commit/dd263dcc3c17e30437fcbe4b3427caf0eb226635
diff --git a/src/terminal.c b/src/terminal.c
index 9c74fcd..cc0169e 100644
--- a/src/terminal.c
+++ b/src/terminal.c
@@ -509,36 +509,14 @@ terminal_move_to_line(EditLine *el, int where)
return;
}
if ((del = where - el->el_cursor.v) > 0) {
- while (del > 0) {
- if (EL_HAS_AUTO_MARGINS &&
- el->el_display[el->el_cursor.v][0] != '\0') {
- size_t h = (size_t)
- (el->el_terminal.t_size.h - 1);
- for (; h > 0 &&
- el->el_display[el->el_cursor.v][h] ==
- MB_FILL_CHAR;
- h--)
- continue;
- /* move without newline */
- terminal_move_to_char(el, (int)h);
- terminal_overwrite(el, &el->el_display
- [el->el_cursor.v][el->el_cursor.h],
- (size_t)(el->el_terminal.t_size.h -
- el->el_cursor.h));
- /* updates Cursor */
- del--;
- } else {
- if ((del > 1) && GoodStr(T_DO)) {
- terminal_tputs(el, tgoto(Str(T_DO), del,
- del), del);
- del = 0;
- } else {
- for (; del > 0; del--)
- terminal__putc(el, '\n');
- /* because the \n will become \r\n */
- el->el_cursor.h = 0;
- }
- }
+ if ((del > 1) && GoodStr(T_DO)) {
+ terminal_tputs(el, tgoto(Str(T_DO), del, del), del);
+ del = 0;
+ } else {
+ for (; del > 0; del--)
+ terminal__putc(el, '\n');
+ /* because the \n will become \r\n */
+ el->el_cursor.h = 0;
}
} else { /* del < 0 */
if (GoodStr(T_UP) && (-del > 1 || !GoodStr(T_up)))
From: Nikhil Benesch <nikhil.benesch@gmail.com>
To: gnats-bugs@netbsd.org
Cc:
Subject: Re: lib/53682
Date: Tue, 20 Nov 2018 23:10:14 -0500
Apologies. It appears my email client is swallowing indentation. Here's
a link to the patch instead.
https://gist.githubusercontent.com/benesch/48ac40e76179ec7f71c58c6360f9c391/raw/150d515f7254256766869937923260fe0fd68e7a/term-no-space.patch.
I've now tested this patch on macOS Terminal app 2.7.4 (388.1.2), GNOME
terminal 3.18.3, and XTerm 322 without observing any problems.
From: christos@zoulas.com (Christos Zoulas)
To: gnats-bugs@NetBSD.org, lib-bug-people@netbsd.org,
gnats-admin@netbsd.org, netbsd-bugs@netbsd.org,
jordan@cockroachlabs.com
Cc:
Subject: Re: lib/53682
Date: Wed, 21 Nov 2018 03:29:31 -0500
On Nov 21, 4:15am, nikhil.benesch@gmail.com (Nikhil Benesch) wrote:
-- Subject: Re: lib/53682
| Apologies. It appears my email client is swallowing indentation. Here's
| a link to the patch instead.
|
| https://gist.githubusercontent.com/benesch/48ac40e76179ec7f71c58c6360f9c391/raw/150d515f7254256766869937923260fe0fd68e7a/term-no-space.patch.
|
| I've now tested this patch on macOS Terminal app 2.7.4 (388.1.2), GNOME
| terminal 3.18.3, and XTerm 322 without observing any problems.
|
As I mentioned before there will be no 'display' problems (well
you removed the loop too, so I don't know what happens if it needs
to move more than one line). You just removed the code that does
<space><backspace> in the automatic margins case to move the cursor
to the next line when it is at the right margin. This code is there
to avoid putting a newline into the character buffer. If you try
to cut and paste a multi-line string from a shell that uses libedit
using mouse selection, now it will break into multiple lines, where
before it would work just fine.
christos
From: Jordan Lewis <jordan@cockroachlabs.com>
To: christos@zoulas.com
Cc: gnats-bugs@netbsd.org, lib-bug-people@netbsd.org, gnats-admin@netbsd.org,
netbsd-bugs@netbsd.org
Subject: Re: lib/53682
Date: Wed, 21 Nov 2018 08:53:14 -0500
--000000000000f876f1057b2d12b2
Content-Type: text/plain; charset="UTF-8"
>
> If you try
> to cut and paste a multi-line string from a shell that uses libedit
> using mouse selection, now it will break into multiple lines, where
> before it would work just fine.
But this "breaking into multiple lines" is precisely the behavior that we're
trying to preserve. If the input was multiple lines (newlines entered during
entry of text), then the pasted "output" should be multiple lines too.
libedit
currently modifies the history from the perspective of the copy/pasting user
by inserting padding spaces instead of those newlines *that were inputted*.
Another way to see what's broken here is window resizing. Try entering a
multi-line string, scrolling up to it via history, then resizing the
window. You'll
see that the retrieved, historical one breaks in funny ways because of the
added padding spaces, but the actual user-inputted one doesn't break.
On Wed, Nov 21, 2018 at 3:29 AM Christos Zoulas <christos@zoulas.com> wrote:
> On Nov 21, 4:15am, nikhil.benesch@gmail.com (Nikhil Benesch) wrote:
> -- Subject: Re: lib/53682
>
> | Apologies. It appears my email client is swallowing indentation. Here's
> | a link to the patch instead.
> |
> |
> https://gist.githubusercontent.com/benesch/48ac40e76179ec7f71c58c6360f9c391/raw/150d515f7254256766869937923260fe0fd68e7a/term-no-space.patch
> .
> |
> | I've now tested this patch on macOS Terminal app 2.7.4 (388.1.2), GNOME
> | terminal 3.18.3, and XTerm 322 without observing any problems.
> |
>
> As I mentioned before there will be no 'display' problems (well
> you removed the loop too, so I don't know what happens if it needs
> to move more than one line). You just removed the code that does
> <space><backspace> in the automatic margins case to move the cursor
> to the next line when it is at the right margin. This code is there
> to avoid putting a newline into the character buffer. If you try
> to cut and paste a multi-line string from a shell that uses libedit
> using mouse selection, now it will break into multiple lines, where
> before it would work just fine.
>
> christos
>
--000000000000f876f1057b2d12b2
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
<div dir=3D"ltr"><blockquote class=3D"gmail_quote" style=3D"margin:0px 0px =
0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">If you t=
ry<br>to cut and paste a multi-line string from a shell that uses libedit<b=
r>using mouse selection, now it will break into multiple lines, where<br>be=
fore it would work just fine.</blockquote><div><br></div><div>But this &quo=
t;breaking into multiple lines" is precisely the behavior that we'=
re</div><div>trying to preserve. If the input was multiple lines (newlines =
entered during</div><div>entry of text), then the pasted "output"=
should be multiple lines too. libedit</div><div>currently modifies the his=
tory from the perspective of the copy/pasting user</div><div>by inserting p=
adding spaces instead of those newlines *that were inputted*.</div><div><br=
></div><div>Another way to see what's broken here is window resizing. T=
ry entering a</div><div>multi-line string, scrolling up to it via history, =
then resizing the window. You'll</div><div>see that the retrieved, hist=
orical one breaks in funny ways because of the</div><div>added padding spac=
es, but the actual user-inputted one doesn't break.=C2=A0</div></div><b=
r><div class=3D"gmail_quote"><div dir=3D"ltr">On Wed, Nov 21, 2018 at 3:29 =
AM Christos Zoulas <<a href=3D"mailto:christos@zoulas.com">christos@zoul=
as.com</a>> wrote:<br></div><blockquote class=3D"gmail_quote" style=3D"m=
argin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Nov 21,=C2=
=A0 4:15am, <a href=3D"mailto:nikhil.benesch@gmail.com" target=3D"_blank">n=
ikhil.benesch@gmail.com</a> (Nikhil Benesch) wrote:<br>
-- Subject: Re: lib/53682<br>
<br>
|=C2=A0 Apologies. It appears my email client is swallowing indentation. He=
re's<br>
|=C2=A0 a link to the patch instead.<br>
|=C2=A0 <br>
|=C2=A0 <a href=3D"https://gist.githubusercontent.com/benesch/48ac40e76179e=
c7f71c58c6360f9c391/raw/150d515f7254256766869937923260fe0fd68e7a/term-no-sp=
ace.patch" rel=3D"noreferrer" target=3D"_blank">https://gist.githubusercont=
ent.com/benesch/48ac40e76179ec7f71c58c6360f9c391/raw/150d515f72542567668699=
37923260fe0fd68e7a/term-no-space.patch</a>.<br>
|=C2=A0 <br>
|=C2=A0 I've now tested this patch on macOS Terminal app 2.7.4 (388.1.2=
), GNOME<br>
|=C2=A0 terminal 3.18.3, and XTerm 322 without observing any problems.<br>
|=C2=A0 <br>
<br>
As I mentioned before there will be no 'display' problems (well<br>
you removed the loop too, so I don't know what happens if it needs<br>
to move more than one line). You just removed the code that does<br>
<space><backspace> in the automatic margins case to move the cu=
rsor<br>
to the next line when it is at the right margin. This code is there<br>
to avoid putting a newline into the character buffer. If you try<br>
to cut and paste a multi-line string from a shell that uses libedit<br>
using mouse selection, now it will break into multiple lines, where<br>
before it would work just fine.<br>
<br>
christos<br>
</blockquote></div>
--000000000000f876f1057b2d12b2--
From: christos@zoulas.com (Christos Zoulas)
To: Jordan Lewis <jordan@cockroachlabs.com>
Cc: gnats-bugs@netbsd.org, lib-bug-people@netbsd.org,
gnats-admin@netbsd.org, netbsd-bugs@netbsd.org
Subject: Re: lib/53682
Date: Wed, 21 Nov 2018 09:22:53 -0500
On Nov 21, 8:53am, jordan@cockroachlabs.com (Jordan Lewis) wrote:
-- Subject: Re: lib/53682
| But this "breaking into multiple lines" is precisely the behavior that we're
| trying to preserve. If the input was multiple lines (newlines entered during
| entry of text), then the pasted "output" should be multiple lines too.
| libedit
| currently modifies the history from the perspective of the copy/pasting user
| by inserting padding spaces instead of those newlines *that were inputted*.
Yes, these newlines should be preserved.
| Another way to see what's broken here is window resizing. Try entering a
| multi-line string, scrolling up to it via history, then resizing the
| window. You'll
| see that the retrieved, historical one breaks in funny ways because of the
| added padding spaces, but the actual user-inputted one doesn't break.
Yes, still if the user did not enter a newline, then when cutting and pasting
the single line it should not end up as two lines, don't you agree?
christos
From: Jordan Lewis <jordan@cockroachlabs.com>
To: Christos Zoulas <christos@zoulas.com>
Cc: gnats-bugs@netbsd.org, lib-bug-people@netbsd.org, gnats-admin@netbsd.org,
netbsd-bugs@netbsd.org
Subject: Re: lib/53682
Date: Wed, 21 Nov 2018 09:39:03 -0500
--000000000000eba4b7057b2db627
Content-Type: text/plain; charset="UTF-8"
I understand what you were getting at now. I agree that wrapped lines
shouldn't end up with embedded newlines that weren't entered by the user.
Do you have an idea about how to correct the behavior?
On Wed, Nov 21, 2018, 09:23 Christos Zoulas <christos@zoulas.com wrote:
> On Nov 21, 8:53am, jordan@cockroachlabs.com (Jordan Lewis) wrote:
> -- Subject: Re: lib/53682
>
> | But this "breaking into multiple lines" is precisely the behavior that
> we're
> | trying to preserve. If the input was multiple lines (newlines entered
> during
> | entry of text), then the pasted "output" should be multiple lines too.
> | libedit
> | currently modifies the history from the perspective of the copy/pasting
> user
> | by inserting padding spaces instead of those newlines *that were
> inputted*.
>
> Yes, these newlines should be preserved.
>
> | Another way to see what's broken here is window resizing. Try entering a
> | multi-line string, scrolling up to it via history, then resizing the
> | window. You'll
> | see that the retrieved, historical one breaks in funny ways because of
> the
> | added padding spaces, but the actual user-inputted one doesn't break.
>
> Yes, still if the user did not enter a newline, then when cutting and
> pasting
> the single line it should not end up as two lines, don't you agree?
>
> christos
>
--000000000000eba4b7057b2db627
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
<div dir=3D"auto"><div>I understand what you were getting at now. I agree t=
hat wrapped lines shouldn't end up with embedded newlines that weren=
9;t entered by the user.</div><div dir=3D"auto"><br></div><div dir=3D"auto"=
>Do you have an idea about how to correct the behavior?<br><br><div class=
=3D"gmail_quote" dir=3D"auto"><div dir=3D"ltr">On Wed, Nov 21, 2018, 09:23 =
Christos Zoulas <<a href=3D"mailto:christos@zoulas.com">christos@zoulas.=
com</a> wrote:<br></div><blockquote class=3D"gmail_quote" style=3D"margin:0=
0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Nov 21,=C2=A0 8:5=
3am, <a href=3D"mailto:jordan@cockroachlabs.com" target=3D"_blank" rel=3D"n=
oreferrer">jordan@cockroachlabs.com</a> (Jordan Lewis) wrote:<br>
-- Subject: Re: lib/53682<br>
<br>
| But this "breaking into multiple lines" is precisely the behavi=
or that we're<br>
| trying to preserve. If the input was multiple lines (newlines entered dur=
ing<br>
| entry of text), then the pasted "output" should be multiple lin=
es too.<br>
| libedit<br>
| currently modifies the history from the perspective of the copy/pasting u=
ser<br>
| by inserting padding spaces instead of those newlines *that were inputted=
*.<br>
<br>
Yes, these newlines should be preserved.<br>
<br>
| Another way to see what's broken here is window resizing. Try enterin=
g a<br>
| multi-line string, scrolling up to it via history, then resizing the<br>
| window. You'll<br>
| see that the retrieved, historical one breaks in funny ways because of th=
e<br>
| added padding spaces, but the actual user-inputted one doesn't break.=
<br>
<br>
Yes, still if the user did not enter a newline, then when cutting and pasti=
ng<br>
the single line it should not end up as two lines, don't you agree?<br>
<br>
christos<br>
</blockquote></div></div></div>
--000000000000eba4b7057b2db627--
From: Nikhil Benesch <nikhil.benesch@gmail.com>
To: gnats-bugs@netbsd.org
Cc: christos@zoulas.com, jordan@cockroachlabs.com
Subject: Re: lib/53682
Date: Wed, 21 Nov 2018 10:57:19 -0500
> As I mentioned before there will be no 'display' problems
Sorry, I'm struggling to stay in the loop on this PR since I'm not
subscribed to gnats-bugs. If you've previously rendered feedback on
this patch or the issue (i.e., before the mail that I've quoted above),
I'm afraid I can't see it.
> (well you removed the loop too, so I don't know what happens if it needs
> to move more than one line)
If you look more closely at the patch, you'll see that the remaining code
either issues a T_DO to move down multiple lines at once, or uses a for
loop to issue the requisite number of individual newlines. So it works
just fine if it needs to move more than one line.
> You just removed the code that does <space><backspace> in the
> automatic margins case to move the cursor to the next line when it is
> at the right margin.
Is that what the code is intended to do? AFAICT you wrote and/or
imported this code into NetBSD, so you are the right person to ask. :)
The code I removed *unconditionally* issues
<move-to-right-margin><space><backspace>
regardless of whether the cursor is already at the right margin. If it
was only supposed to do that if the cursor was already at the right
margin, perhaps fixing that will also fix the problem described in this
PR.
> If you try to cut and paste a multi-line string from a shell that
> uses libedit using mouse selection, now it will break into multiple
> lines, where before it would work just fine.
The behavior you describe is the behavior I see both with and without
my patch. Are you speculating, are do you see incorrect behavior in
your terminal emulator with my patch? If the latter, I'd be happy to
investigate further if you tell me what terminal emulator you use.
From: christos@zoulas.com (Christos Zoulas)
To: Jordan Lewis <jordan@cockroachlabs.com>
Cc: gnats-bugs@netbsd.org, lib-bug-people@netbsd.org,
gnats-admin@netbsd.org, netbsd-bugs@netbsd.org
Subject: Re: lib/53682
Date: Wed, 21 Nov 2018 12:14:03 -0500
On Nov 21, 9:39am, jordan@cockroachlabs.com (Jordan Lewis) wrote:
-- Subject: Re: lib/53682
| I understand what you were getting at now. I agree that wrapped lines
| shouldn't end up with embedded newlines that weren't entered by the user.
|
| Do you have an idea about how to correct the behavior?
We are going to have to identify both cases (when to put newlines and when not
to). Perhaps it could be an editor setting. I am not sure.
christos
From: christos@zoulas.com (Christos Zoulas)
To: Nikhil Benesch <nikhil.benesch@gmail.com>, gnats-bugs@netbsd.org
Cc: jordan@cockroachlabs.com
Subject: Re: lib/53682
Date: Wed, 21 Nov 2018 12:22:52 -0500
On Nov 21, 10:57am, nikhil.benesch@gmail.com (Nikhil Benesch) wrote:
-- Subject: Re: lib/53682
| Sorry, I'm struggling to stay in the loop on this PR since I'm not
| subscribed to gnats-bugs. If you've previously rendered feedback on
| this patch or the issue (i.e., before the mail that I've quoted above),
| I'm afraid I can't see it.
I probably replied to it in email before the PR was opened.
| If you look more closely at the patch, you'll see that the remaining code
| either issues a T_DO to move down multiple lines at once, or uses a for
| loop to issue the requisite number of individual newlines. So it works
| just fine if it needs to move more than one line.
Ok, that is fine.
| Is that what the code is intended to do? AFAICT you wrote and/or
| imported this code into NetBSD, so you are the right person to ask. :)
Yes, the code is from tcsh, we ended up using the <space><backspace> trick
a long time ago to avoid breaking the lines.
| The code I removed *unconditionally* issues
|
| <move-to-right-margin><space><backspace>
I thought it does it only for terminals that have automatic margins. There
is also the magic margins complication... This code is from 25 years ago,
see the Fixes file for the magic margins changes.
| regardless of whether the cursor is already at the right margin. If it
| was only supposed to do that if the cursor was already at the right
| margin, perhaps fixing that will also fix the problem described in this
| PR.
I think that it should do it only when we've reached the right margin.
If that change fixes your issue then the problem is simple; if not, then
we need to either figure out when to do the <space><backspace> trick
and when not to.
| The behavior you describe is the behavior I see both with and without
| my patch. Are you speculating, are do you see incorrect behavior in
| your terminal emulator with my patch? If the latter, I'd be happy to
| investigate further if you tell me what terminal emulator you use.
No, I have not tried your patch but I am guessing that it will break the
single line in two. I will try to test, but I am really busy this week.
The easiest way to test is to make the change on a NetBSD box and use
the NetBSD /bin/sh with the editor on using an xterm (which has automatic
margins and magic margins in the default setup).
christos
From: Nikhil Benesch <nikhil.benesch@gmail.com>
To: christos@zoulas.com
Cc: gnats-bugs@netbsd.org, jordan@cockroachlabs.com
Subject: Re: lib/53682
Date: Wed, 21 Nov 2018 13:43:08 -0500
> | The code I removed *unconditionally* issues
> |
> | <move-to-right-margin><space><backspace>
>
> I thought it does it only for terminals that have automatic margins. There
> is also the magic margins complication... This code is from 25 years ago,
> see the Fixes file for the magic margins changes.
Sorry, yes. I meant that, once you have an automargin terminal, the
code unconditionally uses the <space><backspace> trick, regardless of
where your cursor is located.
> No, I have not tried your patch but I am guessing that it will break the
> single line in two. I will try to test, but I am really busy this week.
> The easiest way to test is to make the change on a NetBSD box and use
> the NetBSD /bin/sh with the editor on using an xterm (which has automatic
> margins and magic margins in the default setup).
No problem. I appreciate you taking the time to respond. I'll set myself
up a NetBSD VM in the meantime.
The reason I suspect that things work fine with the patch is that if
re_update_line draws a line that reaches the right margin,
terminal_overwrite will properly mark the cursor as having advanced to
the next line if the terminal has automatic margins. The next call to
terminal_move_to_line will thus be a no-op and no newline will be
emitted.
From: Nikhil Benesch <nikhil.benesch@gmail.com>
To: christos@zoulas.com
Cc: gnats-bugs@netbsd.org, jordan@cockroachlabs.com
Subject: Re: lib/53682
Date: Wed, 21 Nov 2018 22:14:12 -0500
> > No, I have not tried your patch but I am guessing that it will break the
> > single line in two. I will try to test, but I am really busy this week.
> > The easiest way to test is to make the change on a NetBSD box and use
> > the NetBSD /bin/sh with the editor on using an xterm (which has automatic
> > margins and magic margins in the default setup).
>
> No problem. I appreciate you taking the time to respond. I'll set myself
> up a NetBSD VM in the meantime.
I've now tested this on NetBSD 8.0's xterm. Lines without newlines that
exceed the terminal width are not split in two when copied and pasted,
even with the patch applied.
The hardest part of this ordeal was finding a mouse with middle click.
From: "Christos Zoulas" <christos@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc:
Subject: PR/53682 CVS commit: src/lib/libedit
Date: Sat, 24 Nov 2018 07:17:35 -0500
Module Name: src
Committed By: christos
Date: Sat Nov 24 12:17:35 UTC 2018
Modified Files:
src/lib/libedit: terminal.c
Log Message:
PR/53682: Jordan Lewis: use newlines instead of padded spaces when restoring
multi-line histories.
To generate a diff of this commit:
cvs rdiff -u -r1.33 -r1.34 src/lib/libedit/terminal.c
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
(Contact us)
$NetBSD: query-full-pr,v 1.43 2018/01/16 07:36:43 maya Exp $
$NetBSD: gnats_config.sh,v 1.9 2014/08/02 14:16:04 spz Exp $
Copyright © 1994-2017
The NetBSD Foundation, Inc. ALL RIGHTS RESERVED.
Home