From 1c95e05ffb45751b6c212d9ad9ca9752a741f06e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=3D=3FUTF-8=3Fq=3FVille=3D20Skytt=3DC3=3DA4=3F=3D?= Date: Wed, 24 Aug 2022 20:15:05 +0100 Subject: [PATCH] bash completion improvements style(bash-v2): out is not an array variable, do not refer to it as such Even though this to my surprise works, it doesn't accomplish anything but some confusion. Remove it. Merge https://github.com/spf13/cobra/pull/1681 --- perf(bash-v2): use backslash escape string expansion for tab Using a command substitution, i.e. a subshell, with `printf` is expensive for this purpose. For example `__*_format_comp_descriptions` is run once for each completion candidate; the expense adds up and shows when there are a lot of them. This shaves off roughly 25% of the times I receive for my test case in #1680 (~2 seconds before, ~1.5 after). Merge https://github.com/spf13/cobra/pull/1682 --- perf(bash-v2): standard completion optimizations Refactor to remove two loops over the entire list of candidates. Format descriptions only for completions that are actually going to be displayed, instead of for all candidates. Format descriptions inline in completions array, removing need for a command substitution/subshell and a printf escape per displayed completion. These changes shave off a second or so from my case at hand in #1680, it was roughly ~1.7s before, is now ~0.7s after. The diff is largish, but the bulk of it is in `__%[1]s_format_comp_descriptions` due to indentation changes, there aren't that many lines actually changed in it either. `git show -w` helps with the review. Caveat: I have not actually tested generating completions with this code. I worked on the improvements by tweaking an existing emitted completion shell file, then manually ported the changes to the template in Go code here. Hopefully I didn't introduce bugs while doing that. (The code compiles, and test suite fails just like it did before this change, no regressions noticed.) Merge https://github.com/spf13/cobra/pull/1683 --- perf(bash-v2): short-circuit descriptionless candidate lists If the list of candidates has no descriptions, short circuit all the description processing logic, basically just do a `compgen -W` for the whole list and be done with it. We could conceivably do some optimizations like this and more when generating the completions with `--no-descriptions` in Go code, by omitting some parts we know won't be needed, or doing some things differently. But doing it this way in bash, the improvements are available also to completions generated with descriptions enabled when they are invoked for completion cases that produce no descriptions. The result after this for descriptionless entries seems fast enough so it seems there's no immediate need to look into doing that. This alone pretty much eliminates all the slowness related to my specific use case in #1680, bringing it down to ~0.07s (yes, there _is_ a zero on the right of the period), so for the time being I'm not inclined to look into further optimizations at least for the code path where there are no descriptions. Related to #1683, but I suggest merging both as they affect different scenarios. Same caveat as in #1683 about testing and the way the change was made applies here. Merge https://github.com/spf13/cobra/pull/1686 --- perf(bash-v2): speed up filtering entries with descriptions Use simple prefix match instead of single word `compgen -W` command substitution for each candidate match. It's a bit late, but I can't think of a case where this replacement wouldn't be ok this late at night. Performancewise this improves the cases with descriptions quite a bit, with current https://github.com/marckhouzam/cobra-completion-testing/pull/15 on my box, the v2 numbers look like the following (I've stripped the v1 numbers from each list below). Before (current master, #1686 not merged yet): ``` Processing 1000 completions took 0.492 seconds which is less than the 1.0 seconds limit Processing 1000 completions took 0.600 seconds which is less than the 2.0 seconds limit Processing 1000 completions took 0.455 seconds which is less than the 1.0 seconds limit Processing 1000 completions took 0.578 seconds which is less than the 2.0 seconds limit Processing 1000 completions took 0.424 seconds which is less than the 1.0 seconds limit Processing 1000 completions took 0.570 seconds which is less than the 2.0 seconds limit Processing 1000 completions took 0.502 seconds which is less than the 1.0 seconds limit Processing 1000 completions took 0.585 seconds which is less than the 2.0 seconds limit ``` After (also without #1686): ``` Processing 1000 completions took 0.070 seconds which is less than the 1.0 seconds limit Processing 1000 completions took 0.165 seconds which is less than the 2.0 seconds limit Processing 1000 completions took 0.072 seconds which is less than the 1.0 seconds limit Processing 1000 completions took 0.181 seconds which is less than the 2.0 seconds limit Processing 1000 completions took 0.089 seconds which is less than the 1.0 seconds limit Processing 1000 completions took 0.254 seconds which is less than the 2.0 seconds limit Processing 1000 completions took 0.058 seconds which is less than the 1.0 seconds limit Processing 1000 completions took 0.122 seconds which is less than the 2.0 seconds limit ``` For curiosity, even though this improves the descriptionless case quite a bit too, #1686 is still relevant, with it applied on top of this one: ``` Processing 1000 completions took 0.036 seconds which is less than the 1.0 seconds limit Processing 1000 completions took 0.165 seconds which is less than the 2.0 seconds limit Processing 1000 completions took 0.047 seconds which is less than the 1.0 seconds limit Processing 1000 completions took 0.183 seconds which is less than the 2.0 seconds limit Processing 1000 completions took 0.051 seconds which is less than the 1.0 seconds limit Processing 1000 completions took 0.264 seconds which is less than the 2.0 seconds limit Processing 1000 completions took 0.036 seconds which is less than the 1.0 seconds limit Processing 1000 completions took 0.122 seconds which is less than the 2.0 seconds limit ``` Merge https://github.com/spf13/cobra/pull/1689 --- fix(bash-v2): skip empty completions when filtering descriptions `read` gives a last null value following a trailing newline. Regression from fb8031162c2ffab270774f13c6904bb04cbba5a7. We could drop the `\n` from the feeding `printf` to accomplish the same end result, but I'm planning to submit some related cleanups a bit later that wouldn't play well with that approach, so I went with explicit filtering here. Merge https://github.com/spf13/cobra/pull/1691 --- perf(bash-v2): speed up filtering menu-complete descriptions Similarly as fb8031162c2ffab270774f13c6904bb04cbba5a7 (+ the empty entry fix in https://github.com/spf13/cobra/pull/1691) did for the "regular", non-menu completion cases. I have no numbers on this, but I'm assuming the speedups seen elsewhere are here too, and didn't notice anything breaking in casual manual tests. Could be useful to add some perf numbers for the menu-complete code path too to [marckhouzam/cobra-completion-testing](https://github.com/marckhouzam/cobra-completion-testing/) (maybe as well as other tests, haven't checked if there are any). Merge https://github.com/spf13/cobra/pull/1692 --- perf(bash-v2): read directly to COMPREPLY on descriptionless short circuit Not that it'd really matter that much performancewise given the level we are at for this case, but this change makes the short circuit roughly twice as fast on my box as it was for the 1000 rounds done in marckhouzam/cobra-completion-testing. Perhaps more importantly, this makes the code arguably slightly cleaner. Before: ``` ... <= TIMING => no descriptions: 1000 completions took 0.039 seconds < 0.2 seconds limit ... <= TIMING => no descriptions: 1000 completions took 0.046 seconds < 0.2 seconds limit ... ``` After: ``` ... <= TIMING => no descriptions: 1000 completions took 0.022 seconds < 0.2 seconds limit ... <= TIMING => no descriptions: 1000 completions took 0.024 seconds < 0.2 seconds limit ... ``` Merge https://github.com/spf13/cobra/pull/1700 --- resources/bash_completion.sh.gotmpl | 133 +++++++++++++--------------- 1 file changed, 64 insertions(+), 69 deletions(-) diff --git a/resources/bash_completion.sh.gotmpl b/resources/bash_completion.sh.gotmpl index 4184ef9..372a60e 100644 --- a/resources/bash_completion.sh.gotmpl +++ b/resources/bash_completion.sh.gotmpl @@ -57,7 +57,7 @@ __{{ .CMDVarName }}_get_completion_results() { directive=0 fi __{{ .CMDVarName }}_debug "The completion directive is: ${directive}" - __{{ .CMDVarName }}_debug "The completions are: ${out[*]}" + __{{ .CMDVarName }}_debug "The completions are: ${out}" } __{{ .CMDVarName }}_process_completion_results() { @@ -96,7 +96,7 @@ __{{ .CMDVarName }}_process_completion_results() { # Do not use quotes around the $out variable or else newline # characters will be kept. - for filter in ${out[*]}; do + for filter in ${out}; do fullFilter+="$filter|" done @@ -108,7 +108,7 @@ __{{ .CMDVarName }}_process_completion_results() { # Use printf to strip any trailing newline local subdir - subdir=$(printf "%s" "${out[0]}") + subdir=$(printf "%s" "${out}") if [ -n "$subdir" ]; then __{{ .CMDVarName }}_debug "Listing directories in $subdir" pushd "$subdir" >/dev/null 2>&1 && _filedir -d && popd >/dev/null 2>&1 || return @@ -133,17 +133,16 @@ __{{ .CMDVarName }}_handle_completion_types() { # If the user requested inserting one completion at a time, or all # completions at once on the command-line we must remove the descriptions. # https://github.com/spf13/cobra/issues/1508 - local tab comp - tab=$(printf '\t') + local tab=$'\t' comp while IFS='' read -r comp; do + [[ -z $comp ]] && continue # Strip any description comp=${comp%%$tab*} # Only consider the completions that match - comp=$(compgen -W "$comp" -- "$cur") - if [ -n "$comp" ]; then + if [[ $comp == "$cur"* ]]; then COMPREPLY+=("$comp") fi - done < <(printf "%s\n" "${out[@]}") + done < <(printf "%s\n" "${out}") ;; *) @@ -154,44 +153,37 @@ __{{ .CMDVarName }}_handle_completion_types() { } __{{ .CMDVarName }}_handle_standard_completion_case() { - local tab comp - tab=$(printf '\t') + local tab=$'\t' comp + + # Short circuit to optimize if we don't have descriptions + if [[ $out != *$tab* ]]; then + IFS=$'\n' read -ra COMPREPLY -d '' < <(IFS=$'\n' compgen -W "$out" -- "$cur") + return 0 + fi local longest=0 + local compline # Look for the longest completion so that we can format things nicely - while IFS='' read -r comp; do + while IFS='' read -r compline; do + [[ -z $compline ]] && continue # Strip any description before checking the length - comp=${comp%%$tab*} + comp=${compline%%$tab*} # Only consider the completions that match - comp=$(compgen -W "$comp" -- "$cur") + [[ $comp == "$cur"* ]] || continue + COMPREPLY+=("$compline") if ((${#comp}>longest)); then longest=${#comp} fi - done < <(printf "%s\n" "${out[@]}") - - local completions=() - while IFS='' read -r comp; do - if [ -z "$comp" ]; then - continue - fi - - __{{ .CMDVarName }}_debug "Original comp: $comp" - comp="$(__{{ .CMDVarName }}_format_comp_descriptions "$comp" "$longest")" - __{{ .CMDVarName }}_debug "Final comp: $comp" - completions+=("$comp") - done < <(printf "%s\n" "${out[@]}") - - while IFS='' read -r comp; do - COMPREPLY+=("$comp") - done < <(compgen -W "${completions[*]}" -- "$cur") + done < <(printf "%s\n" "${out}") # If there is a single completion left, remove the description text if [ ${#COMPREPLY[*]} -eq 1 ]; then __{{ .CMDVarName }}_debug "COMPREPLY[0]: ${COMPREPLY[0]}" - comp="${COMPREPLY[0]%% *}" + comp="${COMPREPLY[0]%%$tab*}" __{{ .CMDVarName }}_debug "Removed description from single completion, which is now: ${comp}" - COMPREPLY=() - COMPREPLY+=("$comp") + COMPREPLY[0]=$comp + else # Format the descriptions + __%[1]s_format_comp_descriptions $longest fi } @@ -210,45 +202,48 @@ __{{ .CMDVarName }}_handle_special_char() __{{ .CMDVarName }}_format_comp_descriptions() { - local tab - tab=$(printf '\t') - local comp="$1" - local longest=$2 - - # Properly format the description string which follows a tab character if there is one - if [[ "$comp" == *$tab* ]]; then - desc=${comp#*$tab} - comp=${comp%%$tab*} - - # $COLUMNS stores the current shell width. - # Remove an extra 4 because we add 2 spaces and 2 parentheses. - maxdesclength=$(( COLUMNS - longest - 4 )) - - # Make sure we can fit a description of at least 8 characters - # if we are to align the descriptions. - if [[ $maxdesclength -gt 8 ]]; then - # Add the proper number of spaces to align the descriptions - for ((i = ${#comp} ; i < longest ; i++)); do - comp+=" " - done - else - # Don't pad the descriptions so we can fit more text after the completion - maxdesclength=$(( COLUMNS - ${#comp} - 4 )) - fi + local tab=$'\t' + local comp desc maxdesclength + local longest=$1 + + local i ci + for ci in ${!COMPREPLY[*]}; do + comp=${COMPREPLY[ci]} + # Properly format the description string which follows a tab character if there is one + if [[ "$comp" == *$tab* ]]; then + __%[1]s_debug "Original comp: $comp" + desc=${comp#*$tab} + comp=${comp%%%%$tab*} + + # $COLUMNS stores the current shell width. + # Remove an extra 4 because we add 2 spaces and 2 parentheses. + maxdesclength=$(( COLUMNS - longest - 4 )) + + # Make sure we can fit a description of at least 8 characters + # if we are to align the descriptions. + if [[ $maxdesclength -gt 8 ]]; then + # Add the proper number of spaces to align the descriptions + for ((i = ${#comp} ; i < longest ; i++)); do + comp+=" " + done + else + # Don't pad the descriptions so we can fit more text after the completion + maxdesclength=$(( COLUMNS - ${#comp} - 4 )) + fi - # If there is enough space for any description text, - # truncate the descriptions that are too long for the shell width - if [ $maxdesclength -gt 0 ]; then - if [ ${#desc} -gt $maxdesclength ]; then - desc=${desc:0:$(( maxdesclength - 1 ))} - desc+="…" + # If there is enough space for any description text, + # truncate the descriptions that are too long for the shell width + if [ $maxdesclength -gt 0 ]; then + if [ ${#desc} -gt $maxdesclength ]; then + desc=${desc:0:$(( maxdesclength - 1 ))} + desc+="…" + fi + comp+=" ($desc)" fi - comp+=" ($desc)" + COMPREPLY[ci]=$comp + __%[1]s_debug "Final comp: $comp" fi - fi - - # Must use printf to escape all special characters - printf "%q" "${comp}" + done } __start_{{ .CMDVarName }}()