RFC: [PATCH] Completion/Unix/Command/_git: replace a few "*::" with "*:"

RFC: [PATCH] Completion/Unix/Command/_git: replace a few "*::" with "*:"

[Daniel Hahler]

From: Daniel Hahler <git@thequod.de>

When using "*::" with _arguments instead of "*:", it will not complete
options after args, e.g. with:

    compdef -e '_arguments "-a[foo]" "--bar[bar]" "*:: :_files"' foo

"foo -a . -<tab>" does not complete --bar.

This causes `git checkout -b foo bar` to not complete options at the
end, e.g. "--no-track".

This patch changes some of the "*::" to "*:", but is not really tested
in detail.  Mostly based on trying if "--help" worked after any arg.
---
 Completion/Unix/Command/_git | 49 ++++++++++++++++--------------------
 1 file changed, 22 insertions(+), 27 deletions(-)

diff --git a/Completion/Unix/Command/_git b/Completion/Unix/Command/_git
index b3e54f7f9..e7015f395 100644
--- a/Completion/Unix/Command/_git
+++ b/Completion/Unix/Command/_git
@@ -69,7 +69,7 @@ _git-add () {
     '--ignore-errors[continue adding if an error occurs]' \
     $ignore_missing \
     '--chmod[override the executable bit of the listed files]:override:(-x +x)' \
-    '*:: :->file' && return
+    '*: :->file' && return
 
   case $state in
     (file)
@@ -420,14 +420,14 @@ _git-bundle () {
 
 (( $+functions[_git-check-ignore] )) ||
 _git-check-ignore () {
-  _arguments \
+  _arguments -S \
     '(-q --quiet)'{-q,--quiet}'[do not output anything, just set exit status]' \
     '(-v --verbose)'{-v,--verbose}'[output details about the matching pattern (if any) for each pathname]' \
     '--stdin[read file names from stdin instead of from the command-line]' \
     '-z[make output format machine-parseable and treat input-paths as NUL-separated with --stdin]' \
     '(-n --non-matching)'{-n,--non-matching}'[show given paths which do not match any pattern]' \
     '--no-index[do not look in the index when undertaking the checks]' \
-    '*:: :_files'
+    '*: :_files'
 }
 
 (( $+functions[_git-check-mailmap] )) ||
@@ -449,7 +449,7 @@ _git-checkout () {
   local curcontext=$curcontext state line ret=1
   declare -A opt_args
 
-  _arguments -C -s \
+  _arguments -C -S -s \
     '(-q --quiet --progress)'{-q,--quiet}'[suppress progress reporting]' \
     '(-f --force -m --merge --conflict --patch)'{-f,--force}'[force branch switch/ignore unmerged entries]' \
     '(-q --quiet -2 --ours -3 --theirs --patch)'{-2,--ours}'[check out stage #2 for unmerged paths]' \
@@ -469,15 +469,10 @@ _git-checkout () {
     '--recurse-submodules=-[control recursive updating of submodules]::checkout:__git_commits' \
     '(-q --quiet)--progress[force progress reporting]' \
     '(-)--[start file arguments]' \
-    '*:: :->branch-or-tree-ish-or-file' && ret=0
+    '*: :->branch-or-tree-ish-or-file' && ret=0
 
   case $state in
     (branch-or-tree-ish-or-file)
-      # TODO: Something about *:: brings us here when we complete at "-".  I
-      # guess that this makes sense in a way, as we might want to treat it as
-      # an argument, but I can't find anything in the documentation about this
-      # behavior.
-      [[ $line[CURRENT] = -* ]] && return
       if (( CURRENT == 1 )) && [[ -z $opt_args[(I)--] ]]; then
         # TODO: Allow A...B
         local \
@@ -759,7 +754,7 @@ _git-diff () {
   __git_setup_diff_options
   __git_setup_diff_stage_options
 
-  _arguments -C -s \
+  _arguments -C -S -s \
     $* \
     $diff_options \
     '(--exit-code)--quiet[disable all output]' \
@@ -767,7 +762,7 @@ _git-diff () {
     '(--cached --staged)--no-index[show diff between two paths on the filesystem]' \
     '(--cached --staged --no-index)'{--cached,--staged}'[show diff between index and named commit]' \
     '(-)--[start file arguments]' \
-    '*:: :->from-to-file' && ret=0
+    '*: :->from-to-file' && ret=0
 
   case $state in
     (from-to-file)
@@ -874,7 +869,7 @@ _git-fetch () {
     \*{-o+,--server-option=}'[send specified string to the server when using protocol version 2]:option' \
     '--negotiation-tip=[only report refs reachable from specified object to the server]:commit:__git_commits' \
     '--filter=[object filtering]:filter:_git_rev-list_filters' \
-    '*:: :->repository-or-group-or-refspec' && ret=0
+    '*: :->repository-or-group-or-refspec' && ret=0
 
   case $state in
     (repository-or-group-or-refspec)
@@ -983,7 +978,7 @@ _git-grep () {
   declare -A opt_args
 
   # TODO: Need to implement -<num> as a shorthand for -C<num>
-  _arguments -C -A '-*' \
+  _arguments -C -S -A '-*' \
     '(-O --open-files-in-pager --no-index)--cached[search blobs registered in index file instead of working tree]' \
     '(--cached)--no-index[search files in current directory, not just tracked files]' \
     '(--exclude-standard)--no-exclude-standard[search also in ignored files]' \
@@ -1029,7 +1024,7 @@ _git-grep () {
     $pattern_operators \
     '--all-match[all patterns must match]' \
     ': :_guard "^-*" pattern' \
-    '*:: :->tree-or-file' && ret=0
+    '*: :->tree-or-file' && ret=0
 
   # TODO: If --cached, --no-index, -O, or --open-files-in-pager was given,
   # don't complete treeishs.
@@ -1236,7 +1231,7 @@ _git-mv () {
     '-k[skip rename/move that would lead to errors]' \
     '(-n --dry-run)'{-n,--dry-run}'[only show what would happen]' \
     ':source:__git_cached_files' \
-    '*:: :->source-or-destination' && ret=0
+    '*: :->source-or-destination' && ret=0
 
   case $state in
     (source-or-destination)
@@ -1563,7 +1558,7 @@ _git-rm () {
     '--cached[only remove files from the index]' \
     '--ignore-unmatch[exit with 0 status even if no files matched]' \
     '(-q --quiet)'{-q,--quiet}"[don't list removed files]" \
-    '*:: :->file' && ret=0
+    '*: :->file' && ret=0
 
   case $state in
     (file)
@@ -1592,7 +1587,7 @@ _git-shortlog () {
     '-w-[linewrap the output]:: :->wrap' \
     $revision_options \
     '(-)--[start file arguments]' \
-    '*:: :->commit-range-or-file' && ret=0
+    '*: :->commit-range-or-file' && ret=0
 
   case $state in
     (wrap)
@@ -1650,7 +1645,7 @@ _git-show () {
     $log_options \
     $revision_options \
     '(-q --quiet)'{-q,--quiet}'[suppress diff output]' \
-    '*:: :->object' && ret=0
+    '*: :->object' && ret=0
 
   case $state in
     (object)
@@ -3511,7 +3506,7 @@ _git-prune () {
     '--progress[show progress]' \
     '--expire=[only expire loose objects older than specified date]: :__git_datetimes' \
     '--exclude-promisor-objects[limit traversal to objects outside promisor packfiles]' \
-    '*:: :__git_heads'
+    '*: :__git_heads'
 }
 
 (( $+functions[_git-reflog] )) ||
@@ -4869,7 +4864,7 @@ _git-update-index () {
     '--fsmonitor-valid[mark files as fsmonitor valid]' \
     '--no-fsmonitor-valid[clear fsmonitor valid bit]' \
     $z_opt \
-    '*:: :_files'
+    '*: :_files'
 }
 
 (( $+functions[_git-update-ref] )) ||
@@ -5066,7 +5061,7 @@ _git-ls-files () {
     '--recurse-submodules[recurse through submodules]' \
     '--abbrev=[set minimum SHA1 display-length]: :__git_guard_number length' \
     '--debug[show debugging data]' \
-    '*:: :_files'
+    '*: :_files'
 }
 
 (( $+functions[_git-ls-remote] )) ||
@@ -5103,7 +5098,7 @@ _git-ls-tree () {
     '--full-name[output full path-names]' \
     '(--full-name)--full-tree[do not limit listing to current working-directory]' \
     ': :__git_tree_ishs' \
-    '*:: :->file' && ret=0
+    '*: :->file' && ret=0
 
   case $state in
     (file)
@@ -5172,7 +5167,7 @@ _git-rev-list () {
     '(         --bisect-vars --bisect-all)--bisect[show only middlemost commit object]' \
     '(--bisect)--bisect-vars[same as --bisect, displaying shell-evalable code]' \
     '(--bisect)--bisect-all[display all commit objects between included and excluded commits]' \
-    '*:: :->commit-or-path' && ret=0
+    '*: :->commit-or-path' && ret=0
 
   case $state in
     (commit-or-path)
@@ -5460,14 +5455,14 @@ _git-check-attr () {
     z_opt='-z[paths are separated with NUL character when reading from stdin]'
   fi
 
-  _arguments -C \
+  _arguments -C -S \
     {-a,--all}'[list all attributes that are associated with the specified paths]' \
     '--stdin[read file names from stdin instead of from command line]' \
     '--cached[consider .gitattributes in the index only, ignoring the working tree.]' \
     '-z[terminate input and output records by a NUL character]' \
     $z_opt \
     '(-)--[interpret preceding arguments as attributes and following arguments as path names]' \
-    '*:: :->attribute-or-file' && ret=0
+    '*: :->attribute-or-file' && ret=0
 
   case $state in
     (attribute-or-file)
@@ -5542,7 +5537,7 @@ _git-mailsplit () {
     '-d-[specify number of leading zeros]: :__git_guard_number precision' \
     '-f-[skip the first N numbers]: :__git_guard_number' \
     '--keep-cr[do not remove CR from lines ending with CR+LF]' \
-    '*::mbox file:_files'
+    '*:mbox file:_files'
 }
 
 (( $+functions[_git-merge-one-file] )) ||
-- 
2.20.1

Re: RFC: [PATCH] Completion/Unix/Command/_git: replace a few "*::" with "*:"

[dana]

On 24 Feb 2019, at 07:58, Daniel Hahler <genml+zsh-workers@thequod.de> wrote:
>This patch changes some of the "*::" to "*:", but is not really tested
>in detail.  Mostly based on trying if "--help" worked after any arg.

Some of these do look like they were just erroneously copied and pasted, but
in other cases it's actually necessary, at least with the way it's written
now. For example, the commit-range-or-file state checks to see if $CURRENT is
1, which can only ever be true if the *:: syntax (or equivalent) is used.

I didn't look at it much harder than that, but, in general, anywhere you're
thinking about removing *:: or *:::, you need to check the corresponding state
code to see if it does anything with $words or $CURRENT, or calls another
function that does.

dana

Re: RFC: [PATCH] Completion/Unix/Command/_git: replace a few "*::" with "*:"

[Daniel Hahler]

[-- Attachment #1.1: Type: text/plain, Size: 2894 bytes --]

On 24.02.19 20:46, dana wrote:

>> This patch changes some of the "*::" to "*:", but is not really tested
>> in detail.  Mostly based on trying if "--help" worked after any arg.
> 
> Some of these do look like they were just erroneously copied and pasted, but
> in other cases it's actually necessary, at least with the way it's written
> now. For example, the commit-range-or-file state checks to see if $CURRENT is
> 1, which can only ever be true if the *:: syntax (or equivalent) is used.

I see, thanks for looking into it!

Unfortunately this also affects / is true for state
branch-or-tree-ish-or-file with _git-checkout already, which I've wanted
to fix in the first place.

Can this be fixed in a different way then?

Currently it just returns:

  case $state in
    (branch-or-tree-ish-or-file)
      # TODO: Something about *:: brings us here when we complete at "-".  I
      # guess that this makes sense in a way, as we might want to treat it as
      # an argument, but I can't find anything in the documentation about this
      # behavior.
      [[ $line[CURRENT] = -* ]] && return

What about something like this (against master)?

diff --git i/Completion/Unix/Command/_git w/Completion/Unix/Command/_git
index b3e54f7f9..c134f13b8 100644
--- i/Completion/Unix/Command/_git
+++ w/Completion/Unix/Command/_git
@@ -449,7 +449,8 @@ _git-checkout () {
   local curcontext=$curcontext state line ret=1
   declare -A opt_args

-  _arguments -C -s \
+  local -a options
+  options=(
     '(-q --quiet --progress)'{-q,--quiet}'[suppress progress reporting]' \
     '(-f --force -m --merge --conflict --patch)'{-f,--force}'[force branch switch/ignore unmerged entries]' \
     '(-q --quiet -2 --ours -3 --theirs --patch)'{-2,--ours}'[check out stage #2 for unmerged paths]' \
@@ -469,6 +470,9 @@ _git-checkout () {
     '--recurse-submodules=-[control recursive updating of submodules]::checkout:__git_commits' \
     '(-q --quiet)--progress[force progress reporting]' \
     '(-)--[start file arguments]' \
+  )
+
+  _arguments -C -s $options \
     '*:: :->branch-or-tree-ish-or-file' && ret=0

   case $state in
@@ -477,7 +481,7 @@ _git-checkout () {
       # guess that this makes sense in a way, as we might want to treat it as
       # an argument, but I can't find anything in the documentation about this
       # behavior.
-      [[ $line[CURRENT] = -* ]] && return
+      _arguments -C -s $options && ret=0 && return
       if (( CURRENT == 1 )) && [[ -z $opt_args[(I)--] ]]; then
         # TODO: Allow A...B
         local \


> I didn't look at it much harder than that, but, in general, anywhere you're
> thinking about removing *:: or *:::, you need to check the corresponding state
> code to see if it does anything with $words or $CURRENT, or calls another
> function that does.
> 
> dana
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: RFC: [PATCH] Completion/Unix/Command/_git: replace a few "*::" with "*:"

[Daniel Hahler]

[-- Attachment #1.1: Type: text/plain, Size: 3703 bytes --]

On 25.02.19 10:46, Daniel Hahler wrote:

> What about something like this (against master)?

As for _git_checkout, the following appears to work better:


 Completion/Unix/Command/_git | 54 ++++++++++++++++++++------------------------
 1 file changed, 25 insertions(+), 29 deletions(-)

diff --git i/Completion/Unix/Command/_git w/Completion/Unix/Command/_git
index b3e54f7f9..e9f6f3587 100644
--- i/Completion/Unix/Command/_git
+++ w/Completion/Unix/Command/_git
@@ -449,7 +449,7 @@ _git-checkout () {
   local curcontext=$curcontext state line ret=1
   declare -A opt_args
 
-  _arguments -C -s \
+  _arguments -C -s -S \
     '(-q --quiet --progress)'{-q,--quiet}'[suppress progress reporting]' \
     '(-f --force -m --merge --conflict --patch)'{-f,--force}'[force branch switch/ignore unmerged entries]' \
     '(-q --quiet -2 --ours -3 --theirs --patch)'{-2,--ours}'[check out stage #2 for unmerged paths]' \
@@ -469,37 +469,33 @@ _git-checkout () {
     '--recurse-submodules=-[control recursive updating of submodules]::checkout:__git_commits' \
     '(-q --quiet)--progress[force progress reporting]' \
     '(-)--[start file arguments]' \
-    '*:: :->branch-or-tree-ish-or-file' && ret=0
+    '1: :->first-arg' \
+    '*: :->other-arg' && ret=0
 
   case $state in
-    (branch-or-tree-ish-or-file)
-      # TODO: Something about *:: brings us here when we complete at "-".  I
-      # guess that this makes sense in a way, as we might want to treat it as
-      # an argument, but I can't find anything in the documentation about this
-      # behavior.
-      [[ $line[CURRENT] = -* ]] && return
-      if (( CURRENT == 1 )) && [[ -z $opt_args[(I)--] ]]; then
-        # TODO: Allow A...B
-        local \
-              remote_branch_noprefix_arg='remote-branch-names-noprefix::__git_remote_branch_names_noprefix' \
-              tree_ish_arg='tree-ishs::__git_commits_prefer_recent' \
-              file_arg='modified-files::__git_modified_files'
+    (first-arg)
+      # TODO: Allow A...B
+      local \
+	    remote_branch_noprefix_arg='remote-branch-names-noprefix::__git_remote_branch_names_noprefix' \
+	    tree_ish_arg='tree-ishs::__git_commits_prefer_recent' \
+	    file_arg='modified-files::__git_modified_files'
 
-        if [[ -n ${opt_args[(I)-b|-B|--orphan|--detach]} ]]; then
-          _alternative $tree_ish_arg && ret=0
-        elif [[ -n $opt_args[(I)--track] ]]; then
-          _alternative remote-branches::__git_remote_branch_names && ret=0
-        elif [[ -n ${opt_args[(I)--ours|--theirs|-m|--conflict|--patch]} ]]; then
-          _alternative $tree_ish_arg $file_arg && ret=0
-        else
-          _alternative \
-            $file_arg \
-            $tree_ish_arg \
-            $remote_branch_noprefix_arg \
-            && ret=0
-        fi
-
-      elif [[ -n ${opt_args[(I)-b|-B|-t|--track|--orphan|--detach]} ]]; then
+      if [[ -n ${opt_args[(I)-b|-B|--orphan|--detach]} ]]; then
+	_alternative $tree_ish_arg && ret=0
+      elif [[ -n $opt_args[(I)--track] ]]; then
+	_alternative remote-branches::__git_remote_branch_names && ret=0
+      elif [[ -n ${opt_args[(I)--ours|--theirs|-m|--conflict|--patch]} ]]; then
+	_alternative $tree_ish_arg $file_arg && ret=0
+      else
+	_alternative \
+	  $file_arg \
+	  $tree_ish_arg \
+	  $remote_branch_noprefix_arg \
+	  && ret=0
+      fi
+      ;;
+    (other-arg)
+      if [[ -n ${opt_args[(I)-b|-B|-t|--track|--orphan|--detach]} ]]; then
         _nothing
       elif [[ -n $line[1] ]] && __git_is_treeish ${(Q)line[1]}; then
         __git_ignore_line __git_tree_files ${PREFIX:-.} ${(Q)line[1]} && ret=0


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: RFC: [PATCH] Completion/Unix/Command/_git: replace a few "*::" with "*:"

[dana]

On 25 Feb 2019, at 04:04, Daniel Hahler <genml+zsh-workers@thequod.de> wrote:
>As for _git_checkout, the following appears to work better:
>...
>-    '*:: :->branch-or-tree-ish-or-file' && ret=0
>+    '1: :->first-arg' \
>+    '*: :->other-arg' && ret=0

I only played with it for a second, but yes, that seems better to me. It does
produce a 'not a git repository' message when you complete options, though,
which is slightly irritating. I guess you could re-add that -* prefix check
there, can't think of anything nicer right now

Your white space is broken, also. That file does have some tabs in it, but
mostly it's spaces

dana