fix(git-transport): Use single quotes for ssh path arg #713
Conversation
There was a problem hiding this comment.
Thanks a lot, indeed git quotes the path when using ssh.
Before I forget, please put changes to git-quote into their own commit, ideally prefixing it with feat: … mentioning the new quoting mechanism. That way the changelog is written with the note automatically.
Thanks again for your help.
| @@ -216,7 +231,8 @@ impl client::Transport for SpawnProcessOnDemand { | |||
| if self.ssh_cmd.is_some() { | |||
| cmd.args.push(service.as_str().into()); | |||
| } | |||
| cmd.args.push(self.path.to_os_str_lossy().into_owned()); | |||
| cmd.args | |||
| .push(to_single_quoted(self.path.as_ref()).to_os_str_lossy().into_owned()); | |||
There was a problem hiding this comment.
SpawnProcessOnDemand isn't only used by ssh, but also when spawning processes locally.
This transformation should only happen when ssh_cmd is set.
| use crate::client::file::to_single_quoted; | ||
|
|
||
| #[test] | ||
| fn quoted_strings() { |
There was a problem hiding this comment.
I think a couple of tests are missing to implement this behaviour equivalently. CI seems to fail due to it as well.
Please also move it to this module - in there it's easier to test as well. Try to break it with your tests :).
exactly-one-kas
force-pushed
the
ssh-quoting
branch
from
c3d5203 to
78815e6
Compare
This function takes a string and transforms it into a form safe for consumption by Bourne compatible shells.
Git uses this method of quoting args for SSH transport too Some Git SSH servers require this method to be used (eg. BitBucket)
|
Thanks a lot. I have a feeling the tests were written to reflect the function as it was already implemented, instead of what's described in the git implementation, as there was plenty to correct. Furthermore it's advisable to follow the structure of the crate closely to keep things as consistent as possible. |
|
Ah, I didn't think of using the documented examples as tests |
|
Thanks for pointing this out! The function is still under-tested and it's probably one of these cases where it has to follow its C counterpart precisely, fortunate it's small. In theory it should be enough to implement the Edit: I was also completely mis-interpreting the little documentation that there is and set the test-cases up to match my expectation (according to what I read) instead of what was clearly written as example. |
This improves how `gix_quote::single` documents how it handles the cases it treats specially. The fix in GitoxideLabs#717 (discussed in GitoxideLabs#713) wasn't accompanied by a change to the documentation, which continued either to describe the preceding behavior or to be ambiguous. This changes it to describe the current behavior unambiguously. Only documentation is changed.
Git uses this method of quoting args for SSH transport too Some Git SSH servers require this method to be used (eg. BitBucket)
I have no idea how to test for the quoted string actually being used in
handshake()without ripping the method's guts out. Do you have any pointers for me?