-
Notifications
You must be signed in to change notification settings - Fork 66
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
RUSTIC_PASSWORD_COMMAND and --password-command are not working properly #1177
Comments
Passing |
Hello @drdo, After looking a bit I found out that rustic-rs/rustic_core#211 came with a breaking-change about this. A non-intuitive workaround is to use several time the --password-command flag such as: rustic -r test-repo --password-command "echo" --password-command "password" snapshots However this:
I would suggest moving this issue to rustic_core as the code dealing with that is at https://github.com/rustic-rs/rustic_core/blob/1e7b4a8d49703b5016d153c7ba2a30f6d2b644b3/crates/core/src/repository.rs#L174 To me, we could image having both the behaviors as before and after rustic-rs/rustic_core#211 with such code: fn split_command(command: &Vec<String>) -> RusticResult<Vec<String>> {
if command.is_empty() {
Ok(Vec::new())
} else if command.len() > 1 {
// command already split
Ok(command.to_owned())
} else {
// vec size is 1 -> it might need to be splitted
let split_command =
shell_words::split(&command[0]).map_err(RepositoryErrorKind::FromSplitError)?;
Ok(split_command)
}
} |
Thanks @drdo for opening this issue (and sorry for the late reply)! The mentioned change was done to make using commands in the config easier. But, yes, it seems this breaks CLI. I'll need to think about what to do here... |
Hello again, @aawsome would you say the proposed changes in my former comment (#1177 (comment)) represents a viable and acceptable solution ? If so, I'd be happy to make a PR (and add the test config). Let me know what you think. |
@nardoor one of the reason we didn't want to go with that splitting was that treating spaces is always complicated for users: when using spaces to split, how do we specify real spaces? IMO your proposed solution would even complicate this more. I think we should try to do the splitting into the Vec differently for clap and serde. For clap we should imo use the split function, for serde we should leave it as it is... |
I don't know yet how to implement this in clap, maybe some value_parser could work... |
@aawsome |
Neither way works to run a command with arguments.
In either case clap always parses this into a vector of one element with the entire string, which will then fail to run with
error: No such file or directory (os error 2)
.For example if you run
rustic --password-command "echo foo"
clap will setpassword_command
inRepositoryOptions
to a value like["echo foo"]
, rather than the presumably expected["echo", "foo"]
.I didn't test but I expect that
warm_up_command
has the exact same issue.I'm happy to take a crack at solving this but I'd like some input.
In my opinion the nicest way from a user perspective would be to parse the string into arguments supporting at least quoting so you can have arguments with spaces in them.
Additional issue:
The error message is very unhelpful for a user.
There is no other information, just the
error: No such file or directory (os error 2)
.Had to do some chasing around in the code to figure out what it was.
The text was updated successfully, but these errors were encountered: