Redis Add support for reverse history search in redis-cli

The problem/use-case that the feature addresses

As a frequent user of redis-cli, I really need reverse history search.

Note: I've created a proof of concept for this feature.

Description of the feature

Pressing Ctrl-r in redis-cli should start a reverse history search mode that behaves similarly to readline-style reverse history search. (As a comparison, see psql, etc.)

Alternatives you've considered

Using readline is of course an option, but redis intentionally does not use readline. Instead, we would need to add a reverse history search feature to the linenoise library, which Redis uses for line editing in redis-cli.

Additional information

I've created a proof of concept for this feature in linenoise.

Comment From: oranagra

@abrookins this sounds like a good idea. can you make a PR to this repository and push this to completion? (this repo is no longer following antirez/linenoise)

Comment From: abrookins

You bet! Thanks for looking.

Comment From: oranagra

@abrookins any news? would love a PR for this repository with improvements to redis-cli. p.s. other than reverse history, i'm also missing Alt+Backspace, Ctrl+Left and such.

Comment From: abrookins

I should have mentioned - while reviewing my code to prepare a PR in this repo rather than linenoise, I realized I had missed a code path. There are two editing modes in linenoise: single-line and multi-line. I only added support for single-line.

I still long for these features myself... every day. :) But I've been slammed with other work all year. Realistically, I might not get back to this for a few more months. If no one else does by then, I still intend to finish the job.

Comment From: ClaytonNorthey92

hey @abrookins @oranagra I like this idea and I wanted to pick it up. It seems that the maintainers of linenoise are not very responsive when users create issues/prs 🤔

@abrookins you mentioned

...while reviewing my code to prepare a PR in this repo rather than linenoise...

I assume that means you had to keep track of history outside of linenoise? i.e. you had to allocate memory in redis to store the history to make it searchable? That is the only way I can think of how to do this without changing linenoise as they keep the history as a static variable in linenoise.c

Comment From: oranagra

The other repo is not actively maintained. We host our own version nowadays. Just edit the files in the deps folder.

Comment From: ClaytonNorthey92

@oranagra I have been working on this issue the past week, how do you feel about this UX? When in reverse-i-search mode, I highlight in bold the keys that match the user's input (search term). Then, if they press "enter", used the search result.

Comment From: oranagra

if i understand correctly, that's the same as bash does, so in that case we can't get it wrong (will do what most people are expecting). am i missing something?

Comment From: ClaytonNorthey92

it's similar, this is what I get if I try it on theubuntu:latest docker image:

it looks like they put the search term to the left of the colon, then the colon, then highlight the search term in the result. I am not sure I like this interface, as it feels a little busy.

Comment From: oranagra

i tend to agree, although i'm sure they have / had their reasons. i suppose we can deviate in that respect and it won't cause much confusion.

Comment From: ClaytonNorthey92

@oranagra cool, thanks for the feedback. I should have a PR in either Saturday or Sunday.

Comment From: ClaytonNorthey92

hey @oranagra - I am writing test cases now and I admit I am a little unsure how to proceed...

I was thinking about originally doing something similar to how testing hints works, since the two features behave similarly (ultimately, redis-cli gives a suggestion based on user input). But I think having a .txt file with its own format is overkill.

I was thinking about, instead of a .txt file, a .c and .h file with a set of configured test cases, something like...

typedef struct {
    char * input;
    char * expectedOutput;
    char history [n][m];
} TestCliReverseISearchTestCase;


TestCliReverseISearchTestCase [] testCases = {
   ...
}

Then in redis-cli, I can accept a flag --test_reverse_i_search, similar to --test_hint_file, but --test_reverse_i_search runs through the aforementioned test cases.

What do you think?

Comment From: oranagra

I think that it that test contains a ton of cases (like the hints system), then i'd rather have the list of cases (which we may extend over time) be in the tests folder rather than coded in the source code.

if we do code the tests (not just the testing infrastructure) into the source code, we better use REDIS_TEST, and run them together with the other unit tests (see .github/workflows/daily.yml).

however, it looks to me that in this case we don't need a ton of test cases (per odd command syntax), just a few tests that validate the mechanism works correctly (including the user hitting backspace or arrows). i suppose it would be best to test it externally (by actually executing redis-cli and passing key strokes to stdin). maybe @zuiderkwast or @yossigo have some advise or a reference to a similar test you can look at.

Comment From: ClaytonNorthey92

hey @oranagra : I added a PR for what I think will add this feature. I wrote in the PR how it is expected to behave

I only implemented this for multi-line mode, as it seems that we seem to force it when calling repl ... let me know if I am misunderstanding

also, let me know what you think and/or if you want me to fix/change anything, thanks for your help!