-
Notifications
You must be signed in to change notification settings - Fork 399
scp remote file completion extended with port parsing #738
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the report and the pull request! We'd like to finally support it. Could you check the following comments?
In addition, we want to see the test cases for this one. Test cases can be added in test/t/test_scp.py. Could you try adding the test cases there?
completions/ssh
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
__sshportshould be made local. The variable name doesn't need to start with the double underscores in this function. -
`...`is deprecated by POSIX.$(...)should be used instead. -
\wis the GNU extension so is not always available. -
[ 0-9]*may unexpectedly capture multiple numbers separated by spaces. - We call
sedascommand sedto prevent the alias of the same name from taking effect. -
$"{...}"must be a typo of"${...}". Note that$"..."has a different meaning.
However, in the codebase of bash-completion, we usually scan all the words one by one looping over the array words. I'm afraid that the above code would capture some unexpected numbers in confusing filenames.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for updating the PR! These are requests for further improvements and cosmetic changes:
- Could you quote the words as
"${COMP_WORDS[*]}"to prevent unwanted pathname expansions? -
\([ 0-9][0-9]*\}can also match with the case without any digits. I would suggest\( \{0,1\}[0-9]\{1,\}\). [ Note:?and+in typical regular-expression language are\{0,1\}and\{1,\}, respectively, in POSIX BRE. ]
The following ones are cosmetic/style changes.
- We'd like to remove
commandbeforeechofor consistency with the codebase. I'm sorry that I should have made it clearer, but we actually have a rule that we prefixcommandonly to the pre-defined set of commands that are commonly overridden by aliases (cf here for the list). See also the corresponding section of CONTRIBUTING.md (where you can find the section by searching for the stringcommand sed). - Could you remove the quoting by
"..."inside[[ ... ]]? In the conditional command[[ ... ]]the quoting is not needed because the pathname expansions and word splitting are not performed inside the conditional command. In the codebase, we are not quoting the parameter expansions inside the conditional command (except for the edge case of${arr[*]}where certain versions of Bash have a bug). - Can you just use the form
[[ ${...} ]]instead of[[ -n ${...} ]]? We are currently standardizing the choice of[[ -n xxx ]]vs[[ xxx ]](see style: prefer empty/!over-n/-z#740 ).
The tests are still welcome if possible.
Hi akinomyoga;
Thank you for your kind review and fast response. I have done the changes as you suggested.
I also re-test in Linux machine and this seems to work fine.
However, in the codebase of bash-completion, we usually scan all the words one by one looping over the array words. I'm afraid that the above code would capture some unexpected numbers in confusing filenames.
I had saw the words variable. I will try to change the algorithm with using words variable soon.
In addition, we want to see the test cases for this one. Test cases can be added in test/t/test_scp.py. Could you try adding the test cases there?
I am willing to do it. But currently I have no idea about how to do use pytest at all. So it will take some time for me to learn and code for test part. Some volunteer could do it faster than I can if it has priority.
Meanwhile for rsync over ssh, the port passing issue should also fix. As far as I saw rsync uses ssh _scp_remote_files also. It may lead a conflict and could need a different implementation that i did.
Thank you for your quick responses and the kind reply!
I had saw the
wordsvariable. I will try to change the algorithm with usingwordsvariable soon.
Got it! If so, you can just discard the above comments in the corresponding section!
I am willing to do it. But currently I have no idea about how to do use
pytestat all. So it will take some time for me to learn and code for test part. Some volunteer could do it faster than I can if it has priority.
Maybe I can take a look later (but not soon).
Meanwhile for rsync over ssh, the port passing issue should also fix. As far as I saw rsync uses
ssh _scp_remote_filesalso. It may lead a conflict and could need a different implementation that i did.
Good catch. rsync seems to have the option --port=PORT. sshfs completion also seems to use _scp_remote_files, where sshfs accepts -p PORT and -o port=PORT. Actually, scp also accepts -o Port=PORT.
I had saw the
wordsvariable. I will try to change the algorithm with usingwordsvariable soon.
For reference, I think you can refer to the existing implementations found by the following command:
$ grep -F '${words[i]}' completions/*
Actually, we have so many similar/duplicate codes for the option-argument extractions. Maybe we can create a utility to perfom it.
...' is also parsing
- I moved port parser to
_scpfunction from_scp_remote_files. Since_scp_remote_filesis called from different files asrsyncandsshfs(or any future new command integration). Uniq parser will need for different commands. _scp_remote_filesfunction caller should give the correct port as function argument. Argument should be in the format-p portnumber. So that besides port number different kinds of argument can be pass by caller with same argument. Likebind_interfaceorbind_addresswhich should set correctly as Port number for successful connection.-[r]P 1234-[r]P1234-[r]oPORT=1234-[r]o PORT=1234is parsing correctly.- in
-o PORT=1234formatPORTparsed as case insensitive as inscpcommand.
Edit: I have just realized that positional argument I have used in _scp_remote_files conflicts with -d case used in sshfs. I am gone fix that.
_comp_xfunc_ssh_scp_remote_files() function argument conflict is fixed.
-dargument can pass in any order.-p portnumberargument can pass in any order.
In this case, my first thought for passing multiple ssh options with in single argument will not work now.
Hi there,
There was a issue with the remote file path completion with scp command.
If remote server uses non-default or not configured port number, remote file completion does not work at all because of it does not know which port to use for connection. (actually it tries out default port for connection and it fails eventually).
What I did to fix this issue is: parse
-P portnumberpart from the scp command and pass theportnumberas argument to ssh command for successful connection.I have checked these different command variations and all of them is working as intended.