bash-completion icon indicating copy to clipboard operation
bash-completion copied to clipboard

scp remote file completion extended with port parsing

Open vegadj opened this issue 3 years ago • 5 comments

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/

vegadj avatar Apr 12 '22 08:04 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.

vegadj avatar Apr 12 '22 22:04 vegadj

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 avatar Apr 12 '22 22:04 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.

akinomyoga avatar Apr 12 '22 22:04 akinomyoga

  • 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 avatar Apr 15 '22 00:04 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.

vegadj avatar Apr 15 '22 14:04 vegadj