Zsh Mailing List Archive
Messages sorted by: Reverse Date, Date, Thread, Author

Re: [PATCH] (2nd try) new function passphrase generator

On 9/3/2025 02:20, dana wrote:

On Tue 2 Sep 2025, at 21:53, Clinton Bunch wrote:

Taking Mikael's feedback into consideration, I've reworked the function
and renamed it zgenpassphrase.

additional feedback, ignore anything you think is too nit-picky

(your mail client seems to insert hard line breaks at 72 columns, and it
converts multiple spaces into alternating spaces and nbsps (0xa0), so
the diff is broken and i was too lazy to fix it so i could actually
test the functionality. you should probably change that, or re-send as
an attachment)

I'll try to send the next iteration as both.



+# Zsh completion function for the 'zgenpassphrase' command.

still a bunch of unnecessary comments like this imo


+local -a context

you don't need any of these parameters, since the function doesn't use
those features


+_arguments -C \

you don't need the -C either. i would add -s -S (to support option
stacking and '--' as parsing terminator respectively) though


+  '(-h --help)'{-h,--help}'[Display help message]' \

don't use sentence case for completion descriptions. also it's common
for --help to be exclusive with all other arguments, e.g.
`(- *){-h,--help}`


+  '(-n --num-words)'{-n+,--num-words=}'[Specify the number of words
(default: 6,min 2)]:number of words:' \

usually you don't put defaults and stuff in the option description. you
can put them in the argument one. for numbers there is a helper function
that formats everything the conventional way, e.g.

   '...: :_numbers -u words -l2 -d6'
   # or, if you like,
   '...: :_numbers -l2 -d6 "number of words"'


+  '(-s --separator)'{-s+,--separator=}'[Use SEP as the word separator
(default: " ")]:separator string:' \

meta-variable names like SEP are meaningless in completion descriptions,
use wording like 'use specified string as word separator'. and again you
can put the default in the argument description using the normal
convention e.g. `separator string [ ]`


+  '(-d --digits)'{-d+,--digits}'[Include random digits (optional
COUNT)]:number of digits:' \

as mentioned below, the parsing for the optional argument here seems
broken. after fixing that you should use :: instead of :

also, if you switch to `zparseopts -G` you should change this to -d- and
--digits=-. if you keep your current parsing code i think you will
probably still want --digits=

you can use _numbers here too


+# diceware - Generates a diceware style passphrase

forgot to update the name


+   -p SEARCH_PATH, --add-search-path SEARCH_PATH
+   -P SEARCH_PATH, --override-path SEARCH_PATH

the naming --add-search-path vs --override-path feels inconsistent.
maybe --prepend-path would be better for the former


+zparseopts -E -D -M -A parsed_opts \

you don't need -E here, or -D either really. but you should use -F to
ensure that it actually fails when an invalid option is given

if you don't mind that the function will only work in zsh 5.10+, you can
use the new -G option to get proper gnu-style parsing
Didn't know about -G.  I'll read up.  Since it relies on zsh/random a 5.10 requirement is not a problem. 


+  d:='-digits' -digits: \

it seems like you intended for this option's argument to be optional,
but that's not how you've defined it (and the code below that tries to
handle it that way looks broken to me). use :: instead of :
last minute change.  I changed my mind about it being optional.  (It seemed to confuse completion, so I decided the use case for optional wasn't worth it.) 


+  h='-help' "'?'"='-help' -help \

i don't think you meant to double-quote ? here
I had problems getting it to accept ? as an option, though it may have been something else that was causing the problem. 


+# Process parsed options

this whole post-processing step is kind of weird/superfluous. i guess
you're doing it that way to handle the gnu syntax? if you use -G as
suggested above you can get rid of this and just use the results
directly like

   (( $+parsed_opts[--capitalize] )) && capitalize=1
   (( $+parsed_opts[--separator] )) && separator=$parsed_opts[--separator]
   (( $+parsed_opts[--digits] )) && digits=${parsed_opts[--digits]:-1}

or maybe

   capitalize=${parsed_opts[--capitalize]+1}
   separator=${parsed_opts[--separator]:- }
   digits=${${parsed_opts[--digits]-0}:-1}

etc


+      capitalize=1

it looks like you lost the code to actually do the capitalisation


+      # Check if an argument was provided for -N/--numbers
Previous name for -d .  This is why it's good to have other people read your code. :) 
there's no such option. superfluous comment either way


+    --help)
+      print -u2 $usage
+      return 0
+      ;;

probably don't need -u2 here. also probably a good idea to use -r and --
when printing any non-literal value

The function may be used in $(...) so -u2 is probably still a good idea



+  for p in "${(@e)wordlist_search_paths}"; do

feels weird to do expansion here, especially since it's not documented
that it works that way
Used the same variable in $usage and didn't want $ZDOTDIR expanded in the usage message I suppose I could use another variable and expand it earlier, but this was the only other place it was used. 


+if [[ -n "$separator" && "$separator" =~ [[:cntrl:]] ]]; then

first condition is superfluous


+    local -a tmp_sed_parts=(${^${(n)line_numbers}}p)

wrong indent level


+       raw_words=( ${(f)"$(sed -n "$sed_command" "$wordlist_path")"} )

wrong indent again. and probably should add -- to the sed command to
protect $wordlist_path on gnu systems


+    raw_words=() # No words to fetch if num_words was 0

superfluous assignment


+  passphrase_words+=( "${${(Az)word}[-1]}" )

you might want to lower-case the word here, and then apply your
capitalisation after if applicable. other pass-phrase generators i've
used seem to work this way (proper nouns aren't capitalised by default)


+    integer paste_position=$(( zrand_int(2) ))

superfluous assignment


+
passphrase_words[target_word_index]="${random_digit}${passphrase_words[target_word_index]}"

is indent messed up here too or is it just your mail client
Probably Thunderbird.  It really likes HTML mail even when it's supposed to detect plain text only. 


+local final_passphrase="${(pj:$separator:)passphrase_words}"

superfluous assignment

also there is a lot of unnecessary quoting, [@], ${...}, =(), etc. but
it doesn't hurt anything obv

dana


Thanks for the feedback.
Messages sorted by: Reverse Date, Date, Thread, Author