Allow conversion of std::process::Command into a openssh::Command given a openssh::Session.

[aalekhpatel07]

The std::process::Command and the openssh::Command APIs are fairly similar, so I wonder if it is reasonable to allow an arbitrary std::process::Command to be run over ssh.

Example

use std::process;
use openssh::{OverSsh, Session, KnownHosts};

#[tokio::main]
pub async fn main() {
    let session = Session::connect_mux("me@ssh.example.com", KnownHosts::Strict).await.unwrap();

    let ls = process::Command::new("ls")
        .arg("-l")
        .arg("-h")
        .over_ssh(&session)
        .expect("no env vars or current_dir are specified")
        .output()
        .await
        .unwrap();

    println!("{}", String::from_utf8_lossy(&ls.stdout));
}

[jonhoo]

This change is 

[NobodyXu]

Thanks!

I really like the design and the implementation is simple, but there are a few places where it can be improved.

[aalekhpatel07]

wow thank you for your kind words and a quick review. I'll clean it up right away! Thanks for your time.

[NobodyXu]

Ehhh we now have a rustc ICE.

@aalekhpatel07 Would you like to open an issue in rust-lang/rust?

[codecov-commenter]

Codecov Report

Merging #112 (f30d34e) into master (3319a81) will increase coverage by 0.32%.
The diff coverage is 87.50%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

Files Changed	Coverage Δ
src/error.rs	89.16% <ø> (ø)
src/command.rs	89.92% <69.23%> (-8.08%)	⬇️
src/escape.rs	100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

[aalekhpatel07]

Ehhh we now have a rustc ICE.

@aalekhpatel07 Would you like to open an issue in rust-lang/rust?

Haha for sure. Any preference for a name?

[NobodyXu]

@aalekhpatel07 Looks like some of the ICEs happens in mir borrow checking, so perhaps "mir borrow checking ICE in openssh-rust" will be a good name.

Also I recommend you to create a separate branch, containing the commit causing the ICEs just so it's easier for others to reproduce the ICEs.

[aalekhpatel07]

I believe the new escape(s: &[u8]) -> String should do the job for us. lmk

(I'm sorry I think I accidentally removed the review request for jonhoo).

[NobodyXu]

(I'm sorry I think I accidentally removed the review request for jonhoo).

No worries, I've re-requested it.

[NobodyXu]

Aha, the CI reliably failed with ICEs from rustc.

[aalekhpatel07]

okay how about these changes? (thank you for being patient.)

[NobodyXu]

okay how about these changes? (thank you for being patient.)

It's much better, but we would still need @jonhoo 's approval on this.
I'm not entirely sure about escaping since the utf encoding is a mess, so I just want to be safe and ask @jonhoo to also review this.

[NobodyXu]

@aalekhpatel07 BTW, don't forget to create a new issue on rust-lang/rust and linked it to this.
We would also have to waiting for the compiler guys to investigate and fix the issues.

Since we are on rust 1.67, not nightly channel, this might requires a new minor release.

I think this ICE might be a regression.

[NobodyXu]

@aalekhpatel07 Hmmm, let me create the bug report for this.

[NobodyXu]

@aalekhpatel07 I've created an ICE bug report on rust-lang/rust.

Note that the lint CI did work as intended and you need to fix all cargo-clippy warnings.

[NobodyXu]

Almost done from me, some small changes requested...

Regarding the ICEs, there's already a PR in rust-lang/rust to fix this.

[NobodyXu]

Now we just need to wait for @jonhoo to review this and upstream to fix the ICEs on stable channel.

[aalekhpatel07]

They have a PR for that ICE already??

Wow! I'm blown away by how responsive, knowledgeable, and supportive the contributors are in the Rust ecosystem.

Thank you for all your help, @NobodyXu, for helping me improve the PR and develop my understanding of Cows and OsStrs in Rust.

[NobodyXu]

They have a PR for that ICE already??

Yes, rust-lang/rust#107532

Though I have no idea on whether it will land in 1.68 or will they have a backport it to 1.67.1 since whether a backport will happen depends on the severity of the bug.

Thank you for all your help, @NobodyXu, for helping me improve the PR and develop my understanding of Cows and OsStrs in Rust.

No worries!

[jonhoo]

This is a really neat idea! Thank you both for pushing it forward. I've left some notes.

[jonhoo]

Can we submit a PR to shell-escape to add a version supporting OsStr there directly? I'm okay with us landing our own version initially, but it'd be way better if we could just use a version from them in the future.

[aalekhpatel07]

Great suggestion! I've initiated a PR to shell-escape and if it gets merged, we can avoid having a src/escape.rs altogether.

[NobodyXu]

Since the ICE is fixed and the CI has passed, I will merge this PR.