From: Lessley Dennington Date: Thu, 13 Jan 2022 19:02:37 GMT Subject: Re: [PATCH v3 3/3] sparse-checkout: limit tab completion to a single level Message-ID: <92368b84-1d82-b594-9232-e67abcffe18a@gmail.com> In-Reply-To: On 1/12/22 6:38 PM, Elijah Newren wrote: > On Wed, Jan 12, 2022 at 3:43 PM Lessley Dennington > 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 > (note the space after README.md before pressing ), 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 > (with no space between README.md and ), 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 Doctecm > > then you'll note after the first you'll see > ls Documentation/ > with no trailing space; after the second you'll see > ls Documentation/technical/ > with no trailing space; and after the third 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