sparse checkout: custom bash completion updates

From: Lessley Dennington <[email protected]>

Add tests for missing/incorrect components of custom tab completion for the sparse-checkout command. These tests specifically highlight the following:

1. git sparse-checkout <TAB> results in an incomplete list of subcommands (it is missing reapply and add).
2. git sparse-checkout --<TAB> does not complete the help option.
3. Options for subcommands are not tab-completable.
4. git sparse-checkout set <TAB> and git sparse-checkout add <TAB> show both file names and directory names.

t/t9902-completion.sh +11 -0

+	EOF
+
+	test_completion "git sparse-checkout add --" <<-\EOF
+	--stdin Z
+	EOF
+'
 
+test_expect_failure 'sparse-checkout completes directory names' '
+	# set up sparse-checkout repo
+	git init sparse-checkout &&
+	(
+		cd sparse-checkout &&

t/t9902-completion.sh test_expect_success 'git checkout - with --detach, complete only references' ' · line 1472

Quoted 9 lines

t/t9902-completion.sh +9 -0

+test_expect_failure 'sparse-checkout completes subcommand options' '
+	test_completion "git sparse-checkout init --" <<-\EOF &&
+	--cone Z
+	--sparse-index Z
+	--no-sparse-index Z
+	EOF
+
+	test_completion "git sparse-checkout set --" <<-\EOF &&
+	--stdin Z

In en/sparse-checkout-set, the 'set' subcommand learns the options for init, including --cone, --sparse-index, and --no-sparse-index. I think technically, --no-cone is in there, too.

Further, the 'reapply' subcommand learns these options in that series and I see you do not include that subcommand in this test.

You might want to rebase onto en/sparse-checkout-set and adjust these tests for the new options (plus change the next patch that implements the completion).

t/t9902-completion.sh test_expect_success 'git checkout - with --detach, complete only references' ' · line 1494

Quoted 1 line

t/t9902-completion.sh +1 -0

+	git -C sparse-checkout config index.sparse false &&

I'm guessing that the implementation actually requires this, but I'll take a look in the next patch. It might just be slow to expand the full list of directories in the sparse index case.

t/t9902-completion.sh test_expect_success 'git checkout - with --detach, complete only references' ' · line 1507

Quoted 9 lines

t/t9902-completion.sh +9 -0

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

This tab-completion doing a full directory walk seems like it could be expensive for a large repository, but I suppose it is the only way to allow the following sequence:

	fol<tab> -> folder
	folder1/<tab> -> folder1/0
	folder1/0/<tab> -> folder1/0/1

(Hopefully that makes sense.)

It would be more efficient to only go one level deep at a time, but that might not be possible with the tab completion mechanisms.

Quoted 14 lines · Derrick Stolee

Quoted 9 lines

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

This tab-completion doing a full directory walk seems like it could be expensive for a large repository, but I suppose it is the only way to allow the following sequence:

Quoted 6 lines

    fol<tab> -> folder
    folder1/<tab> -> folder1/0
    folder1/0/<tab> -> folder1/0/1

(Hopefully that makes sense.)

Yes, it does.

Quoted 3 lines

It would be more efficient to only go one level deep at a time, but that might not be possible with the tab completion mechanisms.

When you say one level deep, do you mean that from the sparse-checkout directory, tab completion would only show the following?

    folder1
    folder2
    folder3

Quoted 29 lines · Lessley Dennington

Quoted 14 lines · Derrick Stolee

t/t9902-completion.sh test_expect_success 'git checkout - with --detach, complete only references' ' · line 1507

Quoted 9 lines

t/t9902-completion.sh +9 -0

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

This tab-completion doing a full directory walk seems like it could be expensive for a large repository, but I suppose it is the only way to allow the following sequence:

Quoted 6 lines

    fol<tab> -> folder
    folder1/<tab> -> folder1/0
    folder1/0/<tab> -> folder1/0/1

(Hopefully that makes sense.)

Yes, it does.

Quoted 3 lines

It would be more efficient to only go one level deep at a time, but that might not be possible with the tab completion mechanisms.

When you say one level deep, do you mean that from the sparse-checkout directory, tab completion would only show the following?

Quoted 3 lines

folder1
folder2
folder3

That's what I mean by one level deep at a time, but I also am not sure that that is the correct functionality. I would leave the full expansion as you have now as the design.

Quoted 31 lines · Derrick Stolee

Quoted 29 lines · Lessley Dennington

Quoted 14 lines · Derrick Stolee

t/t9902-completion.sh test_expect_success 'git checkout - with --detach, complete only references' ' · line 1507

Quoted 9 lines

t/t9902-completion.sh +9 -0

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

This tab-completion doing a full directory walk seems like it could be expensive for a large repository, but I suppose it is the only way to allow the following sequence:

Quoted 6 lines

     fol<tab> -> folder
     folder1/<tab> -> folder1/0
     folder1/0/<tab> -> folder1/0/1

(Hopefully that makes sense.)

Yes, it does.

Quoted 3 lines

It would be more efficient to only go one level deep at a time, but that might not be possible with the tab completion mechanisms.

When you say one level deep, do you mean that from the sparse-checkout directory, tab completion would only show the following?

Quoted 7 lines

Quoted 3 lines

folder1
folder2
folder3

That's what I mean by one level deep at a time, but I also am not sure that that is the correct functionality. I would leave the full expansion as you have now as the design.

I think one level at a time is better and is the optimal design. By way of comparison, note that if I do the following:

mkdir tmp
cd tmp
mkdir -p a/b/c/d
touch a/b/c/d/e
cd <TAB>

I do not see a/b/c/d as the completion, I only get "a/" as the completion. If I hit tab again, then I get "a/b/". Tab again, and I get "a/b/c/".

I don't think normal tab completion recursively checks directories, so it'd be better for us to not do so with git either. However, if it's hard to avoid automatically completing just a directory at a time, then I think a fair first cut is completing to full depths, but call it out as something we'd like to fix in the commit message.

From: Lessley Dennington <[email protected]>

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 <[email protected]>

---
 contrib/completion/git-completion.bash | 34 +++++++++++++++++---------
 t/t9902-completion.sh                  |  8 +++---
 2 files changed, 26 insertions(+), 16 deletions(-)

contrib/completion/git-completion.bash +4 -1

index 377d6c5494a..b8f1caece83 100644
@@ -2986,24 +2986,34 @@ _git_show_branch ()
 	__git_complete_revlist
 }
 
+__git_sparse_checkout_init_opts="--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

contrib/completion/git-completion.bash +0 -6

 	fi
 
-	case "$subcommand,$cur" in
-	init,--*)
-		__gitcomp "--cone"
-		;;
-	set,--*)
-		__gitcomp "--stdin"

Quoted 14 lines

contrib/completion/git-completion.bash +7 -3

+__git_sparse_checkout_init_opts="--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"

contrib/completion/git-completion.bash _git_show_branch () · line 3004

Quoted 5 lines

contrib/completion/git-completion.bash +5 -0

+				;;
+			*)
+				__gitcomp "$subcommands"
+				;;
+		esac

This part fixes the --<tab> completion. I suppose if someone did "-<tab>" then nothing would show up?

Quoted 16 lines

fi

contrib/completion/git-completion.bash +5 -9

-	case "$subcommand,$cur" in
-	init,--*)
-		__gitcomp "--cone"
-		;;
-	set,--*)
-		__gitcomp "--stdin"
-		;;
-	*)
-		;;
+	case "$prev" in
+		init)
+			__gitcomp "$__git_sparse_checkout_init_opts"
+			;;
+		add|set)

Review

Quoted 2 lines

(quoted diff) +2 -0

+			__gitcomp "--stdin"
+			__gitcomp "$(git ls-tree -d -r HEAD --name-only)"> +			;;

With the thinking of rebasing onto en/sparse-checkout-set, this could possibly be rearranged so the add|set) cases pass-through into the init) and reapply) cases (skip the ;; between) to save some duplication. Or, it is possible that doesn't work, but it might be worth a try.

Also, since you are using 'git ls-tree' and not 'git ls-files', the sparse index will not have an effect on the performance. There will be some corner cases where a directory exists in one of HEAD or the index but not the other. That's probably still the right way to go since 'git ls-files' doesn't have a way to only list directories. It just means that you probably don't need to explicitly disable the sparse index in your test.

From: Lessley Dennington <[email protected]>

Add tests for missing/incorrect components of custom tab completion for the sparse-checkout command. These tests specifically highlight the following:

