git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* [PATCH 0/2] sparse checkout: custom bash completion updates
@ 2021-12-30  0:32 Lessley Dennington via GitGitGadget
  2021-12-30  0:32 ` [PATCH 1/2] sparse-checkout: custom tab completion tests Lessley Dennington via GitGitGadget
                   ` (2 more replies)
  0 siblings, 3 replies; 51+ messages in thread
From: Lessley Dennington via GitGitGadget @ 2021-12-30  0:32 UTC (permalink / raw)
  To: git; +Cc: stolee, gitster, johannes.schindelin, Lessley Dennington

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

Thanks, Lessley

Lessley Dennington (2):
  sparse-checkout: custom tab completion tests
  sparse-checkout: custom tab completion

 contrib/completion/git-completion.bash | 34 +++++++-----
 t/t9902-completion.sh                  | 74 ++++++++++++++++++++++++++
 2 files changed, 96 insertions(+), 12 deletions(-)


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

^ permalink raw reply	[flat|nested] 51+ messages in thread

* [PATCH 1/2] sparse-checkout: custom tab completion tests
  2021-12-30  0:32 [PATCH 0/2] sparse checkout: custom bash completion updates Lessley Dennington via GitGitGadget
@ 2021-12-30  0:32 ` Lessley Dennington via GitGitGadget
  2021-12-30 13:43   ` Derrick Stolee
  2021-12-30  0:32 ` [PATCH 2/2] sparse-checkout: custom tab completion Lessley Dennington via GitGitGadget
  2021-12-30 19:26 ` [PATCH v2 0/2] sparse checkout: custom bash completion updates Lessley Dennington via GitGitGadget
  2 siblings, 1 reply; 51+ messages in thread
From: Lessley Dennington via GitGitGadget @ 2021-12-30  0:32 UTC (permalink / raw)
  To: git
  Cc: stolee, gitster, johannes.schindelin, Lessley Dennington,
	Lessley Dennington

From: Lessley Dennington <lessleydennington@gmail.com>

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 <lessleydennington@gmail.com>
---
 t/t9902-completion.sh | 74 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 74 insertions(+)

diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 0f28c4ad940..22271ac2f3b 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -1444,6 +1444,80 @@ 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
+	--sparse-index Z
+	--no-sparse-index Z
+	EOF
+
+	test_completion "git sparse-checkout set --" <<-\EOF &&
+	--stdin 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 config index.sparse false &&
+	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


^ permalink raw reply	[flat|nested] 51+ messages in thread

* [PATCH 2/2] sparse-checkout: custom tab completion
  2021-12-30  0:32 [PATCH 0/2] sparse checkout: custom bash completion updates Lessley Dennington via GitGitGadget
  2021-12-30  0:32 ` [PATCH 1/2] sparse-checkout: custom tab completion tests Lessley Dennington via GitGitGadget
@ 2021-12-30  0:32 ` Lessley Dennington via GitGitGadget
  2021-12-30 13:50   ` Derrick Stolee
  2021-12-30 19:26 ` [PATCH v2 0/2] sparse checkout: custom bash completion updates Lessley Dennington via GitGitGadget
  2 siblings, 1 reply; 51+ messages in thread
From: Lessley Dennington via GitGitGadget @ 2021-12-30  0:32 UTC (permalink / raw)
  To: git
  Cc: stolee, gitster, johannes.schindelin, Lessley Dennington,
	Lessley Dennington

From: Lessley Dennington <lessleydennington@gmail.com>

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

1. The full list of subcommands is provided when users enter git
sparse-checkout <TAB>.
2. The --help option is tab-completable.
3. Subcommand options are tab-completable.
4. A list of directories (but not files) is provided when users enter git
sparse-checkout add <TAB> or git sparse-checkout set <TAB>.

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

Signed-off-by: Lessley Dennington <lessleydennington@gmail.com>
---
 contrib/completion/git-completion.bash | 34 +++++++++++++++++---------
 t/t9902-completion.sh                  |  8 +++---
 2 files changed, 26 insertions(+), 16 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 377d6c5494a..b8f1caece83 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -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
-		__gitcomp "$subcommands"
-		return
+		case "$cur" in
+			--*)
+				__gitcomp "--help"
+				;;
+			*)
+				__gitcomp "$subcommands"
+				;;
+		esac
 	fi
 
-	case "$subcommand,$cur" in
-	init,--*)
-		__gitcomp "--cone"
-		;;
-	set,--*)
-		__gitcomp "--stdin"
-		;;
-	*)
-		;;
+	case "$prev" in
+		init)
+			__gitcomp "$__git_sparse_checkout_init_opts"
+			;;
+		add|set)
+			__gitcomp "--stdin"
+			__gitcomp "$(git ls-tree -d -r HEAD --name-only)"
+			;;
+		*)
+			;;
 	esac
 }
 
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 22271ac2f3b..9f6eb06fbab 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -1444,7 +1444,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
@@ -1455,13 +1455,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
 	--sparse-index Z
@@ -1477,7 +1477,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

^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 1/2] sparse-checkout: custom tab completion tests
  2021-12-30  0:32 ` [PATCH 1/2] sparse-checkout: custom tab completion tests Lessley Dennington via GitGitGadget
@ 2021-12-30 13:43   ` Derrick Stolee
  2021-12-30 16:19     ` Lessley Dennington
  0 siblings, 1 reply; 51+ messages in thread
From: Derrick Stolee @ 2021-12-30 13:43 UTC (permalink / raw)
  To: Lessley Dennington via GitGitGadget, git
  Cc: gitster, johannes.schindelin, Lessley Dennington

On 12/29/2021 7:32 PM, Lessley Dennington via GitGitGadget wrote:
> From: Lessley Dennington <lessleydennington@gmail.com>
> 
> 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.

Thanks for these. I've never looked at this test script before, but
I can surmise how it works from your tests.
 
> Although these tests currently fail, they will succeed with the
> sparse-checkout modifications in git-completion.bash in the next commit in
> this series.

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

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

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

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.

> +		EOF
> +	) &&
> +
> +	(
> +		cd sparse-checkout/folder1 &&
> +		test_completion "git sparse-checkout add " <<-\EOF
> +		./ Z
> +		0 Z
> +		0/1 Z
> +		EOF
I do like seeing this test within a directory. Thanks!

-Stolee

^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 2/2] sparse-checkout: custom tab completion
  2021-12-30  0:32 ` [PATCH 2/2] sparse-checkout: custom tab completion Lessley Dennington via GitGitGadget
@ 2021-12-30 13:50   ` Derrick Stolee
  2021-12-30 16:24     ` Lessley Dennington
  0 siblings, 1 reply; 51+ messages in thread
From: Derrick Stolee @ 2021-12-30 13:50 UTC (permalink / raw)
  To: Lessley Dennington via GitGitGadget, git
  Cc: gitster, johannes.schindelin, Lessley Dennington

On 12/29/2021 7:32 PM, Lessley Dennington via GitGitGadget wrote:
> From: Lessley Dennington <lessleydennington@gmail.com>
> 
> Fix custom tab completion for sparse-checkout command. This will ensure:
> 
> 1. The full list of subcommands is provided when users enter git
> sparse-checkout <TAB>.
> 2. The --help option is tab-completable.
> 3. Subcommand options are tab-completable.
> 4. A list of directories (but not files) is provided when users enter git
> sparse-checkout add <TAB> or git sparse-checkout set <TAB>.
> 
> Failing tests that were added in the previous commit to verify these
> scenarios are now passing with these updates.
> 
> Signed-off-by: Lessley Dennington <lessleydennington@gmail.com>
> ---
>  contrib/completion/git-completion.bash | 34 +++++++++++++++++---------
>  t/t9902-completion.sh                  |  8 +++---
>  2 files changed, 26 insertions(+), 16 deletions(-)
> 
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 377d6c5494a..b8f1caece83 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -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
> -		__gitcomp "$subcommands"
> -		return
> +		case "$cur" in
> +			--*)
> +				__gitcomp "--help"
> +				;;
> +			*)
> +				__gitcomp "$subcommands"
> +				;;
> +		esac

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

>  	fi
>  
> -	case "$subcommand,$cur" in
> -	init,--*)
> -		__gitcomp "--cone"
> -		;;
> -	set,--*)
> -		__gitcomp "--stdin"
> -		;;
> -	*)
> -		;;
> +	case "$prev" in
> +		init)
> +			__gitcomp "$__git_sparse_checkout_init_opts"
> +			;;
> +		add|set)
> +			__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.

Thanks,
-Stolee

^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 1/2] sparse-checkout: custom tab completion tests
  2021-12-30 13:43   ` Derrick Stolee
@ 2021-12-30 16:19     ` Lessley Dennington
  2021-12-30 17:43       ` Derrick Stolee
  0 siblings, 1 reply; 51+ messages in thread
From: Lessley Dennington @ 2021-12-30 16:19 UTC (permalink / raw)
  To: Derrick Stolee, Lessley Dennington via GitGitGadget, git
  Cc: gitster, johannes.schindelin

On 12/30/21 7:43 AM, Derrick Stolee wrote:
>> +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).
> 
Ah great point - I was going off of the sparse-checkout docs and
forgot about Elijah's changes. I will do the rebase and make your
suggested corrections in v2.
>> +	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.
> 
I'll plan to remove in v2 per your comments in [1].>> +	# 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
> 
> 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.)
> 
Yes, it does.
> 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

-Lessley

[1]: 
https://lore.kernel.org/git/c79ccf4a-4da9-1c60-32eb-124d3fa94400@gmail.com/

^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 2/2] sparse-checkout: custom tab completion
  2021-12-30 13:50   ` Derrick Stolee
@ 2021-12-30 16:24     ` Lessley Dennington
  0 siblings, 0 replies; 51+ messages in thread
From: Lessley Dennington @ 2021-12-30 16:24 UTC (permalink / raw)
  To: Derrick Stolee, Lessley Dennington via GitGitGadget, git
  Cc: gitster, johannes.schindelin

On 12/30/21 7:50 AM, Derrick Stolee wrote:
>> +__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"
>> +				;;
>> +			*)
>> +				__gitcomp "$subcommands"
>> +				;;
>> +		esac
> 
> This part fixes the --<tab> completion. I suppose if someone
> did "-<tab>" then nothing would show up?
> 
It actually shows a list of files that contain -- (if any exist).
>>   	fi
>>   
>> -	case "$subcommand,$cur" in
>> -	init,--*)
>> -		__gitcomp "--cone"
>> -		;;
>> -	set,--*)
>> -		__gitcomp "--stdin"
>> -		;;
>> -	*)
>> -		;;
>> +	case "$prev" in
>> +		init)
>> +			__gitcomp "$__git_sparse_checkout_init_opts"
>> +			;;
>> +		add|set)
>> +			__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.
> 
Thanks, I'll give this a go!
> 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.
> 
Will correct in v2.

-Lessley

^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 1/2] sparse-checkout: custom tab completion tests
  2021-12-30 16:19     ` Lessley Dennington
@ 2021-12-30 17:43       ` Derrick Stolee
  2021-12-31 19:27         ` Elijah Newren
  0 siblings, 1 reply; 51+ messages in thread
From: Derrick Stolee @ 2021-12-30 17:43 UTC (permalink / raw)
  To: Lessley Dennington, Lessley Dennington via GitGitGadget, git
  Cc: gitster, johannes.schindelin

On 12/30/2021 11:19 AM, Lessley Dennington wrote:
> On 12/30/21 7:43 AM, Derrick Stolee wrote:

>>> +    (
>>> +        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.)
>>
> Yes, it does.
>> 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

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.

Thanks,
-Stolee

^ permalink raw reply	[flat|nested] 51+ messages in thread

* [PATCH v2 0/2] sparse checkout: custom bash completion updates
  2021-12-30  0:32 [PATCH 0/2] sparse checkout: custom bash completion updates Lessley Dennington via GitGitGadget
  2021-12-30  0:32 ` [PATCH 1/2] sparse-checkout: custom tab completion tests Lessley Dennington via GitGitGadget
  2021-12-30  0:32 ` [PATCH 2/2] sparse-checkout: custom tab completion Lessley Dennington via GitGitGadget
