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

Improve ssh/scp completion #3781

Closed
wants to merge 5 commits into
from

Conversation

Projects
None yet
3 participants
@daleeidd
Contributor

daleeidd commented Jan 26, 2017

The commits (especially long form) explain everything. The benefits of some of the changes may be disputable. This should modernise ssh to the latest release, and provide support for customisation options which can make ssh adhere to XDG (you are probably sick of hearing that acronym by now) standards; furthermore, it fixes and removes completions that only get in the way.

I haven't touched other ssh related commands since I don't use them and do not know what is ideal.

Also, I hope you don't mind me using the long options as I think it makes the source easier to understand. It does take up a lot more lines, but the tradeoff is worth it. I also tend to be heavy with the documentation when regex is involved since one cannot describe intent -- but only implementation -- with regex.

@daleeidd

This comment has been minimized.

Show comment
Hide comment
@daleeidd

daleeidd Jan 26, 2017

Contributor

One thing I forgot to implement is relative paths for the Include directive. I will do a rebase by the end of the weekend.

Contributor

daleeidd commented Jan 26, 2017

One thing I forgot to implement is relative paths for the Include directive. I will do a rebase by the end of the weekend.

@daleeidd

This comment has been minimized.

Show comment
Hide comment
@daleeidd

daleeidd Jan 28, 2017

Contributor

Done. If anyone has time, a code review will be greatly appreciated. I am still learning fish, and I generally dislike shell scripting for any tasks which are not trivial.

Contributor

daleeidd commented Jan 28, 2017

Done. If anyone has time, a code review will be greatly appreciated. I am still learning fish, and I generally dislike shell scripting for any tasks which are not trivial.

daleeidd added some commits Jan 26, 2017

Add IPV6 /etc/hosts completion support
Parses columns rather than values which produces improved output
Ignore ssh Hostname and Host with wildcard
The following only get in the way:
- Hostname: Host resolves to Hostname
- Wildcard Host: Cannot ssh to a glob pattern
Improve scp completions
* complete only local files when no host provided
* complete only remote files when host is provided
* complete local files or hosts when no separator
Disable username completion for ssh/scp
Username completion only provides local users which will unlikely be
useful on a remote machine. ssh will use the current username (the only
useful one) or one provided in the ssh config.
@daleeidd

This comment has been minimized.

Show comment
Hide comment
@daleeidd

daleeidd Jan 28, 2017

Contributor

I adhered to the style guide with the exception to using fish_indent since it didn't do a very good job at indentation. Please see the before and after using fish_indent:

    for file in $ssh_configs
        if test -r $file
            # Print hosts from system wide ssh configuration file
            string match --regex --ignore-case '^\s*Host\s+\S+' <$file \
                # We only want the value(s)
                | string replace --regex --ignore-case '^\s*Host\s+' '' \
                # Print one per line
                | string trim | string replace --regex '\s+' ' ' | string split ' ' \
                # Ignore hosts with a glob
                | string match --invert '*\**'
            # Extract known_host paths
            set known_hosts $known_hosts (string match -ri '^\s*UserKnownHostsFile|^\s*GlobalKnownHostsFile' <$file \
                | string replace -ri '.*KnownHostsFile\s*' '')
        end
    end
    for file in $ssh_configs
        if test -r $file
            # Print hosts from system wide ssh configuration file
            string match --regex --ignore-case '^\s*Host\s+\S+' <$file # We only want the value(s)
 | string replace --regex --ignore-case '^\s*Host\s+' '' # Print one per line
 | string trim | string replace --regex '\s+' ' ' | string split ' ' # Ignore hosts with a glob
 | string match --invert '*\**'
            # Extract known_host paths
            set known_hosts $known_hosts (string match -ri '^\s*UserKnownHostsFile|^\s*GlobalKnownHostsFile' <$file \
                | string replace -ri '.*KnownHostsFile\s*' '')
        end
    end
Contributor

daleeidd commented Jan 28, 2017

I adhered to the style guide with the exception to using fish_indent since it didn't do a very good job at indentation. Please see the before and after using fish_indent:

    for file in $ssh_configs
        if test -r $file
            # Print hosts from system wide ssh configuration file
            string match --regex --ignore-case '^\s*Host\s+\S+' <$file \
                # We only want the value(s)
                | string replace --regex --ignore-case '^\s*Host\s+' '' \
                # Print one per line
                | string trim | string replace --regex '\s+' ' ' | string split ' ' \
                # Ignore hosts with a glob
                | string match --invert '*\**'
            # Extract known_host paths
            set known_hosts $known_hosts (string match -ri '^\s*UserKnownHostsFile|^\s*GlobalKnownHostsFile' <$file \
                | string replace -ri '.*KnownHostsFile\s*' '')
        end
    end
    for file in $ssh_configs
        if test -r $file
            # Print hosts from system wide ssh configuration file
            string match --regex --ignore-case '^\s*Host\s+\S+' <$file # We only want the value(s)
 | string replace --regex --ignore-case '^\s*Host\s+' '' # Print one per line
 | string trim | string replace --regex '\s+' ' ' | string split ' ' # Ignore hosts with a glob
 | string match --invert '*\**'
            # Extract known_host paths
            set known_hosts $known_hosts (string match -ri '^\s*UserKnownHostsFile|^\s*GlobalKnownHostsFile' <$file \
                | string replace -ri '.*KnownHostsFile\s*' '')
        end
    end
