[PATCH] Add xxd completion

[PATCH] Add xxd completion

[Daniel Shahaf]

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

Attached completion for xxd (a hex dump utility from Vim).  It works,
but I haven't written completion functions before so I might have
overlooked something.

Cheers,

Daniel

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

>From 104556bea66766b73f0b7b0812973c6da300670a Mon Sep 17 00:00:00 2001
From: Daniel Shahaf <d.s@daniel.shahaf.name>
Date: Wed, 9 Jul 2014 16:23:53 +0000
Subject: [PATCH] Add xxd completion

---
 Completion/Unix/Command/_xxd |   48 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 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..62cf628
--- /dev/null
+++ b/Completion/Unix/Command/_xxd
@@ -0,0 +1,48 @@
+#compdef xxd
+
+local curcontext="$curcontext" ret=1 arguments
+
+# TODO: permit two hyphens (--autoskip, --groupsize, etc)
+# TODO: xxd -<tab> should show '-x' and '-x:' differently - give visual hint that there's a required argument
+
+# 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
+
+
+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}'[output ARG octets per line]:number of octets per line'
+    {-g+,-groupsize}'[separate the output every ARG octets]:number of octets per group'
+    {-l+,-len}'[output ARG octets]:number of octets to output'
+    {-s,-skip,-seek}'[add ARG to file positions in the input]:file offset (absolute or relative)'
+   
+    ':files:_files'
+)
+
+_arguments -S $arguments && ret=0
+
+return ret
-- 
1.7.10.4

Re: [PATCH] Add xxd completion

[Oliver Kiddle]

Daniel Shahaf wrote:
> Attached completion for xxd (a hex dump utility from Vim).  It works,

Great, thanks.

> 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.

> +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.

> +# 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 -

> +# 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
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.

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"'

Oliver

Re: [PATCH] Add xxd completion

[Daniel Shahaf]

[-- 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

Re: [Bulk] Re: [PATCH] Add xxd completion

[Oliver Kiddle]

Daniel Shahaf wrote:
> Thanks for the review.
[ snip ]
> Done.

As you may have noticed, I've now pushed _xxd.

> > > +# 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:

>     -u                                         -- user name
> I would prefer:
>     -u +                                       -- user name

If you want to dig into the code, it should be possible to make
_arguments do that: match descriptions can differ from the actual match.
I'm not sure how useful it would really be in practice. It'd also need
to allow for optional arguments, and cases where it is -u=USER or
-uUSER. While I say it would be possible, it wouldn't be easy.

> Incidentally, I ran into another general issue:

> 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?

The -- and ./ tricks won't work for all commands and aren't always
necessary. We'd need to handle it in lots of functions. Files starting
with a - are rare, and usually a mistake. Perhaps it'd be worth doing in
_rm as much to see what it involves. Maybe _files should be cleverer,
perhaps also putting the (-.) in globs by default too.
 
> 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?

It'll perhaps seem clearer if you set the format style with the
descriptions tag, e.g:
  zstyle ':completion:*:descriptions' format '%B%d%b'
You may also need this:
  zstyle ':completion:*' group-name ''

This makes it clearer that you have two groups, one of which has no
associated matches, just a heading.

What subsequent tab presses does, depends a lot on your configuration.

Oliver

Re: [Bulk] Re: [PATCH] Add xxd completion

[Bart Schaefer]

} Daniel Shahaf wrote:
}  
} > 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?

At this point in the completion, "-g" is an ignored prefix of the
"-g+" completion, which means it is part of the word to be completed
but should not be shown in the list of matches.  So it isn't.

} > And if "-groupsize" is offered in the message, why does pressing
} > <tab> not complete the command-line word to the one possible
} > completion?

The listing doesn't change on subsequent TABs because the fact that -g
is a possible completion makes -groupsize ambiguous.  You have to do
something to disambiguate, e.g., type a number (of octets) or an "r"
before pressing TAB.

On Oct 11, 11:46am, Oliver Kiddle wrote:
}
} It'll perhaps seem clearer if you set the format style with the
} descriptions tag

We had a brief discussion a few weeks ago of supplying a set of default
zstyles, I've been gradually building up a suggested list.  Both of
these were the first things on it:

}   zstyle ':completion:*:descriptions' format '%B%d%b'
}   zstyle ':completion:*' group-name ''
} 
} This makes it clearer that you have two groups, one of which has no
} associated matches, just a heading.
} 
} What subsequent tab presses does, depends a lot on your configuration.

In this case it doesn't, really; because one of the groups is empty,
there's not much else that can happen, e.g., menu completion has no
alternatives to cycle through.

Re: [Bulk] Re: [PATCH] Add xxd completion

[Daniel Shahaf]

Oliver Kiddle wrote on Sat, Oct 11, 2014 at 11:46:21 +0200:
> Daniel Shahaf wrote:
> > Thanks for the review.
> [ snip ]
> > Done.
> 
> As you may have noticed, I've now pushed _xxd.

I haven't; thanks for applying.

[ I'm short on time right now so I'll reply to the rest of your and
Bart's posts later. ]

Cheers,

Daniel

Re: [Bulk] Re: [PATCH] Add xxd completion

[Daniel Shahaf]

[ Sorry for the delay; I didn't have time during the week. ]

Bart Schaefer wrote on Sat, Oct 11, 2014 at 09:19:35 -0700:
> On Oct 11, 11:46am, Oliver Kiddle wrote:
> }
> } It'll perhaps seem clearer if you set the format style with the
> } descriptions tag
> 
> We had a brief discussion a few weeks ago of supplying a set of default
> zstyles, I've been gradually building up a suggested list.  Both of
> these were the first things on it:
> 

Perhaps add a snippet to enable edit-command-line to that list of suggestions?
It's not a zstyle, but it's asked regularly on IRC.  (I bind it to ^Fc.)

(Also: I like the idea of having such a list — it would nicely bridge the gap
between "keep features off-by-default for backwards compatibility" and "have
helpful features on by default", i.e., "require no configuration in the common
case".)

> }   zstyle ':completion:*:descriptions' format '%B%d%b'
> }   zstyle ':completion:*' group-name ''
> } 
> } This makes it clearer that you have two groups, one of which has no
> } associated matches, just a heading.
> } 

Indeed, the situation with -g<tab> is much clearer with these settings.  They
also make the nature of the -z file suggestion clearer.  I think that addresses
all my upthread questions, then.

> } What subsequent tab presses does, depends a lot on your configuration.
> 

Sorry, forgot to mention explicitly: I was using the default settings provided
by compinstall (autoload compinit && compinit && zstyle :compinstall filename).

> 

Thanks for the detailed replies, Oliver and Bart.

Daniel