[PATCH] Readline's mimic for reverse history search

[Denys Vlasenko]

On Thursday 30 June 2011 08:35, kyak wrote:
> Implemented readline's mimic for reverse history search (Ctrl+r)
> 
> Signed-off-by: kyak <bas at bmail.ru>
> ---
>   libbb/Config.src |    7 ++
>   libbb/lineedit.c |  193 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 200 insertions(+), 0 deletions(-)
> 
> +#if ENABLE_FEATURE_REVERSE_SEARCH
> +/* mimic readline's Ctrl+r behaviour for reverse history search */

Please shortly describe the implemented behavior in this comment.

> +static smallint reverse_i_search(void)
> +{
> +	/* prepare the prompt */
> +	const char prmt[] = "(reverse-i-search)`";

It should be static.

> +	int prmt_len = strlen(prmt);

Use sizeof(prmt)-1, it has no runtime cost.

> +	char *prmt_mem_ptr = xzalloc(1);
> +	/* save the real prompt */
> +	char *prmt_mem_ptr_save = xzalloc(1);
> +	/* user input is collected here */
> +	char *match_buf;
> +	/* matched lines from history are here */
> +	char *cmdline_buf;
> +	char read_key_buffer[KEYCODE_BUFFER_SIZE];
> +	int ic;
> +	char buf[MB_CUR_MAX + 1];
> +	wchar_t wc[MAX_LINELEN];
> +	ssize_t len, len_cmd;
> +	mbstate_t mbst = { 0 };
> +	smallint break_out = 0;
> +	smallint break_out_search = 0;
> +	smallint has_match = 0;
> +	int i;
> +	int cur = state->cur_history, cur_match = state->cur_history;
> +	int cmdedit_y_add = 0, cmdedit_y_add_prev = 0, cmdedit_y_add_cmp = 0;

In complex cases like this, I prefer to have assignments not intermixed
with declarations.

> +	read_key_buffer[0] = 0;
> +	prmt_mem_ptr = strcat(xrealloc(prmt_mem_ptr, prmt_len+1), prmt);

Basically you do this:

prmt_len = strlen(prmt);
prmt_mem_ptr = xzalloc(1);
prmt_mem_ptr = strcat(xrealloc(prmt_mem_ptr, prmt_len+1), prmt);

This looks very suboptimal, should be: prmt_mem_ptr = xstrdup(prmt);

> +	/* save prompt */
> +	prmt_mem_ptr_save = strcat(xrealloc(prmt_mem_ptr_save, cmdedit_prmt_len+1), cmdedit_prompt);
> +	/* overwrite the prompt */
> +	cmdedit_prompt = prmt_mem_ptr;
> +	cmdedit_prmt_len = prmt_len;

> +	/* save what's already typed */
> +	match_buf = xmalloc(MAX_LINELEN * sizeof(int16_t));
> +	cmdline_buf = xmalloc(MAX_LINELEN * sizeof(int16_t));
> +	save_string(match_buf, MAX_LINELEN);

Why do you think MAX_LINELEN * sizeof(int16_t) will be enough?

> +	command_len = load_string("", MAX_LINELEN);
> +	redraw(cmdedit_y, 0);
> +	fputs(match_buf, stdout);
> +	fputs("': ", stdout);
> +	fputs(match_buf, stdout);
> +	cmdedit_y_add_cmp = (prmt_len+1 + 2 * strlen(match_buf)) / (cmdedit_termw);
> +	cmdedit_y_add_prev = cmdedit_y_add_cmp;
> +	/* switch to char-consumption mode... */
> +	while (1) {
> +		fflush_all();
> +		ic = lineedit_read_key(read_key_buffer, -1);
> +		switch (ic) {
> +			case KEYCODE_RIGHT: /* left-right keys act differently from Enter */
> +			case KEYCODE_LEFT:
> +				if (has_match == 1) {
> +					command_len = load_string(cmdline_buf, MAX_LINELEN);
> +				} else {
> +					command_len = load_string(match_buf, MAX_LINELEN);
> +				}
> +				cmdedit_prompt = prmt_mem_ptr_save;
> +				cmdedit_prmt_len = strlen(cmdedit_prompt);
> +				redraw(cmdedit_y + cmdedit_y_add, 0);
> +				break_out = 0;
> +				break;
> +			case CTRL('R'): /* searching for the next match */
> +				break_out = 2;
> +				break;
> +			case CTRL('C'):
> +			case KEYCODE_UP:
> +			case KEYCODE_DOWN:
> +			case KEYCODE_HOME:
> +			case KEYCODE_END:
> +			case KEYCODE_DELETE:
> +			case KEYCODE_CTRL_RIGHT:
> +			case KEYCODE_CTRL_LEFT:
> +			case '\x1b': /* ESC */
> +				command_len = load_string("", MAX_LINELEN);
> +				cmdedit_prompt = prmt_mem_ptr_save;
> +				redraw(cmdedit_y + cmdedit_y_add, 0);
> +				break_out = 0;
> +				break;
> +			case '\b':   /* ^H */
> +			case '\x7f': /* DEL */
> +				/* convert to wide char string,
> +				 * delete char, then convert back */
> +				len = mbstowcs(wc, match_buf, sizeof(wc));

What will happen if CONFIG_UNICODE_SUPPORT is off?

> +				if (len < 0)
> +					len = 0;
> +				wc[len-1] = '\0';
> +				wcstombs(match_buf, wc, MAX_LINELEN);
> +				break_out = 2;
> +				has_match = 0;
> +				break;
> +			case '\r':
> +			case '\n': /* Enter */
> +				if (has_match == 1) {
> +					command_len = load_string(cmdline_buf, MAX_LINELEN);
> +				} else {
> +					command_len = load_string(match_buf, MAX_LINELEN);
> +				}
> +				cmdedit_prompt = prmt_mem_ptr_save;
> +				redraw(cmdedit_y + cmdedit_y_add, 0);
> +				goto_new_line();
> +				break_out = 1;
> +				break;
> +			default: /* process the char */
> +				len = wcrtomb(buf, ic, &mbst);
> +				if (len > 0) {
> +					buf[len] = '\0';
> +				}
> +				strcat(match_buf, buf);

Is this overflow-safe?

> +				break_out = 2;
> +				break;
> +		}
> +		if (break_out != 2)
> +			break;
> +		/* if there is something in type buffer,
> +		 * do iterative search in history */
> +		if (strlen(match_buf) > 0) {

Should be: match_buf[0] != '\0'

> +			/* save current position in history */
> +			cur = state->cur_history;
> +			if (ic != CTRL('R')) {
> +				cur_match = state->cur_history;
> +			} else {
> +				/* if we hit Ctrl+r (again),
> +				 * start searching from the last matched position */
> +				state->cur_history = cur_match;
> +			}
> +			while(get_previous_history()) {
> +				cmdline_buf = state->history[state->cur_history] ?
> +					state->history[state->cur_history] : '\0';

cmdline_buf is a pointer. Assigning '\0' to a pointer is strange.

> +				has_match = 0;
> +				break_out_search = 0;

At the minimum, break_out_search should be local to while(get_previous_history()) block.
But better if you do without it (I think you can do it using goto).

> +				for(i=0;i+strlen(match_buf)<=strlen(cmdline_buf);i++) {
> +					if (strncmp(match_buf, cmdline_buf+i, strlen(match_buf)) == 0) {

Can you cut down on repetitive strlen's?

> +						has_match = 1;
> +						break_out_search = 1;
> +						/* save position of current match */
> +						cur_match = state->cur_history;
> +						break;
> +					}
> +				}
> +				if (break_out_search == 1)
> +					break;
> +			}
> +		}
> +		/* TODO: this could be done better. There are still some (rare)
> +		 * cases of wrapping miscalculation */
> +		len = mbstowcs(wc, match_buf, sizeof(wc));
> +		if (len < 0)
> +			len = strlen(match_buf);
> +
> +		len_cmd = mbstowcs(wc, cmdline_buf, sizeof(wc));
> +		if (len_cmd < 0)
> +			len_cmd = strlen(cmdline_buf);
> +
> +		cmdedit_y_add = (prmt_len+1 + len + (has_match ? len_cmd : len)) /
> +			(cmdedit_termw);
> +
> +		if (cmdedit_y_add > cmdedit_y_add_cmp) {
> +			for (int j = 1; j <= (cmdedit_y_add-cmdedit_y_add_cmp); j++) {
> +				/* scroll up */
> +				printf(ESC"D");
> +			}
> +			if (cmdedit_y_add-cmdedit_y_add_cmp > 1)
> +				redraw(cmdedit_y + cmdedit_y_add - cmdedit_y_add_cmp, 0);

Maybe using a temporary variable int t = cmdedit_y_add - cmdedit_y_add_cmp
will simplify code above?

-- 
vda