@krader1961

I, and the rest of the fish community, appreciate your work to improve these command completions. However, there is an established pattern of using short flags in the completion scripts. While I personally prefer long flags in scripts (as opposed to interactive use) the established pattern should be followed when changing the behavior of the existing code. Please open a new issue arguing for using long flags in this script, and related scripts, and defer the non-functional flag changes to that issue.

Doing those style changes in a separate PR will also make it much easier for us to approve the functional changes you wish to make.

else if test -r /etc/hosts
# Ignore commented lines and functionally empty lines. Strip comments.
string match -r -v '^\s*0.0.0.0|^\s*#|^\s*$' </etc/hosts | string replace -ra '#.*$' '' | string replace -r '[0-9.]*\s*' '' | string trim | string replace -ra '\s+' '\n'
# Ignore commented lines and functionally empty lines

This comment has been minimized.

@krader1961

krader1961 Jan 28, 2017

Contributor

Please, do not mix comments and statements in this way. Not just here but everywhere else you've done this. It is confusing, breaks fish_indent, and the behavior is likely to change in the future. Also, such a complicated sequence of operations should be done in a discrete function that is invoked here.

@krader1961

krader1961 Jan 28, 2017

Contributor

Please, do not mix comments and statements in this way. Not just here but everywhere else you've done this. It is confusing, breaks fish_indent, and the behavior is likely to change in the future. Also, such a complicated sequence of operations should be done in a discrete function that is invoked here.

This comment has been minimized.

@krader1961

krader1961 Jan 28, 2017

Contributor

Also, for good or bad, your attempt to reformat this code is for naught since fish_indent is the final arbiter of acceptable fish script style. The next time someone runs make style-all this will be reverted. I actually like the comments you've added. And since there isn't any need for a pipeline it would be better to rewrite this as a sequence of standalone statements using a local variable.

@krader1961

krader1961 Jan 28, 2017

Contributor

Also, for good or bad, your attempt to reformat this code is for naught since fish_indent is the final arbiter of acceptable fish script style. The next time someone runs make style-all this will be reverted. I actually like the comments you've added. And since there isn't any need for a pipeline it would be better to rewrite this as a sequence of standalone statements using a local variable.

Show outdated Hide outdated share/functions/__fish_print_hostnames.fish
# Print all hosts from /etc/hosts
# use 'getent hosts' on OSes that support it (OpenBSD and Cygwin do not)
if type -q getent
and getent hosts >/dev/null 2>&1 # test if 'getent hosts' works and redirect output so errors don't print
if type -q getent; and getent hosts > /dev/null 2>&1 # test if 'getent hosts' works and redirect output so errors don't print

This comment has been minimized.

@krader1961

krader1961 Jan 28, 2017

Contributor

Please run your change through fish_indent. The canonical form for this is

if type -q getent
and getent hosts >/dev/null 2>&1
@krader1961

krader1961 Jan 28, 2017

Contributor

Please run your change through fish_indent. The canonical form for this is

if type -q getent
and getent hosts >/dev/null 2>&1
--command scp \
--description Hostname \
--condition "commandline --cut-at-cursor --current-token | string match --invert '*:*'" \
--arguments "

This comment has been minimized.

@krader1961

krader1961 Jan 28, 2017

Contributor

I have no idea why the existing completion uses a multiline command in this manner but it is grotesque. If you're going to change this please abstract the code into a discrete function that is invoked by this completion. There is too much magic interpolation happening in this code.

@krader1961

krader1961 Jan 28, 2017

Contributor

I have no idea why the existing completion uses a multiline command in this manner but it is grotesque. If you're going to change this please abstract the code into a discrete function that is invoked by this completion. There is too much magic interpolation happening in this code.

@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Jan 28, 2017

Contributor

I adhered to the style guide with the exception to using fish_indent since it didn't do a very good job at indentation.

I understand why you deviated from the output of fish_indent but that doesn't matter. Just as the C++ code has to conform to what clang-format dictates so does the fish script code have to conform to fish_indent. The solution is to improve fish_indent and/or rewrite the code into discrete steps so that it doesn't need special-casing.

Contributor

krader1961 commented Jan 28, 2017

I adhered to the style guide with the exception to using fish_indent since it didn't do a very good job at indentation.

I understand why you deviated from the output of fish_indent but that doesn't matter. Just as the C++ code has to conform to what clang-format dictates so does the fish script code have to conform to fish_indent. The solution is to improve fish_indent and/or rewrite the code into discrete steps so that it doesn't need special-casing.

@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Jan 28, 2017

Contributor

