Re: [PATCH v2 2/2] sparse-checkout: custom tab completion

[Lessley Dennington <lessleydennington@gmail.com>]

On 1/4/22 2:42 PM, Elijah Newren wrote:
> On Tue, Jan 4, 2022 at 11:41 AM Lessley Dennington
> <lessleydennington@gmail.com> wrote:
>>
>>
>>
>> On 12/31/21 4:52 PM, Elijah Newren wrote:
>>> On Fri, Dec 31, 2021 at 2:33 AM Lessley Dennington via GitGitGadget
>>> <gitgitgadget@gmail.com> wrote:
>>>>
>>>> From: Lessley Dennington <lessleydennington@gmail.com>
>>>>
>>>> Fix custom tab completion for sparse-checkout command. This will ensure:
>>>>
>>>> 1. The full list of subcommands is provided when users enter git
>>>> sparse-checkout <TAB>.
>>>> 2. The --help option is tab-completable.
>>>> 3. Subcommand options are tab-completable.
>>>> 4. A list of directories (but not files) is provided when users enter git
>>>> sparse-checkout add <TAB> or git sparse-checkout set <TAB>.
>>>>
>>>> Failing tests that were added in the previous commit to verify these
>>>> scenarios are now passing with these updates.
>>>>
>>>> Signed-off-by: Lessley Dennington <lessleydennington@gmail.com>
>>>> ---
>>>>    contrib/completion/git-completion.bash | 38 ++++++++++++++++++--------
>>>>    t/t9902-completion.sh                  |  8 +++---
>>>>    2 files changed, 30 insertions(+), 16 deletions(-)
>>>>
>>>> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
>>>> index c82ccaebcc7..7de997ee64e 100644
>>>> --- a/contrib/completion/git-completion.bash
>>>> +++ b/contrib/completion/git-completion.bash
>>>> @@ -2986,24 +2986,38 @@ _git_show_branch ()
>>>>           __git_complete_revlist
>>>>    }
>>>>
>>>> +__git_sparse_checkout_subcommand_opts="--cone --no-cone --sparse-index --no-sparse-index"
>>>> +
>>>>    _git_sparse_checkout ()
>>>>    {
>>>> -       local subcommands="list init set disable"
>>>> +       local subcommands="list init set disable add reapply"
>>>>           local subcommand="$(__git_find_on_cmdline "$subcommands")"
>>>> +
>>>>           if [ -z "$subcommand" ]; then
>>>> -               __gitcomp "$subcommands"
>>>> -               return
>>>> +               case "$cur" in
>>>> +                       --*)
>>>> +                               __gitcomp "--help"
>>>> +                               ;;
>>>> +                       *)
>>>> +                               __gitcomp "$subcommands"
>>>> +                               ;;
>>>> +               esac
>>>>           fi
>>>>
>>>> -       case "$subcommand,$cur" in
>>>> -       init,--*)
>>>> -               __gitcomp "--cone"
>>>> -               ;;
>>>> -       set,--*)
>>>> -               __gitcomp "--stdin"
>>>> -               ;;
>>>> -       *)
>>>> -               ;;
>>>> +       case "$prev" in
>>>
>>> Shouldn't this be "$subcommand" rather than "$prev"?  I think with
>>> prev, it will only correctly complete the first path after "set",
>>> "add", etc.
>>>
>> Good catch, thank you! Fixing in v3.
>>>> +               set)
>>>> +                       __gitcomp "$__git_sparse_checkout_subcommand_opts --stdin"
>>>> +                       __gitcomp "$(git ls-tree -d -r HEAD --name-only)"
>>>> +                       ;;
>>>> +               add)
>>>> +                       __gitcomp "--stdin"
>>>> +                       __gitcomp "$(git ls-tree -d -r HEAD --name-only)"
>>>
>>> I was going to make a simple suggestion for making it just complete
>>> one additional level at a time and leaving out the -r, and then tried
>>> it out and found out it wasn't simple.  I got something working,
>>> eventually, but it's slightly ugly...so it probably belongs in a
>>> separate patch anyway.  If you're curious, it's basically replacing
>>> the second __gitcomp... call for each of set and add with
>>> `__gitcomp_directories`, after first defining:
>>>
>>> __gitcomp_directories ()
>>> {
>>>       local _tmp_dir _tmp_completions
>>>
>>>       # Get the directory of the current token; this differs from dirname
>>>       # in that it keeps up to the final trailing slash.  If no slash found
>>>       # that's fine too.
>>>       [[ "$cur" =~ .*/ ]]
>>>       _tmp_dir=$BASH_REMATCH
>>>
>>>       # Find possible directory completions, adding trailing '/' characters
>>>       _tmp_completions="$(git ls-tree -d --name-only HEAD $_tmp_dir |
>>>           sed -e s%$%/%)"
>>>
>>>       if [[ -n "$_tmp_completions" ]]; then
>>>           # There were some directory completions, so find ones that
>>>           # start with "$cur", the current token, and put those in COMPREPLY
>>>           local i=0 c IFS=$' \t\n'
>>>           for c in $_tmp_completions; do
>>>               if [[ $c == "$cur"* ]]; then
>>>                   COMPREPLY+=("$c")
>>>               fi
>>>           done
>>>       elif [[ "$cur" =~ /$ ]]; then
>>>           # No possible further completions any deeper, so assume we're at
>>>           # a leaf directory and just consider it complete
>>>           __gitcomp_direct_append "$cur "
>>>       fi
>>> }
>>>
>>> But I don't think that needs to be part of this series; I can submit
>>> it later and hopefully get a completion expert to point out
>>> better/cleaner ways of what I've done above.
>>>
>> I'm admittedly curious about what made this so difficult. I removed the
>> '-r' and updated my tests to expect only directories at one level, and
>> they passed. But I imagine I'm being too simplistic.
> 
> I've forgotten some details since last Saturday, but I think the
> problem was that doing that would only ever complete toplevel
> directories; after completing those you could keep tabbing to get a
> deeper directory.  First, let's get a comparison point; ignoring
> sparse-checkout, I can do:
> 
>      cd $GIT_CLONE
>      cd cont<TAB>b<TAB><TAB>
> 
> and the ls line will replace those <TAB>s so that I see
> 
>      ls contrib/buildsystems/Generators
> 
> Now, if we just removed the '-r' from your git-completion.bash
> changes, and then typed
> 
>      git sparse-checkout set cont<TAB>b<TAB><TAB>
> 
> Then you'd see
> 
>      git sparse-checkout set contrib
> 
> and see 'bin-wrappers', 'block-sha1', and 'builtin' as potential
> completions, but not as subdirs of contrib but instead sibling dirs to
> contrib.  That completion rule wouldn't let you look within contrib/.
> My more complicated rule had to avoid calling __gitcomp to avoid
> adding spaces so that completions could continue (but should add them
> if we have tabbed all the way down and there are no more
> subdirectories), had to add trailing '/' characters so that we know
> when we have the full directory name to pass along to ls-tree, and
> then had to manually do the work that __gitcomp would manually do with
> making sure to only provide completions relevant to what has been
> typed so far.
Ah, I see. Thank you so much for the thorough explanation. I know you said
this series could go through without that update, but I feel like it
should probably be added. Don't want to start off with the wrong behavior.