@ 2021-12-30 19:26 ` Lessley Dennington via GitGitGadget
  2021-12-30 19:26   ` [PATCH v2 1/2] sparse-checkout: custom tab completion tests Lessley Dennington via GitGitGadget
                     ` (2 more replies)
  2 siblings, 3 replies; 51+ messages in thread
From: Lessley Dennington via GitGitGadget @ 2021-12-30 19:26 UTC (permalink / raw)
  To: git; +Cc: stolee, gitster, johannes.schindelin, Lessley Dennington

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.


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 (2):
  sparse-checkout: custom tab completion tests
  sparse-checkout: custom tab completion

 contrib/completion/git-completion.bash | 38 ++++++++----
 t/t9902-completion.sh                  | 85 ++++++++++++++++++++++++++
 2 files changed, 111 insertions(+), 12 deletions(-)


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

Range-diff vs v1:

 1:  a7f3ae5cdda ! 1:  955fcab0052 sparse-checkout: custom tab completion tests
     @@ t/t9902-completion.sh: test_expect_success 'git checkout - with --detach, comple
      +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
     @@ t/t9902-completion.sh: test_expect_success 'git checkout - with --detach, comple
      +	) &&
      +
      +	# initialize sparse-checkout definitions
     -+	git -C sparse-checkout config index.sparse false &&
      +	git -C sparse-checkout sparse-checkout init --cone &&
      +	git -C sparse-checkout sparse-checkout set folder1/0 folder3 &&
      +
 2:  ab51236d18c ! 2:  cecf501e076 sparse-checkout: custom tab completion
     @@ contrib/completion/git-completion.bash: _git_show_branch ()
       	__git_complete_revlist
       }
       
     -+__git_sparse_checkout_init_opts="--cone --sparse-index --no-sparse-index"
     ++__git_sparse_checkout_subcommand_opts="--cone --no-cone --sparse-index --no-sparse-index"
      +
       _git_sparse_checkout ()
       {
     @@ contrib/completion/git-completion.bash: _git_show_branch ()
      -	*)
      -		;;
      +	case "$prev" in
     -+		init)
     -+			__gitcomp "$__git_sparse_checkout_init_opts"
     ++		set)
     ++			__gitcomp "$__git_sparse_checkout_subcommand_opts --stdin"
     ++			__gitcomp "$(git ls-tree -d -r HEAD --name-only)"
      +			;;
     -+		add|set)
     ++		add)
      +			__gitcomp "--stdin"
      +			__gitcomp "$(git ls-tree -d -r HEAD --name-only)"
      +			;;
     ++		init|reapply)
     ++			__gitcomp "$__git_sparse_checkout_subcommand_opts"
     ++			;;
      +		*)
      +			;;
       	esac
     @@ t/t9902-completion.sh: test_expect_failure 'sparse-checkout completes subcommand
      +test_expect_success 'sparse-checkout completes subcommand options' '
       	test_completion "git sparse-checkout init --" <<-\EOF &&
       	--cone Z
     - 	--sparse-index Z
     + 	--no-cone Z
      @@ t/t9902-completion.sh: test_expect_failure 'sparse-checkout completes subcommand options' '
       	EOF
       '

-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 51+ messages in thread

* [PATCH v2 1/2] sparse-checkout: custom tab completion tests
  2021-12-30 19:26 ` [PATCH v2 0/2] sparse checkout: custom bash completion updates Lessley Dennington via GitGitGadget
@ 2021-12-30 19:26   ` Lessley Dennington via GitGitGadget
  2021-12-31 20:03     ` Elijah Newren
  2021-12-30 19:26   ` [PATCH v2 2/2] sparse-checkout: custom tab completion Lessley Dennington via GitGitGadget
  2022-01-10 18:59   ` [PATCH v3 0/3] sparse checkout: custom bash completion updates Lessley Dennington via GitGitGadget
  2 siblings, 1 reply; 51+ messages in thread
From: Lessley Dennington via GitGitGadget @ 2021-12-30 19:26 UTC (permalink / raw)
  To: git
  Cc: stolee, gitster, johannes.schindelin, Lessley Dennington,
	Lessley Dennington

From: Lessley Dennington <lessleydennington@gmail.com>

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 <lessleydennington@gmail.com>
---
 t/t9902-completion.sh | 85 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 85 insertions(+)

diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 518203fbe07..51d0f2d93a1 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -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


^ permalink raw reply	[flat|nested] 51+ messages in thread

* [PATCH v2 2/2] sparse-checkout: custom tab completion
  2021-12-30 19:26 ` [PATCH v2 0/2] sparse checkout: custom bash completion updates Lessley Dennington via GitGitGadget
  2021-12-30 19:26   ` [PATCH v2 1/2] sparse-checkout: custom tab completion tests Lessley Dennington via GitGitGadget
@ 2021-12-30 19:26   ` Lessley Dennington via GitGitGadget
  2021-12-31 22:52     ` Elijah Newren
  2022-01-10 18:59   ` [PATCH v3 0/3] sparse checkout: custom bash completion updates Lessley Dennington via GitGitGadget
  2 siblings, 1 reply; 51+ messages in thread
From: Lessley Dennington via GitGitGadget @ 2021-12-30 19:26 UTC (permalink / raw)
  To: git
  Cc: stolee, gitster, johannes.schindelin, Lessley Dennington,
	Lessley Dennington

From: Lessley Dennington <lessleydennington@gmail.com>

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

1. The full list of subcommands is provided when users enter git
sparse-checkout <TAB>.
2. The --help option is tab-completable.
3. Subcommand options are tab-completable.
4. A list of directories (but not files) is provided when users enter git
sparse-checkout add <TAB> or git sparse-checkout set <TAB>.

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

Signed-off-by: Lessley Dennington <lessleydennington@gmail.com>
---
 contrib/completion/git-completion.bash | 38 ++++++++++++++++++--------
 t/t9902-completion.sh                  |  8 +++---
 2 files changed, 30 insertions(+), 16 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index c82ccaebcc7..7de997ee64e 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2986,24 +2986,38 @@ _git_show_branch ()
 	__git_complete_revlist
 }
 
+__git_sparse_checkout_subcommand_opts="--cone --no-cone --sparse-index --no-sparse-index"
+
 _git_sparse_checkout ()
 {
-	local subcommands="list init set disable"
+	local subcommands="list init set disable add reapply"
 	local subcommand="$(__git_find_on_cmdline "$subcommands")"
+
 	if [ -z "$subcommand" ]; then
-		__gitcomp "$subcommands"
-		return
+		case "$cur" in
+			--*)
+				__gitcomp "--help"
+				;;
+			*)
+				__gitcomp "$subcommands"
+				;;
+		esac
 	fi
 
-	case "$subcommand,$cur" in
-	init,--*)
-		__gitcomp "--cone"
-		;;
-	set,--*)
-		__gitcomp "--stdin"
-		;;
-	*)
-		;;
+	case "$prev" in
+		set)
+			__gitcomp "$__git_sparse_checkout_subcommand_opts --stdin"
+			__gitcomp "$(git ls-tree -d -r HEAD --name-only)"
+			;;
+		add)
+			__gitcomp "--stdin"
+			__gitcomp "$(git ls-tree -d -r HEAD --name-only)"
+			;;
+		init|reapply)
+			__gitcomp "$__git_sparse_checkout_subcommand_opts"
+			;;
+		*)
+			;;
 	esac
 }
 
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 51d0f2d93a1..4dc93ee0595 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -1447,7 +1447,7 @@ test_expect_success 'git checkout - with --detach, complete only references' '
 	EOF
 '
 
-test_expect_failure 'sparse-checkout completes subcommands' '
+test_expect_success 'sparse-checkout completes subcommands' '
 	test_completion "git sparse-checkout " <<-\EOF
 	list Z
 	init Z
@@ -1458,13 +1458,13 @@ test_expect_failure 'sparse-checkout completes subcommands' '
 	EOF
 '
 
-test_expect_failure 'sparse-checkout completes options' '
+test_expect_success 'sparse-checkout completes options' '
 	test_completion "git sparse-checkout --" <<-\EOF
 	--help Z
 	EOF
 '
 
-test_expect_failure 'sparse-checkout completes subcommand options' '
+test_expect_success 'sparse-checkout completes subcommand options' '
 	test_completion "git sparse-checkout init --" <<-\EOF &&
 	--cone Z
 	--no-cone Z
@@ -1492,7 +1492,7 @@ test_expect_failure 'sparse-checkout completes subcommand options' '
 	EOF
 '
 
-test_expect_failure 'sparse-checkout completes directory names' '
+test_expect_success 'sparse-checkout completes directory names' '
 	# set up sparse-checkout repo
 	git init sparse-checkout &&
 	(
-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 1/2] sparse-checkout: custom tab completion tests
  2021-12-30 17:43       ` Derrick Stolee
@ 2021-12-31 19:27         ` Elijah Newren
  2022-01-04 19:19           ` Lessley Dennington
  0 siblings, 1 reply; 51+ messages in thread
From: Elijah Newren @ 2021-12-31 19:27 UTC (permalink / raw)
  To: Derrick Stolee
  Cc: Lessley Dennington, Lessley Dennington via GitGitGadget,
	Git Mailing List, Junio C Hamano, johannes.schindelin

On Fri, Dec 31, 2021 at 2:30 AM Derrick Stolee <stolee@gmail.com> wrote:
>
> On 12/30/2021 11:19 AM, Lessley Dennington wrote:
> > On 12/30/21 7:43 AM, Derrick Stolee wrote:
>
> >>> +    (
> >>> +        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.)
> >>
> > Yes, it does.
> >> 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
>
> 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.

^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH v2 1/2] sparse-checkout: custom tab completion tests
  2021-12-30 19:26   ` [PATCH v2 1/2] sparse-checkout: custom tab completion tests Lessley Dennington via GitGitGadget
@ 2021-12-31 20:03     ` Elijah Newren
  2021-12-31 22:20       ` Junio C Hamano
  2022-01-04 19:24       ` Lessley Dennington
  0 siblings, 2 replies; 51+ messages in thread
From: Elijah Newren @ 2021-12-31 20:03 UTC (permalink / raw)
  To: Lessley Dennington via GitGitGadget
  Cc: Git Mailing List, Derrick Stolee, Junio C Hamano,
	johannes.schindelin, Lessley Dennington

On Fri, Dec 31, 2021 at 2:32 AM Lessley Dennington via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Lessley Dennington <lessleydennington@gmail.com>
>
> 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.

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/6e09ab19-7ffb-e58e-7b08-6e560b421c06@gmail.com/


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.

> 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 <lessleydennington@gmail.com>
> ---
>  t/t9902-completion.sh | 85 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 85 insertions(+)
>
> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> index 518203fbe07..51d0f2d93a1 100755
> --- a/t/t9902-completion.sh
> +++ b/t/t9902-completion.sh
> @@ -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

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.

^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH v2 1/2] sparse-checkout: custom tab completion tests
  2021-12-31 20:03     ` Elijah Newren
@ 2021-12-31 22:20       ` Junio C Hamano
  2021-12-31 22:25         ` Elijah Newren
  2022-01-04 19:25         ` Lessley Dennington
  2022-01-04 19:24       ` Lessley Dennington
  1 sibling, 2 replies; 51+ messages in thread
From: Junio C Hamano @ 2021-12-31 22:20 UTC (permalink / raw)
  To: Elijah Newren
  Cc: Lessley Dennington via GitGitGadget, Git Mailing List,
	Derrick Stolee, johannes.schindelin, Lessley Dennington

Elijah Newren <newren@gmail.com> writes:

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


^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH v2 1/2] sparse-checkout: custom tab completion tests
  2021-12-31 22:20       ` Junio C Hamano