1. git sparse-checkout <TAB> results in an incomplete list of subcommands (it is missing reapply and add).
2. git sparse-checkout --<TAB> does not complete the help option.
3. Options for subcommands are not tab-completable.
4. git sparse-checkout set <TAB> and git sparse-checkout add <TAB> show both file names and directory names.

Although these tests currently fail, they will succeed with the sparse-checkout modifications in git-completion.bash in the next commit in this series.

Signed-off-by Lessley Dennington <[email protected]>

---
 t/t9902-completion.sh | 85 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 85 insertions(+)

t/t9902-completion.sh +85 -1

index 518203fbe07..51d0f2d93a1 100755
@@ -1447,6 +1447,91 @@ test_expect_success 'git checkout - with --detach, complete only references' '
        EOF
 '
 
+test_expect_failure '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_failure 'sparse-checkout completes options' '
+       test_completion "git sparse-checkout --" <<-\EOF
+       --help Z
+       EOF
+'
+
+test_expect_failure 'sparse-checkout completes subcommand options' '
+       test_completion "git sparse-checkout init --" <<-\EOF &&
+       --cone Z
+       --no-cone Z
+       --sparse-index Z
+       --no-sparse-index Z
+       EOF
+
+       test_completion "git sparse-checkout set --" <<-\EOF &&
+       --cone Z
+       --no-cone Z
+       --sparse-index Z
+       --no-sparse-index Z
+       --stdin Z
+       EOF
+
+       test_completion "git sparse-checkout reapply --" <<-\EOF &&
+       --cone Z
+       --no-cone Z
+       --sparse-index Z
+       --no-sparse-index Z
+       EOF
+
+       test_completion "git sparse-checkout add --" <<-\EOF
+       --stdin Z
+       EOF
+'
+
+test_expect_failure 'sparse-checkout completes directory names' '
+       # 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"
+       ) &&
+
+       # initialize sparse-checkout definitions
+       git -C sparse-checkout sparse-checkout init --cone &&
+       git -C sparse-checkout sparse-checkout set 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 'git switch - with -d, complete all references' '
        test_completion "git switch -d " <<-\EOF
        HEAD Z
--

gitgitgadget

Second, and this item is unrelated to your series but your comment made me realize it....sparse-checkout unfortunately ignores prefix and creates a bad .git/info/sparse-checkout file. For example:
...
I think the loss of the current working directory will be fixed by the en/keep-cwd directory (currently in next and marked for merging to master), but the fact that the wrong paths end up in the sparse-checkout file is unfortunate. It basically means that the set and add subcommands of sparse-checkout can only be safely run from the toplevel directory.

Quoted 10 lines · Elijah Newren

Second, and this item is unrelated to your series but your comment made me realize it....sparse-checkout unfortunately ignores prefix and creates a bad .git/info/sparse-checkout file. For example:
...
I think the loss of the current working directory will be fixed by the en/keep-cwd directory (currently in next and marked for merging to master), but the fact that the wrong paths end up in the sparse-checkout file is unfortunate. It basically means that the set and add subcommands of sparse-checkout can only be safely run from the toplevel directory.

You made it sound as if this is a fundamental limitation, but it sounds more like a bug that needs to be fixed (outside the completion series, of course) to me.

Quoted 10 lines · Elijah Newren

Second, and this item is unrelated to your series but your comment made me realize it....sparse-checkout unfortunately ignores prefix and creates a bad .git/info/sparse-checkout file. For example:
...
I think the loss of the current working directory will be fixed by the en/keep-cwd directory (currently in next and marked for merging to master), but the fact that the wrong paths end up in the sparse-checkout file is unfortunate. It basically means that the set and add subcommands of sparse-checkout can only be safely run from the toplevel directory.

You made it sound as if this is a fundamental limitation, but it sounds more like a bug that needs to be fixed (outside the completion series, of course) to me.

Quoted 13 lines · Junio C Hamano

Quoted 10 lines · Elijah Newren

Second, and this item is unrelated to your series but your comment made me realize it....sparse-checkout unfortunately ignores prefix and creates a bad .git/info/sparse-checkout file. For example:
...
I think the loss of the current working directory will be fixed by the en/keep-cwd directory (currently in next and marked for merging to master), but the fact that the wrong paths end up in the sparse-checkout file is unfortunate. It basically means that the set and add subcommands of sparse-checkout can only be safely run from the toplevel directory.

Quoted 4 lines

You made it sound as if this is a fundamental limitation, but it sounds more like a bug that needs to be fixed (outside the completion series, of course) to me.

I can file a bug report if that's the correct way to proceed.

Quoted 17 lines · Lessley Dennington

Quoted 13 lines · Junio C Hamano

Quoted 10 lines · Elijah Newren

Second, and this item is unrelated to your series but your comment made me realize it....sparse-checkout unfortunately ignores prefix and creates a bad .git/info/sparse-checkout file. For example:
...
I think the loss of the current working directory will be fixed by the en/keep-cwd directory (currently in next and marked for merging to master), but the fact that the wrong paths end up in the sparse-checkout file is unfortunate. It basically means that the set and add subcommands of sparse-checkout can only be safely run from the toplevel directory.

Quoted 5 lines

Quoted 4 lines

You made it sound as if this is a fundamental limitation, but it sounds more like a bug that needs to be fixed (outside the completion series, of course) to me.

I can file a bug report if that's the correct way to proceed.

We don't really have a bug tracker.

