From b1280cde162e8bb7b57a3a7c49484c0f50ec8fba Mon Sep 17 00:00:00 2001 From: Anton Lindqvist Date: Sun, 15 Apr 2018 15:54:03 +0200 Subject: [PATCH 1/6] Switch to an event driven design This is probably my last remaining annoyance with pick, the fact that it does a blocking read of stdin until EOF prior to entering its main loop. If stdin is slow, this will take time; like running git-log in a huge repository. The new approach uses poll(2) and can therefore read from stdin as new data arrives and simultaneously handle user commands. Some notes about the changes: - get_choices() is made re-entrant. It still allocates a single block of memory for the stdin data. Each choice stores an indirect pointer to this piece of memory since it could be realloacted invalidating any previously stored pointer. This approach is more complex than storing the string in each choice but much faster due to less allocations and strdup(3):ing. - tty_getc() no longer uses the FILE API from libc since it's buffered. Meaning, it could potentially read more input stored in its internal buffer during a single call to getc(3). If all that data is not consumed during the call to get_key(), poll will never flag the underlying file descriptor as non-block readable again since the data is already consumed. Issue exhibited by the test suite. To see it in action: $ while :; do sleep 1; echo bar; done | pick -q bar --- pick.c | 283 +++++++++++++++++++++++++++++++++++---------------------- 1 file changed, 172 insertions(+), 111 deletions(-) diff --git a/pick.c b/pick.c index 4ba17de3..55a7cfca 100644 --- a/pick.c +++ b/pick.c @@ -6,6 +6,7 @@ #include #include +#include #include #include #include @@ -60,19 +61,22 @@ enum key { }; struct choice { - const char *description; - const char *string; + char *const *string; + size_t offset; size_t length; + size_t description; ssize_t match_start; /* inclusive match start offset */ ssize_t match_end; /* exclusive match end offset */ double score; }; static int choicecmp(const void *, const void *); +static const char *choice_description(const struct choice *); +static const char *choice_string(const struct choice *); static void delete_between(char *, size_t, size_t, size_t); static char *eager_strpbrk(const char *, const char *); static int filter_choices(size_t); -static char *get_choices(void); +static size_t get_choices(int); static enum key get_key(const char **); static void handle_sigwinch(int); static int isu8cont(unsigned char); @@ -117,7 +121,6 @@ int main(int argc, char *argv[]) { const struct choice *choice; - char *input; int c; int output_description = 0; int rc = 0; @@ -178,7 +181,6 @@ main(int argc, char *argv[]) err(1, NULL); } - input = get_choices(); tty_init(1); #ifdef HAVE_PLEDGE @@ -189,14 +191,15 @@ main(int argc, char *argv[]) choice = selected_choice(); tty_restore(1); if (choice != NULL) { - printf("%s\n", choice->string); + printf("%s\n", choice_string(choice)); if (output_description) - printf("%s\n", choice->description); + printf("%s\n", choice_description(choice)); } else { rc = 1; } - free(input); + if (choices.length > 0) + free(*choices.v[0].string); free(choices.v); free(query); @@ -220,67 +223,72 @@ usage(int status) exit(status); } -char * -get_choices(void) +size_t +get_choices(int fd) { - char *buf, *description, *ifs, *start, *stop; - ssize_t n; - size_t length = 0; - size_t size = BUFSIZ; - - if ((ifs = getenv("IFS")) == NULL || *ifs == '\0') + static const char *ifs; + static char *buf; + static size_t length, offset; + static size_t size = BUFSIZ; + char *desc, *start, *stop; + ssize_t n; + + if (ifs == NULL && ((ifs = getenv("IFS")) == NULL || *ifs == '\0')) ifs = " "; - if ((buf = malloc(size)) == NULL) - err(1, NULL); - for (;;) { - if ((n = read(STDIN_FILENO, buf + length, size - length)) == -1) - err(1, "read"); - else if (n == 0) - break; - - length += n; - if (length + 1 < size) - continue; - if ((buf = reallocarray(buf, 2, size)) == NULL) + if (buf == NULL || length + 1 == size) { + buf = reallocarray(buf, 2, size); + if (buf == NULL) err(1, NULL); size *= 2; } + + n = read(fd, buf + length, size - length - 1); + if (n == -1) + err(1, "read"); + length += n; buf[length] = '\0'; - choices.size = 16; - if ((choices.v = reallocarray(NULL, choices.size, - sizeof(struct choice))) == NULL) - err(1, NULL); + if (choices.v == NULL) { + choices.v = reallocarray(NULL, 16, sizeof(struct choice)); + if (choices.v == NULL) + err(1, NULL); + choices.size = 16; + } - start = buf; + start = buf + offset; while ((stop = strchr(start, '\n')) != NULL) { *stop = '\0'; - if (descriptions && (description = eager_strpbrk(start, ifs))) - *description++ = '\0'; - else - description = ""; + /* Ensure room for a extra choice when ALT_ENTER is invoked. */ + if (choices.length + 2 >= choices.size) { + choices.v = reallocarray(choices.v, 2 * choices.size, + sizeof(struct choice)); + if (choices.v == NULL) + err(1, NULL); + choices.size *= 2; + } + choices.v[choices.length].string = &buf; + choices.v[choices.length].offset = start - buf; choices.v[choices.length].length = stop - start; - choices.v[choices.length].string = start; - choices.v[choices.length].description = description; choices.v[choices.length].match_start = -1; choices.v[choices.length].match_end = -1; choices.v[choices.length].score = 0; + if (descriptions && (desc = eager_strpbrk(start, ifs))) { + *desc++ = '\0'; + choices.v[choices.length].description = desc - buf; + } else { + choices.v[choices.length].description = -1; + } - start = stop + 1; + choices.length++; - /* Ensure room for a extra choice when ALT_ENTER is invoked. */ - if (++choices.length + 1 < choices.size) - continue; - choices.size *= 2; - if ((choices.v = reallocarray(choices.v, choices.size, - sizeof(struct choice))) == NULL) - err(1, NULL); + start = stop + 1; } + offset = start - buf; - return buf; + return n; } char * @@ -300,59 +308,62 @@ eager_strpbrk(const char *string, const char *separators) const struct choice * selected_choice(void) { + struct pollfd fds[2]; const char *buf; - size_t cursor_position, i, j, length, xscroll; + size_t cursor_position, i, j, length, nfds, xscroll; size_t choices_count = 0; size_t selection = 0; size_t yscroll = 0; + int dokey, doread, timo; int dochoices = 0; int dofilter = 1; int query_grew = 0; cursor_position = query_length; + fds[0].fd = fileno(tty_in); + fds[0].events = POLLIN; + fds[1].fd = STDIN_FILENO; + fds[1].events = POLLIN; + nfds = 2; + /* No timeout on first call to poll in order to render the UI fast. */ + timo = 0; for (;;) { - /* - * If the user didn't add more characters to the query all - * choices have to be reconsidered as potential matches. - * In the opposite scenario, there's no point in reconsidered - * all choices again since the ones that didn't match the - * previous query will clearly not match the current one due to - * the fact that previous query is a left-most substring of the - * current one. - */ - if (!query_grew) - choices_count = choices.length; - query_grew = 0; - if (dofilter) { - if ((dochoices = filter_choices(choices_count))) - dofilter = selection = yscroll = 0; + dokey = doread = 0; + toggle_sigwinch(1); + if (poll(fds, nfds, timo) == -1 && errno != EINTR) + err(1, "poll"); + toggle_sigwinch(0); + if (gotsigwinch) { + gotsigwinch = 0; + goto resize; } + timo = -1; + for (i = 0; i < nfds; i++) { + if (fds[i].revents & (POLLERR | POLLNVAL)) + errx(1, "%d: bad file descriptor", fds[i].fd); + if ((fds[i].revents & (POLLIN | POLLHUP)) == 0) + continue; - tty_putp(cursor_invisible, 0); - tty_putp(carriage_return, 1); /* move cursor to first column */ - if (cursor_position >= tty_columns) - xscroll = cursor_position - tty_columns + 1; - else - xscroll = 0; - print_line(&query[xscroll], query_length - xscroll, 0, -1, -1); - if (dochoices) { - if (selection - yscroll >= choices_lines) - yscroll = selection - choices_lines + 1; - choices_count = print_choices(yscroll, selection); + if (fds[i].fd == fds[0].fd) + dokey = 1; + else if (fds[i].fd == fds[1].fd) + doread = 1; } - tty_putp(carriage_return, 1); /* move cursor to first column */ - for (i = j = 0; i < cursor_position; j++) - while (isu8cont(query[++i])) - continue; - if (j > 0) - /* - * parm_right_cursor interprets 0 as 1, therefore only - * move the cursor if the position is non zero. - */ - tty_putp(tty_parm1(parm_right_cursor, j), 1); - tty_putp(cursor_normal, 0); - fflush(tty_out); + + length = choices.length; + if (doread) { + if (get_choices(STDIN_FILENO)) { + if (query_length > 0) + dofilter = 1; + } else { + nfds = 1; /* EOF */ + } + } + if (!dokey && query_length == 0 && length >= choices_lines) + continue; /* prevent redundant rendering */ + if (!dokey) + goto render; switch (get_key(&buf)) { case ENTER: @@ -360,8 +371,9 @@ selected_choice(void) return &choices.v[selection]; break; case ALT_ENTER: - choices.v[choices.length].string = query; - choices.v[choices.length].description = ""; + choices.v[choices.length].string = &query; + choices.v[choices.length].offset = 0; + choices.v[choices.length].description = -1; return &choices.v[choices.length]; case CTRL_C: return NULL; @@ -408,6 +420,7 @@ selected_choice(void) dofilter = 1; break; case CTRL_L: + resize: tty_size(); break; case CTRL_O: @@ -508,6 +521,49 @@ selected_choice(void) case UNKNOWN: break; } + +render: + /* + * If the user didn't add more characters to the query all + * choices have to be reconsidered as potential matches. + * In the opposite scenario, there's no point in reconsidered + * all choices again since the ones that didn't match the + * previous query will clearly not match the current one due to + * the fact that previous query is a left-most substring of the + * current one. + */ + if (!query_grew) + choices_count = choices.length; + query_grew = 0; + if (dofilter) { + if ((dochoices = filter_choices(choices_count))) + dofilter = selection = yscroll = 0; + } + + tty_putp(cursor_invisible, 0); + tty_putp(carriage_return, 1); /* move cursor to first column */ + if (cursor_position >= tty_columns) + xscroll = cursor_position - tty_columns + 1; + else + xscroll = 0; + print_line(&query[xscroll], query_length - xscroll, 0, -1, -1); + if (dochoices) { + if (selection - yscroll >= choices_lines) + yscroll = selection - choices_lines + 1; + choices_count = print_choices(yscroll, selection); + } + tty_putp(carriage_return, 1); /* move cursor to first column */ + for (i = j = 0; i < cursor_position; j++) + while (isu8cont(query[++i])) + continue; + if (j > 0) + /* + * parm_right_cursor interprets 0 as 1, therefore only + * move the cursor if the position is non zero. + */ + tty_putp(tty_parm1(parm_right_cursor, j), 1); + tty_putp(cursor_normal, 0); + fflush(tty_out); } } @@ -527,7 +583,7 @@ filter_choices(size_t nchoices) for (i = 0; i < nchoices; i++) { c = &choices.v[i]; - if (min_match(c->string, 0, + if (min_match(choice_string(c), 0, &c->match_start, &c->match_end) == INT_MAX) { c->match_start = c->match_end = -1; c->score = 0; @@ -565,18 +621,31 @@ choicecmp(const void *p1, const void *p2) return -1; /* * The two choices have an equal score. - * Sort based on the address of string since it reflects the initial - * input order. + * Sort based on the offset since it reflects the initial input order. * The comparison is inverted since the choice with the lowest address * must come first. */ - if (c1->string < c2->string) + if (c1->offset < c2->offset) return -1; - if (c1->string > c2->string) + if (c1->offset > c2->offset) return 1; return 0; } +const char * +choice_description(const struct choice *c) +{ + if (c->description == (size_t)-1) + return ""; + return *c->string + c->description; +} + +const char * +choice_string(const struct choice *c) +{ + return *c->string + c->offset; +} + size_t min_match(const char *string, size_t offset, ssize_t *start, ssize_t *end) { @@ -866,7 +935,7 @@ print_choices(size_t offset, size_t selection) break; if (i - offset < choices_lines) - print_line(choice->string, choice->length, + print_line(choice_string(choice), choice->length, i == selection, choice->match_start, choice->match_end); } @@ -959,18 +1028,7 @@ get_key(const char **key) *key = (const char *)buf; len = 0; - /* - * Allow SIGWINCH on the first read. If the signal is received, return - * CTRL_L which will trigger a resize. - */ - toggle_sigwinch(1); buf[len++] = tty_getc(); - toggle_sigwinch(0); - if (gotsigwinch) { - gotsigwinch = 0; - return CTRL_L; - } - for (;;) { for (i = 0; keys[i].key != UNKNOWN; i++) { if (keys[i].tio >= 0) { @@ -1042,11 +1100,14 @@ get_key(const char **key) int tty_getc(void) { - int c; - - if ((c = getc(tty_in)) == ERR && !gotsigwinch) - err(1, "getc"); - + ssize_t n; + unsigned char c; + + n = read(fileno(tty_in), &c, sizeof(c)); + if (n == -1) + err(1, "read"); + if (n == 0) + return EOF; return c; } From 6d4328a0b0e6551e7bd46273c61af1a142c355a5 Mon Sep 17 00:00:00 2001 From: Anton Lindqvist Date: Sun, 15 Apr 2018 16:16:49 +0200 Subject: [PATCH 2/6] Rename function for consistency --- pick.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pick.c b/pick.c index 55a7cfca..7f86a223 100644 --- a/pick.c +++ b/pick.c @@ -70,7 +70,7 @@ struct choice { double score; }; -static int choicecmp(const void *, const void *); +static int choice_cmp(const void *, const void *); static const char *choice_description(const struct choice *); static const char *choice_string(const struct choice *); static void delete_between(char *, size_t, size_t, size_t); @@ -603,13 +603,13 @@ filter_choices(size_t nchoices) return 0; } } - qsort(choices.v, nchoices, sizeof(struct choice), choicecmp); + qsort(choices.v, nchoices, sizeof(struct choice), choice_cmp); return 1; } int -choicecmp(const void *p1, const void *p2) +choice_cmp(const void *p1, const void *p2) { const struct choice *c1, *c2; From fd972a79e05d3db4ec862ba23aa566f0e0e3dc12 Mon Sep 17 00:00:00 2001 From: Anton Lindqvist Date: Sun, 15 Apr 2018 18:43:17 +0200 Subject: [PATCH 3/6] Insert new choices after previously matched ones If the user already has supplied a query that yields M (where M > 0) matches, inserting the K newly read choices after index M only requires searching through M + K choices instead of N (where N being all choices). This improves performance when the user types a query faster than the input pace on stdin. --- pick.c | 61 +++++++++++++++++++++++++++++++++++----------------------- 1 file changed, 37 insertions(+), 24 deletions(-) diff --git a/pick.c b/pick.c index 7f86a223..f76b8ee5 100644 --- a/pick.c +++ b/pick.c @@ -76,7 +76,7 @@ static const char *choice_string(const struct choice *); static void delete_between(char *, size_t, size_t, size_t); static char *eager_strpbrk(const char *, const char *); static int filter_choices(size_t); -static size_t get_choices(int); +static size_t get_choices(int, size_t); static enum key get_key(const char **); static void handle_sigwinch(int); static int isu8cont(unsigned char); @@ -224,14 +224,16 @@ usage(int status) } size_t -get_choices(int fd) +get_choices(int fd, size_t insert) { static const char *ifs; static char *buf; static size_t length, offset; static size_t size = BUFSIZ; + struct choice *new, *old, tmp; char *desc, *start, *stop; ssize_t n; + size_t dchoices, i, nchoices; if (ifs == NULL && ((ifs = getenv("IFS")) == NULL || *ifs == '\0')) ifs = " "; @@ -256,6 +258,7 @@ get_choices(int fd) choices.size = 16; } + nchoices = choices.length; start = buf + offset; while ((stop = strchr(start, '\n')) != NULL) { *stop = '\0'; @@ -287,6 +290,18 @@ get_choices(int fd) start = stop + 1; } offset = start - buf; + dchoices = choices.length - nchoices; + if (dchoices == 0 || nchoices == 0) + return n; + + /* Move new choices after the given insert index. */ + for (i = 0; i < dchoices; i++) { + old = choices.v + insert + i; + new = choices.v + nchoices + i; + tmp = *new; + *new = *old; + *old = tmp; + } return n; } @@ -315,9 +330,9 @@ selected_choice(void) size_t selection = 0; size_t yscroll = 0; int dokey, doread, timo; + int choices_reset = 1; int dochoices = 0; int dofilter = 1; - int query_grew = 0; cursor_position = query_length; @@ -353,9 +368,14 @@ selected_choice(void) length = choices.length; if (doread) { - if (get_choices(STDIN_FILENO)) { - if (query_length > 0) + if (get_choices(STDIN_FILENO, query_length > 0 ? + choices_count : 0)) { + if (query_length > 0) { dofilter = 1; + choices_reset = 0; + choices_count += choices.length - + length; + } } else { nfds = 1; /* EOF */ } @@ -392,7 +412,7 @@ selected_choice(void) cursor_position - length, cursor_position); cursor_position -= length; query_length -= length; - dofilter = 1; + dofilter = choices_reset = 1; } break; case DEL: @@ -404,20 +424,20 @@ selected_choice(void) delete_between(query, query_length, cursor_position, cursor_position + length); query_length -= length; - dofilter = 1; + dofilter = choices_reset = 1; } break; case CTRL_U: delete_between(query, query_length, 0, cursor_position); query_length -= cursor_position; cursor_position = 0; - dofilter = 1; + dofilter = choices_reset = 1; break; case CTRL_K: delete_between(query, query_length, cursor_position, query_length); query_length = cursor_position; - dofilter = 1; + dofilter = choices_reset = 1; break; case CTRL_L: resize: @@ -425,7 +445,7 @@ selected_choice(void) break; case CTRL_O: sort = !sort; - dofilter = 1; + dofilter = choices_reset = 1; break; case CTRL_W: if (cursor_position == 0) @@ -446,7 +466,7 @@ selected_choice(void) delete_between(query, query_length, i, cursor_position); query_length -= cursor_position - i; cursor_position = i; - dofilter = 1; + dofilter = choices_reset = 1; break; case CTRL_A: cursor_position = 0; @@ -498,8 +518,10 @@ selected_choice(void) yscroll = selection = 0; break; case PRINTABLE: - length = strlen(buf); + if (query_length == 0) + choices_reset = 1; + length = strlen(buf); if (query_length + length >= query_size) { query_size = 2*query_length + length; if ((query = reallocarray(query, query_size, @@ -516,25 +538,16 @@ selected_choice(void) cursor_position += length; query_length += length; query[query_length] = '\0'; - dofilter = query_grew = 1; + dofilter = 1; break; case UNKNOWN: break; } render: - /* - * If the user didn't add more characters to the query all - * choices have to be reconsidered as potential matches. - * In the opposite scenario, there's no point in reconsidered - * all choices again since the ones that didn't match the - * previous query will clearly not match the current one due to - * the fact that previous query is a left-most substring of the - * current one. - */ - if (!query_grew) + if (choices_reset) choices_count = choices.length; - query_grew = 0; + choices_reset = 0; if (dofilter) { if ((dochoices = filter_choices(choices_count))) dofilter = selection = yscroll = 0; From 902d0b4c5198ef6ee96ae783583479d3c750da72 Mon Sep 17 00:00:00 2001 From: Anton Lindqvist Date: Mon, 16 Apr 2018 18:19:44 +0200 Subject: [PATCH 4/6] macOS poll work-around Since polling of character devices is not supported on macOS, add the following work-around: - While stdin has not reached EOF, put the tty in non-blocking mode in order to simulate polling by peeking for new data. - Once stdin has reached EOF, just do a blocking read of the tty. --- Makefile.am | 6 ++- compat-xpoll.c | 121 ++++++++++++++++++++++++++++++++++++++++++++++ compat.h | 2 + config.h.in | 3 ++ configure.ac | 17 ++++++- pick.c | 44 ++++++++++------- tests/pick-test.c | 1 + 7 files changed, 175 insertions(+), 19 deletions(-) create mode 100644 compat-xpoll.c diff --git a/Makefile.am b/Makefile.am index 803d026a..f6ccb444 100644 --- a/Makefile.am +++ b/Makefile.am @@ -4,7 +4,11 @@ AM_CFLAGS=-Wall -Wextra AM_CPPFLAGS=-D_GNU_SOURCE bin_PROGRAMS=pick -pick_SOURCES=pick.c compat-reallocarray.c compat-strtonum.c compat.h +pick_SOURCES=compat-reallocarray.c \ + compat-strtonum.c \ + compat-xpoll.c \ + compat.h \ + pick.c pick_CPPFLAGS=$(AM_CPPFLAGS) $(NCURSES_CFLAGS) pick_LDADD=$(NCURSES_LIBS) diff --git a/compat-xpoll.c b/compat-xpoll.c new file mode 100644 index 00000000..19571e1e --- /dev/null +++ b/compat-xpoll.c @@ -0,0 +1,121 @@ +#ifdef HAVE_CONFIG_H +#include "config.h" +#endif + +#include +#include + +#include "compat.h" + +#ifdef HAVE_BROKEN_POLL + +#include +#include +#include +#include + +static int tty_peekc(int); + +int +xpoll(struct pollfd *fds, nfds_t nfds, int timeout) +{ + static int state; + int flags, inready, ttyready; + int nready = 0; + + /* XXX filter_choices() poll disabled */ + if (nfds == 1 && timeout == 0) + return 0; + + if (state == 0) { + assert(nfds == 2); + + flags = fcntl(fds[0].fd, F_GETFL, 0); + if (flags == -1) + return -1; + flags |= O_NONBLOCK; + if (fcntl(fds[0].fd, F_SETFL, flags) == -1) + return -1; + state++; + } + + if (state == 1) { + if (timeout < 0) + timeout = 100; + while (nready == 0) { + if (nfds == 1) { + state++; + break; + } + + inready = poll(&fds[1], 1, timeout); + if (inready == -1) + return -1; + nready += inready; + + ttyready = tty_peekc(fds[0].fd); + if (ttyready == -1) { + return -1; + } else if (ttyready > 0) { + fds[0].revents = POLLIN; + nready++; + } else { + fds[0].revents = 0; + } + } + } + + if (state == 2) { + flags = fcntl(fds[0].fd, F_GETFL, 0); + if (flags == -1) + return -1; + flags &= ~O_NONBLOCK; + if (fcntl(fds[0].fd, F_SETFL, flags) == -1) + return -1; + state++; + } + + if (state == 3) { + ttyready = tty_peekc(fds[0].fd); + if (ttyready == -1) { + return -1; + } else if (ttyready > 0) { + fds[0].revents = POLLIN; + nready++; + } else { + fds[0].revents = 0; + } + } + + return nready; +} + +static int +tty_peekc(int fd) +{ + extern int tty_getc_peek; + ssize_t n; + unsigned char c; + + n = read(fd, &c, sizeof(c)); + if (n == -1) { + if (errno == EAGAIN) + return 0; + return -1; + } + if (n > 0) { + assert(tty_getc_peek == -1); + tty_getc_peek = c; + } + return n; +} + +#else + +int +xpoll(struct pollfd *fds, nfds_t nfds, int timeout) +{ + return poll(fds, nfds, timeout); +} + +#endif /* HAVE_BROKEN_POLL */ diff --git a/compat.h b/compat.h index f8bbc0ce..1f706948 100644 --- a/compat.h +++ b/compat.h @@ -32,3 +32,5 @@ long long strtonum(const char *, long long, long long, const char **); #endif /* !HAVE_STRTONUM */ #endif /* COMPAT_H */ + +int xpoll(struct pollfd *fds, nfds_t nfds, int timeout); diff --git a/config.h.in b/config.h.in index 66b3dbd6..246da3fa 100644 --- a/config.h.in +++ b/config.h.in @@ -1,5 +1,8 @@ /* config.h.in. Generated from configure.ac by autoheader. */ +/* Define if poll does not support character devices */ +#undef HAVE_BROKEN_POLL + /* Define if ncursesw is available */ #undef HAVE_NCURSESW_H diff --git a/configure.ac b/configure.ac index dfa1fa12..84b292de 100644 --- a/configure.ac +++ b/configure.ac @@ -2,6 +2,7 @@ AC_PREREQ([2.61]) AC_INIT([pick], [2.0.2], [pick-maintainers@calleerlandsson.com]) AM_INIT_AUTOMAKE([subdir-objects]) AC_CONFIG_HEADERS([config.h]) +AC_CANONICAL_HOST AC_PROG_CC AM_PROG_CC_C_O AC_CHECK_FUNCS([pledge reallocarray strtonum]) @@ -16,7 +17,6 @@ AC_SEARCH_LIBS([setupterm], [curses], [], [ ) ]) AC_DEFUN([AC_MALLOC_OPTIONS], [ - AC_CANONICAL_HOST AC_MSG_CHECKING([for $host_os malloc hardening options]) case "$host_os" in openbsd*) malloc_options="RS";; @@ -30,5 +30,20 @@ AC_DEFUN([AC_MALLOC_OPTIONS], [ AC_SUBST([MALLOC_OPTIONS], [$malloc_options]) ]) AC_MALLOC_OPTIONS +AC_DEFUN([AC_BROKEN_POLL], [ + AC_MSG_CHECKING([for broken poll implementation]) + case "$host_os" in + darwin*) broken=1;; + *) broken=0;; + esac + if test "$broken" -eq 1; then + AC_MSG_RESULT([yes]) + AC_DEFINE([HAVE_BROKEN_POLL], [1], [ + Define if poll does not support character devices]) + else + AC_MSG_RESULT([no]) + fi +]) +AC_BROKEN_POLL AC_CONFIG_FILES([Makefile]) AC_OUTPUT diff --git a/pick.c b/pick.c index f76b8ee5..8adbc261 100644 --- a/pick.c +++ b/pick.c @@ -70,13 +70,15 @@ struct choice { double score; }; +int tty_getc_peek = -1; + static int choice_cmp(const void *, const void *); static const char *choice_description(const struct choice *); static const char *choice_string(const struct choice *); static void delete_between(char *, size_t, size_t, size_t); static char *eager_strpbrk(const char *, const char *); static int filter_choices(size_t); -static size_t get_choices(int, size_t); +static size_t get_choices(int, ssize_t); static enum key get_key(const char **); static void handle_sigwinch(int); static int isu8cont(unsigned char); @@ -224,7 +226,7 @@ usage(int status) } size_t -get_choices(int fd, size_t insert) +get_choices(int fd, ssize_t insert) { static const char *ifs; static char *buf; @@ -291,7 +293,7 @@ get_choices(int fd, size_t insert) } offset = start - buf; dchoices = choices.length - nchoices; - if (dchoices == 0 || nchoices == 0) + if (dchoices == 0 || nchoices == 0 || insert == -1) return n; /* Move new choices after the given insert index. */ @@ -325,11 +327,12 @@ selected_choice(void) { struct pollfd fds[2]; const char *buf; + ssize_t insert; size_t cursor_position, i, j, length, nfds, xscroll; size_t choices_count = 0; size_t selection = 0; size_t yscroll = 0; - int dokey, doread, timo; + int dokey, doread, nready, timo; int choices_reset = 1; int dochoices = 0; int dofilter = 1; @@ -346,7 +349,8 @@ selected_choice(void) for (;;) { dokey = doread = 0; toggle_sigwinch(1); - if (poll(fds, nfds, timo) == -1 && errno != EINTR) + nready = xpoll(fds, nfds, timo); + if (nready == -1 && errno != EINTR) err(1, "poll"); toggle_sigwinch(0); if (gotsigwinch) { @@ -355,21 +359,23 @@ selected_choice(void) } timo = -1; for (i = 0; i < nfds; i++) { - if (fds[i].revents & (POLLERR | POLLNVAL)) - errx(1, "%d: bad file descriptor", fds[i].fd); + if (fds[i].revents & (POLLERR | POLLNVAL)) { + errno = EBADF; + err(1, "poll"); + } if ((fds[i].revents & (POLLIN | POLLHUP)) == 0) continue; - if (fds[i].fd == fds[0].fd) - dokey = 1; - else if (fds[i].fd == fds[1].fd) + if (fds[i].fd == STDIN_FILENO) doread = 1; + else if (fds[i].fd == fileno(tty_in)) + dokey = 1; } length = choices.length; if (doread) { - if (get_choices(STDIN_FILENO, query_length > 0 ? - choices_count : 0)) { + insert = query_length > 0 ? (ssize_t)choices_count : -1; + if (get_choices(fds[1].fd, insert)) { if (query_length > 0) { dofilter = 1; choices_reset = 0; @@ -377,11 +383,9 @@ selected_choice(void) length; } } else { - nfds = 1; /* EOF */ + nfds--; /* EOF */ } } - if (!dokey && query_length == 0 && length >= choices_lines) - continue; /* prevent redundant rendering */ if (!dokey) goto render; @@ -589,8 +593,8 @@ selected_choice(void) int filter_choices(size_t nchoices) { - struct choice *c; struct pollfd pfd; + struct choice *c; size_t i, match_length; int nready; @@ -610,7 +614,7 @@ filter_choices(size_t nchoices) if (i > 0 && i % 50 == 0) { pfd.fd = fileno(tty_in); pfd.events = POLLIN; - if ((nready = poll(&pfd, 1, 0)) == -1) + if ((nready = xpoll(&pfd, 1, 0)) == -1) err(1, "poll"); if (nready == 1 && pfd.revents & (POLLIN | POLLHUP)) return 0; @@ -1116,6 +1120,12 @@ tty_getc(void) ssize_t n; unsigned char c; + if (tty_getc_peek != -1) { + c = tty_getc_peek; + tty_getc_peek = -1; + return c; + } + n = read(fileno(tty_in), &c, sizeof(c)); if (n == -1) err(1, "read"); diff --git a/tests/pick-test.c b/tests/pick-test.c index ab7cea7d..c17ec96f 100644 --- a/tests/pick-test.c +++ b/tests/pick-test.c @@ -6,6 +6,7 @@ #include #include #include +#include #include #include #include From b38e545ac0f983a29cf688cd74b08769d51e42c5 Mon Sep 17 00:00:00 2001 From: Anton Lindqvist Date: Mon, 16 Apr 2018 22:20:03 +0200 Subject: [PATCH 5/6] Improve performance by limiting the search to newly read choices only --- pick.c | 27 ++++++++++++++++----------- 1 file changed, 16 insertions(+), 11 deletions(-) diff --git a/pick.c b/pick.c index 8adbc261..1391dd73 100644 --- a/pick.c +++ b/pick.c @@ -77,7 +77,7 @@ static const char *choice_description(const struct choice *); static const char *choice_string(const struct choice *); static void delete_between(char *, size_t, size_t, size_t); static char *eager_strpbrk(const char *, const char *); -static int filter_choices(size_t); +static int filter_choices(size_t, size_t); static size_t get_choices(int, ssize_t); static enum key get_key(const char **); static void handle_sigwinch(int); @@ -328,7 +328,8 @@ selected_choice(void) struct pollfd fds[2]; const char *buf; ssize_t insert; - size_t cursor_position, i, j, length, nfds, xscroll; + size_t choices_offset, cursor_position, i, j, length, nfds, + xscroll; size_t choices_count = 0; size_t selection = 0; size_t yscroll = 0; @@ -347,7 +348,7 @@ selected_choice(void) /* No timeout on first call to poll in order to render the UI fast. */ timo = 0; for (;;) { - dokey = doread = 0; + choices_offset = dokey = doread = 0; toggle_sigwinch(1); nready = xpoll(fds, nfds, timo); if (nready == -1 && errno != EINTR) @@ -379,8 +380,9 @@ selected_choice(void) if (query_length > 0) { dofilter = 1; choices_reset = 0; - choices_count += choices.length - - length; + choices_offset = choices_count; + choices_count += + choices.length - length; } } else { nfds--; /* EOF */ @@ -553,7 +555,9 @@ selected_choice(void) choices_count = choices.length; choices_reset = 0; if (dofilter) { - if ((dochoices = filter_choices(choices_count))) + dochoices = filter_choices(choices_offset, + choices_count); + if (dochoices) dofilter = selection = yscroll = 0; } @@ -585,20 +589,21 @@ selected_choice(void) } /* - * Filter the first nchoices number of choices using the current query and - * regularly check for new user input in order to abort filtering. This - * improves the performance when the cardinality of the choices is large. + * Filter nchoices - offset number of choices starting at offset using the + * current query. + * In addition, regularly check for new user input and abort filtering since any + * previous matches could be invalidated by the new query. * Returns non-zero if the filtering was not aborted. */ int -filter_choices(size_t nchoices) +filter_choices(size_t offset, size_t nchoices) { struct pollfd pfd; struct choice *c; size_t i, match_length; int nready; - for (i = 0; i < nchoices; i++) { + for (i = offset; i < nchoices; i++) { c = &choices.v[i]; if (min_match(choice_string(c), 0, &c->match_start, &c->match_end) == INT_MAX) { From 95ddf40c75b47ec74f186d8e59503b0a7a7ad42b Mon Sep 17 00:00:00 2001 From: Anton Lindqvist Date: Sun, 22 Apr 2018 18:09:13 +0200 Subject: [PATCH 6/6] Do not reset the selection on arrival of new choices ... assuming it's still in bounds. --- pick.c | 48 ++++++++++++++++++++++--------------------- tests/key-printable.t | 14 ++++++++++++- 2 files changed, 38 insertions(+), 24 deletions(-) diff --git a/pick.c b/pick.c index 1391dd73..db4f407d 100644 --- a/pick.c +++ b/pick.c @@ -77,7 +77,7 @@ static const char *choice_description(const struct choice *); static const char *choice_string(const struct choice *); static void delete_between(char *, size_t, size_t, size_t); static char *eager_strpbrk(const char *, const char *); -static int filter_choices(size_t, size_t); +static int filter_choices(size_t, size_t *); static size_t get_choices(int, ssize_t); static enum key get_key(const char **); static void handle_sigwinch(int); @@ -86,7 +86,7 @@ static int isu8start(unsigned char); static int isword(const char *); static size_t min_match(const char *, size_t, ssize_t *, ssize_t *); -static size_t print_choices(size_t, size_t); +static void print_choices(size_t, size_t, size_t); static void print_line(const char *, size_t, int, ssize_t, ssize_t); static const struct choice *selected_choice(void); @@ -383,6 +383,8 @@ selected_choice(void) choices_offset = choices_count; choices_count += choices.length - length; + } else { + choices_reset = 1; } } else { nfds--; /* EOF */ @@ -556,9 +558,9 @@ selected_choice(void) choices_reset = 0; if (dofilter) { dochoices = filter_choices(choices_offset, - choices_count); + &choices_count); if (dochoices) - dofilter = selection = yscroll = 0; + dofilter = 0; } tty_putp(cursor_invisible, 0); @@ -569,9 +571,11 @@ selected_choice(void) xscroll = 0; print_line(&query[xscroll], query_length - xscroll, 0, -1, -1); if (dochoices) { + if (selection >= choices_count) + selection = yscroll = 0; if (selection - yscroll >= choices_lines) yscroll = selection - choices_lines + 1; - choices_count = print_choices(yscroll, selection); + print_choices(yscroll, choices_count, selection); } tty_putp(carriage_return, 1); /* move cursor to first column */ for (i = j = 0; i < cursor_position; j++) @@ -596,14 +600,15 @@ selected_choice(void) * Returns non-zero if the filtering was not aborted. */ int -filter_choices(size_t offset, size_t nchoices) +filter_choices(size_t offset, size_t *nchoices) { struct pollfd pfd; struct choice *c; size_t i, match_length; + size_t n = 0; int nready; - for (i = offset; i < nchoices; i++) { + for (i = offset; i < *nchoices; i++) { c = &choices.v[i]; if (min_match(choice_string(c), 0, &c->match_start, &c->match_end) == INT_MAX) { @@ -615,6 +620,8 @@ filter_choices(size_t offset, size_t nchoices) match_length = c->match_end - c->match_start; c->score = (double)query_length/match_length/c->length; } + if (c->score > 0 || query_length == 0) + n++; if (i > 0 && i % 50 == 0) { pfd.fd = fileno(tty_in); @@ -625,7 +632,8 @@ filter_choices(size_t offset, size_t nchoices) return 0; } } - qsort(choices.v, nchoices, sizeof(struct choice), choice_cmp); + qsort(choices.v, *nchoices, sizeof(struct choice), choice_cmp); + *nchoices = offset + n; return 1; } @@ -941,25 +949,21 @@ print_line(const char *str, size_t len, int standout, } /* - * Output as many choices as possible starting from offset and return the number - * of choices with a positive score. If the query is empty, all choices are - * considered having a positive score. + * Print length - offset number of choices. */ -size_t -print_choices(size_t offset, size_t selection) +void +print_choices(size_t offset, size_t length, size_t selection) { - const struct choice *choice; + const struct choice *c; size_t i; - for (i = offset; i < choices.length; i++) { - choice = choices.v + i; - if (choice->score == 0 && query_length > 0) + for (i = offset; i < length; i++) { + if (i - offset >= choices_lines) break; - if (i - offset < choices_lines) - print_line(choice_string(choice), choice->length, - i == selection, choice->match_start, - choice->match_end); + c = choices.v + i; + print_line(choice_string(c), c->length, i == selection, + c->match_start, c->match_end); } if (i - offset < choices.length && i - offset < choices_lines) { @@ -985,8 +989,6 @@ print_choices(size_t offset, size_t selection) tty_putp(tty_parm1(parm_up_cursor, i < choices_lines ? i : choices_lines), 1); } - - return i; } enum key diff --git a/tests/key-printable.t b/tests/key-printable.t index b265396b..54e2751b 100644 --- a/tests/key-printable.t +++ b/tests/key-printable.t @@ -33,7 +33,7 @@ stdin: stdout: 💩 -description: changing the query resets vertical scroll +description: changing the query does not reset the selection if it is still in bounds keys: \016 \016 \016 \016 \016 0 \n #DOWN ENTER env: LINES=5 stdin: @@ -43,4 +43,16 @@ stdin: 04 05 stdout: +05 + +description: changing the query does reset the selection if it is out of bounds +keys: \016 \016 \016 \016 \016 1 \n #DOWN ENTER +env: LINES=5 +stdin: +01 +02 +03 +04 +05 +stdout: 01