[v3,3/3] sparse-checkout: limit tab completion to a single level

Commit Message

Lessley Dennington Jan. 10, 2022, 6:59 p.m. UTC

From: Lessley Dennington <[e-mail]>

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 <[e-mail]>
Co-authored-by: Lessley Dennington <[e-mail]>
Signed-off-by: Lessley Dennington <[e-mail]>
---
 contrib/completion/git-completion.bash | 34 ++++++++++++++++++++++++--
 t/t9902-completion.sh                  | 29 ++++++++++++++--------
 2 files changed, 51 insertions(+), 12 deletions(-)

Comments

Lessley Dennington Jan. 12, 2022, 11:43 p.m. UTC | #1

> +__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/[e-mail]/T/#md3da435452988b0366ab4c2ee4bc06df2d17cb36

Junio C Hamano Jan. 13, 2022, midnight UTC | #2

Lessley Dennington <[e-mail]> 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.

Elijah Newren Jan. 13, 2022, 12:38 a.m. UTC | #3

On Wed, Jan 12, 2022 at 3:43 PM Lessley Dennington
<[e-mail]> 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/[e-mail]/T/#md3da435452988b0366ab4c2ee4bc06df2d17cb36

Lessley Dennington Jan. 13, 2022, 7:02 p.m. UTC | #4

On 1/12/22 6:38 PM, Elijah Newren wrote:
> On Wed, Jan 12, 2022 at 3:43 PM Lessley Dennington
> <[e-mail]> 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/[e-mail]/T/#md3da435452988b0366ab4c2ee4bc06df2d17cb36

Patch

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