Re: [PATCH] Add xxd completion

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

[-- Attachment #1: Type: text/plain, Size: 4210 bytes --]

Oliver Kiddle wrote on Wed, Oct 08, 2014 at 16:35:07 +0200:
> Daniel Shahaf wrote:
> > but I haven't written completion functions before so I might have
> > overlooked something.
> 
> With that in mind, I've taken a look and have a couple of
> recommendations.
> 

Thanks for the review.

> > +local curcontext="$curcontext" ret=1 arguments
> 
> None of these are actually needed. curcontext is only relevant if you
> call _arguments or _values with the -C option. You do use ret but a zsh
> function will pass on the return status of the last command it calls and
> _xxd only adds matches with a final call to _arguments. Finally the
> arguments array is only expanded once and could be specified inline at
> that point.
> 

Okay; I removed 'curcontext' and 'ret'.

As to 'arguments', I don't see a benefit to inlining it, and keeping it as a
separate variable makes the code easier to read.  What would be the benefit of
inlining it at the single use site?

> > +# TODO: permit two hyphens (--autoskip, --groupsize, etc)
> 
> Given that two dashes is accepted but not documented by xxd and that it
> also works for the short options, e.g. --u, I would just strip a dash as
> follows:
> 
>   [[ -prefix -- ]] && compset -P -
> 

Done.

> > +# TODO: xxd -<tab> should show '-x' and '-x:' differently - give visual hint that there's a required argument
> 
> Unless I'm missing something, there's nothing xxd specific in that
> desire. Perhaps it should be considered for the general case. In many

Yes, that's a general issue.  For example, instead of:

    % sudo -<tab>
    -s                                         -- run SHELL
    -u                                         -- user name

I would prefer:

    % sudo -<tab>
    -s                                         -- run SHELL
    -u +                                       -- user name

or even:

    % sudo -<tab>
    -s                                         -- run SHELL
    -u USER                                    -- user name

>

Incidentally, I ran into another general issue:

    % xxd -<tab>
    -z
    -autoskip    -a               -- a single '*' replaces runs of NUL (toggleable
    -bits        -b               -- output in binary digits, rather than hex     
    -cols        -c               -- specify number of octets per line            
    ...

It wasn't immediately obvious to me, but I eventually realized the "-z" in the
output was caused by a stray file named "-z" (via the _files completion defined
for positional arguments).  However, since the filename begins with a hyphen,
specifying it as "-z" won't work as intended.  Perhaps "xxd -<tab>" should
offer "./-z" or "-- -z" (two words) as possible completions?

> cases, you can tell options that take an argument by the fact that the
> description starts with the word "specify". Taking -c as an example, you
> have:
> 
> > +    {-c+,-cols}'[output ARG octets per line]:number of octets per line'
> 
> Until I checked, and found otherwise, I was guessing that the "ARG" came
> from a direct cut and paste from the -help output.
> 

Okay, I switched the descriptions from the "ARG" convention to the "specify"
convention.

> The arguments to -c, -g, -l and -s also get in the way of completing
> -cols, -groupsize, -len and -seek:
>   % xxd -grou<tab>
>   number of octets per group
>   option
>   -groupsize
> 
> To _arguments, the rou characters could be the number of octets. By
> using _guard, we can tell it that the number can only be the characters
> 0-9 (xxd allows hex so a few more characters are allowed there). With
> _guard, it looks like this:
> 
>   {-c+,-cols}'[specify number of octets per line]: :_guard "[0-9a-fA-Fx]#" "number of octets per line"'
> 

Done.  'xxd -grou<tab>' now DTRTs; however, 'xxd -g<tab>' gives me:

    % xxd -g<tab>
    -groupsize  -- specify the number of octets per group

and the display doesn't change even if I press <tab> again.  This seems odd:
why am I not offered both -g and -groupsize as possible completions?  And if
"-groupsize" is offered in the message, why does pressing <tab> not complete
the command-line word to the one possible completion?

> Oliver

Thanks for the tips.  Revised patch attached.

Daniel

[-- Attachment #2: 0001-Add-xxd-completion.patch --]
[-- Type: text/x-patch, Size: 2598 bytes --]

>From 81619a056a812d53f93415d9dddaf8b59aa56eed Mon Sep 17 00:00:00 2001
From: Daniel Shahaf <d.s@daniel.shahaf.name>
Date: Wed, 8 Oct 2014 17:12:37 +0000
Subject: [PATCH] Add xxd completion

---
 Completion/Unix/Command/_xxd |   45 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)
 create mode 100644 Completion/Unix/Command/_xxd

diff --git a/Completion/Unix/Command/_xxd b/Completion/Unix/Command/_xxd
new file mode 100644
index 0000000..12f2c91
--- /dev/null
+++ b/Completion/Unix/Command/_xxd
@@ -0,0 +1,45 @@
+#compdef xxd
+
+local arguments
+
+# Output options compatibility matrix
+# 
+# 0 - options conflict
+# 1 - options coexist
+#
+# (The matrix is symmetric, so implied values are not shown.)
+#
+#     bEipru
+#    bx10000
+#    E-x0001
+#    i--x001
+#    p---x11
+#    r----x0
+#    u-----x
+
+# xxd supports either double or single dashes on long options.
+[[ -prefix -- ]] && compset -P -
+
+arguments=(
+    # output options
+    '(-b -bits            -i -include -p -postscript -plain -ps -r -reverse -u -uppercase)'{-b,-bits}'[output in binary digits, rather than hex]'
+    '(         -E -EBCDIC -i -include -p -postscript -plain -ps -r -reverse              )'{-E,-EBCDIC}'[print human-readable part in EBCDIC rather than ASCII]'
+    '(-b -bits -E -EBCDIC -i -include -p -postscript -plain -ps -r -reverse              )'{-i,-include}'[output in C include file style]'
+    '(-b -bits -E -EBCDIC -i -include -p -postscript -plain -ps                          )'{-p,-postscript,-plain,-ps}'[read or write a plain hexdump (no line numbers or ASCII rendering)]'
+
+    '(-b -bits -E -EBCDIC -i -include                           -r -reverse -u -uppercase)'{-r,-reverse}'[reverse mode\: read a hex dump and output binary data]'
+    '(-b -bits                                                  -r -reverse -u -uppercase)'{-u,-uppercase}'[output upper-case hex digits]'
+   
+    {-h,-help}'[display usage message]'
+    {-v,-version}'[show program version]'
+    '*'{-a,-autoskip}"[a single '*' replaces runs of NUL (toggleable)]"
+   
+    {-c+,-cols}'[specify number of octets per line]: :_guard "[0-9a-fA-Fx]#" "number of octets per line"'
+    {-g+,-groupsize}'[specify the number of octets per group]: :_guard "[0-9]#" "number of octets per group"'
+    {-l+,-len}'[specify number of octets to output]: :_guard "[0-9]#" "number of octets to output"'
+    {-s,-skip,-seek}'[specify file offset to dump from]: :_guard "[0-9]#" "file offset to dump from (absolute or relative)"'
+   
+    ':files:_files'
+)
+
+_arguments -S $arguments
-- 
1.7.10.4