On 1/4/22 2:42 PM, Elijah Newren wrote:

On Tue, Jan 4, 2022 at 11:41 AM Lessley Dennington
[off-list ref] wrote:

quoted

On 12/31/21 4:52 PM, Elijah Newren wrote:

quoted

On Fri, Dec 31, 2021 at 2:33 AM Lessley Dennington via GitGitGadget
[off-list ref] wrote:

quoted

From: Lessley Dennington <redacted>

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 <redacted>
---
   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.

quoted

quoted

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