_git-reset doesn't complete newly staged files

_git-reset doesn't complete newly staged files

[Jun T.]

Suppose we are in a simple git repo with two files:
% ls
foo.c    main.c

Now create a new file and stage it:
% vi bar.c
% git add bar.c
% git status
# On branch master
# Changes to be committed:
#   (use "git reset HEAD <file>..." to unstage)
#
#	new file:   bar.c
#

Then if I type
% git reset HEAD <TAB>
foo.c   main.c          # bar.c is NOT offered.

_git-reset() calls __git_tree_files(), which lists only
the already tracked files.

Maybe we could use
git status -uno --porcelain | grep '^[ADM]'
to get a list of staged files (files which can be reset)?

In the case of 'git reset <TAB>' or 'git reset HEAD <TAB>',
I think it would be better to offer only bar.c (because
'git reset HEAD main.c' is just a nop), but at least
bar.c need be offered in addition to foo.c and main.c.

Re: _git-reset doesn't complete newly staged files

[Jun T.]

In the case of

% git reset HEAD <TAB>

the patch below tries to offer only staged files (including
newly staged files which have never been committed).
A new function __git_staged_files() ignores unmerged (conflicting)
files, but I'm not sure this is correct or not.

In the case of

% git reset commit <TAB>    # commit is not HEAD

I have no idea what to do, so I just added __git_ignore_line
to the original code. 



