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

[Lessley Dennington <lessleydennington@gmail.com>]

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