* [PATCH 0/1] *** Small improvement for gpg completion ***
@ 2018-05-26 15:16 doron.behar
2018-05-26 15:16 ` [PATCH 1/1] _gpg: Use explicit UIDs for state = public keys doron.behar
0 siblings, 1 reply; 6+ messages in thread
From: doron.behar @ 2018-05-26 15:16 UTC (permalink / raw)
To: zsh-workers
From: Doron Behar <doron.behar@gmail.com>
*** Small improvement for gpg completion ***
Doron Behar (1):
_gpg: Use explicit UIDs for state = public keys.
Completion/Unix/Command/_gpg | 15 +++++++++++++--
1 file changed, 13 insertions(+), 2 deletions(-)
--
2.17.0
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/1] _gpg: Use explicit UIDs for state = public keys.
2018-05-26 15:16 [PATCH 0/1] *** Small improvement for gpg completion *** doron.behar
@ 2018-05-26 15:16 ` doron.behar
2018-05-26 16:25 ` Daniel Shahaf
0 siblings, 1 reply; 6+ messages in thread
From: doron.behar @ 2018-05-26 15:16 UTC (permalink / raw)
To: zsh-workers
From: Doron Behar <doron.behar@gmail.com>
Use the `--with-colons` option and parse the output while IFS=":"
according to the output format.
---
Completion/Unix/Command/_gpg | 15 +++++++++++++--
1 file changed, 13 insertions(+), 2 deletions(-)
diff --git a/Completion/Unix/Command/_gpg b/Completion/Unix/Command/_gpg
index 48a36eff2..71fa7667d 100644
--- a/Completion/Unix/Command/_gpg
+++ b/Completion/Unix/Command/_gpg
@@ -206,8 +206,19 @@ fi
case "$state" in
public-keys)
- _wanted public-keys expl 'public key' \
- compadd ${${(Mo)$(_call_program public-keys $words[1] $needed --list-public-keys --list-options no-show-photos):%<*>}//(<|>)/} && return
+ OLDIFS="${IFS}"
+ IFS=":"
+ public_keys=($($words[1] $needed --list-public-keys --list-options no-show-photos --with-colons))
+ for i in {1..${#public_keys[@]}}; do
+ if [[ ${public_keys[$i]} =~ "fpr" ]] && [[ ${public_keys[$((i + 19))]} =~ "@" ]] ; then
+ # +9 is the uid
+ # +19 is the description
+ uids_and_emails+=(${public_keys[$((i + 9))]}":"${public_keys[$((i + 19))]})
+ i=$((i+20))
+ fi
+ done
+ _describe uids uids_and_emails
+ IFS="${OLDIFS}"
;;
secret-keys)
_wanted secret-keys expl 'secret key' compadd \
--
2.17.0
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] _gpg: Use explicit UIDs for state = public keys.
2018-05-26 15:16 ` [PATCH 1/1] _gpg: Use explicit UIDs for state = public keys doron.behar
@ 2018-05-26 16:25 ` Daniel Shahaf
2018-05-29 14:11 ` Doron Behar
0 siblings, 1 reply; 6+ messages in thread
From: Daniel Shahaf @ 2018-05-26 16:25 UTC (permalink / raw)
To: doron.behar; +Cc: zsh-workers
On Sat, May 26, 2018 at 06:16:28PM +0300, doron.behar@gmail.com wrote:
> From: Doron Behar <doron.behar@gmail.com>
>
> Use the `--with-colons` option and parse the output while IFS=":"
> according to the output format.
> ---
> Completion/Unix/Command/_gpg | 15 +++++++++++++--
> 1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/Completion/Unix/Command/_gpg b/Completion/Unix/Command/_gpg
> index 48a36eff2..71fa7667d 100644
> --- a/Completion/Unix/Command/_gpg
> +++ b/Completion/Unix/Command/_gpg
> @@ -206,8 +206,19 @@ fi
>
> case "$state" in
> public-keys)
> - _wanted public-keys expl 'public key' \
> - compadd ${${(Mo)$(_call_program public-keys $words[1] $needed --list-public-keys --list-options no-show-photos):%<*>}//(<|>)/} && return
> + OLDIFS="${IFS}"
OLDIFS isn't made local so it leaks.
You could alternatively have set IFS in a precommand assignment (e.g., «IFS=:
/bin/echo foo:bar»), or made it local to a function. Another approach is use
parameter expansion flags such as «${(s.:.)foo}».
> + IFS=":"
> + public_keys=($($words[1] $needed --list-public-keys --list-options no-show-photos --with-colons))
Another parameter leak. (See WARN_CREATE_GLOBAL.)
Why did you remove the use of _call_program?
> + for i in {1..${#public_keys[@]}}; do
> + if [[ ${public_keys[$i]} =~ "fpr" ]] && [[ ${public_keys[$((i + 19))]} =~ "@" ]] ; then
The parameter 'i' leaks.
"fpr" should be looked for as a complete string, not as a substring. Also, it
should be looked for only in the first column, not in every single output
field.
A subscript is always parsed as a math context so you can just do «$foo[i+9]»
without an additional $((…)) inside.
> + # +9 is the uid
> + # +19 is the description
> + uids_and_emails+=(${public_keys[$((i + 9))]}":"${public_keys[$((i + 19))]})
I'm sorry, but that's not forward compatible. The output format spec
(doc/DETAILS, which is referred to from the manpage) states that fields may be
added in the future, so that "19" may not be hardcoded here.
> + i=$((i+20))
This line doesn't have any effect, does it? This isn't an arithmetic for, it's
a list-of-words for, and the next word is equal to $(( pre_assignment_value_of_i + 1 )).
> + fi
> + done
> + _describe uids uids_and_emails
The use of describe doesn't set the 'public-keys' tag that _wanted set. (You
need to pass -t.)
> + IFS="${OLDIFS}"
Thanks for the patch. I agree that it would be better to use --with-colons.
We look forward to a revised patch. :-)
Cheers,
Daniel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] _gpg: Use explicit UIDs for state = public keys.
2018-05-26 16:25 ` Daniel Shahaf
@ 2018-05-29 14:11 ` Doron Behar
2018-05-30 19:04 ` Daniel Shahaf
0 siblings, 1 reply; 6+ messages in thread
From: Doron Behar @ 2018-05-29 14:11 UTC (permalink / raw)
To: zsh-workers
I've successfully managed to solve address all the comments you've had
for my patch. Yet, I'm having trouble with `_call_program`. The line I'm
trying to put is this:
local public_keys=(${(@s.:.)$(_call_program public-keys $words[1] $needed --list-public-keys --list-options no-show-photos --with-colons)})
I debugged this a little bit and the variable `$public_keys` is empty
but if I remove the words: `_call_program public-keys` it works good.
I've had a glimpse at `htop` when this function was called when I tested
the completion function and I saw these commands running there:
pkgfile -b -v -- --list-public-keys
pkgfile -b -v -- --list-options
pkgfile -b -v -- no-show-photos
pkgfile -b -v -- --with-colons
What is _call_program supposed to be doing?
Besides that, the revised patch is ready. I've also included similar
improvements for the other states - `secret-keys` and `public-keyids`.
On Sat, May 26, 2018 at 04:25:41PM +0000, Daniel Shahaf wrote:
> On Sat, May 26, 2018 at 06:16:28PM +0300, doron.behar@gmail.com wrote:
> > From: Doron Behar <doron.behar@gmail.com>
> >
> > Use the `--with-colons` option and parse the output while IFS=":"
> > according to the output format.
> > ---
> > Completion/Unix/Command/_gpg | 15 +++++++++++++--
> > 1 file changed, 13 insertions(+), 2 deletions(-)
> >
> > diff --git a/Completion/Unix/Command/_gpg b/Completion/Unix/Command/_gpg
> > index 48a36eff2..71fa7667d 100644
> > --- a/Completion/Unix/Command/_gpg
> > +++ b/Completion/Unix/Command/_gpg
> > @@ -206,8 +206,19 @@ fi
> >
> > case "$state" in
> > public-keys)
> > - _wanted public-keys expl 'public key' \
> > - compadd ${${(Mo)$(_call_program public-keys $words[1] $needed --list-public-keys --list-options no-show-photos):%<*>}//(<|>)/} && return
> > + OLDIFS="${IFS}"
>
> OLDIFS isn't made local so it leaks.
>
> You could alternatively have set IFS in a precommand assignment (e.g., «IFS=:
> /bin/echo foo:bar»), or made it local to a function. Another approach is use
> parameter expansion flags such as «${(s.:.)foo}».
>
> > + IFS=":"
> > + public_keys=($($words[1] $needed --list-public-keys --list-options no-show-photos --with-colons))
>
> Another parameter leak. (See WARN_CREATE_GLOBAL.)
>
> Why did you remove the use of _call_program?
>
> > + for i in {1..${#public_keys[@]}}; do
> > + if [[ ${public_keys[$i]} =~ "fpr" ]] && [[ ${public_keys[$((i + 19))]} =~ "@" ]] ; then
>
> The parameter 'i' leaks.
>
> "fpr" should be looked for as a complete string, not as a substring. Also, it
> should be looked for only in the first column, not in every single output
> field.
>
> A subscript is always parsed as a math context so you can just do «$foo[i+9]»
> without an additional $((…)) inside.
>
> > + # +9 is the uid
> > + # +19 is the description
> > + uids_and_emails+=(${public_keys[$((i + 9))]}":"${public_keys[$((i + 19))]})
>
> I'm sorry, but that's not forward compatible. The output format spec
> (doc/DETAILS, which is referred to from the manpage) states that fields may be
> added in the future, so that "19" may not be hardcoded here.
>
> > + i=$((i+20))
>
> This line doesn't have any effect, does it? This isn't an arithmetic for, it's
> a list-of-words for, and the next word is equal to $(( pre_assignment_value_of_i + 1 )).
>
> > + fi
> > + done
> > + _describe uids uids_and_emails
>
> The use of describe doesn't set the 'public-keys' tag that _wanted set. (You
> need to pass -t.)
>
> > + IFS="${OLDIFS}"
>
> Thanks for the patch. I agree that it would be better to use --with-colons.
> We look forward to a revised patch. :-)
>
> Cheers,
>
> Daniel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] _gpg: Use explicit UIDs for state = public keys.
2018-05-29 14:11 ` Doron Behar
@ 2018-05-30 19:04 ` Daniel Shahaf
2018-06-02 15:08 ` Doron Behar
0 siblings, 1 reply; 6+ messages in thread
From: Daniel Shahaf @ 2018-05-30 19:04 UTC (permalink / raw)
To: Doron Behar; +Cc: zsh-workers
Doron Behar wrote on Tue, May 29, 2018 at 17:11:23 +0300:
> I've successfully managed to solve address all the comments you've had
> for my patch. Yet, I'm having trouble with `_call_program`. The line I'm
> trying to put is this:
>
> local public_keys=(${(@s.:.)$(_call_program public-keys $words[1] $needed --list-public-keys --list-options no-show-photos --with-colons)})
>
> I debugged this a little bit and the variable `$public_keys` is empty
> but if I remove the words: `_call_program public-keys` it works good.
>
> I've had a glimpse at `htop` when this function was called when I tested
> the completion function and I saw these commands running there:
>
> pkgfile -b -v -- --list-public-keys
> pkgfile -b -v -- --list-options
> pkgfile -b -v -- no-show-photos
> pkgfile -b -v -- --with-colons
>
> What is _call_program supposed to be doing?
>
_call_program (doc'd in zshcompsys(1)) is supposed to run the command
"$words[1] $needed --list-public-keys --list-options no-show-photos
--with-colons". The purpose of using _call_program is to allow the user to
override the command to use by setting a zstyle.
I can't explain the multiple commands you glimpsed in htop.
Try changing «$words[1] $needed» to «${(q)words[1]} ${(q)needed}». That adds
an additional level of quoting, since one of the differences between «foo bar
baz…» and «_call_program x foo bar baz…» is that in the latter case, the «foo
bar baz» are eval'd. I'm not sure whether that would fix the failure mode
you're seeing, but it's a correct change regardless.
I would recommend debugging that line inside out, e.g., start by ensuring the
_call_program by itself gets the expected arguments and produced the expected
values on its stdout. Enabling tracing («set -x» or «functions -T _call_program»)
might help.
> Besides that, the revised patch is ready. I've also included similar
> improvements for the other states - `secret-keys` and `public-keyids`.
I look forward to the revised patch. However, I'll be busy in the next few
days so I hope another developer would beat me to reviewing/applying it ;-).
Cheers,
Daniel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] _gpg: Use explicit UIDs for state = public keys.
2018-05-30 19:04 ` Daniel Shahaf
@ 2018-06-02 15:08 ` Doron Behar
0 siblings, 0 replies; 6+ messages in thread
From: Doron Behar @ 2018-06-02 15:08 UTC (permalink / raw)
To: zsh-workers
That didn't work.. I debugged this a little bit and I discovered that
setting IFS=$'\n' is what makes this problem. Perhaps should it be
reported as a bug of `_call_program`?
On Wed, May 30, 2018 at 07:04:14PM +0000, Daniel Shahaf wrote:
> Doron Behar wrote on Tue, May 29, 2018 at 17:11:23 +0300:
> > I've successfully managed to solve address all the comments you've had
> > for my patch. Yet, I'm having trouble with `_call_program`. The line I'm
> > trying to put is this:
> >
> > local public_keys=(${(@s.:.)$(_call_program public-keys $words[1] $needed --list-public-keys --list-options no-show-photos --with-colons)})
> >
> > I debugged this a little bit and the variable `$public_keys` is empty
> > but if I remove the words: `_call_program public-keys` it works good.
> >
> > I've had a glimpse at `htop` when this function was called when I tested
> > the completion function and I saw these commands running there:
> >
> > pkgfile -b -v -- --list-public-keys
> > pkgfile -b -v -- --list-options
> > pkgfile -b -v -- no-show-photos
> > pkgfile -b -v -- --with-colons
> >
> > What is _call_program supposed to be doing?
> >
>
> _call_program (doc'd in zshcompsys(1)) is supposed to run the command
> "$words[1] $needed --list-public-keys --list-options no-show-photos
> --with-colons". The purpose of using _call_program is to allow the user to
> override the command to use by setting a zstyle.
>
> I can't explain the multiple commands you glimpsed in htop.
>
> Try changing «$words[1] $needed» to «${(q)words[1]} ${(q)needed}». That adds
> an additional level of quoting, since one of the differences between «foo bar
> baz…» and «_call_program x foo bar baz…» is that in the latter case, the «foo
> bar baz» are eval'd. I'm not sure whether that would fix the failure mode
> you're seeing, but it's a correct change regardless.
>
> I would recommend debugging that line inside out, e.g., start by ensuring the
> _call_program by itself gets the expected arguments and produced the expected
> values on its stdout. Enabling tracing («set -x» or «functions -T _call_program»)
> might help.
>
> > Besides that, the revised patch is ready. I've also included similar
> > improvements for the other states - `secret-keys` and `public-keyids`.
>
> I look forward to the revised patch. However, I'll be busy in the next few
> days so I hope another developer would beat me to reviewing/applying it ;-).
>
> Cheers,
>
> Daniel
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-06-02 15:08 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-26 15:16 [PATCH 0/1] *** Small improvement for gpg completion *** doron.behar
2018-05-26 15:16 ` [PATCH 1/1] _gpg: Use explicit UIDs for state = public keys doron.behar
2018-05-26 16:25 ` Daniel Shahaf
2018-05-29 14:11 ` Doron Behar
2018-05-30 19:04 ` Daniel Shahaf
2018-06-02 15:08 ` Doron Behar
Code repositories for project(s) associated with this public inbox
https://git.vuxu.org/mirror/zsh/
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).