We often just file an email, and add additional searchable strings (e.g. #leftoverbits, though that doesn't apply here), and then search via https://lore.kernel.org/git/.

There is 'git bugreport', but it just generates an email to send to the mailing list...but we already have the emails in this thread.

There is https://bugs.chromium.org/p/git/issues/list, which is used by a few folks, but I suspect I'm the only one who has looked at it that touches sparse-checkout related stuff.

There is https://github.com/git-for-windows/git/issues, but this isn't Windows specific. (Sometimes they'll track stuff that isn't windows specific, but I've seen Dscho close out issues after being reported to this list.)

I've also kept private files with lists of things to work on. Doesn't help anyone else track it. (Which is why I'll sometimes use one of the two above trackers instead.)

...not sure if that helps, but that's the basic state of our "bug tracking".

Quoted 6 lines · Lessley Dennington via GitGitGadget

From: Lessley Dennington <[email protected]>

Add tests for missing/incorrect components of custom tab completion for the sparse-checkout command. These tests specifically highlight the following:

Quoted 6 lines

1. git sparse-checkout <TAB> results in an incomplete list of subcommands (it is missing reapply and add).
2. git sparse-checkout --<TAB> does not complete the help option.
3. Options for subcommands are not tab-completable.
4. git sparse-checkout set <TAB> and git sparse-checkout add <TAB> show both file names and directory names.

Two thoughts on this last item:

I completely agree that only directories should be completed in cone mode, but completing on both might technically be considered correct behavior for non-cone mode. However, even in non-cone mode, I kind of like the idea of encouraging people to sparsify only on directories so I'm totally fine with us only tab-completing directories. (I have a bit of a disdain for non-cone mode, though, so my desire to deprecate it might be showing through. At the very least, I'm still thinking we should make cone mode the default in each of sparse-checkout init, sparse-checkout set, and clone --sparse[1])

[1] https://lore.kernel.org/git/[email protected]/

Second, and this item is unrelated to your series but your comment made me realize it....sparse-checkout unfortunately ignores prefix and creates a bad .git/info/sparse-checkout file. For example:

$ git init -b main tmp
$ cd tmp
$ mkdir -p a/b/c
$ touch a/b/c/d a/b/c/e
$ git add a/
$ git commit -m "initial"
$ cd a/  # Not at the toplevel anymore
$ git sparse-checkout set --cone b/c  # So we expect that a/b/c will
be the specified sparsity path
$ git -C .. sparse-checkout list
b/c
$ cat ../.git/info/sparse-checkout
/*
!/*/
/b/
!/b/*/
/b/c/
$ pwd -P
pwd: error retrieving current directory: getcwd: cannot access parent
directories: No such file or directory

I think the loss of the current working directory will be fixed by the en/keep-cwd directory (currently in next and marked for merging to master), but the fact that the wrong paths end up in the sparse-checkout file is unfortunate. It basically means that the set and add subcommands of sparse-checkout can only be safely run from the toplevel directory.

Quoted 103 lines

Although these tests currently fail, they will succeed with the sparse-checkout modifications in git-completion.bash in the next commit in this series.

Signed-off-by Lessley Dennington <[email protected]>

---
  t/t9902-completion.sh | 85 +++++++++++++++++++++++++++++++++++++++++++
  1 file changed, 85 insertions(+)

t/t9902-completion.sh +85 -0

index 518203fbe07..51d0f2d93a1 100755
@@ -1447,6 +1447,91 @@ test_expect_success 'git checkout - with --detach, complete only references' '
         EOF
  '
 
+test_expect_failure '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_failure 'sparse-checkout completes options' '
+       test_completion "git sparse-checkout --" <<-\EOF
+       --help Z
+       EOF
+'
+
+test_expect_failure 'sparse-checkout completes subcommand options' '
+       test_completion "git sparse-checkout init --" <<-\EOF &&
+       --cone Z
+       --no-cone Z
+       --sparse-index Z
+       --no-sparse-index Z
+       EOF
+
+       test_completion "git sparse-checkout set --" <<-\EOF &&
+       --cone Z
+       --no-cone Z
+       --sparse-index Z
+       --no-sparse-index Z
+       --stdin Z
+       EOF
+
+       test_completion "git sparse-checkout reapply --" <<-\EOF &&
+       --cone Z
+       --no-cone Z
+       --sparse-index Z
+       --no-sparse-index Z
+       EOF
+
+       test_completion "git sparse-checkout add --" <<-\EOF
+       --stdin Z
+       EOF
+'
+
+test_expect_failure 'sparse-checkout completes directory names' '
+       # 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"
+       ) &&
+
+       # initialize sparse-checkout definitions
+       git -C sparse-checkout sparse-checkout init --cone &&
+       git -C sparse-checkout sparse-checkout set 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 'git switch - with -d, complete all references' '

Quoted 4 lines

t/t9902-completion.sh +0 -1

         test_completion "git switch -d " <<-\EOF
         HEAD Z
--

gitgitgadget

Patch looks okay to me, but we might want to add some kind of wording around the directories-only decision and cone vs. non-cone mode to the commit message.

From: Lessley Dennington <[email protected]>

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 <[email protected]>

---
 contrib/completion/git-completion.bash | 38 ++++++++++++++++++--------
 t/t9902-completion.sh                  |  8 +++---
 2 files changed, 30 insertions(+), 16 deletions(-)

contrib/completion/git-completion.bash +12 -3

index c82ccaebcc7..7de997ee64e 100644
@@ -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
 

contrib/completion/git-completion.bash +6 -0

+                       ;;
+               init|reapply)
+                       __gitcomp "$__git_sparse_checkout_subcommand_opts"
+                       ;;
+               *)
+                       ;;
        esac
 }
 

t/t9902-completion.sh +4 -5

index 51d0f2d93a1..4dc93ee0595 100755
@@ -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

Quoted 50 lines · Lessley Dennington via GitGitGadget

From: Lessley Dennington <[email protected]>

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 <[email protected]>

---
  contrib/completion/git-completion.bash | 38 ++++++++++++++++++--------
  t/t9902-completion.sh                  |  8 +++---
  2 files changed, 30 insertions(+), 16 deletions(-)

contrib/completion/git-completion.bash +12 -3

index c82ccaebcc7..7de997ee64e 100644
@@ -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
 

contrib/completion/git-completion.bash _git_show_branch () · line 3007

Quoted 10 lines

contrib/completion/git-completion.bash +1 -9

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

contrib/completion/git-completion.bash _git_show_branch () · line 3014

Quoted 7 lines

contrib/completion/git-completion.bash +7 -0

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

Quoted 42 lines

contrib/completion/git-completion.bash +6 -0

+                       ;;
+               init|reapply)
+                       __gitcomp "$__git_sparse_checkout_subcommand_opts"
+                       ;;
+               *)
+                       ;;
         esac
  }
 

t/t9902-completion.sh +3 -3

index 51d0f2d93a1..4dc93ee0595 100755
@@ -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
  '
 

Quoted 7 lines

t/t9902-completion.sh +1 -2

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

Quoted 52 lines · Elijah Newren

Quoted 50 lines · Lessley Dennington via GitGitGadget

From: Lessley Dennington <[email protected]>

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 <[email protected]>

---
  contrib/completion/git-completion.bash | 38 ++++++++++++++++++--------
  t/t9902-completion.sh                  |  8 +++---
  2 files changed, 30 insertions(+), 16 deletions(-)

contrib/completion/git-completion.bash +12 -3

index c82ccaebcc7..7de997ee64e 100644
@@ -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
 

Quoted 15 lines

contrib/completion/git-completion.bash _git_show_branch () · line 3007

Quoted 10 lines

contrib/completion/git-completion.bash +1 -9

-       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 46 lines

contrib/completion/git-completion.bash _git_show_branch () · line 3014

Quoted 7 lines

contrib/completion/git-completion.bash +7 -0

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

Quoted 4 lines

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.

Quoted 118 lines · Lessley Dennington

Quoted 52 lines · Elijah Newren

Quoted 50 lines · Lessley Dennington via GitGitGadget

From: Lessley Dennington <[email protected]>

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 <[email protected]>

---
   contrib/completion/git-completion.bash | 38 ++++++++++++++++++--------
   t/t9902-completion.sh                  |  8 +++---
   2 files changed, 30 insertions(+), 16 deletions(-)

contrib/completion/git-completion.bash +12 -3

index c82ccaebcc7..7de997ee64e 100644
@@ -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
 

Quoted 15 lines

contrib/completion/git-completion.bash _git_show_branch () · line 3007

Quoted 10 lines

contrib/completion/git-completion.bash +1 -9

-       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 46 lines

contrib/completion/git-completion.bash _git_show_branch () · line 3014

Quoted 7 lines

contrib/completion/git-completion.bash +7 -0

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

Quoted 7 lines

Quoted 4 lines

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.

Quoted 150 lines · Elijah Newren

Quoted 118 lines · Lessley Dennington

Quoted 52 lines · Elijah Newren

Quoted 50 lines · Lessley Dennington via GitGitGadget

From: Lessley Dennington <[email protected]>

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 <[email protected]>

---
   contrib/completion/git-completion.bash | 38 ++++++++++++++++++--------
   t/t9902-completion.sh                  |  8 +++---
   2 files changed, 30 insertions(+), 16 deletions(-)

contrib/completion/git-completion.bash +12 -3

index c82ccaebcc7..7de997ee64e 100644
@@ -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
 

Quoted 15 lines

contrib/completion/git-completion.bash _git_show_branch () · line 3007

Quoted 10 lines

contrib/completion/git-completion.bash +1 -9

-       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 46 lines

contrib/completion/git-completion.bash _git_show_branch () · line 3014

Quoted 7 lines

contrib/completion/git-completion.bash +7 -0

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

Quoted 7 lines

Quoted 4 lines

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

Quoted 11 lines

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.

Subject: Re: [PATCH v3 2/3] sparse-checkout: custom tab completion

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

It is
important to note that this will apply for both cone mode and non-cone mode (even though non-cone mode matches on patterns rather than directories).

Failing tests that were added in the previous commit to verify these scenarios are now passing with these updates.

Signed-off-by Lessley Dennington <[email protected]>

---
 contrib/completion/git-completion.bash | 38 ++++++++++++++++++--------
 t/t9902-completion.sh                  |  8 +++---
 2 files changed, 30 insertions(+), 16 deletions(-)

contrib/completion/git-completion.bash +12 -5

index c82ccaebcc7..f478883f51c 100644
@@ -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,--*)

contrib/completion/git-completion.bash +6 -0

+			;;
+		init|reapply)
+			__gitcomp "$__git_sparse_checkout_subcommand_opts"
+			;;
+		*)
+			;;
 	esac
 }
 

Quoted 1 line · Lessley Dennington via GitGitGadget

Subject: Re: [PATCH v3 2/3] sparse-checkout: custom tab completion

None of these patches touch sparse-checkout, but only the completion script and its tests. Therefore "completion:" would be a better matching area prefix.

Quoted 2 lines

4. A list of directories (but not files) is provided when users enter git sparse-checkout add <TAB> or git sparse-checkout set <TAB>.

Why limit completion only to directories? Both of those subcommands accept files,

and I think 'git sparse-checkout set README.md' is a perfectly reasonable command.

... If our
guide is merely what the command swallows, then we should forgo completion for these subcommands, because it's not possible to enumerate all possible completions.

I don't think that's a useful guide or starting point, so we instead need to discuss what are reasonable completions.

Quoted 4 lines · Elijah Newren

... If our
guide is merely what the command swallows, then we should forgo completion for these subcommands, because it's not possible to enumerate all possible completions.

I am not sure if I follow. I do not think it makes sense to aim for enumerating EVERYTHING the command would accept. But I do not know that is the same as "merely what the command swallows".

Quoted 2 lines

I don't think that's a useful guide or starting point, so we instead need to discuss what are reasonable completions.

I do not think it is a good idea to refrain from suggesting anything that has a possibility of being wring, either, though. If a path that is not a directory (either because it is a file in the current checkout, or because it is a directory only in a different branch) is given, it might not make sense in the cone-mode for the current checkout, but it may well make sense when a different branch is checked out.

Or you may not even in the cone-mode, and in which
case, as SZEDER suggested, a single filename may perfectly make sense. A user who said READM<TAB> and does not see it completed to README.md would be quite confused.

Are we limiting ourselves to directories only when we know we are in the cone-mode and showing both directories and files otherwise?

I think the guiding principle ought to be that we show completion that

- is cheap enough to compute in interactive use (e.g. we should refrain from looking for directories in all possible branches, but instead just look at the working tree and possibly in the index),

- is simple enough to explain to the users to understand what the rules are, and

- gives useful enough candidates.

"We only look for directories (without going recursive) in the working tree" does satisfy the first two, but I am not sure it is more useful than "We show files and directories (without going recursive) in the working tree", which also satisfies the first two.

Of course, if the completion limits to directories only in a repository in the cone-mode, I would not worry about the exclusion of non-directories will confuse users.

I'm not sure I follow. READM<TAB> already doesn't complete to README.md in the following example command lines:

   'cd READM<TAB>'
   'ssh READM<TAB>'

Quoted 4 lines · Elijah Newren

I'm not sure I follow. READM<TAB> already doesn't complete to README.md in the following example command lines:

    'cd READM<TAB>'
    'ssh READM<TAB>'

If the example were "ls READM<TAB>", it would have been worth the time to read and think about this again, but all users expect "cd" to go into directories, and "ssh" to go visit another host, and taking a local filename as a hint to complete would be nonsense.

But that is not what we are talking about (and it is frustrating that you know it).

To users, what sparse-checkout takes look like local pathnames, not necessarily limited to directory names (if you disagree, go back and read what SZEDER wrote to trigger this thread).

I know it is your preference to complete only directories and exclude filenames, but I question if the confusion such a design causes to end-users is worth it.

Quoted 3 lines

I know it is your preference to complete only directories and exclude filenames, but I question if the confusion such a design causes to end-users is worth it.

I think perhaps we're a little caught up in exemplifying commands that are unrelated to sparse-checkout. As Elijah said in [1], the documentation states that directories and patterns are acceptable to sparse-checkout but not files. While it is not reasonable to try to offer every pattern a user could possibly pass to sparse-checkout, it is reasonable to offer directories and (in my opinion) will help guide users toward correct usage of the command.

However, since completion on directories is cone-mode-specific, I am willing to accept the suggestion to only complete directories if we are in a cone-mode sparse-checkout and apply it in v4 of this series.

[1]: https://lore.kernel.org/git/CABPp-BErg-RtyycXaRXYfQHEQXA4q-FU9Q6nYkSHJsqL-04oXw@mail.gmail.com/

Quoted 12 lines

Quoted 3 lines

I know it is your preference to complete only directories and exclude filenames, but I question if the confusion such a design causes to end-users is worth it.

I think perhaps we're a little caught up in exemplifying commands that are unrelated to sparse-checkout. As Elijah said in [1], the documentation states that directories and patterns are acceptable to sparse-checkout but not files. While it is not reasonable to try to offer every pattern a user could possibly pass to sparse-checkout, it is reasonable to offer directories and (in my opinion) will help guide users toward correct usage of the command.

Quoted 6 lines

However, since completion on directories is cone-mode-specific, I am willing to accept the suggestion to only complete directories if we are in a cone-mode sparse-checkout and apply it in v4 of this series.

[1]: https://lore.kernel.org/git/CABPp-BErg-RtyycXaRXYfQHEQXA4q-FU9Q6nYkSHJsqL-04oXw@mail.gmail.com/

In light of non-cone mode being removed in the near future (see [1]), it actually seems it does not make sense to add different behaviors for cone mode and non-cone mode. I also ran this by some other contributors, who thought it would be best to complete on both files and directories so as not to confuse users (as Junio and Szeder have indicated). So, instead of differentiating between cone mode and non-cone mode in V4, I will plan to remove directory completion.

[1]: https://lore.kernel.org/git/CABPp-BEwMAPHGt5xD9jDU58grbrAqCdqNY9Nh8UJGLKuLbArXQ@mail.gmail.com/

Quoted 27 lines · Lessley Dennington

Quoted 12 lines

Quoted 3 lines

I know it is your preference to complete only directories and exclude filenames, but I question if the confusion such a design causes to end-users is worth it.

I think perhaps we're a little caught up in exemplifying commands that are unrelated to sparse-checkout. As Elijah said in [1], the documentation states that directories and patterns are acceptable to sparse-checkout but not files. While it is not reasonable to try to offer every pattern a user could possibly pass to sparse-checkout, it is reasonable to offer directories and (in my opinion) will help guide users toward correct usage of the command.

Quoted 6 lines

However, since completion on directories is cone-mode-specific, I am willing to accept the suggestion to only complete directories if we are in a cone-mode sparse-checkout and apply it in v4 of this series.

[1]: https://lore.kernel.org/git/CABPp-BErg-RtyycXaRXYfQHEQXA4q-FU9Q6nYkSHJsqL-04oXw@mail.gmail.com/

In light of non-cone mode being removed in the near future (see [1]), it actually seems it does not make sense to add different behaviors for cone mode and non-cone mode. I also ran this by some other contributors, who thought it would be best to complete on both files and directories so as not to confuse users (as Junio and Szeder have indicated). So, instead of differentiating between cone mode and non-cone mode in V4, I will plan to remove directory completion.

Quoted 2 lines

[1]: https://lore.kernel.org/git/CABPp-BEwMAPHGt5xD9jDU58grbrAqCdqNY9Nh8UJGLKuLbArXQ@mail.gmail.com/

My apologies, I will not remove directory completion, but rather will return to completing on both files and directories.

the
documentation never explicitly states files are acceptable to sparse-checkout {set,add}. It always mentions "directories" and "patterns".

Quoted 4 lines · Elijah Newren

the
documentation never explicitly states files are acceptable to sparse-checkout {set,add}. It always mentions "directories" and "patterns".

The "Full Pattern Set" section of the manpage does in fact explicitly states that files are supported.

Quoted 4 lines · SZEDER Gábor

Quoted 1 line · Lessley Dennington via GitGitGadget

Subject: Re: [PATCH v3 2/3] sparse-checkout: custom tab completion

Quoted 3 lines

None of these patches touch sparse-checkout, but only the completion script and its tests. Therefore "completion:" would be a better matching area prefix.

Thanks for the detailed feedback and guidance in your review. Very helpful. I'll omit quoting most of it here, but I do want to comment on the point about directories.

Quoted 7 lines · SZEDER Gábor

Quoted 1 line · Lessley Dennington via GitGitGadget

Subject: Re: [PATCH v3 2/3] sparse-checkout: custom tab completion

None of these patches touch sparse-checkout, but only the completion script and its tests. Therefore "completion:" would be a better matching area prefix.

Thanks for the detailed feedback and guidance in your review. Very helpful. I'll omit quoting most of it here, but I do want to comment on the point about directories.

...

Quoted 5 lines

Quoted 2 lines

4. A list of directories (but not files) is provided when users enter git sparse-checkout add <TAB> or git sparse-checkout set <TAB>.

Why limit completion only to directories? Both of those subcommands accept files,

Discussed in part at [1], but let me give a more detailed answer.

Both of these commands accept not only directories and files, but also nearly arbitrary input as far as I can tell. (In cone-mode, it'll accept anything so long as it doesn't look like a relative path that tries to reach above the toplevel directory with '../' sequences. In non-cone mode, I think it accepts completely arbitrary input). If our guide is merely what the command swallows, then we should forgo completion for these subcommands, because it's not possible to enumerate all possible completions. I don't think that's a useful guide or starting point, so we instead need to discuss what are reasonable completions.

cone-mode works exclusively on directories. So, in that mode, directories are what we want to complete on. (And if a file is specified, cone-mode will treat it as a directory and add expressions for including all the files under that "directory", which might be confusing. sparse-checkout doesn't verify it is a diretory, because it *might* name a directory in a different branch, including one not yet downloaded. But "might name a directory on another branch" is no reason to suggest picking that pathname with completion.)

In non-cone mode, arbitrary expressions are valid and will be treated as gitignore-style expressions. That again leaves us with either not providing completions, or choosing a subset of possible inputs that are reasonable suggestions for users. I prefer the latter, and in particular I feel that directories are reasonable suggestions. In contrast, I don't think providing files is helpful, because it reinforces the design flaw of non-cone mode. Non-cone mode has quadratic performance baked into its design, and since sparse-checkouts are about performance, non-cone mode kind of defeats the purpose of the command.

(In addition to other problems[2].) So,
I think non-cone mode should be deprecated and excised. Patches elsewhere are moving in the direction of deprecation already[3], and we've already discussed multiple steps we'll likely take soon continuing in that direction. In the meantime, providing just directories for completion seems like a good direction to me.

[1] https://lore.kernel.org/git/CABPp-BG=wr81CPtW1M12xFN_0dyS8mAZjM6o=77LA20Zge8Xng@mail.gmail.com/
[2] https://lore.kernel.org/git/CABPp-BF=-1aZd=nFHF6spo7Ksa7f7Wb7ervCt0QvtNitMY=ZBA@mail.gmail.com/
[3] https://lore.kernel.org/git/0af00779128e594aff0ee4ec5378addeac8e88a2.1642175983.git.gitgitgadget@gmail.com/
("This mode is harder to use and less performant, and is thus not
recommended.")

Quoted 2 lines

and I think 'git sparse-checkout set README.md' is a perfectly reasonable command.

Reasonable in what sense? That it makes it (vastly) easier to implement the completion and sparse-checkout set|add will swallow it, or that it's something that should actually be recommended for users doing sparse-checkouts? While the former certainly holds, I don't think the latter does.

Quoted 3 lines · Elijah Newren

Quoted 2 lines · SZEDER Gábor

On Mon, Jan 10, 2022 at 06:59:51PM +0000, Lessley Dennington via GitGitGadget wrote:

Quoted 7 lines

Quoted 5 lines

Quoted 2 lines

4. A list of directories (but not files) is provided when users enter git sparse-checkout add <TAB> or git sparse-checkout set <TAB>.

Why limit completion only to directories? Both of those subcommands accept files,

Discussed in part at [1], but let me give a more detailed answer.

It was a semi-rhetorical question. Whether the reasons for expluding files are sound or not, it should be convincingly justified in the commit message.

I used the following command to create a sparse-checkout from linux.git to build and play with 'perf':

git sparse-checkout set tools/perf/ tools/scripts/ tools/build/ tools/include/ tools/arch/x86/ tools/lib/ /.gitignore /.gitattributes

Including the top-level '.gitignore' and '.gitattributes' was important, becase those ignore object files and specify userdiff. Now, I wouldn't mind having other files present in the top-level directory, because there are only a handful of files there. However, being able to specify just those two files to 'git sparse-checkout' was great, because I didn't even have to think about what wildcard pattern to use, and what negative pattern to use to exclude anything that might have been included recursively.

I don't remember having any performance issues with it, on the contrary, I do remember that Git suddenly became much faster that in the full worktree.

So I'm fairly convinced that specifying files to sparse-checkout is a feature that can make users' life easier. It certainly made my life easier.

On a related note: I just noticed the leading slashes in '/.gitignore' and '/.gitattributes'. __git_complete_index_file() is not ready for that, I'm afraid; but I don't think the proposed patches could deal with that, either (but I didn't actually try).

It would be great if completion could cope with patterns starting with '/' and/or '!'.

contrib/completion/git-completion.bash +4 -0

+__gitcomp_directories ()
+{
+     local _tmp_dir _tmp_completions
+

+ __gitcomp_direct_append "$cur "

contrib/completion/git-completion.bash _git_show_branch () · line 3002

Quoted 4 lines

contrib/completion/git-completion.bash +4 -0

+     # Find possible directory completions, adding trailing '/' characters
+     _tmp_completions="$(git ls-tree -d --name-only HEAD $_tmp_dir |
+         sed -e s%$%/%)"
+

I am admittedly unfamiliar with the use of this format in sed expressions (I'm generally more accustomed to '/' instead of '%'). It's definitely working as it should, I'm just not quite sure of how.

contrib/completion/git-completion.bash _git_show_branch () · line 3006

Quoted 4 lines

contrib/completion/git-completion.bash +4 -0

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

Does c need to be declared before the loop?

Quoted 1 line

+ for c in $_tmp_completions; do

Quoted 10 lines

contrib/completion/git-completion.bash +9 -0

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

contrib/completion/git-completion.bash _git_show_branch () · line 3002

Quoted 5 lines

contrib/completion/git-completion.bash +5 -0

+
+     # Find possible directory completions, adding trailing '/' characters
+     _tmp_completions="$(git ls-tree -d --name-only HEAD $_tmp_dir |
+         sed -e s%$%/%)"
+

I am admittedly unfamiliar with the use of this format in sed expressions (I'm generally more accustomed to '/' instead of '%'). It's definitely working as it should, I'm just not quite sure of how.

contrib/completion/git-completion.bash _git_show_branch () · line 3006

Quoted 4 lines

contrib/completion/git-completion.bash +4 -0

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

Does c need to be declared before the loop?

contrib/completion/git-completion.bash _git_show_branch () · line 3014

Quoted 8 lines

contrib/completion/git-completion.bash +8 -0

+         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

Thank you so much for the detailed comments on this change - it made it really easy to parse.

Quoted 1 line

+ __gitcomp_direct_append "$cur "

What's the reason for the trailing space here?

Review

Quoted 2 lines

(quoted diff) +2 -0

+     fi
+}

Added my review as mentioned in [1].

[1]:
https://lore.kernel.org/git/[email protected]/T/#md3da435452988b0366ab4c2ee4bc06df2d17cb36

Quoted 10 lines · Lessley Dennington

Quoted 10 lines

contrib/completion/git-completion.bash +9 -0

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

Quoted 8 lines

contrib/completion/git-completion.bash _git_show_branch () · line 3002

Quoted 5 lines

contrib/completion/git-completion.bash +5 -0

+
+     # Find possible directory completions, adding trailing '/' characters
+     _tmp_completions="$(git ls-tree -d --name-only HEAD $_tmp_dir |
+         sed -e s%$%/%)"
+

I am admittedly unfamiliar with the use of this format in sed expressions (I'm generally more accustomed to '/' instead of '%'). It's definitely working as it should, I'm just not quite sure of how.

These are the same in sed:
sed -e s/apple/banana/
sed -e s@apple@banana@
sed -e s%apple%banana%

You can pick any character you like, but '/' is what people most often use. My expression involved a '/', though, so I changed the delimiter to avoid ugly backslash escapes.

Quoted 5 lines

contrib/completion/git-completion.bash _git_show_branch () · line 3006

Quoted 4 lines

contrib/completion/git-completion.bash +4 -0

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

Does c need to be declared before the loop?

It was, but it's super easy to miss. Look at the "local" line just before your comment; it almost reads like line noise, but basically there are three variables declared with two of them given initial values. c is in the middle, without an initial value.

Quoted 8 lines

contrib/completion/git-completion.bash _git_show_branch () · line 3014

Quoted 8 lines

contrib/completion/git-completion.bash +8 -0

+         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

Quoted 4 lines

Thank you so much for the detailed comments on this change - it made it really easy to parse.

Quoted 1 line

+ __gitcomp_direct_append "$cur "

What's the reason for the trailing space here?

The space was related to the "just consider it complete" aspect of the comment above. Anyway, to understand why the space character signals the completion being final for this token, let's use some comparisons with bash-completion of just a plain 'ls' command...

In git.git, at the toplevel, if I type
ls README.md <TAB>
(note the space after README.md before pressing <TAB>), then completion assumes I'm trying to get another term besides just README.md, and can complete on anything in the directory. In contrast, if I type
ls README.md<TAB>
(with no space between README.md and <TAB>), then completion figures I'm trying to find filenames that start with "README.md", finds only one, and returns "README.md " (note the trailing space). That trailing space makes my command line become "ls README.md " (again, with a trailing space), so that if I try to do any more tab completions, it's for adding another argument to the ls command, not extending the README.md one.

You can see similar things with ls and directories. For example, if you type
ls Doc<TAB>tec<TAB>m<TAB>

then you'll note after the first <TAB> you'll see
ls Documentation/
with no trailing space; after the second <TAB> you'll see
ls Documentation/technical/
with no trailing space; and after the third <TAB> you'll see
ls Documentation/technical/multi-pack-index.txt
WITH a trailing space. In the first two cases, further completions were possible so they didn't add a trailing space to signify the completion was final, but in the third case it is final and needed the trailing space to denote that.

Does that help?

Quoted 7 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+     fi
+}

Added my review as mentioned in [1].

[1]:
https://lore.kernel.org/git/[email protected]/T/#md3da435452988b0366ab4c2ee4bc06df2d17cb36

This series is based on en/sparse-checkout-set. It updates custom tab completion for the sparse-checkout command. Specifically, it corrects the following issues with the current method:

1. git sparse-checkout <TAB> results in an incomplete list of subcommands (it is missing reapply and add).
2. git sparse-checkout --<TAB> does not complete the help option.
3. Options for subcommands are not tab-completable.
4. git sparse-checkout set <TAB> and git sparse-checkout add <TAB> show both file names and directory names.

The first commit in this series is a set of failing tests that highlight each of the above issues. The next commit updates the _git_sparse_checkout method in git-completion.bash to enable each of these tests to pass. The final commit modifies the original implementation (which recursively printed prospective directories for completion) to only print directories at the current level for improved performance.

Changes since V2
================

* Change use of $prev to $subcommand in _git_sparse_checkout() method in git-completion.bash.
* State explicitly that directory completion applies in both cone and non-cone mode in 'sparse-checkout: custom tab completion' commit
* Add new patch with __gitcomp_directories method to improve performance by only outputting directories at the current level.

Changes since V1
================

* Rebase onto en/sparse-checkout-set.
* Add subcommand options (including --no-cone) for set and reapply.
* Extend 'sparse-checkout completes subcommand options' test to validate new set/reapply subcommand options.
* No longer set index.sparse to false in 'sparse-checkout completes directory names' test.

Thanks, Lessley

Lessley Dennington (3):
sparse-checkout: custom tab completion tests
sparse-checkout: custom tab completion
sparse-checkout: limit tab completion to a single level

 contrib/completion/git-completion.bash | 68 +++++++++++++++----
 t/t9902-completion.sh                  | 94 ++++++++++++++++++++++++++
 2 files changed, 150 insertions(+), 12 deletions(-)

base-commit: dfac9b609f86cd4f6ce896df9e1172d2a02cde48
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1108%2Fldennington%2Fsparse-checkout-bash-completion-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1108/ldennington/sparse-checkout-bash-completion-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1108

Range-diff vs v2:

 1:  955fcab0052 = 1:  bbc2d21e1d1 sparse-checkout: custom tab completion tests
 2:  cecf501e076 ! 2:  256e5f034c6 sparse-checkout: custom tab completion

@@ Commit message
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>.
+ sparse-checkout add <TAB> or git sparse-checkout set <TAB>. It is
+ important to note that this will apply for both cone mode and non-cone
+ mode (even though non-cone mode matches on patterns rather than
+ directories).

Failing tests that were added in the previous commit to verify these scenarios are now passing with these updates.
@@ contrib/completion/git-completion.bash: _git_show_branch ()
- ;;
- *)
- ;;
-+ case "$prev" in
++ case "$subcommand" in
+ set)
+ __gitcomp "$__git_sparse_checkout_subcommand_opts --stdin"
+ __gitcomp "$(git ls-tree -d -r HEAD --name-only)"
-: ----------- > 3: aa9ea67180d sparse-checkout: limit tab completion to a single level

Quoted 62 lines · Lessley Dennington via GitGitGadget

This series is based on en/sparse-checkout-set. It updates custom tab completion for the sparse-checkout command. Specifically, it corrects the following issues with the current method:

1. git sparse-checkout <TAB> results in an incomplete list of subcommands (it is missing reapply and add).
2. git sparse-checkout --<TAB> does not complete the help option.
3. Options for subcommands are not tab-completable.
4. git sparse-checkout set <TAB> and git sparse-checkout add <TAB> show both file names and directory names.

The first commit in this series is a set of failing tests that highlight each of the above issues. The next commit updates the _git_sparse_checkout method in git-completion.bash to enable each of these tests to pass. The final commit modifies the original implementation (which recursively printed prospective directories for completion) to only print directories at the current level for improved performance.

Changes since V2
================

* Change use of $prev to $subcommand in _git_sparse_checkout() method in git-completion.bash.
* State explicitly that directory completion applies in both cone and non-cone mode in 'sparse-checkout: custom tab completion' commit
* Add new patch with __gitcomp_directories method to improve performance by only outputting directories at the current level.

Changes since V1
================

* Rebase onto en/sparse-checkout-set.
* Add subcommand options (including --no-cone) for set and reapply.
* Extend 'sparse-checkout completes subcommand options' test to validate new set/reapply subcommand options.
* No longer set index.sparse to false in 'sparse-checkout completes directory names' test.

Thanks, Lessley

Lessley Dennington (3):
sparse-checkout: custom tab completion tests
sparse-checkout: custom tab completion
sparse-checkout: limit tab completion to a single level

  contrib/completion/git-completion.bash | 68 +++++++++++++++----
  t/t9902-completion.sh                  | 94 ++++++++++++++++++++++++++
  2 files changed, 150 insertions(+), 12 deletions(-)

base-commit: dfac9b609f86cd4f6ce896df9e1172d2a02cde48
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1108%2Fldennington%2Fsparse-checkout-bash-completion-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1108/ldennington/sparse-checkout-bash-completion-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1108

Range-diff vs v2:

  1:  955fcab0052 = 1:  bbc2d21e1d1 sparse-checkout: custom tab completion tests
  2:  cecf501e076 ! 2:  256e5f034c6 sparse-checkout: custom tab completion

Quoted 9 lines

@@ Commit message
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>.
+ sparse-checkout add <TAB> or git sparse-checkout set <TAB>. It is
+ important to note that this will apply for both cone mode and non-cone
+ mode (even though non-cone mode matches on patterns rather than
+ directories).

I would instead phrase this as "(even though non-cone mode can match general gitignore patterns rather than just directories)".

The basic idea behind the rewording is that I want to highlight that the completions we provide are still valid in non-cone mode, they just aren't comprehensive. Since there's no way to provide a comprehensive list of possible patterns for non-cone mode, I think what we are choosing to provide is a pretty reasonable choice.

Quoted 13 lines

Failing tests that were added in the previous commit to verify these scenarios are now passing with these updates.
@@ contrib/completion/git-completion.bash: _git_show_branch ()
- ;;
- *)
- ;;
-+ case "$prev" in
++ case "$subcommand" in
+ set)
+ __gitcomp "$__git_sparse_checkout_subcommand_opts --stdin"
+ __gitcomp "$(git ls-tree -d -r HEAD --name-only)"
-: ----------- > 3: aa9ea67180d sparse-checkout: limit tab completion to a single level

Other than that one nit, patches 1-2 (and the testcases in patch 3) are:

Reviewed-by Elijah Newren <[email protected]>

Since I wrote the new __gitcomp() function in patch 3, it might be nice if we can find another reviewer for it. Especially since I've only lightly touched the completion code and there might be better ways of achieving what I wrote there.

Quoted 4 lines · Elijah Newren

Other than that one nit, patches 1-2 (and the testcases in patch 3) are:

Reviewed-by Elijah Newren <[email protected]>

Quoted 4 lines

Since I wrote the new __gitcomp() function in patch 3, it might be nice if we can find another reviewer for it. Especially since I've only lightly touched the completion code and there might be better ways of achieving what I wrote there.

I agree - I'll chat with some folks today to try to find the best person to take a look.

Quoted 5 lines · Lessley Dennington

Quoted 4 lines · Elijah Newren

Other than that one nit, patches 1-2 (and the testcases in patch 3) are:

Reviewed-by Elijah Newren <[email protected]>

Quoted 7 lines

Quoted 4 lines

Since I wrote the new __gitcomp() function in patch 3, it might be nice if we can find another reviewer for it. Especially since I've only lightly touched the completion code and there might be better ways of achieving what I wrote there.

I agree - I'll chat with some folks today to try to find the best person to take a look.

Having another set of eyes on this code never hurts, but looking through this file's shortlog (especially with --since=1.year.ago), I'm not sure how many active reviewers in this area we still have.

Perhaps the best thing to do would be to grow some new area experts here instead. If I were in Lessley's shoes, I would "think aloud" to make sure I agreed with your implementation for the new __gitcomp() function.

If other reviewers happen to be around (perhaps Denton, who I added to the CC) that doesn't hurt either, but short of that...

Thanks,
Taylor

From: Lessley Dennington <[email protected]>

Add tests for missing/incorrect components of custom tab completion for the sparse-checkout command. These tests specifically highlight the following:

1. git sparse-checkout <TAB> results in an incomplete list of subcommands (it is missing reapply and add).
2. git sparse-checkout set <TAB> and git sparse-checkout add <TAB> show both file names and directory names. While this is the correct behavior

for non-cone mode, cone mode sparse checkouts should complete only directory names.

Although the first two of these tests currently fail, they will succeed with the sparse-checkout modifications in git-completion.bash in the next commit in this series.

Signed-off-by Lessley Dennington <[email protected]>

---
 t/t9902-completion.sh | 75 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 75 insertions(+)

t/t9902-completion.sh +53 -0

index 518203fbe07..6004d854102 100755
@@ -1447,6 +1447,81 @@ test_expect_success 'git checkout - with --detach, complete only references' '
        EOF
 '
 
+test_expect_failure '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_failure 'cone mode sparse-checkout completes directory names' '
+       # 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"
+       ) &&
+
+       # 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
+               folder2
+               folder3
+               EOF
+       ) &&
+
+       (
+               cd sparse-checkout &&
+               test_completion "git sparse-checkout set folder1/" <<-\EOF
+               folder1/0
+               EOF
+       ) &&
+
+       (
+               cd sparse-checkout &&
+               test_completion "git sparse-checkout set folder1/0/" <<-\EOF
+               folder1/0/1
+               EOF
+       ) &&
+
+       (
+               cd sparse-checkout/folder1 &&

t/t9902-completion.sh +12 -1

+
+       # test tab completion
+       (
+               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 switch - with -d, complete all references' '
        test_completion "git switch -d " <<-\EOF
        HEAD Z
--

gitgitgadget

Quoted 7 lines · Lessley Dennington via GitGitGadget

From: Lessley Dennington <[email protected]>

Add tests for missing/incorrect components of custom tab completion for the sparse-checkout command. These tests specifically highlight the following:

Quoted 4 lines

1. git sparse-checkout <TAB> results in an incomplete list of subcommands (it is missing reapply and add).
2. git sparse-checkout set <TAB> and git sparse-checkout add <TAB> show both file names and directory names. While this is the correct behavior

s/is/may be/

t/t9902-completion.sh test_expect_success 'git checkout - with --detach, complete only references' ' · line 1512

Quoted 4 lines

t/t9902-completion.sh +4 -0

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

Can we add a --no-cone here in preparation for the default to switch?

Quoted 14 lines · Elijah Newren

Quoted 11 lines · Lessley Dennington via GitGitGadget

From: Lessley Dennington <[email protected]>

Add tests for missing/incorrect components of custom tab completion for the sparse-checkout command. These tests specifically highlight the following:

1. git sparse-checkout <TAB> results in an incomplete list of subcommands (it is missing reapply and add).
2. git sparse-checkout set <TAB> and git sparse-checkout add <TAB> show both file names and directory names. While this is the correct behavior

Quoted 1 line

s/is/may be/

I would stop at "this may be a less surprising behaviour" without going into "correctness".

Quoted 6 lines

t/t9902-completion.sh test_expect_success 'git checkout - with --detach, complete only references' ' · line 1512

Quoted 4 lines

t/t9902-completion.sh +4 -0

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

Can we add a --no-cone here in preparation for the default to switch?

It would be good to do so, if we plan to switch the default.

From: Lessley Dennington <[email protected]>

Fix custom tab completion for the sparse-checkout command. This will ensure:

1. The full list of subcommands is provided when users enter git sparse-checkout <TAB>.
2. Subcommand options are tab-completable.
3. A list of directories (but not files) is provided when users enter git sparse-checkout add <TAB> or git sparse-checkout set <TAB> in cone mode only. In non-cone mode, these commands will continue to complete both directory and file names.

Failing tests that were added in the previous commit to verify these scenarios are now passing with these updates.

Signed-off-by Lessley Dennington <[email protected]>

---
 contrib/completion/git-completion.bash | 15 ++++++++-------
 t/t9902-completion.sh                  |  4 ++--
 2 files changed, 10 insertions(+), 9 deletions(-)

contrib/completion/git-completion.bash +2 -4

index c82ccaebcc7..a920cdab201 100644
@@ -2988,21 +2988,22 @@ _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"
                return
        fi
 
        case "$subcommand,$cur" in
-       init,--*)
-               __gitcomp "--cone"
-               ;;

contrib/completion/git-completion.bash +0 -0

                ;;
        esac
 }

t/t9902-completion.sh +2 -3

index 6004d854102..f56ba3f64c9 100755
@@ -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,7 +1458,7 @@ test_expect_failure 'sparse-checkout completes subcommands' '
        EOF
 '
 
-test_expect_failure 'cone mode sparse-checkout completes directory names' '
+test_expect_success 'cone mode sparse-checkout completes directory names' '
        # set up sparse-checkout repo
        git init sparse-checkout &&
        (
--

gitgitgadget

Quoted 44 lines · Lessley Dennington via GitGitGadget

From: Lessley Dennington <[email protected]>

Fix custom tab completion for the sparse-checkout command. This will ensure:

1. The full list of subcommands is provided when users enter git sparse-checkout <TAB>.
2. Subcommand options are tab-completable.
3. A list of directories (but not files) is provided when users enter git sparse-checkout add <TAB> or git sparse-checkout set <TAB> in cone mode only. In non-cone mode, these commands will continue to complete both directory and file names.

Failing tests that were added in the previous commit to verify these scenarios are now passing with these updates.

Signed-off-by Lessley Dennington <[email protected]>

---
  contrib/completion/git-completion.bash | 15 ++++++++-------
  t/t9902-completion.sh                  |  4 ++--
  2 files changed, 10 insertions(+), 9 deletions(-)

contrib/completion/git-completion.bash +2 -4

index c82ccaebcc7..a920cdab201 100644
@@ -2988,21 +2988,22 @@ _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"
                 return
         fi
 
         case "$subcommand,$cur" in
-       init,--*)
-               __gitcomp "--cone"
-               ;;

contrib/completion/git-completion.bash _git_show_branch () · line 3006

Quoted 10 lines

contrib/completion/git-completion.bash +6 -3

-       set,--*)
-               __gitcomp "--stdin"
+               *,--*)
+                       __gitcomp_builtin sparse-checkout_$subcommand "" "--"
                 ;;
-       *)
+               set,*|add,*)
+                       if [ $(__git config core.sparseCheckoutCone) ]; then
+                               __git_complete_index_file "--directory"
+                       fi

--directory only affects how untracked files are presented in git ls-files (which _git_complete_index_file uses), and you're not asking for untracked paths at all here. So you've got a useless flag.

Testing this commit out, I can verify that it completes on all tracked paths in the index. Perhaps that was an intended intermediate step, but if so the commit message needs to be changed since it claims this commit is doing something else.

There's also a problem in that git sparse-checkout --cone path<TAB> is not using this codepath. If the user specifies --cone, then we should give them cone-mode-style completion the same as if they already have the config setting set. Or, we at least need to mention in the commit message that we don't yet correctly check for the --cone option to get the right style of completion and are leaving it for future work.

Quoted 66 lines · Elijah Newren

Quoted 44 lines · Lessley Dennington via GitGitGadget

From: Lessley Dennington <[email protected]>

Fix custom tab completion for the sparse-checkout command. This will ensure:

1. The full list of subcommands is provided when users enter git sparse-checkout <TAB>.
2. Subcommand options are tab-completable.
3. A list of directories (but not files) is provided when users enter git sparse-checkout add <TAB> or git sparse-checkout set <TAB> in cone mode only. In non-cone mode, these commands will continue to complete both directory and file names.

Failing tests that were added in the previous commit to verify these scenarios are now passing with these updates.

Signed-off-by Lessley Dennington <[email protected]>

---
  contrib/completion/git-completion.bash | 15 ++++++++-------
  t/t9902-completion.sh                  |  4 ++--
  2 files changed, 10 insertions(+), 9 deletions(-)

contrib/completion/git-completion.bash +2 -4

index c82ccaebcc7..a920cdab201 100644
@@ -2988,21 +2988,22 @@ _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"
                 return
         fi
 
         case "$subcommand,$cur" in
-       init,--*)
-               __gitcomp "--cone"
-               ;;

contrib/completion/git-completion.bash _git_show_branch () · line 3006

Quoted 10 lines

contrib/completion/git-completion.bash +6 -3

-       set,--*)
-               __gitcomp "--stdin"
+               *,--*)
+                       __gitcomp_builtin sparse-checkout_$subcommand "" "--"
                 ;;
-       *)
+               set,*|add,*)
+                       if [ $(__git config core.sparseCheckoutCone) ]; then
+                               __git_complete_index_file "--directory"
+                       fi

--directory only affects how untracked files are presented in git ls-files (which _git_complete_index_file uses), and you're not asking for untracked paths at all here. So you've got a useless flag.

Testing this commit out, I can verify that it completes on all tracked paths in the index. Perhaps that was an intended intermediate step, but if so the commit message needs to be changed since it claims this commit is doing something else.

Quoted 9 lines

There's also a problem in that git sparse-checkout --cone path<TAB> is not using this codepath. If the user specifies --cone, then we should give them cone-mode-style completion the same as if they already have the config setting set. Or, we at least need to mention in the commit message that we don't yet correctly check for the --cone option to get the right style of completion and are leaving it for future work.

Thank you for your feedback, it is very helpful. As I've been thinking about this, it feels like this commit should perhaps be dropped in favor of the __gitcomp_directories commit (with some modifications per [1]). I know you initially said to add the __gitcomp_directories commit in addition to what I had, though, so I wanted to confirm whether removing this is the right course of action.

[1]:
https://lore.kernel.org/git/CABPp-BEq9pTqsy_R_SR1DSgUK58ubNR1Gk4G1RoL8wkadyo6zw@mail.gmail.com/

From: Lessley Dennington <[email protected]>

Ensure users in a cone mode sparse checkout can tab-complete to directories at multiple levels with multiple <TAB>s. For example, if path a/b/c/ exists in the current directory, running a<TAB>b<TAB> will result in:

        a/b/c/

The 'sparse-checkout completes directory names' test has also been updated with trailing slashes according to these changes.

Co-authored-by Elijah Newren <[email protected]>

Co-authored-by Lessley Dennington <[email protected]>

Signed-off-by Lessley Dennington <[email protected]>

---
 contrib/completion/git-completion.bash | 33 ++++++++++++++++++++++++--
 t/t9902-completion.sh                  | 12 +++++-----
 2 files changed, 37 insertions(+), 8 deletions(-)

contrib/completion/git-completion.bash +19 -0

index a920cdab201..defcc8560c1 100644
@@ -2986,11 +2986,40 @@ _git_show_branch ()
        __git_complete_revlist
 }
 
+__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

contrib/completion/git-completion.bash +2 -2

+
 _git_sparse_checkout ()
 {
        local subcommands="list init set disable add reapply"
        local subcommand="$(__git_find_on_cmdline "$subcommands")"
-
        if [ -z "$subcommand" ]; then
                __gitcomp "$subcommands"
                return

@@ -3002,7 +3031,7 @@ _git_sparse_checkout ()
                ;;
                set,*|add,*)
                        if [ $(__git config core.sparseCheckoutCone) ]; then
-                               __git_complete_index_file "--directory"
+                               __gitcomp_directories
                        fi
                ;;
        esac

t/t9902-completion.sh +6 -7

index f56ba3f64c9..c04fc25fa32 100755
@@ -1478,30 +1478,30 @@ test_expect_success 'cone mode sparse-checkout completes directory names' '
        (
                cd sparse-checkout &&
                test_completion "git sparse-checkout set f" <<-\EOF
-               folder1
-               folder2
-               folder3
+               folder1/
+               folder2/
+               folder3/
                EOF
        ) &&
 
        (
                cd sparse-checkout &&
                test_completion "git sparse-checkout set folder1/" <<-\EOF
-               folder1/0
+               folder1/0/
                EOF
        ) &&
 
        (
                cd sparse-checkout &&
                test_completion "git sparse-checkout set folder1/0/" <<-\EOF
-               folder1/0/1
+               folder1/0/1/
                EOF
        ) &&
 
        (
                cd sparse-checkout/folder1 &&
                test_completion "git sparse-checkout add 0" <<-\EOF
-               0
+               0/
                EOF
        )
 '
--

gitgitgadget

From: Lessley Dennington <[email protected]>

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 <[email protected]>

---
 contrib/completion/git-completion.bash | 16 ++---
 t/t9902-completion.sh                  | 83 ++++++++++++++++++++++++++
 2 files changed, 91 insertions(+), 8 deletions(-)

contrib/completion/git-completion.bash +1 -3

index c82ccaebcc7..2976f63747f 100644
@@ -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"

contrib/completion/git-completion.bash +2 -0

+                       __gitcomp "$(git ls-tree -d -r HEAD --name-only)"
+               fi
        esac
 }
 

t/t9902-completion.sh +76 -0

index 518203fbe07..f6eeb9aa035 100755
@@ -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

Quoted 49 lines · Lessley Dennington via GitGitGadget

From: Lessley Dennington <[email protected]>

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 <[email protected]>

---
  contrib/completion/git-completion.bash | 16 ++---
  t/t9902-completion.sh                  | 83 ++++++++++++++++++++++++++
  2 files changed, 91 insertions(+), 8 deletions(-)

contrib/completion/git-completion.bash +1 -3

index c82ccaebcc7..2976f63747f 100644
@@ -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"

contrib/completion/git-completion.bash _git_sparse_checkout () · line 3003

Quoted 10 lines

contrib/completion/git-completion.bash +4 -5

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

contrib/completion/git-completion.bash _git_sparse_checkout () · line 3004

Quoted 1 line

contrib/completion/git-completion.bash +1 -0

+               [ "$(__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.

Quoted 89 lines

contrib/completion/git-completion.bash +2 -0

+                       __gitcomp "$(git ls-tree -d -r HEAD --name-only)"
+               fi
         esac
  }
 

t/t9902-completion.sh +76 -0

index 518203fbe07..f6eeb9aa035 100755
@@ -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

t/t9902-completion.sh test_expect_success 'git checkout - with --detach, complete only references' ' · line 1535

Quoted 10 lines

t/t9902-completion.sh +7 -0

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

From: Lessley Dennington <[email protected]>

Update the __gitcomp_directories method to de-quote and handle unusual characters in directory names. Although this initially involved an attempt to re-use the logic in __git_index_files, this method removed subdirectories (e.g. folder1/0/ became folder1/), so instead new custom logic was placed directly in the __gitcomp_directories method.

Co-authored-by Johannes Schindelin <[email protected]>

Co-authored-by Lessley Dennington <[email protected]>

Signed-off-by Lessley Dennington <[email protected]>

---
 contrib/completion/git-completion.bash | 50 +++++++++++++-------------
 t/t9902-completion.sh                  | 28 +++++++++++++++
 2 files changed, 52 insertions(+), 26 deletions(-)

contrib/completion/git-completion.bash +14 -26

index c5c8df6b6e5..c47e9ce09b2 100644
@@ -2988,32 +2988,30 @@ _git_show_branch ()
 
 __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
+       local _tmp_dir _tmp_completions _found=0
+
+       # 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,
+       # de-quoting, and handling unusual characters.
+       while IFS= read -r -d $'\0' c ; do
+               # If there are directory completions, find ones that start
+               # with "$cur", the current token, and put those in COMPREPLY
+               if [[ $c == "$cur"* ]]; then

contrib/completion/git-completion.bash +0 -0

 }
 
 _git_sparse_checkout ()

t/t9902-completion.sh +17 -0

index b38a7302249..7f63d6057be 100755
@@ -1508,6 +1508,34 @@ test_expect_success 'cone mode sparse-checkout completes directory names' '
        )
 '
 
+# use FUNNYNAMES to avoid running on Windows, which doesn't permit backslashes in paths
+test_expect_success FUNNYNAMES 'cone mode sparse-checkout completes directory names with special characters' '
+       # reset sparse-checkout
+       git -C sparse-checkout sparse-checkout disable &&
+       (
+               cd sparse-checkout &&
+               mkdir "directory with spaces" &&
+               mkdir "$(printf "directory\twith\ttabs")" &&
+               mkdir "directory\with\backslashes" &&
+               mkdir "directory-with-áccent" &&
+               >"directory with spaces/randomfile" &&
+               >"$(printf "directory\twith\ttabs")/randomfile" &&
+               >"directory\with\backslashes/randomfile" &&
+               >"directory-with-áccent/randomfile" &&
+               git add . &&
+               git commit -m "Add directories containing unusual characters" &&
+               git sparse-checkout set --cone "directory with spaces" \