Re: patch for ssh completion

[Daniel Shahaf <d.s@xxxxxxxxxxxxxxxxxx>]

Jeremy — have you seen my review (quoted below)?  Would you be able to
revise the patch to address these points, at least the first two?

Process-wise, patches that apply to latest master are preferred, but
_ssh hasn't been changed since 5.3.1 so that's okay.

Cheers,

Daniel

Daniel Shahaf wrote on Thu, Mar 02, 2017 at 12:12:29 +0000:
> Jeremy Cantrell wrote on Tue, Feb 28, 2017 at 11:17:29 -0800:
> > Hello!
> > 
> > I've found a tiny problem with the ssh completion script where any host
> > entries defined in included configuration files do not appear as choices
> > when completing. I've attached a patch that fixes it for me. Below is an
> > example that illustrates the problem:
> > 
> > ~/.ssh/config:
> > Include hosts
> > 
> > ~/.ssh/hosts:
> > Host example
> > HostName example.local
> 
> From ssh_config(5):
> 
>      Include
>              Include the specified configuration file(s).  Multiple pathnames
>              may be specified and each pathname may contain glob(3) wildcards
>              and, for user configurations, shell-like ‘~’ references to user
>              home directories.  Files without absolute paths are assumed to be
>              in ~/.ssh if included in a user configuration file or /etc/ssh if
>              included from the system configuration file.  Include directive
>              may appear inside a Match or Host block to perform conditional
>              inclusion.
> 
> > Thanks,
> > Jeremy
> 
> > --- /usr/share/zsh/functions/Completion/Unix/_ssh	2016-12-22 11:27:00.000000000 -0800
> > +++ packages/base/.local/share/zsh/functions/_ssh	2017-02-26 22:37:47.513422235 -0800
> > @@ -681,16 +681,21 @@
> >    fi
> >    if [[ -r $config ]]; then
> >      local key hosts host
> > +    local filename configs=($config)
> > +    grep '^Include\b' "$config" | sed 's/\s\+/ /g' | cut -d' ' -f2 |
> > +    while read -r filename; do
> > +        config=$HOME/.ssh/$filename
> 
> This handles your example, but not the other possibilities documented in
> the man page, e.g., absolute paths.  I expect using those would cause
> errors on stderr.  (I can't test this since my ssh isn't new enough.)
> 
> The «\b» in grep is not in POSIX.  It could be changed to match
> whitespace explicitly.  I assume we'll also want to use builtin
> functionality such as ${(M)…:#Include*} instead of external executables.
> 
> Can filenames be quoted?  I.e., does «Include "foo"» include ~/.ssh/foo
> or ~/.ssh/\"foo\"?
> 
> Can Include directives be nested?  If they can, it'd be nice to handle
> that.  (But I don't think that's a blocker to accepting the patch)
> 
> >          while IFS=$'=\t ' read -r key hosts; do
> >          if [[ "$key" == (#i)host ]]; then
> >              for host in ${(z)hosts}; do
> >                  case $host in
> >                  (*[*?]*) ;;
> >                  (*) config_hosts+=("$host") ;;
> >                  esac
> >              done
> >          fi
> >          done < "$config"
> > +    done
> 
> Thanks for the patch.
> 
> Cheers,
> 
> Daniel