* [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).