Re: [PATCH] Completion for sbuild

[Daniel Shahaf <d.s@daniel.shahaf.name>]

Baptiste BEAUPLAT wrote on Thu, Aug 22, 2019 at 21:53:14 +0200:
> I wrote a completion function for sbuild (a debian tool to build
> packages). I think it would be nice to have it upstream.

Thanks for the patch!

> It covers all options described in the man (expect one, which is
> depreciated).

"deprecated", unless sbuild's financials are weirder than I thought. ☺

> Don't hesitate to give me feedback, I'd be glade to improve it.

I've done a partial review and found some minor issues.  I also have
some suggested additions; those are nice to haves, not blockers, of
course.

> _deb_distributions() {

There's already Completion/Debian/Type/_deb_codenames.  Please use it
(and improve it if needed).

> _get_identity() {
>     [ -n "${DEBFULLNAME}" -a -n "${DEBEMAIL}" ] && \
>         compadd "$@" "${DEBFULLNAME} <${DEBEMAIL}>"

Please add an end-of-options guard:
.
    compadd "$@" -- …
.
Please also fix the other instances of this issue in the patch.

Please use «_wanted» to allow filtering by tags.

I know it's annoying, but it's allowed to do:
.
    unset DEBFULLNAME
    DEBEMAIL="J Random <jrandom@debian.org>"
.
Could you support this?

Please use «[[» instead of «[»: the former is a reserved word, the
latter a builtin.  (No effect in this case, but more for a consistent
coding style.)

> }
> 
> _get_gpg_key() {
>     compadd "$@" $(gpg -K --with-colons 2> /dev/null | grep '^uid:u:' |
>                    grep -o -e '<[^>]*>' | tr -d '<>')

Example output:

[[[
% GNUPGHOME=$PWD gpg -K --with-colons
sec:u:3072:1:A1EA95DE874DB2D5:1566533599:1629605599::u:::scESC:::+::::
fpr:::::::::D1ADEF874C12D90BFB1062E0A1EA95DE874DB2D5:
grp:::::::::835F9949A6E28D3B70696867F064CBFF5DEBF50B:
uid:u::::1566533599::A06E327EA7388C18E4740E350ED4E60F2E04FC41::foobar:
ssb:u:3072:1:7CF9BDFD879FC134:1566533599:1629605599:::::e:::+:::
fpr:::::::::876684635455EB5CC21751587CF9BDFD879FC134:
grp:::::::::CD7D8D5F91E21EE5ADA28CADF7A17233F028C1FC:
]]]

A couple of issues here:

- Please use _call_program, to make the path to gpg configurable by
  zstyle.

- It would be nice to use builtins and parameter expansions instead of
  greps, for performance on Cygwin.  (Yes, I know this completion is
  for sbuild, but someone might do «ssh foo sbuild <TAB>».)  For
  example:

  local -a lines=( ${(f)"$(_call_program …)"} )
  lines=( ${(M)lines:#(#s)uid:u:*}

  and then split each element by colons to get the right field.  (You
  need to do that anyway for two reasons: (1) because the uid line
  needn't contain angle brackets; (2) for forwards compatibility with
  future versions of gpg.)
  
  By the way, consider using _describe.  If you use the "Name <Email>
  (Comment)" as the completion, you can add the keyid as the
  description, and vice-versa.

> }
> 
> _sbuild() {
>         '--add-depends=[add dependencies to source package]:depends' \

The part after the colon should say something like "source package
name" or "source package names, comma separated".  It's actually
displayed during completion if you have the «format» style set (e.g.,
«zstyle ':completion:*:(messages|descriptions)' format '> Completing %d»).

I ran out of time here so haven't reviewed the rest of this function,
sorry.

>         '*:dsc file:_files -g "*.dsc"'
> }
> 
> _sbuild "$@"

Cheers!

Daniel