cli: multi-line history replay produces terminal output with padded spaces instead of newlines

[jordanlewis]

This usability issue is very irritating - it causes pastes from terminal sessions to appear mangled. This happens all the time with customers.

Here's how to reproduce:

root@127.0.0.1:62762/defaultdb> select 1
                             -> + 2;
  ?column?
+----------+
         3
(1 row)

Time: 717µs

Now, press up and enter. That will produce the following output (changes depending on your terminal size):

root@127.0.0.1:62762/defaultdb> select 1                                                                                                                                                                    + 2;
  ?column?
+----------+
         3
(1 row)

Time: 413µs

As you can see, the problem is that the readline renderer produces a single, long string output for the line with an embedded newline, padding the continued lines with spaces up to the edge of the terminal.

I looked into this a bit and unfortunately the problem is not easy to solve. I confirmed that we're passing the correct (unpadded) input into readline. The problem occurs when readline retrieves that input from its history and renders it to the screen. The code is very confusing, but look at refresh.c, specifically the routines that call re__copy_and_pad() - that's the function that pads a line with spaces. Naively messing with that function to stop doing that completely breaks libedit.

[jordanlewis]

I went ahead and made a tiny C reproduction, so this probably can be closed in favor of an upstream bug report... but leaving open anyway at least to track the issue.

Compile with gcc foo.c -ledit, then run:

demo> foo
demo> bar;
Got: foo
bar;
demo> foo                                                                                                              bar;

To produce this, I typed foo<enter>bar;<enter><up>

And lo and behold, the padding issue exists there too.

#include <stdio.h>
#include <string.h>
#include <stdbool.h>
#include <stdlib.h>
#include <editline/readline.h>

int main(int argc, char** argv) {
  char *continuedLine;
  while (1) {
    char* input = readline("demo> ");
    int inputLen = strlen(input);
    bool finished = input[inputLen - 1] == ';';
    if (!finished && continuedLine == NULL) {
        continuedLine = input;
        continue;
    }

    if (continuedLine != NULL) {
        int oldLen = strlen(continuedLine);
        continuedLine = realloc(continuedLine, oldLen + inputLen + 1);
        continuedLine[oldLen] = '\n';
        strncpy(continuedLine + oldLen + 1, input, inputLen);
    }

    if (!finished) {
        continue;
    }

    input = continuedLine;
    continuedLine = NULL;

    add_history(input);
    printf("Got: %s\n", input);
    free(input);
  }
  return 0;
}

[jordanlewis]

Much simpler repro (just feed a newline string into add_history):

#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;
}

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)>

[knz]

Fun fact: the problem faced by libedit, and in turn one of the major reasons why the terminfo database is so much more complex than termcap (and why ncurses exist -- hint, the terminfo db (in the shape of ncurses initially) has primarily been brought into existence to implement GNU readline), is exactly the problem of how to draw multiple lines of text from the current cursor position on the terminal and subsequently bring the cursor back where it was, without waisting the terminal bandwidth by filling with space characters.

I fear this is one of the things that can't be solved unless libedit was grown to expand into the shape of GNU readline and mandate the complexity of terminfo with it, by which time we'd have grown the cockroach binary by a dozen more megabytes just to satisfy the need of a command line client.

The alternative, of course, is to pretend to solve this problem for just one or two terminals, perhaps biased by what the current cockroachdb developers happen to use on macOS, and annoy the majority of sysadmins out there who are unlucky to use various levels of terminal emulators with odd semantics.

[jordanlewis]

@knz note that this still happens on Debian Linux... unless you're saying that it's biased by the shell I'm accessing the machine from

[jordanlewis]

I started a mailing list thread about it http://mail-index.netbsd.org/netbsd-bugs/2018/10/25/msg059467.html

[knz]

Thanks for launching that thread.

Meanwhile I went ahead and dug into the netbsd changes since the version we're using. I haven't seen anything of note.

[nbuwe]