@ 2021-12-31 22:25         ` Elijah Newren
  2022-01-04 19:25         ` Lessley Dennington
  1 sibling, 0 replies; 51+ messages in thread
From: Elijah Newren @ 2021-12-31 22:25 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Lessley Dennington via GitGitGadget, Git Mailing List,
	Derrick Stolee, johannes.schindelin, Lessley Dennington

On Fri, Dec 31, 2021 at 2:21 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Elijah Newren <newren@gmail.com> writes:
>
> > 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.

Oh, sorry, I didn't mean to imply that.  I meant the same thing you
say here: it's another bug we need to fix, in a separate series.

^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH v2 2/2] sparse-checkout: custom tab completion
  2021-12-30 19:26   ` [PATCH v2 2/2] sparse-checkout: custom tab completion Lessley Dennington via GitGitGadget
@ 2021-12-31 22:52     ` Elijah Newren
  2022-01-04 19:41       ` Lessley Dennington
  0 siblings, 1 reply; 51+ messages in thread
From: Elijah Newren @ 2021-12-31 22:52 UTC (permalink / raw)
  To: Lessley Dennington via GitGitGadget
  Cc: Git Mailing List, Derrick Stolee, Junio C Hamano,
	johannes.schindelin, Lessley Dennington

On Fri, Dec 31, 2021 at 2:33 AM Lessley Dennington via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Lessley Dennington <lessleydennington@gmail.com>
>
> Fix custom tab completion for sparse-checkout command. This will ensure:
>
> 1. The full list of subcommands is provided when users enter git
> sparse-checkout <TAB>.
> 2. The --help option is tab-completable.
> 3. Subcommand options are tab-completable.
> 4. A list of directories (but not files) is provided when users enter git
> sparse-checkout add <TAB> or git sparse-checkout set <TAB>.
>
> Failing tests that were added in the previous commit to verify these
> scenarios are now passing with these updates.
>
> Signed-off-by: Lessley Dennington <lessleydennington@gmail.com>
> ---
>  contrib/completion/git-completion.bash | 38 ++++++++++++++++++--------
>  t/t9902-completion.sh                  |  8 +++---
>  2 files changed, 30 insertions(+), 16 deletions(-)
>
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index c82ccaebcc7..7de997ee64e 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -2986,24 +2986,38 @@ _git_show_branch ()
>         __git_complete_revlist
>  }
>
> +__git_sparse_checkout_subcommand_opts="--cone --no-cone --sparse-index --no-sparse-index"
> +
>  _git_sparse_checkout ()
>  {
> -       local subcommands="list init set disable"
> +       local subcommands="list init set disable add reapply"
>         local subcommand="$(__git_find_on_cmdline "$subcommands")"
> +
>         if [ -z "$subcommand" ]; then
> -               __gitcomp "$subcommands"
> -               return
> +               case "$cur" in
> +                       --*)
> +                               __gitcomp "--help"
> +                               ;;
> +                       *)
> +                               __gitcomp "$subcommands"
> +                               ;;
> +               esac
>         fi
>
> -       case "$subcommand,$cur" in
> -       init,--*)
> -               __gitcomp "--cone"
> -               ;;
> -       set,--*)
> -               __gitcomp "--stdin"
> -               ;;
> -       *)
> -               ;;
> +       case "$prev" in

Shouldn't this be "$subcommand" rather than "$prev"?  I think with
prev, it will only correctly complete the first path after "set",
"add", etc.

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

I was going to make a simple suggestion for making it just complete
one additional level at a time and leaving out the -r, and then tried
it out and found out it wasn't simple.  I got something working,
eventually, but it's slightly ugly...so it probably belongs in a
separate patch anyway.  If you're curious, it's basically replacing
the second __gitcomp... call for each of set and add with
`__gitcomp_directories`, after first defining:

__gitcomp_directories ()
{
    local _tmp_dir _tmp_completions

    # Get the directory of the current token; this differs from dirname
    # in that it keeps up to the final trailing slash.  If no slash found
    # that's fine too.
    [[ "$cur" =~ .*/ ]]
    _tmp_dir=$BASH_REMATCH

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

    if [[ -n "$_tmp_completions" ]]; then
        # There were some directory completions, so find ones that
        # start with "$cur", the current token, and put those in COMPREPLY
        local i=0 c IFS=$' \t\n'
        for c in $_tmp_completions; do
            if [[ $c == "$cur"* ]]; then
                COMPREPLY+=("$c")
            fi
        done
    elif [[ "$cur" =~ /$ ]]; then
        # No possible further completions any deeper, so assume we're at
        # a leaf directory and just consider it complete
        __gitcomp_direct_append "$cur "
    fi
}

But I don't think that needs to be part of this series; I can submit
it later and hopefully get a completion expert to point out
better/cleaner ways of what I've done above.

> +                       ;;
> +               init|reapply)
> +                       __gitcomp "$__git_sparse_checkout_subcommand_opts"
> +                       ;;
> +               *)
> +                       ;;
>         esac
>  }
>
> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> index 51d0f2d93a1..4dc93ee0595 100755
> --- a/t/t9902-completion.sh
> +++ b/t/t9902-completion.sh
> @@ -1447,7 +1447,7 @@ test_expect_success 'git checkout - with --detach, complete only references' '
>         EOF
>  '
>
> -test_expect_failure 'sparse-checkout completes subcommands' '
> +test_expect_success 'sparse-checkout completes subcommands' '
>         test_completion "git sparse-checkout " <<-\EOF
>         list Z
>         init Z
> @@ -1458,13 +1458,13 @@ test_expect_failure 'sparse-checkout completes subcommands' '
>         EOF
>  '
>
> -test_expect_failure 'sparse-checkout completes options' '
> +test_expect_success 'sparse-checkout completes options' '
>         test_completion "git sparse-checkout --" <<-\EOF
>         --help Z
>         EOF
>  '
>
> -test_expect_failure 'sparse-checkout completes subcommand options' '
> +test_expect_success 'sparse-checkout completes subcommand options' '
>         test_completion "git sparse-checkout init --" <<-\EOF &&
>         --cone Z
>         --no-cone Z
> @@ -1492,7 +1492,7 @@ test_expect_failure 'sparse-checkout completes subcommand options' '
>         EOF
>  '
>
> -test_expect_failure 'sparse-checkout completes directory names' '
> +test_expect_success 'sparse-checkout completes directory names' '
>         # set up sparse-checkout repo
>         git init sparse-checkout &&
>         (
> --
> gitgitgadget

Patch looks good to me, other than the $prev vs. $subcommand thing.

^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 1/2] sparse-checkout: custom tab completion tests
  2021-12-31 19:27         ` Elijah Newren
@ 2022-01-04 19:19           ` Lessley Dennington
  0 siblings, 0 replies; 51+ messages in thread
From: Lessley Dennington @ 2022-01-04 19:19 UTC (permalink / raw)
  To: Elijah Newren, Derrick Stolee
  Cc: Lessley Dennington via GitGitGadget, Git Mailing List,
	Junio C Hamano, johannes.schindelin



On 12/31/21 1:27 PM, Elijah Newren wrote:
> On Fri, Dec 31, 2021 at 2:30 AM Derrick Stolee <stolee@gmail.com> wrote:
>>
>> On 12/30/2021 11:19 AM, Lessley Dennington wrote:
>>> On 12/30/21 7:43 AM, Derrick Stolee wrote:
>>
>>>>> +    (
>>>>> +        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.)
>>>>
>>> Yes, it does.
>>>> 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
>>
>> 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.

Thank you for the feedback! This is fine by me - I'll respond further to
your comments in [1].

[1] 
https://lore.kernel.org/git/CABPp-BG_Jgyr89z_D355Ytzz31J40nBGX=2cr+aXtcf3U1y6Dg@mail.gmail.com/

^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH v2 1/2] sparse-checkout: custom tab completion tests
  2021-12-31 20:03     ` Elijah Newren
  2021-12-31 22:20       ` Junio C Hamano
@ 2022-01-04 19:24       ` Lessley Dennington
  1 sibling, 0 replies; 51+ messages in thread
From: Lessley Dennington @ 2022-01-04 19:24 UTC (permalink / raw)
  To: Elijah Newren, Lessley Dennington via GitGitGadget
  Cc: Git Mailing List, Derrick Stolee, Junio C Hamano, johannes.schindelin



On 12/31/21 2:03 PM, Elijah Newren wrote:
> On Fri, Dec 31, 2021 at 2:32 AM Lessley Dennington via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>>
>> From: Lessley Dennington <lessleydennington@gmail.com>
>>
>> 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.
> 
> 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/6e09ab19-7ffb-e58e-7b08-6e560b421c06@gmail.com/
> 
> 
I'm still supportive of this 😊. I'll chat with others who are heavily
involved with sparse-checkout to make sure I have buy-in. If so, I can
pick it up as my next work item.
> 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.
> 
>> 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 <lessleydennington@gmail.com>
>> ---
>>   t/t9902-completion.sh | 85 +++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 85 insertions(+)
>>
>> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
>> index 518203fbe07..51d0f2d93a1 100755
>> --- a/t/t9902-completion.sh
>> +++ b/t/t9902-completion.sh
>> @@ -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
> 
> 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.

Can do! Stay tuned for v3.

^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH v2 1/2] sparse-checkout: custom tab completion tests
  2021-12-31 22:20       ` Junio C Hamano
  2021-12-31 22:25         ` Elijah Newren
@ 2022-01-04 19:25         ` Lessley Dennington
  2022-01-04 20:25           ` Elijah Newren
  1 sibling, 1 reply; 51+ messages in thread
From: Lessley Dennington @ 2022-01-04 19:25 UTC (permalink / raw)
  To: Junio C Hamano, Elijah Newren
  Cc: Lessley Dennington via GitGitGadget, Git Mailing List,
	Derrick Stolee, johannes.schindelin



On 12/31/21 4:20 PM, Junio C Hamano wrote:
> Elijah Newren <newren@gmail.com> writes:
> 
>> 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.
> 
I can file a bug report if that's the correct way to proceed.

^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH v2 2/2] sparse-checkout: custom tab completion
  2021-12-31 22:52     ` Elijah Newren
@ 2022-01-04 19:41       ` Lessley Dennington
  2022-01-04 20:42         ` Elijah Newren
  0 siblings, 1 reply; 51+ messages in thread
From: Lessley Dennington @ 2022-01-04 19:41 UTC (permalink / raw)
  To: Elijah Newren, Lessley Dennington via GitGitGadget
  Cc: Git Mailing List, Derrick Stolee, Junio C Hamano, johannes.schindelin



On 12/31/21 4:52 PM, Elijah Newren wrote:
> On Fri, Dec 31, 2021 at 2:33 AM Lessley Dennington via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>>
>> From: Lessley Dennington <lessleydennington@gmail.com>
>>
>> Fix custom tab completion for sparse-checkout command. This will ensure:
>>
>> 1. The full list of subcommands is provided when users enter git
>> sparse-checkout <TAB>.
>> 2. The --help option is tab-completable.
>> 3. Subcommand options are tab-completable.
>> 4. A list of directories (but not files) is provided when users enter git
>> sparse-checkout add <TAB> or git sparse-checkout set <TAB>.
>>
>> Failing tests that were added in the previous commit to verify these
>> scenarios are now passing with these updates.
>>
>> Signed-off-by: Lessley Dennington <lessleydennington@gmail.com>
>> ---
>>   contrib/completion/git-completion.bash | 38 ++++++++++++++++++--------
>>   t/t9902-completion.sh                  |  8 +++---
>>   2 files changed, 30 insertions(+), 16 deletions(-)
>>
>> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
>> index c82ccaebcc7..7de997ee64e 100644
>> --- a/contrib/completion/git-completion.bash
>> +++ b/contrib/completion/git-completion.bash
>> @@ -2986,24 +2986,38 @@ _git_show_branch ()
>>          __git_complete_revlist
>>   }
>>
>> +__git_sparse_checkout_subcommand_opts="--cone --no-cone --sparse-index --no-sparse-index"
>> +
>>   _git_sparse_checkout ()
>>   {
>> -       local subcommands="list init set disable"
>> +       local subcommands="list init set disable add reapply"
>>          local subcommand="$(__git_find_on_cmdline "$subcommands")"
>> +
>>          if [ -z "$subcommand" ]; then
>> -               __gitcomp "$subcommands"
>> -               return
>> +               case "$cur" in
>> +                       --*)
>> +                               __gitcomp "--help"
>> +                               ;;
>> +                       *)
>> +                               __gitcomp "$subcommands"
>> +                               ;;
>> +               esac
>>          fi
>>
>> -       case "$subcommand,$cur" in
>> -       init,--*)
>> -               __gitcomp "--cone"
>> -               ;;
>> -       set,--*)
>> -               __gitcomp "--stdin"
>> -               ;;
>> -       *)
>> -               ;;
>> +       case "$prev" in
> 
> Shouldn't this be "$subcommand" rather than "$prev"?  I think with
> prev, it will only correctly complete the first path after "set",
> "add", etc.
> 
Good catch, thank you! Fixing in v3.
>> +               set)
>> +                       __gitcomp "$__git_sparse_checkout_subcommand_opts --stdin"
>> +                       __gitcomp "$(git ls-tree -d -r HEAD --name-only)"
>> +                       ;;
>> +               add)
>> +                       __gitcomp "--stdin"
>> +                       __gitcomp "$(git ls-tree -d -r HEAD --name-only)"
> 
> I was going to make a simple suggestion for making it just complete
> one additional level at a time and leaving out the -r, and then tried
> it out and found out it wasn't simple.  I got something working,
> eventually, but it's slightly ugly...so it probably belongs in a
> separate patch anyway.  If you're curious, it's basically replacing
> the second __gitcomp... call for each of set and add with
> `__gitcomp_directories`, after first defining:
> 
> __gitcomp_directories ()
> {
>      local _tmp_dir _tmp_completions
> 
>      # Get the directory of the current token; this differs from dirname
>      # in that it keeps up to the final trailing slash.  If no slash found
>      # that's fine too.
>      [[ "$cur" =~ .*/ ]]
>      _tmp_dir=$BASH_REMATCH
> 
>      # Find possible directory completions, adding trailing '/' characters
>      _tmp_completions="$(git ls-tree -d --name-only HEAD $_tmp_dir |
>          sed -e s%$%/%)"
> 
>      if [[ -n "$_tmp_completions" ]]; then
>          # There were some directory completions, so find ones that
>          # start with "$cur", the current token, and put those in COMPREPLY
>          local i=0 c IFS=$' \t\n'
>          for c in $_tmp_completions; do
>              if [[ $c == "$cur"* ]]; then
>                  COMPREPLY+=("$c")
>              fi
>          done
>      elif [[ "$cur" =~ /$ ]]; then
>          # No possible further completions any deeper, so assume we're at
>          # a leaf directory and just consider it complete
>          __gitcomp_direct_append "$cur "
>      fi
> }
> 
> But I don't think that needs to be part of this series; I can submit
> it later and hopefully get a completion expert to point out
> better/cleaner ways of what I've done above.
> 
I'm admittedly curious about what made this so difficult. I removed the 
'-r' and updated my tests to expect only directories at one level, and
they passed. But I imagine I'm being too simplistic.
>> +                       ;;
>> +               init|reapply)
>> +                       __gitcomp "$__git_sparse_checkout_subcommand_opts"
>> +                       ;;
>> +               *)
>> +                       ;;
>>          esac
>>   }
>>
>> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
>> index 51d0f2d93a1..4dc93ee0595 100755
>> --- a/t/t9902-completion.sh
>> +++ b/t/t9902-completion.sh
>> @@ -1447,7 +1447,7 @@ test_expect_success 'git checkout - with --detach, complete only references' '
>>          EOF
>>   '
>>
>> -test_expect_failure 'sparse-checkout completes subcommands' '
>> +test_expect_success 'sparse-checkout completes subcommands' '
>>          test_completion "git sparse-checkout " <<-\EOF
>>          list Z
>>          init Z
>> @@ -1458,13 +1458,13 @@ test_expect_failure 'sparse-checkout completes subcommands' '
>>          EOF
>>   '
>>
>> -test_expect_failure 'sparse-checkout completes options' '
>> +test_expect_success 'sparse-checkout completes options' '
>>          test_completion "git sparse-checkout --" <<-\EOF
>>          --help Z
>>          EOF
>>   '
>>
>> -test_expect_failure 'sparse-checkout completes subcommand options' '
>> +test_expect_success 'sparse-checkout completes subcommand options' '
>>          test_completion "git sparse-checkout init --" <<-\EOF &&
>>          --cone Z
>>          --no-cone Z
>> @@ -1492,7 +1492,7 @@ test_expect_failure 'sparse-checkout completes subcommand options' '
>>          EOF
>>   '
>>
>> -test_expect_failure 'sparse-checkout completes directory names' '
>> +test_expect_success 'sparse-checkout completes directory names' '
>>          # set up sparse-checkout repo
>>          git init sparse-checkout &&
>>          (
>> --
>> gitgitgadget
> 
> Patch looks good to me, other than the $prev vs. $subcommand thing.

Thanks!

^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH v2 1/2] sparse-checkout: custom tab completion tests
  2022-01-04 19:25         ` Lessley Dennington
@ 2022-01-04 20:25           ` Elijah Newren
  2022-01-05 14:05             ` Lessley Dennington
  0 siblings, 1 reply; 51+ messages in thread
From: Elijah Newren @ 2022-01-04 20:25 UTC (permalink / raw)
  To: Lessley Dennington
  Cc: Junio C Hamano, Lessley Dennington via GitGitGadget,
	Git Mailing List, Derrick Stolee, johannes.schindelin

On Tue, Jan 4, 2022 at 11:26 AM Lessley Dennington
<lessleydennington@gmail.com> wrote:
>
>
>
> On 12/31/21 4:20 PM, Junio C Hamano wrote:
> > Elijah Newren <newren@gmail.com> writes:
> >
> >> 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.
> >
> 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".

^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH v2 2/2] sparse-checkout: custom tab completion
  2022-01-04 19:41       ` Lessley Dennington
@ 2022-01-04 20:42         ` Elijah Newren
  2022-01-05 20:20           ` Lessley Dennington
  0 siblings, 1 reply; 51+ messages in thread
From: Elijah Newren @ 2022-01-04 20:42 UTC (permalink / raw)
  To: Lessley Dennington
  Cc: Lessley Dennington via GitGitGadget, Git Mailing List,
	Derrick Stolee, Junio C Hamano, johannes.schindelin

On Tue, Jan 4, 2022 at 11:41 AM Lessley Dennington
<lessleydennington@gmail.com> wrote:
>
>
>
> On 12/31/21 4:52 PM, Elijah Newren wrote:
> > On Fri, Dec 31, 2021 at 2:33 AM Lessley Dennington via GitGitGadget
> > <gitgitgadget@gmail.com> wrote:
> >>
> >> From: Lessley Dennington <lessleydennington@gmail.com>
> >>
> >> Fix custom tab completion for sparse-checkout command. This will ensure:
> >>
> >> 1. The full list of subcommands is provided when users enter git
> >> sparse-checkout <TAB>.
> >> 2. The --help option is tab-completable.
> >> 3. Subcommand options are tab-completable.
> >> 4. A list of directories (but not files) is provided when users enter git
> >> sparse-checkout add <TAB> or git sparse-checkout set <TAB>.
> >>
> >> Failing tests that were added in the previous commit to verify these
> >> scenarios are now passing with these updates.
> >>
> >> Signed-off-by: Lessley Dennington <lessleydennington@gmail.com>
> >> ---
> >>   contrib/completion/git-completion.bash | 38 ++++++++++++++++++--------
> >>   t/t9902-completion.sh                  |  8 +++---
> >>   2 files changed, 30 insertions(+), 16 deletions(-)
> >>
> >> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> >> index c82ccaebcc7..7de997ee64e 100644
> >> --- a/contrib/completion/git-completion.bash
> >> +++ b/contrib/completion/git-completion.bash
> >> @@ -2986,24 +2986,38 @@ _git_show_branch ()
> >>          __git_complete_revlist
> >>   }
> >>
> >> +__git_sparse_checkout_subcommand_opts="--cone --no-cone --sparse-index --no-sparse-index"
> >> +
> >>   _git_sparse_checkout ()
> >>   {
> >> -       local subcommands="list init set disable"
> >> +       local subcommands="list init set disable add reapply"
> >>          local subcommand="$(__git_find_on_cmdline "$subcommands")"
> >> +
> >>          if [ -z "$subcommand" ]; then
> >> -               __gitcomp "$subcommands"
> >> -               return
> >> +               case "$cur" in
> >> +                       --*)
> >> +                               __gitcomp "--help"
> >> +                               ;;
> >> +                       *)
> >> +                               __gitcomp "$subcommands"
> >> +                               ;;
> >> +               esac
> >>          fi
> >>
> >> -       case "$subcommand,$cur" in
> >> -       init,--*)
> >> -               __gitcomp "--cone"
> >> -               ;;
> >> -       set,--*)
> >> -               __gitcomp "--stdin"
> >> -               ;;
> >> -       *)
> >> -               ;;
> >> +       case "$prev" in
> >
> > Shouldn't this be "$subcommand" rather than "$prev"?  I think with
> > prev, it will only correctly complete the first path after "set",
> > "add", etc.
> >
> Good catch, thank you! Fixing in v3.
> >> +               set)
> >> +                       __gitcomp "$__git_sparse_checkout_subcommand_opts --stdin"
> >> +                       __gitcomp "$(git ls-tree -d -r HEAD --name-only)"
> >> +                       ;;
> >> +               add)
> >> +                       __gitcomp "--stdin"
> >> +                       __gitcomp "$(git ls-tree -d -r HEAD --name-only)"
> >
> > I was going to make a simple suggestion for making it just complete
> > one additional level at a time and leaving out the -r, and then tried
> > it out and found out it wasn't simple.  I got something working,
> > eventually, but it's slightly ugly...so it probably belongs in a
> > separate patch anyway.  If you're curious, it's basically replacing
> > the second __gitcomp... call for each of set and add with
> > `__gitcomp_directories`, after first defining:
> >
> > __gitcomp_directories ()
> > {
> >      local _tmp_dir _tmp_completions
> >
> >      # Get the directory of the current token; this differs from dirname
> >      # in that it keeps up to the final trailing slash.  If no slash found
> >      # that's fine too.
> >      [[ "$cur" =~ .*/ ]]
> >      _tmp_dir=$BASH_REMATCH
> >
> >      # Find possible directory completions, adding trailing '/' characters
> >      _tmp_completions="$(git ls-tree -d --name-only HEAD $_tmp_dir |
> >          sed -e s%$%/%)"
> >
> >      if [[ -n "$_tmp_completions" ]]; then
> >          # There were some directory completions, so find ones that
> >          # start with "$cur", the current token, and put those in COMPREPLY
> >          local i=0 c IFS=$' \t\n'
> >          for c in $_tmp_completions; do
> >              if [[ $c == "$cur"* ]]; then
> >                  COMPREPLY+=("$c")
> >              fi
> >          done
> >      elif [[ "$cur" =~ /$ ]]; then
> >          # No possible further completions any deeper, so assume we're at
> >          # a leaf directory and just consider it complete
> >          __gitcomp_direct_append "$cur "
> >      fi
> > }
> >
> > But I don't think that needs to be part of this series; I can submit
> > it later and hopefully get a completion expert to point out
> > better/cleaner ways of what I've done above.
> >
> I'm admittedly curious about what made this so difficult. I removed the
> '-r' and updated my tests to expect only directories at one level, and
> they passed. But I imagine I'm being too simplistic.

I've forgotten some details since last Saturday, but I think the
problem was that doing that would only ever complete toplevel
directories; after completing those you could keep tabbing to get a
deeper directory.  First, let's get a comparison point; ignoring
sparse-checkout, I can do:

    cd $GIT_CLONE
    cd cont<TAB>b<TAB><TAB>

and the ls line will replace those <TAB>s so that I see

    ls contrib/buildsystems/Generators

Now, if we just removed the '-r' from your git-completion.bash
changes, and then typed

    git sparse-checkout set cont<TAB>b<TAB><TAB>

Then you'd see

    git sparse-checkout set contrib

and see 'bin-wrappers', 'block-sha1', and 'builtin' as potential
completions, but not as subdirs of contrib but instead sibling dirs to
contrib.  That completion rule wouldn't let you look within contrib/.
My more complicated rule had to avoid calling __gitcomp to avoid
adding spaces so that completions could continue (but should add them
if we have tabbed all the way down and there are no more
subdirectories), had to add trailing '/' characters so that we know
when we have the full directory name to pass along to ls-tree, and
then had to manually do the work that __gitcomp would manually do with
making sure to only provide completions relevant to what has been
typed so far.

^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH v2 1/2] sparse-checkout: custom tab completion tests
  2022-01-04 20:25           ` Elijah Newren
@ 2022-01-05 14:05             ` Lessley Dennington
  0 siblings, 0 replies; 51+ messages in thread
From: Lessley Dennington @ 2022-01-05 14:05 UTC (permalink / raw)
  To: Elijah Newren
  Cc: Junio C Hamano, Lessley Dennington via GitGitGadget,
	Git Mailing List, Derrick Stolee, johannes.schindelin



On 1/4/22 2:25 PM, Elijah Newren wrote:
> On Tue, Jan 4, 2022 at 11:26 AM Lessley Dennington
> <lessleydennington@gmail.com> wrote:
>>
>>
>>
>> On 12/31/21 4:20 PM, Junio C Hamano wrote:
>>> Elijah Newren <newren@gmail.com> writes:
>>>
>>>> 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.
>>>
>> 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".

This is actually great context - thanks for providing! I'll go with the
email strategy for visibility and will base my format off [1].

[1]: 
https://lore.kernel.org/git/CABceR4bZmtC4rCwgxZ1BBYZP69VOUca1f_moJoP989vTUZWu9Q@mail.gmail.com/

^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH v2 2/2] sparse-checkout: custom tab completion
  2022-01-04 20:42         ` Elijah Newren
@ 2022-01-05 20:20           ` Lessley Dennington
  2022-01-05 22:55             ` Elijah Newren
  0 siblings, 1 reply; 51+ messages in thread
From: Lessley Dennington @ 2022-01-05 20:20 UTC (permalink / raw)
  To: Elijah Newren
  Cc: Lessley Dennington via GitGitGadget, Git Mailing List,
	Derrick Stolee, Junio C Hamano, johannes.schindelin



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

^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH v2 2/2] sparse-checkout: custom tab completion
  2022-01-05 20:20           ` Lessley Dennington
@ 2022-01-05 22:55             ` Elijah Newren
  0 siblings, 0 replies; 51+ messages in thread
From: Elijah Newren @ 2022-01-05 22:55 UTC (permalink / raw)
  To: Lessley Dennington
  Cc: Lessley Dennington via GitGitGadget, Git Mailing List,
	Derrick Stolee, Junio C Hamano, johannes.schindelin

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

The wrong behavior only occurs if you drop the `-r` from your patch.
If you keep the `-r`, as in your patch submission, you get the right
behavior, it just might be a bit slow.  The only reason I investigated
dropping the `-r` and then following up with all the extra workarounds
needed when the `-r` was dropped was because some repositories may be
big enough that immediately recursing trees down to the lowest depths
may be expensive.

For example, in linux.git (very small compared to the Microsoft
repos): `time git ls-tree -rd HEAD >/dev/null` was 0.785s (cold cache;
a mere 0.084s on second run).  If repositories get much bigger than
that, folks might not like the slowness of using `-r`.  But I think
what you have is fine as a first cut.

That said, if you want to add a patch to this series that switches
your straightforward implementation to my much more complex but faster
alternative, that's fine too.

^ permalink raw reply	[flat|nested] 51+ messages in thread

* [PATCH v3 0/3] sparse checkout: custom bash completion updates
  2021-12-30 19:26 ` [PATCH v2 0/2] sparse checkout: custom bash completion updates Lessley Dennington via GitGitGadget
  2021-12-30 19:26   ` [PATCH v2 1/2] sparse-checkout: custom tab completion tests Lessley Dennington via GitGitGadget
  2021-12-30 19:26   ` [PATCH v2 2/2] sparse-checkout: custom tab completion Lessley Dennington via GitGitGadget
@ 2022-01-10 18:59   ` Lessley Dennington via GitGitGadget
  2022-01-10 18:59     ` [PATCH v3 1/3] sparse-checkout: custom tab completion tests Lessley Dennington via GitGitGadget
                       ` (3 more replies)
  2 siblings, 4 replies; 51+ messages in thread
From: Lessley Dennington via GitGitGadget @ 2022-01-10 18:59 UTC (permalink / raw)
  To: git
  Cc: stolee, gitster, johannes.schindelin, Elijah Newren, Lessley Dennington

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

-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 51+ messages in thread

* [PATCH v3 1/3] sparse-checkout: custom tab completion tests
  2022-01-10 18:59   ` [PATCH v3 0/3] sparse checkout: custom bash completion updates Lessley Dennington via GitGitGadget
@ 2022-01-10 18:59     ` Lessley Dennington via GitGitGadget
  2022-01-10 18:59     ` [PATCH v3 2/3] sparse-checkout: custom tab completion Lessley Dennington via GitGitGadget
                       ` (2 subsequent siblings)
  3 siblings, 0 replies; 51+ messages in thread
From: Lessley Dennington via GitGitGadget @ 2022-01-10 18:59 UTC (permalink / raw)
  To: git
  Cc: stolee, gitster, johannes.schindelin, Elijah Newren,
	Lessley Dennington, Lessley Dennington

From: Lessley Dennington <lessleydennington@gmail.com>

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 <lessleydennington@gmail.com>
---
 t/t9902-completion.sh | 85 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 85 insertions(+)

diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 518203fbe07..51d0f2d93a1 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -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


^ permalink raw reply	[flat|nested] 51+ messages in thread

* [PATCH v3 2/3] sparse-checkout: custom tab completion
  2022-01-10 18:59   ` [PATCH v3 0/3] sparse checkout: custom bash completion updates Lessley Dennington via GitGitGadget
  2022-01-10 18:59     ` [PATCH v3 1/3] sparse-checkout: custom tab completion tests Lessley Dennington via GitGitGadget
@ 2022-01-10 18:59     ` Lessley Dennington via GitGitGadget
  2022-01-15  9:57       ` SZEDER Gábor
  2022-01-10 18:59     ` [PATCH v3 3/3] sparse-checkout: limit tab completion to a single level Lessley Dennington via GitGitGadget
  2022-01-10 20:38     ` [PATCH v3 0/3] sparse checkout: custom bash completion updates Elijah Newren
  3 siblings, 1 reply; 51+ messages in thread
From: Lessley Dennington via GitGitGadget @ 2022-01-10 18:59 UTC (permalink / raw)
  To: git
  Cc: stolee, gitster, johannes.schindelin, Elijah Newren,
	Lessley Dennington, Lessley Dennington

From: Lessley Dennington <lessleydennington@gmail.com>

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

1. The full list of subcommands is provided when users enter git
sparse-checkout <TAB>.
2. The --help option is tab-completable.
3. Subcommand options are tab-completable.
4. A list of directories (but not files) is provided when users enter git
sparse-checkout add <TAB> or git sparse-checkout set <TAB>. 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 <lessleydennington@gmail.com>
---
 contrib/completion/git-completion.bash | 38 ++++++++++++++++++--------
 t/t9902-completion.sh                  |  8 +++---
 2 files changed, 30 insertions(+), 16 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index c82ccaebcc7..f478883f51c 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2986,24 +2986,38 @@ _git_show_branch ()
 	__git_complete_revlist
 }
 
+__git_sparse_checkout_subcommand_opts="--cone --no-cone --sparse-index --no-sparse-index"
+
 _git_sparse_checkout ()
 {
-	local subcommands="list init set disable"
+	local subcommands="list init set disable add reapply"
 	local subcommand="$(__git_find_on_cmdline "$subcommands")"
+
 	if [ -z "$subcommand" ]; then
-		__gitcomp "$subcommands"
-		return
+		case "$cur" in
+			--*)
+				__gitcomp "--help"
+				;;
+			*)
+				__gitcomp "$subcommands"
+				;;
+		esac
 	fi
 
-	case "$subcommand,$cur" in
-	init,--*)
-		__gitcomp "--cone"
-		;;
-	set,--*)
-		__gitcomp "--stdin"
-		;;
-	*)
-		;;
+	case "$subcommand" in
+		set)
+			__gitcomp "$__git_sparse_checkout_subcommand_opts --stdin"
+			__gitcomp "$(git ls-tree -d -r HEAD --name-only)"
+			;;
+		add)
+			__gitcomp "--stdin"
+			__gitcomp "$(git ls-tree -d -r HEAD --name-only)"
+			;;
+		init|reapply)
+			__gitcomp "$__git_sparse_checkout_subcommand_opts"
+			;;
+		*)
+			;;
 	esac
 }
 
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 51d0f2d93a1..4dc93ee0595 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -1447,7 +1447,7 @@ test_expect_success 'git checkout - with --detach, complete only references' '
 	EOF
 '
 
-test_expect_failure 'sparse-checkout completes subcommands' '
+test_expect_success 'sparse-checkout completes subcommands' '
 	test_completion "git sparse-checkout " <<-\EOF
 	list Z
 	init Z
@@ -1458,13 +1458,13 @@ test_expect_failure 'sparse-checkout completes subcommands' '
 	EOF
 '
 
-test_expect_failure 'sparse-checkout completes options' '
+test_expect_success 'sparse-checkout completes options' '
 	test_completion "git sparse-checkout --" <<-\EOF
 	--help Z
 	EOF
 '
 
-test_expect_failure 'sparse-checkout completes subcommand options' '
+test_expect_success 'sparse-checkout completes subcommand options' '
 	test_completion "git sparse-checkout init --" <<-\EOF &&
 	--cone Z
 	--no-cone Z
@@ -1492,7 +1492,7 @@ test_expect_failure 'sparse-checkout completes subcommand options' '
 	EOF
 '
 
-test_expect_failure 'sparse-checkout completes directory names' '
+test_expect_success 'sparse-checkout completes directory names' '
 	# set up sparse-checkout repo
 	git init sparse-checkout &&
 	(
-- 
gitgitgadget


^ permalink raw reply	[flat|nested] 51+ messages in thread

* [PATCH v3 3/3] sparse-checkout: limit tab completion to a single level
  2022-01-10 18:59   ` [PATCH v3 0/3] sparse checkout: custom bash completion updates Lessley Dennington via GitGitGadget
  2022-01-10 18:59     ` [PATCH v3 1/3] sparse-checkout: custom tab completion tests Lessley Dennington via GitGitGadget
  2022-01-10 18:59     ` [PATCH v3 2/3] sparse-checkout: custom tab completion Lessley Dennington via GitGitGadget
@ 2022-01-10 18:59     ` Lessley Dennington via GitGitGadget
  2022-01-12 23:43       ` Lessley Dennington
  2022-01-10 20:38     ` [PATCH v3 0/3] sparse checkout: custom bash completion updates Elijah Newren
  3 siblings, 1 reply; 51+ messages in thread
From: Lessley Dennington via GitGitGadget @ 2022-01-10 18:59 UTC (permalink / raw)
  To: git
  Cc: stolee, gitster, johannes.schindelin, Elijah Newren,
	Lessley Dennington, Lessley Dennington

From: Lessley Dennington <lessleydennington@gmail.com>

Ensure only directories at the current level will be tab-completed with
the sparse-checkout command. For example, if paths a/b/c/ and a/d/ exist
in the current directory, running a/<TAB> will result in:

        a/b/
        a/d/

The 'sparse-checkout completes directory names' test has also been
updated/extended according to these changes.

Co-authored-by: Elijah Newren <newren@gmail.com>
Co-authored-by: Lessley Dennington <lessleydennington@gmail.com>
Signed-off-by: Lessley Dennington <lessleydennington@gmail.com>
---
 contrib/completion/git-completion.bash | 34 ++++++++++++++++++++++++--
 t/t9902-completion.sh                  | 29 ++++++++++++++--------
 2 files changed, 51 insertions(+), 12 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index f478883f51c..21875d08d03 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2986,6 +2986,36 @@ _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
+             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
+}
+
 __git_sparse_checkout_subcommand_opts="--cone --no-cone --sparse-index --no-sparse-index"
 
 _git_sparse_checkout ()
@@ -3007,11 +3037,11 @@ _git_sparse_checkout ()
 	case "$subcommand" in
 		set)
 			__gitcomp "$__git_sparse_checkout_subcommand_opts --stdin"
-			__gitcomp "$(git ls-tree -d -r HEAD --name-only)"
+			__gitcomp_directories
 			;;
 		add)
 			__gitcomp "--stdin"
-			__gitcomp "$(git ls-tree -d -r HEAD --name-only)"
+			__gitcomp_directories
 			;;
 		init|reapply)
 			__gitcomp "$__git_sparse_checkout_subcommand_opts"
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 4dc93ee0595..e11ff0c2efe 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -1513,21 +1513,30 @@ test_expect_success 'sparse-checkout completes directory names' '
 	(
 		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
+		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 &&
-		test_completion "git sparse-checkout add " <<-\EOF
-		./ Z
-		0 Z
-		0/1 Z
+		test_completion "git sparse-checkout add 0" <<-\EOF
+		0/
 		EOF
 	)
 '
-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH v3 0/3] sparse checkout: custom bash completion updates
  2022-01-10 18:59   ` [PATCH v3 0/3] sparse checkout: custom bash completion updates Lessley Dennington via GitGitGadget
                       ` (2 preceding siblings ...)
  2022-01-10 18:59     ` [PATCH v3 3/3] sparse-checkout: limit tab completion to a single level Lessley Dennington via GitGitGadget
@ 2022-01-10 20:38     ` Elijah Newren
  2022-01-11 17:17       ` Lessley Dennington
  3 siblings, 1 reply; 51+ messages in thread
From: Elijah Newren @ 2022-01-10 20:38 UTC (permalink / raw)
  To: Lessley Dennington via GitGitGadget
  Cc: Git Mailing List, Derrick Stolee, Junio C Hamano,
	johannes.schindelin, Lessley Dennington

On Mon, Jan 10, 2022 at 10:59 AM Lessley Dennington via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> 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).

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.

>
>           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 <newren@gmail.com>

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.

^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH v3 0/3] sparse checkout: custom bash completion updates
  2022-01-10 20:38     ` [PATCH v3 0/3] sparse checkout: custom bash completion updates Elijah Newren
@ 2022-01-11 17:17       ` Lessley Dennington
  2022-01-11 19:45         ` Taylor Blau
  0 siblings, 1 reply; 51+ messages in thread
From: Lessley Dennington @ 2022-01-11 17:17 UTC (permalink / raw)
  To: Elijah Newren, Lessley Dennington via GitGitGadget
  Cc: Git Mailing List, Derrick Stolee, Junio C Hamano, johannes.schindelin



On 1/10/22 2:38 PM, Elijah Newren wrote:
> On Mon, Jan 10, 2022 at 10:59 AM Lessley Dennington via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>>
>> 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).
> 
> 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.
> 
Thank you! That is much better wording. Will update in v4 along with any
feedback I get on your function (see below).
>>
>>            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 <newren@gmail.com>
> 
> 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.

^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH v3 0/3] sparse checkout: custom bash completion updates
  2022-01-11 17:17       ` Lessley Dennington
@ 2022-01-11 19:45         ` Taylor Blau
  2022-01-12 18:35           ` Lessley Dennington
  0 siblings, 1 reply; 51+ messages in thread
From: Taylor Blau @ 2022-01-11 19:45 UTC (permalink / raw)
  To: Lessley Dennington
  Cc: Elijah Newren, Lessley Dennington via GitGitGadget,
	Git Mailing List, Derrick Stolee, Junio C Hamano,
	johannes.schindelin, Denton Liu

On Tue, Jan 11, 2022 at 09:17:38AM -0800, Lessley Dennington wrote:
> On 1/10/22 2:38 PM, Elijah Newren wrote:
> > Other than that one nit, patches 1-2 (and the testcases in patch 3) are:
> >
> > Reviewed-by: Elijah Newren <newren@gmail.com>
> >
> > 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

^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH v3 0/3] sparse checkout: custom bash completion updates
  2022-01-11 19:45         ` Taylor Blau
@ 2022-01-12 18:35           ` Lessley Dennington
  0 siblings, 0 replies; 51+ messages in thread
From: Lessley Dennington @ 2022-01-12 18:35 UTC (permalink / raw)
  To: Taylor Blau
  Cc: Elijah Newren, Lessley Dennington via GitGitGadget,
	Git Mailing List, Derrick Stolee, Junio C Hamano,
	johannes.schindelin, Denton Liu



On 1/11/22 1:45 PM, Taylor Blau wrote:
> On Tue, Jan 11, 2022 at 09:17:38AM -0800, Lessley Dennington wrote:
>> On 1/10/22 2:38 PM, Elijah Newren wrote:
>>> Other than that one nit, patches 1-2 (and the testcases in patch 3) are:
>>>
>>> Reviewed-by: Elijah Newren <newren@gmail.com>
>>>
>>> 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

Thanks for the help here, Taylor! I like this suggestion and will add my
thoughts to Elijah's __gitcomp_directories function.

^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH v3 3/3] sparse-checkout: limit tab completion to a single level
  2022-01-10 18:59     ` [PATCH v3 3/3] sparse-checkout: limit tab completion to a single level Lessley Dennington via GitGitGadget
@ 2022-01-12 23:43       ` Lessley Dennington
  2022-01-13  0:00         ` Junio C Hamano
  2022-01-13  0:38         ` Elijah Newren
  0 siblings, 2 replies; 51+ messages in thread
From: Lessley Dennington @ 2022-01-12 23:43 UTC (permalink / raw)
  To: Lessley Dennington via GitGitGadget, git
  Cc: stolee, gitster, johannes.schindelin, Elijah Newren

> +__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%$%/%)"
> +
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.
> +     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?
> +         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.
> +         __gitcomp_direct_append "$cur "
What's the reason for the trailing space here?
> +     fi
> +}

Added my review as mentioned in [1].

[1]: 
https://lore.kernel.org/git/pull.1108.v2.git.1640892413.gitgitgadget@gmail.com/T/#md3da435452988b0366ab4c2ee4bc06df2d17cb36

^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH v3 3/3] sparse-checkout: limit tab completion to a single level
  2022-01-12 23:43       ` Lessley Dennington
@ 2022-01-13  0:00         ` Junio C Hamano
  2022-01-13  0:38         ` Elijah Newren
  1 sibling, 0 replies; 51+ messages in thread
From: Junio C Hamano @ 2022-01-13  0:00 UTC (permalink / raw)
  To: Lessley Dennington
  Cc: Lessley Dennington via GitGitGadget, git, stolee,
	johannes.schindelin, Elijah Newren

Lessley Dennington <lessleydennington@gmail.com> writes:

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

The substitution operator "s" in "sed" can take any letter as the
match-replacement delimiter.  People use 's/match/replacement/'
usually because that is how textbooks teach them, but whatever
letter chosen as the delimiter, if it appears in either match or
replacement, it needs to be quoted, i.e. 's/match/replace\/ment/'.

Using a delimiter letter other than '/' relieves you from having to
quote a slash when slash is part of match-replacement.  Even though
it is more common to use a letter that is a more clearly delimiter
looking, e.g. "s|match|replace/ment|", it is not a crime to use
letters like '%' and '#', or even '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'
> Does c need to be declared before the loop?
>> +         for c in $_tmp_completions; do

bash completion script runs in the same namespace as the end-user's
interactive session, so we MUST be careful not to contaminate the
namespace or clobber variable the end-user is using.  "local c" before
we use $c for our own use is a way to make sure that when this function
that says "local c" is left, the value (or non-presence) of "c" is
restored to its original value before this function was entered.


^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH v3 3/3] sparse-checkout: limit tab completion to a single level
  2022-01-12 23:43       ` Lessley Dennington
  2022-01-13  0:00         ` Junio C Hamano
@ 2022-01-13  0:38         ` Elijah Newren
  2022-01-13 19:02           ` Lessley Dennington
  1 sibling, 1 reply; 51+ messages in thread
From: Elijah Newren @ 2022-01-13  0:38 UTC (permalink / raw)
  To: Lessley Dennington
  Cc: Lessley Dennington via GitGitGadget, Git Mailing List,
	Derrick Stolee, Junio C Hamano, johannes.schindelin

On Wed, Jan 12, 2022 at 3:43 PM Lessley Dennington
<lessleydennington@gmail.com> wrote:
>
> > +__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%$%/%)"
> > +
> 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.

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

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

> > +     fi
> > +}
>
> Added my review as mentioned in [1].
>
> [1]:
> https://lore.kernel.org/git/pull.1108.v2.git.1640892413.gitgitgadget@gmail.com/T/#md3da435452988b0366ab4c2ee4bc06df2d17cb36

^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH v3 3/3] sparse-checkout: limit tab completion to a single level
  2022-01-13  0:38         ` Elijah Newren
@ 2022-01-13 19:02           ` Lessley Dennington
  0 siblings, 0 replies; 51+ messages in thread
From: Lessley Dennington @ 2022-01-13 19:02 UTC (permalink / raw)
  To: Elijah Newren
  Cc: Lessley Dennington via GitGitGadget, Git Mailing List,
	Derrick Stolee, Junio C Hamano, johannes.schindelin



On 1/12/22 6:38 PM, Elijah Newren wrote:
> On Wed, Jan 12, 2022 at 3:43 PM Lessley Dennington
> <lessleydennington@gmail.com> wrote:
>>
>>> +__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%$%/%)"
>>> +
>> 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.
> 
Got it, thanks!
>>> +     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.
> 
Sorry, I should have made my question clearer - I saw that c is declared
before the loop but was wondering if that was actually necessary. I
removed it locally and ran the tests, which seemed to work without it.
>>> +         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.
>>> +         __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?
> 
Yes, very much! Thank you.
>>> +     fi
>>> +}
>>
>> Added my review as mentioned in [1].
>>
>> [1]:
>> https://lore.kernel.org/git/pull.1108.v2.git.1640892413.gitgitgadget@gmail.com/T/#md3da435452988b0366ab4c2ee4bc06df2d17cb36

^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH v3 2/3] sparse-checkout: custom tab completion
  2022-01-10 18:59     ` [PATCH v3 2/3] sparse-checkout: custom tab completion Lessley Dennington via GitGitGadget
@ 2022-01-15  9:57       ` SZEDER Gábor
  2022-01-16  1:03         ` Elijah Newren
  0 siblings, 1 reply; 51+ messages in thread
From: SZEDER Gábor @ 2022-01-15  9:57 UTC (permalink / raw)
  To: Lessley Dennington via GitGitGadget
  Cc: git, stolee, gitster, johannes.schindelin, Elijah Newren,
	Lessley Dennington

On Mon, Jan 10, 2022 at 06:59:51PM +0000, Lessley Dennington via GitGitGadget wrote:
> 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.

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

This is inconsistent with the rest of the completion script, because
it doesn't list '--help' for any commands or subcommand (with the sole
exception of 'git --<TAB>').

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

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.

> 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 <lessleydennington@gmail.com>
> ---
>  contrib/completion/git-completion.bash | 38 ++++++++++++++++++--------
>  t/t9902-completion.sh                  |  8 +++---
>  2 files changed, 30 insertions(+), 16 deletions(-)
> 
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index c82ccaebcc7..f478883f51c 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -2986,24 +2986,38 @@ _git_show_branch ()
>  	__git_complete_revlist
>  }
>  
> +__git_sparse_checkout_subcommand_opts="--cone --no-cone --sparse-index --no-sparse-index"
> +
>  _git_sparse_checkout ()
>  {
> -	local subcommands="list init set disable"
> +	local subcommands="list init set disable add reapply"
>  	local subcommand="$(__git_find_on_cmdline "$subcommands")"
> +
>  	if [ -z "$subcommand" ]; then
> -		__gitcomp "$subcommands"
> -		return
> +		case "$cur" in
> +			--*)
> +				__gitcomp "--help"
> +				;;
> +			*)
> +				__gitcomp "$subcommands"
> +				;;
> +		esac
>  	fi
>  
> -	case "$subcommand,$cur" in
> -	init,--*)
> -		__gitcomp "--cone"
> -		;;
> -	set,--*)
> -		__gitcomp "--stdin"
> -		;;
> -	*)
> -		;;
> +	case "$subcommand" in
> +		set)
> +			__gitcomp "$__git_sparse_checkout_subcommand_opts --stdin"

Oh, a hard-coded list of --options is so old school :)

All subcommands of 'git sparse-checkout' use parse-options (even those
that don't have any --options at the moment), so their options can be
completed programmatically, and then we wouldn't have to worry about
updating the list of options in the completion script ever again.  It
would also make several tests added in the previous patch unnecessary.

You could use the completion function of 'git notes' for inspiration
on how to do that.

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

This will have troubles with unusual characters in pathnames:

  - If a pathname contains a space (or any other IFS character), then
    the shell will split that into multiple words.  

  - If a pathname contains a special character, e.g. double quote or
    backslash, then 'git ls-tree' will quote that path.  Furthermore,
    by default it will quote pathnames containing e.g. UTF-8
    characters as well.

  - The shell has its own special characteers that have to be
    quoted/escaped on the command line to strip them from their
    special meaning, that quoting/escaping will interfere with listing
    only paths matching the current word to be completed.

You should use the __git_complete_index_file() helper function
instead, which takes care of most of this.

Furthermore, these two subsequent __gitcomp() calls will offer both
options and paths for 'git sparse-checkout set <TAB>', which is
inconsistent with the rest of the completion script.  Usually it lists
--options only after e.g. 'git rm --<TAB>' or 'git log --<TAB>', but
after 'git rm <TAB>' and 'git log <TAB>' it lists only files and refs,
respectively.

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

Likewise.

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

^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH v3 2/3] sparse-checkout: custom tab completion
  2022-01-15  9:57       ` SZEDER Gábor
@ 2022-01-16  1:03         ` Elijah Newren
  2022-01-16 22:13           ` Junio C Hamano
                             ` (2 more replies)
  0 siblings, 3 replies; 51+ messages in thread
From: Elijah Newren @ 2022-01-16  1:03 UTC (permalink / raw)
  To: SZEDER Gábor
  Cc: Lessley Dennington via GitGitGadget, Git Mailing List,
	Derrick Stolee, Junio C Hamano, johannes.schindelin,
	Lessley Dennington

On Sat, Jan 15, 2022 at 1:57 AM SZEDER Gábor <szeder.dev@gmail.com> wrote:
>
> On Mon, Jan 10, 2022 at 06:59:51PM +0000, Lessley Dennington via GitGitGadget wrote:
> > 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.

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

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

^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH v3 2/3] sparse-checkout: custom tab completion
  2022-01-16  1:03         ` Elijah Newren
@ 2022-01-16 22:13           ` Junio C Hamano
  2022-01-17 18:14             ` Elijah Newren
  2022-01-18 17:59           ` Lessley Dennington
  2022-01-18 22:22           ` SZEDER Gábor
  2 siblings, 1 reply; 51+ messages in thread
From: Junio C Hamano @ 2022-01-16 22:13 UTC (permalink / raw)
  To: Elijah Newren
  Cc: SZEDER Gábor, Lessley Dennington via GitGitGadget,
	Git Mailing List, Derrick Stolee, johannes.schindelin,
	Lessley Dennington

Elijah Newren <newren@gmail.com> writes:

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

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

^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH v3 2/3] sparse-checkout: custom tab completion
  2022-01-16 22:13           ` Junio C Hamano
@ 2022-01-17 18:14             ` Elijah Newren
  2022-01-17 19:40               ` Junio C Hamano
  2022-01-18 21:02               ` SZEDER Gábor
  0 siblings, 2 replies; 51+ messages in thread
From: Elijah Newren @ 2022-01-17 18:14 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: SZEDER Gábor, Lessley Dennington via GitGitGadget,
	Git Mailing List, Derrick Stolee, johannes.schindelin,
	Lessley Dennington

On Sun, Jan 16, 2022 at 2:13 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Elijah Newren <newren@gmail.com> writes:
>
> > ...  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".

I don't understand your distinction; I think your second sentence is
the same thing I said.

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

Completing on files and directories would be a reasonable first cut,
but that's what we already had before this series was proposed.  It
has the downsides of
  * [cone-mode] making the majority of suggested completions wrong
  * [non-cone mode] subtly recommending individually adding all wanted
files, increasing the number of patterns and exacerbating the
quadratic behavior that non-cone mode experiences with every
unpack_trees() call that touches the working tree.
  * [both modes] potentially making it hard to find or select the
valid choices you do want (there are many more files than directories)

But, despite all that, completing on files and directories is a
somewhat reasonable first cut.

Lessley was trying to aim higher with her submission, though, and I
don't see why she should be dissuaded.  Something better would be
nice.

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

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>'
That doesn't seem to cause user confusion, so I don't think
disallowing it in cone-mode would cause confusion.  Suggesting it, on
the other hand, may well cause confusion given that cone-mode is
explicitly about directories and is documented as such.

If you're only talking about non-cone mode, then this may be a
reasonable objection, though I'm not sure I agree with it even then.
In addition to the fact that individual file completion can be
detrimental to the user in non-cone mode for performance reasons, the
documentation never explicitly states files are acceptable to
sparse-checkout {set,add}.  It always mentions "directories" and
"patterns".  We can't complete all patterns, so we have to pick a
subset.  I don't see why "directories and files" is a better subset to
pick than "just directories".

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

That is one possibility, though not the one Lessley proposed.

> 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),

I agree, except with the suggestion to use the working tree or index
in the case of sparse-checkouts.  The working tree shouldn't be used
because it doesn't have the needed information:
  * `git clone --sparse` starts with zero subdirectories present
  * `set` and `add` are likely being used to change sparsity to
include something not already present

The index shouldn't be used because it is not cheap for interactive use:
  * it contains recursive entries below directories of interest that
have to be filtered out
  * with the sparse-index, it may have sparse-directories hiding what
we want, forcing us to fully inflate the index to find the pieces we
do want

I agree with Lessley's choice of using ls-tree and HEAD to achieve
cheap interactive use.

>  - is simple enough to explain to the users to understand what the
>    rules are, and
>
>  - gives useful enough candidates.

I agree with these 3 criteria.

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

Well, Lessley certainly thought directories-only was more useful, and
in fact labelled "files and directories" as a bug/issue in her cover
letter.  Stolee commented on the series and also didn't see anything
wrong with that claim.

> 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'd prefer directories-only for both modes, but could accept
directories-only for cone mode and both-files-and-directories for
non-cone mode.

Especially since I think we're going to deprecate non-cone mode regardless.

^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH v3 2/3] sparse-checkout: custom tab completion
  2022-01-17 18:14             ` Elijah Newren
@ 2022-01-17 19:40               ` Junio C Hamano
  2022-01-18 17:56                 ` Lessley Dennington
  2022-01-18 21:02               ` SZEDER Gábor
  1 sibling, 1 reply; 51+ messages in thread
From: Junio C Hamano @ 2022-01-17 19:40 UTC (permalink / raw)
  To: Elijah Newren
  Cc: SZEDER Gábor, Lessley Dennington via GitGitGadget,
	Git Mailing List, Derrick Stolee, johannes.schindelin,
	Lessley Dennington

Elijah Newren <newren@gmail.com> writes:

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

^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH v3 2/3] sparse-checkout: custom tab completion
  2022-01-17 19:40               ` Junio C Hamano
@ 2022-01-18 17:56                 ` Lessley Dennington
  2022-01-22  1:07                   ` Lessley Dennington
  0 siblings, 1 reply; 51+ messages in thread
From: Lessley Dennington @ 2022-01-18 17:56 UTC (permalink / raw)
  To: Junio C Hamano, Elijah Newren
  Cc: SZEDER Gábor, Lessley Dennington via GitGitGadget,
	Git Mailing List, Derrick Stolee, johannes.schindelin

On 1/17/22 11:40 AM, Junio C Hamano wrote:
> Elijah Newren <newren@gmail.com> writes:
> 
>> 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.

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/

^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH v3 2/3] sparse-checkout: custom tab completion
  2022-01-16  1:03         ` Elijah Newren
  2022-01-16 22:13           ` Junio C Hamano
@ 2022-01-18 17:59           ` Lessley Dennington
  2022-01-18 22:22           ` SZEDER Gábor
  2 siblings, 0 replies; 51+ messages in thread
From: Lessley Dennington @ 2022-01-18 17:59 UTC (permalink / raw)
  To: Elijah Newren, SZEDER Gábor
  Cc: Lessley Dennington via GitGitGadget, Git Mailing List,
	Derrick Stolee, Junio C Hamano, johannes.schindelin



On 1/15/22 5:03 PM, Elijah Newren wrote:
> On Sat, Jan 15, 2022 at 1:57 AM SZEDER Gábor <szeder.dev@gmail.com> wrote:
>>
>> On Mon, Jan 10, 2022 at 06:59:51PM +0000, Lessley Dennington via GitGitGadget wrote:
>>> 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.
> 
I second this - am planning to apply all feedback (with the exception of 
completing
on both files and directories) in v4 of this series.


^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH v3 2/3] sparse-checkout: custom tab completion
  2022-01-17 18:14             ` Elijah Newren
  2022-01-17 19:40               ` Junio C Hamano
@ 2022-01-18 21:02               ` SZEDER Gábor
  2022-01-18 21:43                 ` Elijah Newren
  1 sibling, 1 reply; 51+ messages in thread
From: SZEDER Gábor @ 2022-01-18 21:02 UTC (permalink / raw)
  To: Elijah Newren
  Cc: Junio C Hamano, Lessley Dennington via GitGitGadget,
	Git Mailing List, Derrick Stolee, johannes.schindelin,
	Lessley Dennington

On Mon, Jan 17, 2022 at 10:14:58AM -0800, Elijah Newren wrote:
> 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.


^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH v3 2/3] sparse-checkout: custom tab completion
  2022-01-18 21:02               ` SZEDER Gábor
@ 2022-01-18 21:43                 ` Elijah Newren
  0 siblings, 0 replies; 51+ messages in thread
From: Elijah Newren @ 2022-01-18 21:43 UTC (permalink / raw)
  To: SZEDER Gábor
  Cc: Junio C Hamano, Lessley Dennington via GitGitGadget,
	Git Mailing List, Derrick Stolee, johannes.schindelin,
	Lessley Dennington

On Tue, Jan 18, 2022 at 1:02 PM SZEDER Gábor <szeder.dev@gmail.com> wrote:
>
> On Mon, Jan 17, 2022 at 10:14:58AM -0800, Elijah Newren wrote:
> > 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.

Not sure it matters since Junio has stated his position pretty
strongly, making it irrelevant at this point, but.... I'm not sure I'm
following you.  Is this the quote you're referring to?

"""
While $GIT_DIR/info/sparse-checkout is usually used to specify what
files are included,
"""

If so,
    /foo[A-M]*/pref?x_*.c
could be an entry in the file "used to specify what files are
included", but it's not a file.  The two example lines immediately
following aren't filenames either (though the '!'-prefixed line is
close).  Or are you referring to something else?

^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH v3 2/3] sparse-checkout: custom tab completion
  2022-01-16  1:03         ` Elijah Newren
  2022-01-16 22:13           ` Junio C Hamano
  2022-01-18 17:59           ` Lessley Dennington
@ 2022-01-18 22:22           ` SZEDER Gábor
  2022-01-18 23:30             ` Elijah Newren
  2 siblings, 1 reply; 51+ messages in thread
From: SZEDER Gábor @ 2022-01-18 22:22 UTC (permalink / raw)
  To: Elijah Newren
  Cc: Lessley Dennington via GitGitGadget, Git Mailing List,
	Derrick Stolee, Junio C Hamano, johannes.schindelin,
	Lessley Dennington

On Sat, Jan 15, 2022 at 05:03:24PM -0800, Elijah Newren wrote:
> On Sat, Jan 15, 2022 at 1:57 AM SZEDER Gábor <szeder.dev@gmail.com> wrote:
> >
> > On Mon, Jan 10, 2022 at 06:59:51PM +0000, Lessley Dennington via GitGitGadget wrote:
> > > 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.
> 
> ...
> > > 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.

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

Or about disk space.  Which, because of the potentially significantly
reduced number of files in the work tree can bring along significant
performance benefits, even with quadratic behavior.

>  (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.")
> 
> > 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.

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



^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH v3 2/3] sparse-checkout: custom tab completion
  2022-01-18 22:22           ` SZEDER Gábor
@ 2022-01-18 23:30             ` Elijah Newren
  0 siblings, 0 replies; 51+ messages in thread
From: Elijah Newren @ 2022-01-18 23:30 UTC (permalink / raw)
  To: SZEDER Gábor
  Cc: Lessley Dennington via GitGitGadget, Git Mailing List,
	Derrick Stolee, Junio C Hamano, johannes.schindelin,
	Lessley Dennington

On Tue, Jan 18, 2022 at 2:22 PM SZEDER Gábor <szeder.dev@gmail.com> wrote:
>
> On Sat, Jan 15, 2022 at 05:03:24PM -0800, Elijah Newren wrote:
> > On Sat, Jan 15, 2022 at 1:57 AM SZEDER Gábor <szeder.dev@gmail.com> wrote:
> > >
> > > On Mon, Jan 10, 2022 at 06:59:51PM +0000, Lessley Dennington via GitGitGadget wrote:
...
> > > > 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.

Fair enough.

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

Yeah, we should really make cone mode the default, because then this
simplifies to:

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

In cone-mode you automatically get the directories specified PLUS:

  * all files in the toplevel directory (you always get these in cone mode)
  * all files directly within a parent (or ancestor) of one of the
directories you specified (thus all files directly under tools/ and
tools/arch/).

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

Cool, thanks for testing it out and reporting.

If I can be permitted to share a bit of my experience...

In this specific case, you did keep it to 8 patterns and the linux
kernel "only" has ~60k files.  So O(N*M) isn't too bad here...but
experience at $DAYJOB is that even with only 60k files, the pattern
list tends to grow and it's not clear to users why there are so many
ugly pauses when "Git used to be fast".  Users tend to not make the
connection either between the sparsity patterns and the slowness for
whatever reason, and when I'm asked questions about the slowness, I
may have to check a few other possible causes before finally realizing
that it's just "too many sparsity patterns".

At least, that's the way it was at $DAYJOB before I switched our
internal "sparsify" tool to just use cone mode.  Then that type of
problem went away.

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

Or we can make it even easier and faster by using cone mode.  That
really, really should be the default (as we've been talking about for
a couple cycles now), and I think your email reinforces my belief in
it.  I've got a series ready after some other things merge down to
next.

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

Ah, very good point.  The leading '/' is kind of critical here,
otherwise you get all .gitignore files in every directory at every
depth, so completing on files (as currently done) wouldn't really help
at all in your case.


Since you brought up a new good point here, can I also mention two
others not previously covered in this thread (i.e. besides scaling)?

* If we complete file and directory names in non-cone mode, and those
file and directory names contain a leading '#' or '!' or contain
anywhere in their names a '*', '?', '\', '[', or ']', then the users
may be in for a very nasty surprise.  (Which perhaps suggests that in
non-cone mode, we shouldn't provide file OR directory completions for
set/add?)

* sparse-checkout currently ignores prefix[1], causing file and
directory completions to be wrong if the user is running from a
subdirectory.  This is just a bug in cone mode, though I'm not sure if
it's bug or feature in non-cone mode[2].  A decision about what it is
might have a bearing on what kinds of completions make sense in
non-cone mode (and might reinforce files/directories, or suggest
something else.).

[1] https://lore.kernel.org/git/29f0410e-6dfa-2e86-394d-b1fb735e7608@gmail.com/
[2] https://lore.kernel.org/git/CABPp-BH5woi6KY7OBpnsS-M2EmgLHii9zs8rSwrgcPFkOAvn_A@mail.gmail.com/

^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH v3 2/3] sparse-checkout: custom tab completion
  2022-01-18 17:56                 ` Lessley Dennington
@ 2022-01-22  1:07                   ` Lessley Dennington
  2022-01-22  1:08                     ` Lessley Dennington
  0 siblings, 1 reply; 51+ messages in thread
From: Lessley Dennington @ 2022-01-22  1:07 UTC (permalink / raw)
  To: Junio C Hamano, Elijah Newren
  Cc: SZEDER Gábor, Lessley Dennington via GitGitGadget,
	Git Mailing List, Derrick Stolee, johannes.schindelin

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

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/

^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH v3 2/3] sparse-checkout: custom tab completion
  2022-01-22  1:07                   ` Lessley Dennington
@ 2022-01-22  1:08                     ` Lessley Dennington
  2022-01-22  2:11                       ` Lessley Dennington
  0 siblings, 1 reply; 51+ messages in thread
From: Lessley Dennington @ 2022-01-22  1:08 UTC (permalink / raw)
  To: Junio C Hamano, Elijah Newren
  Cc: SZEDER Gábor, Lessley Dennington via GitGitGadget,
	Git Mailing List, Derrick Stolee, johannes.schindelin



On 1/21/22 5:07 PM, Lessley Dennington wrote:
>>> 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/ 
>>
> 
> 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/ 
> 
My apologies, I will not remove directory completion, but rather will
return to completing on both files and directories.

^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH v3 2/3] sparse-checkout: custom tab completion
  2022-01-22  1:08                     ` Lessley Dennington
@ 2022-01-22  2:11                       ` Lessley Dennington
  0 siblings, 0 replies; 51+ messages in thread
From: Lessley Dennington @ 2022-01-22  2:11 UTC (permalink / raw)
  To: Junio C Hamano, Elijah Newren
  Cc: SZEDER Gábor, Lessley Dennington via GitGitGadget,
	Git Mailing List, Derrick Stolee, johannes.schindelin



On 1/21/22 5:08 PM, Lessley Dennington wrote:
> 
> 
> On 1/21/22 5:07 PM, Lessley Dennington wrote:
>>>> 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/ 
>>>
>>
>> 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/ 
>>
> My apologies, I will not remove directory completion, but rather will
> return to completing on both files and directories.

My apologies again, I think I mistakenly assumed Junio did not want
directory-only completion for cone mode based on this comment from [1]:

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.

However, this line (which I overlooked when I was re-reading this
evening), leads me to believe that is not the case:

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

It has also been pointed out to me that saying cone mode is going away is
incorrect - it is just being deprecated.

So, I will stick to my original plan of continuing to complete on
directories for cone mode and completing on both files and directories
for non-cone mode.

Again, apologies for the noise.

[1]: https://lore.kernel.org/git/xmqqv8yjz5us.fsf@gitster.g/

^ permalink raw reply	[flat|nested] 51+ messages in thread

end of thread, other threads:[~2022-01-22  2:11 UTC | newest]

Thread overview: 51+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-30  0:32 [PATCH 0/2] sparse checkout: custom bash completion updates Lessley Dennington via GitGitGadget
2021-12-30  0:32 ` [PATCH 1/2] sparse-checkout: custom tab completion tests Lessley Dennington via GitGitGadget
2021-12-30 13:43   ` Derrick Stolee
2021-12-30 16:19     ` Lessley Dennington
2021-12-30 17:43       ` Derrick Stolee
2021-12-31 19:27         ` Elijah Newren
2022-01-04 19:19           ` Lessley Dennington
2021-12-30  0:32 ` [PATCH 2/2] sparse-checkout: custom tab completion Lessley Dennington via GitGitGadget
2021-12-30 13:50   ` Derrick Stolee
2021-12-30 16:24     ` Lessley Dennington
2021-12-30 19:26 ` [PATCH v2 0/2] sparse checkout: custom bash completion updates Lessley Dennington via GitGitGadget
2021-12-30 19:26   ` [PATCH v2 1/2] sparse-checkout: custom tab completion tests Lessley Dennington via GitGitGadget
2021-12-31 20:03     ` Elijah Newren
2021-12-31 22:20       ` Junio C Hamano
2021-12-31 22:25         ` Elijah Newren
2022-01-04 19:25         ` Lessley Dennington
2022-01-04 20:25           ` Elijah Newren
2022-01-05 14:05             ` Lessley Dennington
2022-01-04 19:24       ` Lessley Dennington
2021-12-30 19:26   ` [PATCH v2 2/2] sparse-checkout: custom tab completion Lessley Dennington via GitGitGadget
2021-12-31 22:52     ` Elijah Newren
2022-01-04 19:41       ` Lessley Dennington
2022-01-04 20:42         ` Elijah Newren
2022-01-05 20:20           ` Lessley Dennington
2022-01-05 22:55             ` Elijah Newren
2022-01-10 18:59   ` [PATCH v3 0/3] sparse checkout: custom bash completion updates Lessley Dennington via GitGitGadget
2022-01-10 18:59     ` [PATCH v3 1/3] sparse-checkout: custom tab completion tests Lessley Dennington via GitGitGadget
2022-01-10 18:59     ` [PATCH v3 2/3] sparse-checkout: custom tab completion Lessley Dennington via GitGitGadget
2022-01-15  9:57       ` SZEDER Gábor
2022-01-16  1:03         ` Elijah Newren
2022-01-16 22:13           ` Junio C Hamano
2022-01-17 18:14             ` Elijah Newren
2022-01-17 19:40               ` Junio C Hamano
2022-01-18 17:56                 ` Lessley Dennington
2022-01-22  1:07                   ` Lessley Dennington
2022-01-22  1:08                     ` Lessley Dennington
2022-01-22  2:11                       ` Lessley Dennington
2022-01-18 21:02               ` SZEDER Gábor
2022-01-18 21:43                 ` Elijah Newren
2022-01-18 17:59           ` Lessley Dennington
2022-01-18 22:22           ` SZEDER Gábor
2022-01-18 23:30             ` Elijah Newren
2022-01-10 18:59     ` [PATCH v3 3/3] sparse-checkout: limit tab completion to a single level Lessley Dennington via GitGitGadget
2022-01-12 23:43       ` Lessley Dennington
2022-01-13  0:00         ` Junio C Hamano
2022-01-13  0:38         ` Elijah Newren
2022-01-13 19:02           ` Lessley Dennington
2022-01-10 20:38     ` [PATCH v3 0/3] sparse checkout: custom bash completion updates Elijah Newren
2022-01-11 17:17       ` Lessley Dennington
2022-01-11 19:45         ` Taylor Blau
2022-01-12 18:35           ` Lessley Dennington

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).