diff --git a/Completion/Unix/Command/_git b/Completion/Unix/Command/_git
index b7eaf2e..7a459f1 100644
--- a/Completion/Unix/Command/_git
+++ b/Completion/Unix/Command/_git
@@ -1377,7 +1377,11 @@ _git-reset () {
       if [[ -n $line[1] ]] && __git_is_committish $line[1]; then
         commit=$line[1]
       fi
-      __git_tree_files ${PREFIX:-.} $commit && ret=0
+      if [[ $commit == HEAD ]]; then
+	__git_ignore_line __git_staged_files && ret=0
+      else
+	__git_ignore_line __git_tree_files ${PREFIX:-.} $commit && ret=0
+      fi
       ;;
   esac
 
@@ -6131,6 +6135,29 @@ __git_tree_files () {
   _wanted files expl 'tree file' _multi_parts -f $compadd_opts -- / tree_files
 }
 
+(( $+functions[__git_staged_files] )) ||
+__git_staged_files () {
+  local -a slist staged_files
+  local item expl i
+
+  slist=(${(0)"$(_call_program staged-files git status -z -uno 2>/dev/null)"})
+  __git_command_successful $pipestatus || return 1
+
+  for (( i = 1; i <= $#slist; ++i )) do
+    item=$slist[i]
+    if [[ $item == (DD|AA|U|?U)* ]]; then
+      continue	  #XXX skip unmerged files
+    elif [[ $item == R* ]]; then
+      staged_files+=( $item[4,-1] $slist[++i] )
+    elif [[ $item == [ACDM]* ]]; then
+      staged_files+=( $item[4,-1] )
+    fi
+  done
+  staged_files=( ${(0)"$(__git_files_relative ${(pj:\0:)staged_files})"} )
+
+  _wanted staged-files expl 'staged file' compadd "$@" -a - staged_files
+}
+
 # Repository Argument Types
 
 (( $+functions[__git_remote_repositories] )) ||

Re: _git-reset doesn't complete newly staged files

[Jun T.]

Are there any comments to my patch? Especially from those
who know git better.
If there is nothing obviously wrong, I'll push it.

Re: _git-reset doesn't complete newly staged files

[Daniel Shahaf]

Jun T. wrote on Mon, Mar 07, 2016 at 12:16:54 +0900:
> Are there any comments to my patch? Especially from those
> who know git better.
> If there is nothing obviously wrong, I'll push it.

Sorry for not replying earlier: I had this flagged for attention but
haven't spoken up sooner since I expected I wouldn't have time to reply
to responses.

More below.

Jun T. wrote on Thu, Mar 03, 2016 at 14:07:12 +0900:
> In the case of
> 
> % git reset HEAD <TAB>
> 
> the patch below tries to offer only staged files (including
> newly staged files which have never been committed).
> A new function __git_staged_files() ignores unmerged (conflicting)
> files, but I'm not sure this is correct or not.
> 
> In the case of
> 
> % git reset commit <TAB>    # commit is not HEAD
> 
> I have no idea what to do, so I just added __git_ignore_line
> to the original code. 
> 
> 
> 
> diff --git a/Completion/Unix/Command/_git b/Completion/Unix/Command/_git
> index b7eaf2e..7a459f1 100644
> --- a/Completion/Unix/Command/_git
> +++ b/Completion/Unix/Command/_git
> @@ -1377,7 +1377,11 @@ _git-reset () {
>        if [[ -n $line[1] ]] && __git_is_committish $line[1]; then
>          commit=$line[1]
>        fi

This should actually call __git_is_treeish, not __git_is_committish.

(That's a preëxisting problem: it precedes your patch.)

> -      __git_tree_files ${PREFIX:-.} $commit && ret=0
> +      if [[ $commit == HEAD ]]; then

As you say, special-casing HEAD this way is not wrong, but shouldn't be
needed: "HEAD" is a treeish and should be treated like any other
treeish.

I think you're looking for «git diff-index --cached -z --name-only
$treeish».  That gives the names of files that differ between the index
and $treeish.  (I wrote «$treeish» in pseudocode, but the real code's
local parameter «$commit» contains exactly the right value.)

I'm not sure how to handle the syntax 'git reset foo' where 'foo' is
both a file in the index and a treeish (e.g., a tag name).

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

    diff --git a/Completion/Unix/Command/_git b/Completion/Unix/Command/_git
    index b7eaf2e..8043bb9 100644
    --- a/Completion/Unix/Command/_git
    +++ b/Completion/Unix/Command/_git
    @@ -5034,6 +5034,16 @@ __git_describe_commit () {
     
     # Completion Wrappers
     
    +# '__git_ignore_line $callee "${callee_args[@]}" "${callee_compadd_args[@]}"'
    +# invokes '$callee "${callee_args[@]}" "${callee_compadd_args[@]}"' with
    +# callee_compadd_args modified to exclude positional parameters to the completed
    +# command from being completed.  This causes 'git add foo <TAB>' not to offer
    +# 'foo' again.
    +#
    +# Note: This function can't be used to wrap bare 'compadd' calls that use a '--'
    +# argument terminator.  It can wrap functions of the form
    +#     f() { shift $N; compadd "$@" -a - mymatches }
    +# .
     (( $+functions[__git_ignore_line] )) ||
     __git_ignore_line () {
       declare -a ignored

Why does __git_ignore_line escape some characters with backslashes
before adding them to $ignored?

Thanks,

Daniel

> +	__git_ignore_line __git_staged_files && ret=0
> +      else
> +	__git_ignore_line __git_tree_files ${PREFIX:-.} $commit && ret=0
> +      fi
>        ;;
>    esac
>  
> @@ -6131,6 +6135,29 @@ __git_tree_files () {
>    _wanted files expl 'tree file' _multi_parts -f $compadd_opts -- / tree_files
>  }
>  
> +(( $+functions[__git_staged_files] )) ||
> +__git_staged_files () {
> +  local -a slist staged_files
> +  local item expl i
> +
> +  slist=(${(0)"$(_call_program staged-files git status -z -uno 2>/dev/null)"})
> +  __git_command_successful $pipestatus || return 1
> +
> +  for (( i = 1; i <= $#slist; ++i )) do
> +    item=$slist[i]
> +    if [[ $item == (DD|AA|U|?U)* ]]; then
> +      continue	  #XXX skip unmerged files
> +    elif [[ $item == R* ]]; then
> +      staged_files+=( $item[4,-1] $slist[++i] )
> +    elif [[ $item == [ACDM]* ]]; then
> +      staged_files+=( $item[4,-1] )
> +    fi
> +  done
> +  staged_files=( ${(0)"$(__git_files_relative ${(pj:\0:)staged_files})"} )
> +
> +  _wanted staged-files expl 'staged file' compadd "$@" -a - staged_files
> +}
> +
>  # Repository Argument Types
>  
>  (( $+functions[__git_remote_repositories] )) ||
> 
> 
> 
> 
> 

Re: _git-reset doesn't complete newly staged files

[Daniel Shahaf]

[-- Attachment #1: Type: text/plain, Size: 708 bytes --]

Daniel Shahaf wrote on Wed, Mar 09, 2016 at 13:24:44 +0000:
> Jun T. wrote on Mon, Mar 07, 2016 at 12:16:54 +0900:
> > In the case of
> > 
> > % git reset commit <TAB>    # commit is not HEAD
> > 
> > I have no idea what to do, so I just added __git_ignore_line
> > to the original code. 
> 
> I think you're looking for «git diff-index --cached -z --name-only
> $treeish».

Turns out there was already a helper function that did almost exactly
that, so we can just reuse it.  Patch attached.

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.

WDYT?

Thanks,

Daniel

[-- Attachment #2: 0001-NNNNN-after-38074-_git-reset-treeish-complete-only-s.patch --]
[-- Type: text/x-diff, Size: 3390 bytes --]

>From 92fcd0ed18f4c0122f7cdda7930979cd624a33cd Mon Sep 17 00:00:00 2001
From: Daniel Shahaf <d.s@daniel.shahaf.name>
Date: Wed, 9 Mar 2016 22:11:22 +0000
Subject: [PATCH] NNNNN (after 38074): _git reset $treeish: complete only
 staged files

---
 Completion/Unix/Command/_git | 56 ++++++++++++++++----------------------------
 1 file changed, 20 insertions(+), 36 deletions(-)

diff --git a/Completion/Unix/Command/_git b/Completion/Unix/Command/_git
index 2dd9a80..b1a7431 100644
--- a/Completion/Unix/Command/_git
+++ b/Completion/Unix/Command/_git
@@ -1373,16 +1373,11 @@ _git-reset () {
 
   case $state in
     (file)
-      local commit=HEAD
-      if [[ -n $line[1] ]] && __git_is_committish $line[1]; then
-        commit=$line[1]
+      local tree=HEAD
+      if [[ -n $line[1] ]] && __git_is_treeish $line[1]; then
+        tree=$line[1]
       fi
-      if [[ $commit == HEAD ]]; then
-	__git_ignore_line __git_staged_files && ret=0
-      else
-	__git_ignore_line __git_tree_files ${PREFIX:-.} $commit && ret=0
-      fi
-      ;;
+      __git_ignore_line __git_treeish-to-index_files $tree && ret=0
   esac
 
   return ret
@@ -6077,16 +6072,28 @@ __git_killed_files () {
   __git_files --killed killed-files 'killed file' $*
 }
 
-(( $+functions[__git_changed-in-index_files] )) ||
-__git_changed-in-index_files () {
+(( $+functions[__git_diff-index_files] )) ||
+__git_diff-index_files () {
+  local tree=$1 description=$2 tag=$3; shift 3
   local files expl
 
-  files=$(_call_program files git diff-index -z --name-only --no-color --cached HEAD 2>/dev/null)
+  files=$(_call_program files git diff-index -z --name-only --no-color --cached $tree 2>/dev/null)
   __git_command_successful $pipestatus || return 1
   files=(${(0)"$(__git_files_relative $files)"})
   __git_command_successful $pipestatus || return 1
 
-  _wanted changed-in-index-files expl 'changed in index file' _multi_parts $@ - / files
+  _wanted $tag expl $description _multi_parts $@ - / files
+}
+
+(( $+functions[__git_changed-in-index_files] )) ||
+__git_changed-in-index_files () {
+  __git_diff-index_files HEAD 'changed in index file' changed-in-index-files "$@"
+}
+
+(( $+functions[__git_treeish-to-index_files] )) ||
+__git_treeish-to-index_files () {
+  local tree=$1; shift
+  __git_diff-index_files $tree "files different between ${(qq)tree} and the index" treeish-to-index-files "$@"
 }
 
 (( $+functions[__git_changed-in-working-tree_files] )) ||
@@ -6145,29 +6152,6 @@ __git_tree_files () {
   _wanted files expl 'tree file' _multi_parts -f $compadd_opts -- / tree_files
 }
 
-(( $+functions[__git_staged_files] )) ||
-__git_staged_files () {
-  local -a slist staged_files
-  local item expl i
-
-  slist=(${(0)"$(_call_program staged-files git status -z -uno 2>/dev/null)"})
-  __git_command_successful $pipestatus || return 1
-
-  for (( i = 1; i <= $#slist; ++i )) do
-    item=$slist[i]
-    if [[ $item == (DD|AA|U|?U)* ]]; then
-      continue	  #XXX skip unmerged files
-    elif [[ $item == R* ]]; then
-      staged_files+=( $item[4,-1] $slist[++i] )
-    elif [[ $item == [ACDM]* ]]; then
-      staged_files+=( $item[4,-1] )
-    fi
-  done
-  staged_files=( ${(0)"$(__git_files_relative ${(pj:\0:)staged_files})"} )
-
-  _wanted staged-files expl 'staged file' compadd "$@" -a - staged_files
-}
-
 # Repository Argument Types
 
 (( $+functions[__git_remote_repositories] )) ||

Re: _git-reset doesn't complete newly staged files

[Jun T.]

On 2016/03/09, at 22:24, Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> Sorry for not replying earlier: I had this flagged for attention but
> haven't spoken up sooner since I expected I wouldn't have time to reply
> to responses.

You are exactly the one from whom I wanted to have comments. 

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

> 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.

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

Yes, and

> 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.

Re: _git-reset doesn't complete newly staged files

[Mikael Magnusson]

On Thu, Mar 10, 2016 at 1:03 PM, Jun T. <takimoto-j@kba.biglobe.ne.jp> wrote:
>
>> 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.

Why does _git special case this at all, isn't the generic ignore-line
mechanism for the git-add context good enough?

-- 
Mikael Magnusson

[PATCH] _git: Fix completion of diffs against the index when treeish isn't shell-safe

[Daniel Shahaf]

This affects 'git diff --cached -- <TAB>' and 'git reset $treeish <TAB>'.
---
To be applied on top of the last patch.

 Completion/Unix/Command/_git | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Completion/Unix/Command/_git b/Completion/Unix/Command/_git
index f0ad3d9..6c88ad0 100644
--- a/Completion/Unix/Command/_git
+++ b/Completion/Unix/Command/_git
@@ -6077,7 +6077,8 @@ __git_diff-index_files () {
   local tree=$1 description=$2 tag=$3; shift 3
   local files expl
 
-  files=$(_call_program files git diff-index -z --name-only --no-color --cached $tree 2>/dev/null)
+  # $tree needs to be escaped for _call_program; matters for $tree = "HEAD^"
+  files=$(_call_program files git diff-index -z --name-only --no-color --cached ${(q)tree} 2>/dev/null)
   __git_command_successful $pipestatus || return 1
   files=(${(0)"$(__git_files_relative $files)"})
   __git_command_successful $pipestatus || return 1

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

[Daniel Shahaf]

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...

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

[Jun T.]

On 2016/03/11, at 8:20, Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> 
> 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>».
(snip)

With the following style:
zstyle ':completion:*:*:ls:*' ignore-line true               

With the current git HEAD:
[1]% touch foo 'f[a-z]o' '[f]oo'
[2]% ls f\[a-z\]o <TAB>      # OK: f\[a-z\]o not offered
[3]% git add f\[a-z\] <TAB>  # no: f\[a-z\]o is offered again
[4]% ls f[a-z]o <TAB>        # no: foo is offered
[5]% git add f[a-z]o <TAB>   # no: foo is offered

With ${(bQ)..} in __git_ignore_line and _description:
[6]% ls f\[a-z\]o <TAB>      # no: f\[a-z\]o is offered again
[7]% git add f\[a-z\] <TAB>  # OK: f\[a-z\]o is not offered
[8]% ls f[a-z]o <TAB>        # OK: foo is not offered
[9]% git add f[a-z]o <TAB>   # OK: foo is not offered

I don't know why the behavior is different between ls and git add
in the first two cases ([2][3]/[6][7]), because _description and
 __git_ignore_line uses virtually the same escaping method.


> 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)}?

I've copied the line from _rm about two years ago; see workers/32435.
I don't remember the detail (sorry), but it seems I tried to escape
only the characters which are already escaped on the command line so
that a bare (active, unescaped) * etc can go into _comp_ignore and
% ls f* <TAB>
does not offer foo etc. But I abandoned this method because patterns
with qualifier caused a problem like:

% touch foo          # foo does not have execution permission
% ls f*(x) <TAB>     # foo is NOT offered

With (bQ), this does't happen, but now (as in [6] above):

% touch 'a[b]c' 'd<e>f' 'g(h)i'
% ls a\[b\]c d\<e\>f g\(h\)i <TAB>  # all the files are still offered

This doesn't happen with the current HEAD. I don't know which is
better; maybe patterns like f* are frequently used but strange
file names like 'a[b]c' rarely happen, so using (bQ) is somewhat
better...? And I don't understand why ls and 'git add' behaves
differently.


On 2016/03/10, at 23:35, Mikael Magnusson <mikachu@gmail.com> wrote:

> Why does _git special case this at all, isn't the generic ignore-line
> mechanism for the git-add context good enough?

I have no idea why. I just assumed there was a reason to do that.

It would be better to use generic ignore-line if possible, of course.
If I have
zstyle ':completion:*:*:(^rm):*:*files' ignored-patterns '*?.o' '*?.old'
then '-F _comp_ignore' is passed to compadd, and the '-F ignored' added
by __git_ignroe_line has no effect. Of course I can use
zstyle ':completion:*:*:(^(rm|git*)):*:*files' ...
but ...

Sorry I fear I will not be able to reply for several days or so.

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

[Daniel Shahaf]

Jun T. wrote on Fri, Mar 11, 2016 at 17:24:02 +0900:
> On 2016/03/11, at 8:20, Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> > 
> > 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>».
> (snip)
> 
> With the following style:
> zstyle ':completion:*:*:ls:*' ignore-line true               
> 
> With the current git HEAD:
> [1]% touch foo 'f[a-z]o' '[f]oo'
> [2]% ls f\[a-z\]o <TAB>      # OK: f\[a-z\]o not offered
> [3]% git add f\[a-z\] <TAB>  # no: f\[a-z\]o is offered again
> [4]% ls f[a-z]o <TAB>        # no: foo is offered
> [5]% git add f[a-z]o <TAB>   # no: foo is offered
> 
> With ${(bQ)..} in __git_ignore_line and _description:
> [6]% ls f\[a-z\]o <TAB>      # no: f\[a-z\]o is offered again
> [7]% git add f\[a-z\] <TAB>  # OK: f\[a-z\]o is not offered
> [8]% ls f[a-z]o <TAB>        # OK: foo is not offered
> [9]% git add f[a-z]o <TAB>   # OK: foo is not offered
> 
> I don't know why the behavior is different between ls and git add
> in the first two cases ([2][3]/[6][7]), because _description and
>  __git_ignore_line uses virtually the same escaping method.
> 

I don't understand the difference either, and I'm afraid I don't have
the brainwidth to look into the 'ls'/_description issue at the moment.
However, since using (bQ) in _git seems to have no adverse effect, I'll
go ahead and commit 38129.  (We can always back it out if we find it
introduces a regression.)

> If I have
> zstyle ':completion:*:*:(^rm):*:*files' ignored-patterns '*?.o' '*?.old'
> then '-F _comp_ignore' is passed to compadd, and the '-F ignored' added
> by __git_ignore_line has no effect. Of course I can use
> zstyle ':completion:*:*:(^(rm|git*)):*:*files' ...
> but ...

Seems like the easy fix would be to make compadd take multiple -F
options, so «compadd -F foo -F bar» would use both 'foo' and 'bar'.

Implementing this would consist of changing this piece of code ('sp' is
the place to append to, 'p' is a pointer into 'argv' which are the
arguments of compadd; the «if (!*sp)» will need to grow an 'else'
branch):

diff --git a/Src/Zle/complete.c b/Src/Zle/complete.c
index ee4e5b0..30fab54 100644
--- a/Src/Zle/complete.c
+++ b/Src/Zle/complete.c
@@ -704,15 +704,18 @@ bin_compadd(char *name, char **argv, UNUSED(Options ops), UNUSED(int func))
 	    }
 	    if (sp) {
 		if (p[1]) {
+		    /* Pasted argument: -Xfoo. */
 		    if (!*sp)
 			*sp = p + 1;
 		    p = "" - 1;
 		} else if (argv[1]) {
+		    /* Argument in a separate word: -X foo. */
 		    argv++;
 		    if (!*sp)
 			*sp = *argv;
 		    p = "" - 1;
 		} else {
+		    /* Missing argument: argv[N] == "-X", argv[N+1] == NULL. */
 		    zwarnnam(name, e, *p);
 		    zsfree(mstr);
 		    return 1;

Cheers,

Daniel