| Message ID | cecf501e07645b7408dc75f276d477b9b712ab17.1640892413.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | sparse checkout: custom bash completion updates | expand |
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. > + 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. > + ;; > + init|reapply) > + __gitcomp "$__git_sparse_checkout_subcommand_opts" > + ;; > + *) > + ;; > esac > } > > diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh > index 51d0f2d93a1..4dc93ee0595 100755 > --- a/t/t9902-completion.sh > +++ b/t/t9902-completion.sh > @@ -1447,7 +1447,7 @@ test_expect_success 'git checkout - with --detach, complete only references' ' > EOF > ' > > -test_expect_failure 'sparse-checkout completes subcommands' ' > +test_expect_success 'sparse-checkout completes subcommands' ' > test_completion "git sparse-checkout " <<-\EOF > list Z > init Z > @@ -1458,13 +1458,13 @@ test_expect_failure 'sparse-checkout completes subcommands' ' > EOF > ' > > -test_expect_failure 'sparse-checkout completes options' ' > +test_expect_success 'sparse-checkout completes options' ' > test_completion "git sparse-checkout --" <<-\EOF > --help Z > EOF > ' > > -test_expect_failure 'sparse-checkout completes subcommand options' ' > +test_expect_success 'sparse-checkout completes subcommand options' ' > test_completion "git sparse-checkout init --" <<-\EOF && > --cone Z > --no-cone Z > @@ -1492,7 +1492,7 @@ test_expect_failure 'sparse-checkout completes subcommand options' ' > EOF > ' > > -test_expect_failure 'sparse-checkout completes directory names' ' > +test_expect_success 'sparse-checkout completes directory names' ' > # set up sparse-checkout repo > git init sparse-checkout && > ( > -- > gitgitgadget Patch looks good to me, other than the $prev vs. $subcommand thing.
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. >> + ;; >> + init|reapply) >> + __gitcomp "$__git_sparse_checkout_subcommand_opts" >> + ;; >> + *) >> + ;; >> esac >> } >> >> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh >> index 51d0f2d93a1..4dc93ee0595 100755 >> --- a/t/t9902-completion.sh >> +++ b/t/t9902-completion.sh >> @@ -1447,7 +1447,7 @@ test_expect_success 'git checkout - with --detach, complete only references' ' >> EOF >> ' >> >> -test_expect_failure 'sparse-checkout completes subcommands' ' >> +test_expect_success 'sparse-checkout completes subcommands' ' >> test_completion "git sparse-checkout " <<-\EOF >> list Z >> init Z >> @@ -1458,13 +1458,13 @@ test_expect_failure 'sparse-checkout completes subcommands' ' >> EOF >> ' >> >> -test_expect_failure 'sparse-checkout completes options' ' >> +test_expect_success 'sparse-checkout completes options' ' >> test_completion "git sparse-checkout --" <<-\EOF >> --help Z >> EOF >> ' >> >> -test_expect_failure 'sparse-checkout completes subcommand options' ' >> +test_expect_success 'sparse-checkout completes subcommand options' ' >> test_completion "git sparse-checkout init --" <<-\EOF && >> --cone Z >> --no-cone Z >> @@ -1492,7 +1492,7 @@ test_expect_failure 'sparse-checkout completes subcommand options' ' >> EOF >> ' >> >> -test_expect_failure 'sparse-checkout completes directory names' ' >> +test_expect_success 'sparse-checkout completes directory names' ' >> # set up sparse-checkout repo >> git init sparse-checkout && >> ( >> -- >> gitgitgadget > > Patch looks good to me, other than the $prev vs. $subcommand thing. Thanks!
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.
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.
On Wed, Jan 5, 2022 at 12:20 PM Lessley Dennington <lessleydennington@gmail.com> wrote: > > 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. The wrong behavior only occurs if you drop the `-r` from your patch. If you keep the `-r`, as in your patch submission, you get the right behavior, it just might be a bit slow. The only reason I investigated dropping the `-r` and then following up with all the extra workarounds needed when the `-r` was dropped was because some repositories may be big enough that immediately recursing trees down to the lowest depths may be expensive. For example, in linux.git (very small compared to the Microsoft repos): `time git ls-tree -rd HEAD >/dev/null` was 0.785s (cold cache; a mere 0.084s on second run). If repositories get much bigger than that, folks might not like the slowness of using `-r`. But I think what you have is fine as a first cut. That said, if you want to add a patch to this series that switches your straightforward implementation to my much more complex but faster alternative, that's fine too.
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 + 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)" + ;; + init|reapply) + __gitcomp "$__git_sparse_checkout_subcommand_opts" + ;; + *) + ;; esac } diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh index 51d0f2d93a1..4dc93ee0595 100755 --- a/t/t9902-completion.sh +++ b/t/t9902-completion.sh @@ -1447,7 +1447,7 @@ test_expect_success 'git checkout - with --detach, complete only references' ' EOF ' -test_expect_failure 'sparse-checkout completes subcommands' ' +test_expect_success 'sparse-checkout completes subcommands' ' test_completion "git sparse-checkout " <<-\EOF list Z init Z @@ -1458,13 +1458,13 @@ test_expect_failure 'sparse-checkout completes subcommands' ' EOF ' -test_expect_failure 'sparse-checkout completes options' ' +test_expect_success 'sparse-checkout completes options' ' test_completion "git sparse-checkout --" <<-\EOF --help Z EOF ' -test_expect_failure 'sparse-checkout completes subcommand options' ' +test_expect_success 'sparse-checkout completes subcommand options' ' test_completion "git sparse-checkout init --" <<-\EOF && --cone Z --no-cone Z @@ -1492,7 +1492,7 @@ test_expect_failure 'sparse-checkout completes subcommand options' ' EOF ' -test_expect_failure 'sparse-checkout completes directory names' ' +test_expect_success 'sparse-checkout completes directory names' ' # set up sparse-checkout repo git init sparse-checkout && (