Re: [patch] Add ldap completer

[Matthew Martin <phy1729@xxxxxxxxx>]

On Tue, Jun 19, 2018 at 10:21:48PM +0200, Oliver Kiddle wrote:
> Matthew Martin wrote:
> > For the protocol _describe, is -I correct? I don't grok the differences
> > between the prefixes and suffixes. For host is -S '' correct for not
> > adding a space after and should the completer avoid adding the space?
> 
> You should use -S.
> The normal way would actually be to have:
>   compset -S ':*' || suf=( -S :// )
> Then append $suf to _describe.
> 
> -S/-P are what you normally use for separators like : / ,  etc.
> These work together with the suffix auto-removal. The completion
> matching is aware of them and considers them for some purposes.
> 
> -s/-p suffixes/prefixes are included in matching control. So if you use, e.g:
>   compadd -M 'r:|/=* r:|=*' -p "head/" - tail
>   then h/t</tab> will complete to head/tail
> This is behind things like _multi_parts. They are also quoted
> automatically whereas for -P/-S you might need compquote so these are
> sometimes used to take advantage of that.
> 
> -i/-I is akin to directly modifying IPREFIX and ISUFFIX. They tend
> to be used when bypassing matching but can have other uses. Aside
> from quoting, they don't behave exactly like a direct modification of
> IPREFIX/ISUFFIX but that goes beyond my understanding.

Thanks, that helped. It seems that most times using -S sould check the
SUFFIX first, so I've added that to scope as well.

> > The ldapi protocol is not completed correctly because I don't know how
> > to make _files replace / with %2f.
> 
> I don't think you can currently without redoing much of _files. You
> could probably still get _path_files (with -S %2f) to do much of the
> work.
> The completion system does track layers of quoting following compset -q
> but you can't give it custom forms as %2f would be.

(Note in case this ever happens: ed needs something similar since 
ed 'foo\\bar'    edits the file   foo\bar.)

> > +  # [protocol://]host[:port][/basedn[?[attribute,...][?[scope][?[filter]]]]]
> > +  if ! compset -P '*://'; then
> > +    tags=(protocol)
> > +  fi
> > +
> > +  if ! compset -P '*/'; then
> > +    if compset -P '*:'; then
> 
> In general, where you have compset -P. compset -S is also applicable
> when completing whatever falls to the left of the separator.
>
> > +  _tags $tags
> > +  while _tags; do
> > +    _requested protocol && _describe -t protocol protocol protocols -I ://
> > +    _requested host && _hosts -S ''
> > +    _requested port && _guard '|<1-65535>' port
> 
> _guard was created to solve a particular problem. A quick demonstration
> of which is the following: _arguments '--long' ':number:'
>   --l<tab> now does not complete to --long because number is being
> offered.
> I tend to limit it's use to these situations. Even if I've typed some
> letters, getting the "port" description can still be useful.

Noted and fixed.

> > +    _requested basedn expl 'base DN'
> > +    _requested attribute expl attribute
> > +    _requested scope && _describe -t scope scope scopes
> 
> If the scope is followed by '?' and a filter, you might want a ? suffix
> on this. Perhaps in auto-removable form. Or the default suffix could be
> auto-removable.

Added with -suffix check.

> > +    _requested filter expl filter
> > +  done
> 
> The tags loop is not correct. You need to break out of the loop if
> matches were added. _alternative would work here. $tags never contains
> more than one value, however, so the easiest would just be to directly
> generate matches in each place where you assign to $tags.
> 
> If you don't like adding && ret=0 everywhere, compare
> compstate[nmatches] from the start and end of the function.

It's possible for the tags array to have both protocol and host. _tags
makes much more sense now knowing it's looping over groups. Two
questions

Why does _tags not do the compstate[nmatches] check? It would ensure
that the promises made to the user in the tag-order api are kept and
prevents mistaks.

Would it be worthwile to write a function that takes a word specifier
(e.g.  [protocol://]host[:port][/basedn[?[attribute,...][?[scope][?[filter]]]]] )
to make cases like this easier? Next up I was going to submit _smtp
which has a uri argument for -s. Not sure how many other commands take
completable uris.

> > +        ':: :_guard "*=*" "filter"' \
> 
> Is this really doing what you want: offering filter only if there is an
> equals sign in the current word?

Indeed not and removed.

Thanks for the review!

- Matthew Martin


diff --git a/Completion/BSD/Command/_ldap b/Completion/BSD/Command/_ldap
new file mode 100644
index 000000000..8fa17e2f8
--- /dev/null
+++ b/Completion/BSD/Command/_ldap
@@ -0,0 +1,87 @@
+#compdef ldap
+
+local -a commands scopes
+commands=(
+  search:'search a directory'
+)
+scopes=(
+  base:'base object only'
+  one:'one level'
+  sub:subtree
+)
+
+_ldap_url() {
+  local nm=$compstate[nmatches]
+  local -a expl protocols suf_proto suf_scope tags
+  protocols=(
+    ldap:'TCP in plaintext'
+    ldaps:'TLS'
+    ldap+tls:'TCP and use StartTLS'
+    ldapi:'connect to a socket'
+  )
+
+  # [protocol://]host[:port][/basedn[?[attribute,...][?[scope][?[filter]]]]]
+  if ! compset -P '*://'; then
+    tags=(protocol)
+    compset -S ':*' || suf_proto=( -S :// )
+  fi
+
+  if ! compset -P '*/'; then
+    if compset -P '*:'; then
+      tags=(port)
+      compset -S '/*'
+    else
+      if ! compset -S '://*'; then
+        tags+=(host)
+        compset -S '[:/]*'
+      fi
+    fi
+  else
+    case $PREFIX in
+      *\?*\?*\?*) tags=(filter);;
+      *\?*\?*) tags=(scope); [[ -suffix \?* ]] || suf_scope=( -qS \? );;
+      *\?*) tags=(attribute);;
+      *) tags=(basedn);;
+    esac
+    compset -P '*\?'
+    compset -S '\?*'
+  fi
+
+  _tags $tags
+  while _tags; do
+    _requested protocol && _describe -t protocol protocol protocols $suf_proto
+    _requested host && _hosts -S ''
+    _requested port expl port
+    _requested basedn expl 'base DN'
+    _requested attribute expl attribute
+    _requested scope && _describe -t scope scope scopes $suf_scope
+    _requested filter expl filter
+    [[ nm -ne compstate[nmatches] ]] && return 0
+  done
+}
+
+if (( CURRENT == 2 )); then
+  _describe command commands
+else
+  shift words; (( CURRENT-- ))
+  case $words[1] in
+    search)
+      _arguments -s -S -A '-*' \
+        '-b+[specify base DN]:base DN:' \
+        '-c+[specify CA file]:CA file:' \
+        '-D+[specify bind DN]:bind DN:' \
+        '-H+[specify URL]: :_ldap_url' \
+        '-L[output in LDIF]' \
+        '-l+[specify time limit or 0 for no limit]:time limit [0]:' \
+        '-s+[specify scope]:scope [sub]:(($scopes))' \
+        '-v[be verbose]' \
+        '-W[prompt for bind secret]' \
+        '-w+[specify bind secret]:bind secret:' \
+        '-x[use simple authentication]' \
+        '-Z[use StartTLS]' \
+        '-z+[specify maximum number of results or 0 for no limit]:size limit [0]:' \
+        '::filter:' \
+        '*:attribute:'
+      ;;
+  esac
+fi