If you separate your style changes into a separate PR from the functional changes it will be much easier to approve each of them.

Contributor

krader1961 commented Jan 28, 2017

If you separate your style changes into a separate PR from the functional changes it will be much easier to approve each of them.

@zanchey

This comment has been minimized.

Show comment
Hide comment
@zanchey

zanchey Jan 29, 2017

Member

However, there is an established pattern of using short flags in the completion scripts.

It's not universal (duply, launchctl, lxc, obname) and I think long flags are fine.

Member

zanchey commented Jan 29, 2017

However, there is an established pattern of using short flags in the completion scripts.

It's not universal (duply, launchctl, lxc, obname) and I think long flags are fine.

@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Jan 29, 2017

Contributor

It's not universal (duply, launchctl, lxc, obname) and I think long flags are fine.

Sure, but long options are rare. And I can't find a single place where --regex is used rather than -r or --invert rather than -v.

But the main reason for my observation is that the existing scp/ssh complete commands uses the short forms; e.g., -c rather than --command. And being consistent with the surrounding code is a best practice. If someone wants to open an issue to make the case that we should standardize on long options and do the work to restyle the existing scripts I probably wouldn't object since, as I said earlier, I generally prefer long options in scripts for the increased clarity they provide.

Contributor

krader1961 commented Jan 29, 2017

It's not universal (duply, launchctl, lxc, obname) and I think long flags are fine.

Sure, but long options are rare. And I can't find a single place where --regex is used rather than -r or --invert rather than -v.

But the main reason for my observation is that the existing scp/ssh complete commands uses the short forms; e.g., -c rather than --command. And being consistent with the surrounding code is a best practice. If someone wants to open an issue to make the case that we should standardize on long options and do the work to restyle the existing scripts I probably wouldn't object since, as I said earlier, I generally prefer long options in scripts for the increased clarity they provide.

@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Jan 31, 2017

Contributor

@daleeidd: We want to see you contribute more improvements like this one. However, please keep in mind that mixing style and functional changes is always a bad idea since it can delay the inclusion of useful functional changes. Changes to the existing style of the code is almost always controversial. It is almost always better to separate the two types of change into separate pull-requests.

I'll modify your pull-request to address the gratuitous style changes and merge it tomorrow. I would be happy to see you open a new issue that talks about why you felt the need to deviate from the existing fish script style.

Contributor

krader1961 commented Jan 31, 2017

@daleeidd: We want to see you contribute more improvements like this one. However, please keep in mind that mixing style and functional changes is always a bad idea since it can delay the inclusion of useful functional changes. Changes to the existing style of the code is almost always controversial. It is almost always better to separate the two types of change into separate pull-requests.

I'll modify your pull-request to address the gratuitous style changes and merge it tomorrow. I would be happy to see you open a new issue that talks about why you felt the need to deviate from the existing fish script style.

@krader1961 krader1961 self-assigned this Jan 31, 2017

@krader1961 krader1961 added this to the fish-future milestone Jan 31, 2017

@daleeidd

This comment has been minimized.

Show comment
Hide comment
@daleeidd

daleeidd Jan 31, 2017

Contributor

Thank you. I was planning on doing the changes myself, but I have fallen on hard times as of late, and I could not muster the energy to fix it. Cleaning that code was a must needed distraction so I couldn't help myself. My apologies.

Contributor

daleeidd commented Jan 31, 2017

Thank you. I was planning on doing the changes myself, but I have fallen on hard times as of late, and I could not muster the energy to fix it. Cleaning that code was a must needed distraction so I couldn't help myself. My apologies.

@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Feb 1, 2017

Contributor

No need to apologize, @daleeidd. Until roughly ten years ago I too greatly underestimated the value of having a consistent style to the code in any given project. And I've been earning a living as a programmer since 1978. Appreciating the value of such guidelines can take a long time to really sink in to the point where you start objecting when you see a best practice violated. 😄

As I said earlier the non-style aspects of your change are great. I hope I haven't scared you away from making contributions in the future.

Contributor

krader1961 commented Feb 1, 2017

No need to apologize, @daleeidd. Until roughly ten years ago I too greatly underestimated the value of having a consistent style to the code in any given project. And I've been earning a living as a programmer since 1978. Appreciating the value of such guidelines can take a long time to really sink in to the point where you start objecting when you see a best practice violated. 😄

As I said earlier the non-style aspects of your change are great. I hope I haven't scared you away from making contributions in the future.

@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Feb 2, 2017

Contributor

Squash merged as commit eff6b98 plus another commit by myself to address the style problems. Hopefully I didn't introduce any bugs not present in @daleeidd's original change.

Contributor

krader1961 commented Feb 2, 2017

Squash merged as commit eff6b98 plus another commit by myself to address the style problems. Hopefully I didn't introduce any bugs not present in @daleeidd's original change.

@krader1961 krader1961 closed this Feb 2, 2017

@zanchey zanchey modified the milestones: 2.6.0, fish-future Feb 3, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment