git remote set-url completion

git remote set-url completion

[pseyfert]

Dear zsh-workers,

I was not happy with the completion of
git remote set-url <some remote> TAB

in my usage this should yield the same completions as `git clone TAB`
(a url like https://gitlab.my_project.tld or git@github.com or some path to a local repository)

So I put the below patch together which uses __git_repositories (and _urls) instead of _urls.

I also saw there was a todo left in _git for doing
git remote set-url <some remote> <new url> <old url>
which used to just use _urls for the completion of old url,
here I get the output of `git remote get-url <some remote> --all`

Ideally I would like to also cover
git remote set-url --push <some remote> <new url> TAB
which should then complete the output of `git remote get-url <some remote> --push --all`

But atm I don't understand what the current completion does, because
git remote set-url --push <some remote> <new url> TAB
just suggests --add and --delete.

Find my patch below.

Thanks for consideration,
Paul


diff --git a/Completion/Unix/Command/_git b/Completion/Unix/Command/_git
index 21ba65724..e58340d8d 100644
--- a/Completion/Unix/Command/_git
+++ b/Completion/Unix/Command/_git
@@ -3600,10 +3600,7 @@ _git-remote () {
             ':repository:->repository' && ret=0
 	  case $state in
 	    (repository)
-            _alternative \
-              'local-repositories::__git_local_repositories' \
-              'remote-repositories::__git_remote_repositories' \
-	      'urls::_urls' && ret=0
+            __git_repositories_or_url && ret=0
 	    ;;
 	  esac
           ;;
@@ -3634,14 +3631,17 @@ _git-remote () {
             '*: :__git_branch_names' && ret=0
           ;;
         (set-url)
-          # TODO: Old URL should be one of those defined for the remote.
+          # TODO: Old URL does not get completed if --push, --add, or --delete are present
+          # TODO: assumes $line[1] is always the remote name
+          _message "the line is: $line"
           _arguments -S -s \
             '(3)--push[manipulate push URLs instead of fetch URLs]' \
             '--add[add URL to those already defined]' \
             '(3)--delete[delete all matching URLs]' \
             ': :__git_remotes' \
-            ':new url:_urls' \
-            ':old url:_urls' && ret=0
+            ':new url:__git_repositories_or_url' \
+            ':old url:__git_current_remote_url $line[1]' \
+                      && ret=0
           ;;
         (show)
           _arguments -S \
@@ -6949,6 +6949,21 @@ __git_local_repositories () {
   _wanted local-repositories expl 'local repositories' _directories
 }
 
+(( $+functions[__git_repositories_or_url] )) ||
+__git_repositories_or_url () {
+  _alternative \
+    'repositories::__git_repositories' \
+    'url::_urls'
+}
+
+(( $+functions[__git_current_remote_url] )) ||
+__git_current_remote_url () {
+    # TODO: is ${(@)*[1,4]} a proper replacement for $* and passing extra arguments?
+    # TODO: add --push to the `git remote get-url` command in case `git remote set-url --push origin <new url> <TAB>` is completed
+  _wanted remote-urls expl 'current urls' \
+      compadd ${(@)*[1,4]} - ${${(0)"$(_call_program remote-urls git remote get-url $5)"}%%$'\n'*}
+}
+
 (( $+functions[__git_any_repositories] )) ||
 __git_any_repositories () {
   # TODO: should also be $GIT_DIR/remotes/origin

Re: git remote set-url completion

[Oliver Kiddle]

On 21 May, pseyfert wrote:
>
> I was not happy with the completion of
> git remote set-url <some remote> TAB
>
> in my usage this should yield the same completions as `git clone TAB`
> (a url like https://gitlab.my_project.tld or git@github.com or some path to a local repository)

git clone doesn't seem to complete http: style URLs either.

> So I put the below patch together which uses __git_repositories (and _urls) instead of _urls.

Thanks for this. I've got a few comments.

> I also saw there was a todo left in _git for doing
> git remote set-url <some remote> <new url> <old url>
> which used to just use _urls for the completion of old url,
> here I get the output of `git remote get-url <some remote> --all`

Except the --all is missing.

> +          # TODO: Old URL does not get completed if --push, --add, or --delete are present

With --push, the old URL is, I believe, still valid. And with --delete, it
is the new one that should be excluded.

The (3) exclusion should work. Perhaps add the numbers into the next arg
specifications to make it clearer given that they're being referenced.

> +          # TODO: assumes $line[1] is always the remote name

That seems correct to me and not an assumption.

> +          _message "the line is: $line"

That looks like leftover debug.

> +(( $+functions[__git_repositories_or_url] )) ||
> +__git_repositories_or_url () {
> +  _alternative \
> +    'repositories::__git_repositories' \
> +    'url::_urls'

The tag should be 'urls' rather than 'url'.

> +}
> +
> +(( $+functions[__git_current_remote_url] )) ||
> +__git_current_remote_url () {
> +    # TODO: is ${(@)*[1,4]} a proper replacement for $* and passing extra arguments?

NO.

Various styles can affect what _description adds. matcher-list for
example. Don't rely on that.

I'd be inclined to throw away the inherited arguments. A space in the
_arguments spec prevents them from being added:

  '3:old url: __git_current_remote_url $line[1]'

Or make the helper function not be a completion function so that you
have something like:
  
  '3:old url:compadd - $(__git_current_remote_url $line[1])'

> +    # TODO: add --push to the `git remote get-url` command in case `git remote set-url --push origin <new url> <TAB>` is completed

Adding ${(k)opt_args[--push]} should get you that. It also needs --all
adding in case there are more than one.

Oliver

Re: git remote set-url completion

[Oliver Kiddle]

On 21 May, pseyfert wrote:
> I was not happy with the completion of
> git remote set-url <some remote> TAB
 ...
> So I put the below patch together which uses __git_repositories (and _urls) instead of _urls.

The following patch is to be applied after Paul's original patch from
42810. It covers the things I mentioned in my reply to the original
message.

Oliver

diff --git a/Completion/Unix/Command/_git b/Completion/Unix/Command/_git
index c9201bea2..2cae4c82f 100644
--- a/Completion/Unix/Command/_git
+++ b/Completion/Unix/Command/_git
@@ -3607,12 +3607,7 @@ _git-remote () {
             '(-m --master)'{-m,--master=}'[set HEAD of remote to point to given master branch]: :__git_branch_names' \
 	    '--mirror[do not use separate remotes]::mirror type:(fetch pull)' \
             ':name:__git_remotes' \
-            ':repository:->repository' && ret=0
-	  case $state in
-	    (repository)
-            __git_repositories_or_url && ret=0
-	    ;;
-	  esac
+            ':repository:__git_repositories_or_urls' && ret=0
           ;;
         (get-url)
           _arguments -S -s \
@@ -3641,17 +3636,13 @@ _git-remote () {
             '*: :__git_branch_names' && ret=0
           ;;
         (set-url)
-          # TODO: Old URL does not get completed if --push, --add, or --delete are present
-          # TODO: assumes $line[1] is always the remote name
-          _message "the line is: $line"
-          _arguments -S -s \
-            '(3)--push[manipulate push URLs instead of fetch URLs]' \
-            '--add[add URL to those already defined]' \
-            '(3)--delete[delete all matching URLs]' \
-            ': :__git_remotes' \
-            ':new url:__git_repositories_or_url' \
-            ':old url:__git_current_remote_url $line[1]' \
-                      && ret=0
+          _arguments -S \
+            '--push[manipulate push URLs instead of fetch URLs]' \
+            '(3)--add[add URL to those already defined]' \
+            '(2)--delete[delete all matching URLs]' \
+            '1: :__git_remotes' \
+            '2:new url:__git_repositories_or_urls' \
+            '3:old url: __git_current_remote_urls ${(k)opt_args[--push]} $line[1]' && ret=0
           ;;
         (show)
           _arguments -S \
@@ -6971,19 +6962,19 @@ __git_local_repositories () {
   _wanted local-repositories expl 'local repositories' _directories
 }
 
-(( $+functions[__git_repositories_or_url] )) ||
-__git_repositories_or_url () {
+(( $+functions[__git_repositories_or_urls] )) ||
+__git_repositories_or_urls () {
   _alternative \
     'repositories::__git_repositories' \
-    'url::_urls'
+    'urls::_urls'
 }
 
-(( $+functions[__git_current_remote_url] )) ||
-__git_current_remote_url () {
-    # TODO: is ${(@)*[1,4]} a proper replacement for $* and passing extra arguments?
-    # TODO: add --push to the `git remote get-url` command in case `git remote set-url --push origin <new url> <TAB>` is completed
-  _wanted remote-urls expl 'current urls' \
-      compadd ${(@)*[1,4]} - ${${(0)"$(_call_program remote-urls git remote get-url $5)"}%%$'\n'*}
+(( $+functions[__git_current_remote_urls] )) ||
+__git_current_remote_urls () {
+  local expl
+  _description remote-urls expl 'current url'
+  compadd "$expl[@]" -M 'r:|/=* r:|=*' - ${(f)"$(_call_program remote-urls
+      git remote get-url "$@" --all)"}
 }
 
 (( $+functions[__git_any_repositories] )) ||