__git_ignore_line positional argument (un)escaping (was: Re: _git-reset doesn't complete newly staged files)

[Daniel Shahaf <d.s@daniel.shahaf.name>]

I wrote this before I saw Mikael's post, but I'm sending it anyway,
since it may still be relevant.  [Search for "bQ" for the
non-_git-specific discussion.]

Jun T. wrote on Thu, Mar 10, 2016 at 21:03:59 +0900:
> On 2016/03/09, at 22:24, Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> > I think you're looking for «git diff-index --cached -z --name-only
> > $treeish».
> 
> What I was not sure was whether to offer conflicting files or not.
> 'git reset conflicting_file' leaves the working tree unmodified
> (so '<<<<<<< HEAD' etc. remain there) while the index becomes clean
> (conflict fixed) and you can commit it. I thought this would not
> be useful.
> 
> But it is not an error anyway, and maybe there are someone who
> really want to do that (if they are going to run 'git reset' while
> there is a conflict, they may well know what they are doing).
> 

Agreed: it wouldn't be a typical workflow, but it's admitted by the
tool.

I lean on the side of just offering all valid completions, including
conflicted files.  That:

- Keeps the completion interface simpler, with fewer exceptions for
  users to have to know about;

- Leaves issueing warnings about inadvisable usages to git(1) itself,
  which is better placed than compsys to decide when such warnings are
  warranted.

> > I took the liberty of changing the tag and description you used, since
> > "staged files" isn't exactly right for this syntax: files that are in
> > $treeish but not staged may also be completed.
> 
> I thought those files are 'staged for delete', but your more
> descriptive tag/description would be better.
> 

Okay, then I'll keep them when I commit that patch.

> > Incidentally, it would be nice to have a docstring for __git_ignore_line:
> 
> Yes, and
> 

Did you intend to type more words in that sentence?

> > Why does __git_ignore_line escape some characters with backslashes
> > before adding them to $ignored?
> 
> I'm rather confused. Isn't it better to UN-quote them?
> 
> % touch 'a b.txt'
> % git add <TAB>
> this completes a\ b.txt, and
> % git add a\ b.txt <TAB>
> this still completes a\ b.txt.
> 
> Using
>     ignored+=(${(Q)line[1,CURRENT-1]})
>     ignored+=(${(Q)line[CURRENT+1,-1]})
> fixes this.
> 
> Moreover,
>
> % git add *.txt <TAB>
> this does not complete a\ b.txt anymore.
> 
> Or there are something I've overlooked.

With just ${(Q)line[...}}, you lose the ability to distinguish «git add
f\[a-z\]o <TAB>» from «git add f[a-z]o <TAB>».  It took me a while to
find an incantation that works, but I think «${(bQ)line[...}}» is it:

    % ls -1
    [f]oo
    f[a-z]o
    foo
    % () { eval "echo ${(bQ)1}" } '\[f\]oo'
    [f]oo
    % () { eval "echo ${(bQ)1}" } '[f]oo'  
    foo
    % () { eval "echo ${(bQ)1}" } 'f[a-z]o' 
    foo
    % () { eval "echo ${(bQ)1}" } 'f\[a-z\]o'
    f[a-z]o

    % ls -1
    bar.txt
    baz.txt
    % () { eval "echo ${(bQ)1}" } '*.txt'
    bar.txt baz.txt

(The purpose of the "eval" is to force the result of parameter expansion
to be interpreted as a pattern, since elements of $ignored are
interpreted by compadd as patterns.)

With the argument to 'compadd -F' constructed using ${(bQ)}, all these
patterns do the right thing: they ignore exactly the files the anonymous
function prints and no others.  For example, «git add f[a-z]o <TAB>»
offers «f\[a-z\]o» but not «foo», and so on.

So, three questions:

1. Should regression tests be added for these uses of ${(bQ)}?

2. The ignore-line currently uses the following escaping:
.
   # Completion/Base/Core/_description
   50	  if zstyle -s ":completion:${curcontext}:$1" ignore-line hidden; then
   51	    local -a qwords
   52	    qwords=( ${words//(#m)[\[\]()\\*?#<>~\^\|]/\\$MATCH} )
.
Should the ignore-line style use ${(bQ)}?

3. Is the following patch correct?

diff --git a/Completion/Unix/Command/_git b/Completion/Unix/Command/_git
index 2dd9a80..e063a6f 100644
--- a/Completion/Unix/Command/_git
+++ b/Completion/Unix/Command/_git
@@ -4961,7 +4961,7 @@ __git_committish_range_last () {
 
 (( $+functions[__git_pattern_escape] )) ||
 __git_pattern_escape () {
-  print -r -n ${1//(#m)[\[\]()\\*?#<>~\^]/\\$MATCH}
+  print -r -n - ${(b)1}
 }
 
 (( $+functions[__git_is_type] )) ||
@@ -5053,8 +5053,8 @@ __git_ignore_line () {
   declare -a ignored
   ignored=()
   ((CURRENT > 1)) &&
-    ignored+=(${line[1,CURRENT-1]//(#m)[\[\]()\\*?#<>~\^]/\\$MATCH})
+    ignored+=(${(bQ)line[1,CURRENT-1]})
   ((CURRENT < $#line)) &&
-    ignored+=(${line[CURRENT+1,-1]//(#m)[\[\]()\\*?#<>~\^]/\\$MATCH})
+    ignored+=(${(bQ)line[CURRENT+1,-1]})
   $* -F ignored
 }

?

Cheers,

Daniel

P.S. I suppose a tree-wide grep for more of those hand-rolled
implementations of ${(b)}/${(q)} wouldn't be a bad thing...