scp remote file completion extended with port parsing

[vegadj]

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 portnumber part from the scp command and pass the portnumber as argument to ssh command for successful connection.

I have checked these different command variations and all of them is working as intended.

scp -P 1234 server.com:/Users/admin
scp -P1234 server.com:/Users/admin/
scp -rP1234 server.com:/Users/admin/
scp -rP1234 server.com:/Users/admin/
scp -rP 1234 server.com:/Users/admin/
scp -rxxxP 1234 server.com:/Users/admin/
scp -r -P 1234 server.com:/Users/admin/
scp -P 1234 -r server.com:/Users/admin/
scp -P1234 -r server.com:/Users/admin/
scp -r -P1234 server.com:/Users/admin/
scp -r -P1234 server-Px999:/Users/admin/
scp -r -P1234 server-Px999:/Users/admin/
scp -r -P1234 server-P999.com:/Users/admin/
scp -P 1234 serverP9999.com:/Users/admin/
scp -rxXXP 1234 server.com:/Users/admin/
scp -P1234 server.com:/Users/Ps9999/
scp -r -P1234 server.com:/Users/adminP999/
scp -r -P1234 server.com:/Users/adminP999/
scp -r -P1234 server.com:/Users/adminP999
scp -r -P1234 server.com:/Users/admin-P999
scp -P1234 carbon-1P9999:/Users/admin/
scp -P1234 carbonP9999:/Users/admin/
scp carbon-P9999:/Users/admin/
scp carbonP9999:/Users/admin/
scp -r carbonP9999:/Users/admin/

[akinomyoga]

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?

[akinomyoga]

• __sshport should 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.
• \w is the GNU extension so is not always available.
• [ 0-9]* may unexpectedly capture multiple numbers separated by spaces.
• We call sed as command sed to 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.

[akinomyoga]

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 command before echo for consistency with the codebase. I'm sorry that I should have made it clearer, but we actually have a rule that we prefix command only 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 string command 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.

[vegadj]

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.

[akinomyoga]

Thank you for your quick responses and the kind reply!

I had saw the words variable. I will try to change the algorithm with using words variable 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 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.

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_files also. 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.

[akinomyoga]

I had saw the words variable. I will try to change the algorithm with using words variable 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.

[vegadj]

• I moved port parser to _scp function from _scp_remote_files. Since _scp_remote_files is called from different files as rsync and sshfs (or any future new command integration). Uniq parser will need for different commands.
• _scp_remote_files function 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. Like bind_interface or bind_address which should set correctly as Port number for successful connection.
• -[r]P 1234 -[r]P1234 -[r]oPORT=1234 -[r]o PORT=1234 is parsing correctly.
• in -o PORT=1234 format PORT parsed as case insensitive as in scp command.

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.

[vegadj]

_comp_xfunc_ssh_scp_remote_files() function argument conflict is fixed.

• -d argument can pass in any order.
• -p portnumber argument can pass in any order.

In this case, my first thought for passing multiple ssh options with in single argument will not work now.