Fun fact: the problem faced by libedit, and in turn one of the major reasons why the terminfo database is so much more complex than termcap (and why ncurses exist -- hint, the terminfo db (in the shape of ncurses initially) has primarily been brought into existence to implement GNU readline), is exactly the problem of how to draw multiple lines of text from the current cursor position on the terminal and subsequently bring the cursor back where it was, without waisting the terminal bandwidth by filling with space characters.

Just for the record - this is factually wrong. You can perfectly well do this with termcap if termcap can describe your terminal (and for most terminals you don't need the advanced terminfo terminal description language features).

[knz]

@nbuwe my understanding is that termcap entries are not rich enough to allow clients to reliably determine the cursor position after emitting output when the cursor is positioned at the last column of a line, especially on the last line, on several common terminal types.

If you think that's misinformed I would love you to point me to documentation on how to best achieve this using termcaps only.

[nbuwe]

I don't think terminfo has a capability to express that for the lower-right corner either. GNU termcap had an LP extension for that. For other lines there is am (auto margin) capability (terminfo's auto_right_margin).

For the lower-right corner you can avoid the problem by using insert mode or insert character(s) caps. Since ~all terminals that are not completely dumb provide some insertion command, you should be ok for all practical purposes. Worst case, you just avoid writing anything into the corner... The GNU termcap :xn:LP: thing is an optimization that tells you it's safe to just write directly instead of using the insertion dance. As I said, afaik, terminfo doesn't have that extension (xn is eat_newline_glitch).

[knz]

My understanding is that the complexity of GNU readline comes partly from the fact that this problem exists, and GNU readline as a result instead manually uses other terminfo caps to scroll the terminal when handling multi-line input a the bottom of the terminal. These caps in turn are terminfo-specific.

Because these facilities are not available in termcap, libedit limits itself to not do this and is thus affected by the bug filed here.

[nbuwe]

My understanding is that the complexity of GNU readline comes partly from the fact that this problem exists, and GNU readline as a result instead manually uses other terminfo caps to scroll the terminal when handling multi-line input a the bottom of the terminal. These caps in turn are terminfo-specific.

Your understanding is wrong. Again, termcap has two (or rather one and a half) limitations compared to terminfo. One (half) is that two-character capability names are awkward to work with. That made sense back in 16-bit days, but now it's an annoyance. That's not, really, a limitation (unless you need a lot of caps, just letters and numbers give you 36 ** 2 = 1296). The other is that that the terminfo language allows you to express more complex capabilities, but I don't remember (it's been 25+ years) which terminals actually needed that. So in theory there may be a terminal that has some command sequence that you can express with terminfo and cannot express in termcap. I'm pretty sure none of the common terminal types actually falls into that category.

As i said, to work correctly with the lower-right corner you only need some form of character insertion (or, worst case, you just avoid it) and there's definitely nothing terminfo specific about that.

That kind of logic is usually handled by curses and nothing prevents libedit from doing the same. As I said, the format of the capabilities database has nothing to do with it. It's just nobody really bothered.

[jordanlewis]

This was fixed by http://cvsweb.netbsd.org/bsdweb.cgi/src/lib/libedit/terminal.c.diff?r1=1.33&r2=1.34&only_with_tag=MAIN&f=h, now we just need to find a way to vendor that library unconditionally, and not just on linux.

[jordanlewis]

[jordanlewis]

@benesch thank you so much for your determination on this!

[benesch]

💪🏽

…

On Fri, Jun 21, 2019 at 6:07 PM Jordan Lewis ***@***.***> wrote: @benesch <https://github.com/benesch> thank you so much for your determination on this! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#31843?email_source=notifications&email_token=AAGXSIAXHGRVQEBJNSN2GPDP3VGI3A5CNFSM4F7E7NH2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODYJWLDQ#issuecomment-504587662>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAGXSIG45ZHGKWRMHHGMRHDP3VGI3ANCNFSM4F7E7NHQ> .