From: Lessley Dennington Date: Fri, 04 Feb 2022 00:37:36 GMT Subject: Re: [PATCH v5 3/3] completion: handle unusual characters for sparse-checkout Message-ID: In-Reply-To: On 2/3/22 3:58 PM, Elijah Newren wrote: > On Thu, Feb 3, 2022 at 12:44 PM Lessley Dennington via GitGitGadget > wrote: >> >> From: Lessley Dennington >> >> Update the __gitcomp_directories method to de-quote and handle unusual >> characters in directory names. Although this initially involved an attempt >> to re-use the logic in __git_index_files, this method removed >> subdirectories (e.g. folder1/0/ became folder1/), so instead new custom >> logic was placed directly in the __gitcomp_directories method. >> >> Co-authored-by: Johannes Schindelin >> Co-authored-by: Lessley Dennington >> Signed-off-by: Lessley Dennington >> --- >> contrib/completion/git-completion.bash | 50 +++++++++++++------------- >> t/t9902-completion.sh | 28 +++++++++++++++ >> 2 files changed, 52 insertions(+), 26 deletions(-) >> >> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash >> index c5c8df6b6e5..c47e9ce09b2 100644 >> --- a/contrib/completion/git-completion.bash >> +++ b/contrib/completion/git-completion.bash >> @@ -2988,32 +2988,30 @@ _git_show_branch () >> >> __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 >> + local _tmp_dir _tmp_completions _found=0 >> + >> + # 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, >> + # de-quoting, and handling unusual characters. >> + while IFS= read -r -d $'\0' c ; do >> + # If there are directory completions, find ones that start >> + # with "$cur", the current token, and put those in COMPREPLY >> + if [[ $c == "$cur"* ]]; then >> + COMPREPLY+=("$c/") >> + _found=1 >> + fi >> + done < <(git ls-tree -z -d --name-only HEAD $_tmp_dir) >> + >> + if [[ $_found == 0 ]] && [[ "$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 > > The indentation changes are distracting and make the patch harder to > review. Could you either remove those, or apply the indentation > changes to patch 2 so that it starts with the right indentation? I've also corrected this as part of the fix I'm about to submit. > > I'm slightly surprised that __gitcomp_direct_append handles the > quoting for us, but the testcases below seem to cover it, so that's > cool. > > Anyway, looks pretty clever to me; I was worried this was going to > require a much bigger change. > >> } >> >> _git_sparse_checkout () >> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh >> index b38a7302249..7f63d6057be 100755 >> --- a/t/t9902-completion.sh >> +++ b/t/t9902-completion.sh >> @@ -1508,6 +1508,34 @@ test_expect_success 'cone mode sparse-checkout completes directory names' ' >> ) >> ' >> >> +# use FUNNYNAMES to avoid running on Windows, which doesn't permit backslashes in paths >> +test_expect_success FUNNYNAMES 'cone mode sparse-checkout completes directory names with special characters' ' >> + # reset sparse-checkout >> + git -C sparse-checkout sparse-checkout disable && >> + ( >> + cd sparse-checkout && >> + mkdir "directory with spaces" && >> + mkdir "$(printf "directory\twith\ttabs")" && >> + mkdir "directory\with\backslashes" && >> + mkdir "directory-with-áccent" && >> + >"directory with spaces/randomfile" && >> + >"$(printf "directory\twith\ttabs")/randomfile" && >> + >"directory\with\backslashes/randomfile" && >> + >"directory-with-áccent/randomfile" && >> + git add . && >> + git commit -m "Add directories containing unusual characters" && >> + git sparse-checkout set --cone "directory with spaces" \ >> + "$(printf "directory\twith\ttabs")" "directory\with\backslashes" \ >> + "directory-with-áccent" && >> + test_completion "git sparse-checkout add dir" <<-\EOF >> + directory with spaces/ >> + directory with tabs/ >> + directory\with\backslashes/ >> + directory-with-áccent/ >> + EOF >> + ) >> +' > > I'm glad you tested with lots of special characters -- spaces, tabs, > backslashes, and accents. Newlines might also be nice, but probably > makes the test hard. Anyway, looks good to me, other than the > indentation change. > >> + >> test_expect_success 'non-cone mode sparse-checkout uses bash completion' ' >> # reset sparse-checkout repo to non-cone mode >> git -C sparse-checkout sparse-checkout disable && >> -- >> gitgitgadget