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

[dana <dana@xxxxxxx>]

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)

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

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

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

i don't think you meant to double-quote ? here

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

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

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

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

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