Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix SSH reverse search with CTRL+R #7499

Merged
merged 3 commits into from
Sep 28, 2023
Merged

Conversation

jjcarstens
Copy link
Contributor

group.erl now sends put_expand_no_trim when entering a history search. ssh_cli does not support that message and so when entering a search when SSH is the underlying driver, the expanded result would not be displayed even though the search functionality would still technically work.

This adds support for that put_expand_no_trim message when using SSH

I look through the tests and not entirely sure where I would put this. I'm happy to write that if there is any direction of how others might want it tested. In the meantime, this is my quick manual setup to test it works:

Start the Daemon

%% Create System host key
Priv = public_key:generate_key({rsa, 2048, 65537}).
PemEntry = public_key:pem_entry_encode(:"RSAPrivateKey", Priv).
Pem = public_key:pem_encode([PemEntry]).
ok = file:mkdir_p("/tmp/ssh"),
ok = file:write("/tmp/ssh/ssh_host_rsa_key", Pem).

%% Start SSH Daemon
Opts = [{system_dir, "/tmp/ssh"}, {pwdfun, fun (_, _, _, _) -> true end}],
application:ensure_all_started(ssh),
ssh:daemon(2222, Opts).

SSH (no password)

ssh localhost -p 2222
# CTRL+r to search history

`group.erl` now sends `put_expand_no_trim` when entering a history search.
`ssh_cli` does not support that message and so when entering a search when
SSH is the underlying driver, the expanded result would not be displayed
even though the search functionality would still technically work.

This adds support for that `put_expand_no_trim` message when using SSH
@github-actions
Copy link
Contributor

github-actions bot commented Jul 15, 2023

CT Test Results

       2 files       29 suites   57m 36s ⏱️
   453 tests    447 ✔️   5 💤 1
1 667 runs  1 639 ✔️ 27 💤 1

For more details on these failures, see this check.

Results for commit c205174.

♻️ This comment has been updated with latest results.

To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass.

See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally.

Artifacts

// Erlang/OTP Github Action Bot

@u3s u3s added the team:PS Assigned to OTP team PS label Jul 17, 2023
@IngelaAndin
Copy link
Contributor

Thanks as it is vacation time it may take a little longer, but we will handle it.

@jjcarstens jjcarstens changed the title Fix SSH recursive search with CTRL+R Fix SSH reverse search with CTRL+R Aug 1, 2023
@u3s u3s self-assigned this Aug 3, 2023
@u3s u3s added the testing currently being tested, tag is used by OTP internal CI label Aug 18, 2023
@u3s
Copy link
Contributor

u3s commented Aug 18, 2023

I look through the tests and not entirely sure where I would put this. I'm happy to write that if there is any direction of how others might want it tested.

Can you check if some simple test could be developed around ssh_basic_SUITE:do_shell code?

@IngelaAndin IngelaAndin added Planned Focus issue added in sprint planning waiting waiting for changes/input from author labels Aug 25, 2023
@u3s u3s removed the testing currently being tested, tag is used by OTP internal CI label Sep 11, 2023
@u3s
Copy link
Contributor

u3s commented Sep 19, 2023

I look through the tests and not entirely sure where I would put this. I'm happy to write that if there is any direction of how others might want it tested.

I will add a testcase to this PR. No need for you to spent time on it. Rough action plan:

  1. merge ssh: support dumb terminals in ssh client #7627 including a base for test
  2. rebase this PR
  3. add a commit with a testcase dedicated for this PR

@u3s u3s added the testing currently being tested, tag is used by OTP internal CI label Sep 20, 2023
@IngelaAndin IngelaAndin removed the waiting waiting for changes/input from author label Sep 20, 2023
@jjcarstens
Copy link
Contributor Author

Sorry! We seemed to have swapped vacation times and then I let this slip away from me until your recent comment 🤦🏼 Thanks for the help!

@u3s
Copy link
Contributor

u3s commented Sep 27, 2023

change of plans. we will merge both tests in this PR (removing the dependency towards #7627). I hope to merge your PR this week.

@u3s u3s added testing currently being tested, tag is used by OTP internal CI and removed testing currently being tested, tag is used by OTP internal CI labels Sep 27, 2023
@u3s u3s added testing currently being tested, tag is used by OTP internal CI and removed testing currently being tested, tag is used by OTP internal CI labels Sep 27, 2023
@u3s u3s added this to the OTP-26.2 milestone Sep 27, 2023
@u3s u3s merged commit 1d6a14a into erlang:maint Sep 28, 2023
13 of 15 checks passed
@jjcarstens jjcarstens deleted the ssh-search-fix branch October 4, 2023 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Planned Focus issue added in sprint planning team:PS Assigned to OTP team PS testing currently being tested, tag is used by OTP internal CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants