Skip to content
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

fix(scp): keep tilde in local paths by using _filedir instead of _expand #765

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

maxnikulin
Copy link

Do not replace ~ by /home/user/ for local files to get behavior of scp consistent with other tools like ls. I hope, dropping _expand and adding _filedir to have file name completion will not cause issues like Ville Skyttä [Bash-completion-devel] _expand, scp and quoting when a patch intended to fix Debian #521406. bash-completion: leading tilde always expanded
was reverted.

@maxnikulin
Copy link
Author

I have a little experience with bash completion, so I can easily miss important use cases. You can consider this pull request as an invitation to discuss how to properly fix the issue with expanding of tilde during completion of scp arguments.

I do not like when ~ is expanded by /home/user since it makes commands longer and more noisy. For a decade I had a hack from [1] redefining _expand in my .bashrc file. Recently I decided to try if the problem has been fixed and the snippet may be removed. For most of commands completion works accordingly to my preferences, but for some tools tilde is expanded. I checked bug reports another time and realized that a fix was committed and reverted. Due to other changes, _longopt widely used for various tools preserves tilde however. Maybe it is better to fix particular tools including scp than try to patch _expand (that is not used by _filedir called from _longopt and helpers specific to various tools).

The cases that this change should address:

scp ~/Do
scp localhost:/tmp/file ~us

[1] lp:622403 bash-completion: readline expand-tilde not acknowledged

@akinomyoga
Copy link
Collaborator

akinomyoga commented Jun 26, 2022

The pathname completions are originally processed by _comp_xfunc_ssh_scp_local_files. With this PR, filenames are doubly generated.

$ scp ./COPYING[TAB twice]
COPYING   COPYING

@maxnikulin
Copy link
Author

$ scp ./COPYING[TAB twice]
COPYING   COPYING

Mea culpa. I have added a bit more code to avoid _comp_xfunc_ssh_scp_local_files when possible, however I am not sure that I have not broken something else. Counting COMPREPLY options may be fragile, but I have not managed to pass variants through some function to determine if file exists and whether it is a directory, but keeping tildes. I do not consider code with a subprocess per each completion variant.

@scop
Copy link
Owner

scop commented Jun 28, 2022

While I dislike the tilde expansion myself as well and do support efforts to get rid of it as much as possible, I don't think we should make behavioral changes here without a set of unit tests having good coverage over the possibilities. This has been a tricky and bug prone thing in the past.

The same probably applies to all other uses of _expand we have as well (rsync and sshfs at least -- I suspect the instance in povray is a false one that could be just removed, but IIRC I had no way to verify it when I tried back in the day).

Do not replace `~` by `/home/user/` for local files to get behavior
consistent with other tools like `ls`. Previous variant of candidates
generating using `ls` is slightly modified to match literal tilde
after a prefix like in `-F~configfile`.

Earlier attempt to suppress `_expand` of tilde for whole bash completion
[Debian #521406. bash-completion: leading tilde always expanded](https://bugs.debian.org/521406)
was reverted since it broke tilde completion for `scp`
[Ville Skyttä \[Bash-completion-devel\] `_expand`, scp and quoting](https://alioth-lists-archive.debian.net/pipermail/bash-completion-devel/2009-February/000973.html)
@maxnikulin maxnikulin force-pushed the ssh-preserve-tilde branch from 58ed923 to c555c90 Compare July 3, 2022 13:43
@maxnikulin
Copy link
Author

While I dislike the tilde expansion myself as well and do support efforts to get rid of it as much as possible, I don't think we should make behavioral changes here without a set of unit tests having good coverage over the possibilities. This has been a tricky and bug prone thing in the past.

It would be great to have unit tests and it can be done by an independent pull request since it would be improvement even in the case of declining of this change.

In the meanwhile I have updated the patch to avoid changing of nospace option. I still do not like scp related code. Frankly speaking, creating the pull request I was expecting reaction like "it is a legacy code, look at the command X that can do the same in a robust way". Are there some hints which files may be considered as a kind of style guide?

I am not motivated enough to create unit tests for so fragile code. I have missed an obvious issue with initial variant of my patch partially because I noticed enough glitches with original code:

  • File names with newline characters (M-/ works) bash completion mangles file names containing newline characters #704.
  • \*2 for *2 file unescaped in response to [Tab].
  • File names with trailing @ are completed without it due to ls-based approach.
  • -F as a prefix is supported, but it seems there are -iidentity and -Sprogram variants.
  • IPv6 addressed are displayed as completion variants but not actually completed.
  • scp -F-F [Tab] causes sed help message due to unknown -F option (I was intended to check ./-F file name).
  • Looking at the file list to find a source for inspiration, I discovered that perl -x~fil[Tab] treats tab as at the beginning of word, so completes user names, not files started with literal tilde.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants