[1/2] sparse-checkout: custom tab completion tests

Commit Message

Lessley Dennington Dec. 30, 2021, 12:32 a.m. UTC

From: Lessley Dennington <[e-mail]>

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 <[e-mail]>
---
 t/t9902-completion.sh | 74 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 74 insertions(+)

Comments

Derrick Stolee Dec. 30, 2021, 1:43 p.m. UTC | #1

On 12/29/2021 7:32 PM, Lessley Dennington via GitGitGadget wrote:
> From: Lessley Dennington <[e-mail]>
> 
> 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

Lessley Dennington Dec. 30, 2021, 4:19 p.m. UTC | #2

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/[e-mail]/

Derrick Stolee Dec. 30, 2021, 5:43 p.m. UTC | #3

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

Elijah Newren Dec. 31, 2021, 7:27 p.m. UTC | #4

On Fri, Dec 31, 2021 at 2:30 AM Derrick Stolee <[e-mail]> 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.

Lessley Dennington Jan. 4, 2022, 7:19 p.m. UTC | #5

On 12/31/21 1:27 PM, Elijah Newren wrote:
> On Fri, Dec 31, 2021 at 2:30 AM Derrick Stolee <[e-mail]> 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=[e-mail]/

Patch

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