Re: [PATCH v5 1/3] completion: address sparse-checkout issues

[Lessley Dennington <lessleydennington@gmail.com>]

On 2/3/22 3:52 PM, Elijah Newren wrote:
> On Thu, Feb 3, 2022 at 12:44 PM Lessley Dennington via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>>
>> From: Lessley Dennington <lessleydennington@gmail.com>
>>
>> Correct multiple issues with tab completion of the git sparse-checkout
>> command. These issues were:
>>
>> 1. git sparse-checkout <TAB> previously resulted in an incomplete list of
>> subcommands (it was missing reapply and add).
>> 2. Subcommand options were not tab-completable.
>> 3. git sparse-checkout set <TAB> and git sparse-checkout add <TAB> showed
>> both file names and directory names. While this may be a less surprising
>> behavior for non-cone mode, cone mode sparse checkouts should complete
>> only directory names.
>>
>> Note that while the new strategy of just using git ls-tree to complete on
>> directory names is simple and a step in the right direction, it does have
>> some caveats. These are:
>>
>> 1. Likelihood of poor performance in large monorepos (as a result of
>> recursively completing directory names).
>> 2. Inability to handle paths containing unusual characters.
>>
>> These caveats will be fixed by subsequent commits in this series.
>>
>> Signed-off-by: Lessley Dennington <lessleydennington@gmail.com>
>> ---
>>   contrib/completion/git-completion.bash | 16 ++---
>>   t/t9902-completion.sh                  | 83 ++++++++++++++++++++++++++
>>   2 files changed, 91 insertions(+), 8 deletions(-)
>>
>> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
>> index c82ccaebcc7..2976f63747f 100644
>> --- a/contrib/completion/git-completion.bash
>> +++ b/contrib/completion/git-completion.bash
>> @@ -2988,7 +2988,7 @@ _git_show_branch ()
>>
>>   _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"
>> @@ -2996,14 +2996,14 @@ _git_sparse_checkout ()
>>          fi
>>
>>          case "$subcommand,$cur" in
>> -       init,--*)
>> -               __gitcomp "--cone"
>> -               ;;
>> -       set,--*)
>> -               __gitcomp "--stdin"
>> -               ;;
>> -       *)
>> +       *,--*)
>> +               __gitcomp_builtin sparse-checkout_$subcommand "" "--"
>>                  ;;
>> +       set,*|add,*)
>> +               if [ $(__git config core.sparseCheckoutCone) ] ||
> 
> Shouldn't this be
> 
>                   if [ "$(__git config core.sparseCheckoutCone)" == "true" ] ||
> 
> otherwise I think you'd trigger when it's manually set to false.
> 
>> +               [ "$(__git_find_on_cmdline --cone)" ]; then
> 
> This might read better as
> 
>                  [ -n "$(__git_find_on_cmdline --cone)" ]; then
> 
> because otherwise you are relying on
> 
>       [ "--cone" ]
> 
> to evaluate as true which just seems slightly weird.
> 
Yup, this was causing some problems. Odd that the failure only cropped up
when merged with 'seen' but I have a fix prepared and will submit as soon
as GGG CI completes.
>> +                       __gitcomp "$(git ls-tree -d -r HEAD --name-only)"
>> +               fi
>>          esac
>>   }
>>
>> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
>> index 518203fbe07..f6eeb9aa035 100755
>> --- a/t/t9902-completion.sh
>> +++ b/t/t9902-completion.sh
>> @@ -1447,6 +1447,89 @@ test_expect_success 'git checkout - with --detach, complete only references' '
>>          EOF
>>   '
>>
>> +test_expect_success 'setup sparse-checkout tests' '
>> +       # set up sparse-checkout repo
>> +       git init sparse-checkout &&
>> +       (
>> +               cd sparse-checkout &&
>> +               mkdir -p folder1/0/1 folder2/0 folder3 &&
>> +               touch folder1/0/1/t.txt &&
>> +               touch folder2/0/t.txt &&
>> +               touch folder3/t.txt &&
>> +               git add . &&
>> +               git commit -am "Initial commit"
>> +       )
>> +'
>> +
>> +test_expect_success 'sparse-checkout completes subcommands' '
>> +       test_completion "git sparse-checkout " <<-\EOF
>> +       list Z
>> +       init Z
>> +       set Z
>> +       add Z
>> +       reapply Z
>> +       disable Z
>> +       EOF
>> +'
>> +
>> +test_expect_success 'cone mode sparse-checkout completes directory names' '
>> +       # initialize sparse-checkout definitions
>> +       git -C sparse-checkout sparse-checkout set --cone folder1/0 folder3 &&
>> +
>> +       # test tab completion
>> +       (
>> +               cd sparse-checkout &&
>> +               test_completion "git sparse-checkout set f" <<-\EOF
>> +               folder1 Z
>> +               folder1/0 Z
>> +               folder1/0/1 Z
>> +               folder2 Z
>> +               folder2/0 Z
>> +               folder3 Z
>> +               EOF
>> +       ) &&
>> +
>> +       (
>> +               cd sparse-checkout/folder1 &&
>> +               test_completion "git sparse-checkout add " <<-\EOF
>> +               ./ Z
>> +               0 Z
>> +               0/1 Z
>> +               EOF
>> +       )
>> +'
>> +
>> +test_expect_success 'non-cone mode sparse-checkout uses bash completion' '
>> +       # reset sparse-checkout repo to non-cone mode
>> +       git -C sparse-checkout sparse-checkout disable &&
>> +       git -C sparse-checkout sparse-checkout set --no-cone &&
>> +
>> +       (
>> +               cd sparse-checkout &&
>> +               # expected to be empty since we have not configured
>> +               # custom completion for non-cone mode
>> +               test_completion "git sparse-checkout set f" <<-\EOF
>> +
>> +               EOF
>> +       )
>> +'
>> +
>> +test_expect_success 'git sparse-checkout set --cone completes directory names' '
>> +       git -C sparse-checkout sparse-checkout disable &&
>> +
>> +       (
>> +               cd sparse-checkout &&
>> +               test_completion "git sparse-checkout set --cone f" <<-\EOF
>> +               folder1 Z
>> +               folder1/0 Z
>> +               folder1/0/1 Z
>> +               folder2 Z
>> +               folder2/0 Z
>> +               folder3 Z
>> +               EOF
>> +       )
>> +'
>> +
>>   test_expect_success 'git switch - with -d, complete all references' '
>>          test_completion "git switch -d " <<-\EOF
>>          HEAD Z
> 
> The rest looks good to me.