From: "SZEDER Gábor" <szeder@ira.uka.de>
To: Felipe Contreras <felipe.contreras@gmail.com>
Cc: git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>,
Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Subject: Re: [PATCH v5 1/3] completion: add new __gitcompadd helper
Date: Wed, 17 Oct 2012 19:28:08 +0200 [thread overview]
Message-ID: <20121017172808.GD2156@goldbirke> (raw)
In-Reply-To: <1350229971-9343-2-git-send-email-felipe.contreras@gmail.com>
On Sun, Oct 14, 2012 at 05:52:49PM +0200, Felipe Contreras wrote:
> The idea is to never touch the COMPREPLY variable directly.
>
> This allows other completion systems override __gitcompadd, and do
> something different instead.
>
> Also, this allows the simplifcation of the completino tests (separate
> patch).
>
> There should be no functional changes.
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
> contrib/completion/git-completion.bash | 65 ++++++++++++++++++----------------
> 1 file changed, 34 insertions(+), 31 deletions(-)
>
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index d743e56..01325de 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -225,6 +225,11 @@ _get_comp_words_by_ref ()
> fi
> fi
>
> +__gitcompadd ()
> +{
> + COMPREPLY=($(compgen -W "$1" -P "$2" -S "$4" -- "$3"))
> +}
> +
> # Generates completion reply with compgen, appending a space to possible
> # completion words, if necessary.
> # It accepts 1 to 4 arguments:
> @@ -238,13 +243,11 @@ __gitcomp ()
>
> case "$cur_" in
> --*=)
> - COMPREPLY=()
> + __gitcompadd
> ;;
> *)
> local IFS=$'\n'
> - COMPREPLY=($(compgen -P "${2-}" \
> - -W "$(__gitcomp_1 "${1-}" "${4-}")" \
> - -- "$cur_"))
> + __gitcompadd "$(__gitcomp_1 "${1-}" "${4-}")" "${2-}" "$cur_" ""
> ;;
> esac
> }
> @@ -261,7 +264,7 @@ __gitcomp ()
> __gitcomp_nl ()
> {
> local IFS=$'\n'
> - COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$1" -- "${3-$cur}"))
> + __gitcompadd "$1" "${2-}" "${3-$cur}" "${4- }"
> }
I feel hesitant about this change. One of the ways I'm exploring to
fix the issues with shell metacharacters and expansion in compgen is
to actually replace compgen. We already iterate over all possible
completion words in __gitcomp_1(), so it doesn't make much of a
difference to do the filtering for the current word while we are at
it. However, the way __gitcompadd() encapsulates COMPREPLY=($(compgen
...)), and tha basic idea of never touching COMPREPLY directly make
this basically impossible.
> __git_heads ()
> @@ -486,7 +489,7 @@ __git_complete_remote_or_refspec ()
> case "$cmd" in
> push) no_complete_refspec=1 ;;
> fetch)
> - COMPREPLY=()
> + __gitcompadd
> return
> ;;
> *) ;;
> @@ -502,7 +505,7 @@ __git_complete_remote_or_refspec ()
> return
> fi
> if [ $no_complete_refspec = 1 ]; then
> - COMPREPLY=()
> + __gitcompadd
> return
> fi
> [ "$remote" = "." ] && remote=
> @@ -776,7 +779,7 @@ _git_am ()
> "
> return
> esac
> - COMPREPLY=()
> + __gitcompadd
These changes effectively run compgen in a subshell to generate an
empty completion reply. While it doesn't really matter on Linux,
it'll add another half a tenth of a second delay in those cases on my
Windows machine. At least it should be conditional, i.e. $(compgen
...) shouldn't be executed when there are no possible completion
words.
However, I think those COMPREPLY=() assignments are pointless anyway.
COMPREPLY is always empty when completion functions are invoked, so
there is no need to explicitly set it to an empty array when we don't
provide any words for completion. Their only use is basically to
explicitly tell us humans that in those cases we don't offer any words
for completion. But we don't do that consistently: there are several
places without offering words for completion and without COMPREPLY=(),
e.g. the '__git_has_doubledash && return' pattern.
Perhaps it would be time to get rid of these COMPREPLY=() assignments?
> }
>
> _git_apply ()
> @@ -796,7 +799,7 @@ _git_apply ()
> "
> return
> esac
> - COMPREPLY=()
> + __gitcompadd
> }
>
> _git_add ()
> @@ -811,7 +814,7 @@ _git_add ()
> "
> return
> esac
> - COMPREPLY=()
> + __gitcompadd
> }
>
> _git_archive ()
> @@ -856,7 +859,7 @@ _git_bisect ()
> __gitcomp_nl "$(__git_refs)"
> ;;
> *)
> - COMPREPLY=()
> + __gitcompadd
> ;;
> esac
> }
> @@ -965,7 +968,7 @@ _git_clean ()
> return
> ;;
> esac
> - COMPREPLY=()
> + __gitcompadd
> }
>
> _git_clone ()
> @@ -989,7 +992,7 @@ _git_clone ()
> return
> ;;
> esac
> - COMPREPLY=()
> + __gitcompadd
> }
>
> _git_commit ()
> @@ -1023,7 +1026,7 @@ _git_commit ()
> "
> return
> esac
> - COMPREPLY=()
> + __gitcompadd
> }
>
> _git_describe ()
> @@ -1154,7 +1157,7 @@ _git_fsck ()
> return
> ;;
> esac
> - COMPREPLY=()
> + __gitcompadd
> }
>
> _git_gc ()
> @@ -1165,7 +1168,7 @@ _git_gc ()
> return
> ;;
> esac
> - COMPREPLY=()
> + __gitcompadd
> }
>
> _git_gitk ()
> @@ -1242,7 +1245,7 @@ _git_init ()
> return
> ;;
> esac
> - COMPREPLY=()
> + __gitcompadd
> }
>
> _git_ls_files ()
> @@ -1261,7 +1264,7 @@ _git_ls_files ()
> return
> ;;
> esac
> - COMPREPLY=()
> + __gitcompadd
> }
>
> _git_ls_remote ()
> @@ -1377,7 +1380,7 @@ _git_mergetool ()
> return
> ;;
> esac
> - COMPREPLY=()
> + __gitcompadd
> }
>
> _git_merge_base ()
> @@ -1393,7 +1396,7 @@ _git_mv ()
> return
> ;;
> esac
> - COMPREPLY=()
> + __gitcompadd
> }
>
> _git_name_rev ()
> @@ -1563,7 +1566,7 @@ _git_send_email ()
> return
> ;;
> esac
> - COMPREPLY=()
> + __gitcompadd
> }
>
> _git_stage ()
> @@ -1616,7 +1619,7 @@ _git_config ()
> local remote="${prev#remote.}"
> remote="${remote%.fetch}"
> if [ -z "$cur" ]; then
> - COMPREPLY=("refs/heads/")
> + __gitcompadd "refs/heads/"
> return
> fi
> __gitcomp_nl "$(__git_refs_remotes "$remote")"
> @@ -1676,7 +1679,7 @@ _git_config ()
> return
> ;;
> *.*)
> - COMPREPLY=()
> + __gitcompadd
> return
> ;;
> esac
> @@ -2056,7 +2059,7 @@ _git_remote ()
> __gitcomp "$c"
> ;;
> *)
> - COMPREPLY=()
> + __gitcompadd
> ;;
> esac
> }
> @@ -2100,7 +2103,7 @@ _git_rm ()
> return
> ;;
> esac
> - COMPREPLY=()
> + __gitcompadd
> }
>
> _git_shortlog ()
> @@ -2170,7 +2173,7 @@ _git_stash ()
> if [ -z "$(__git_find_on_cmdline "$save_opts")" ]; then
> __gitcomp "$subcommands"
> else
> - COMPREPLY=()
> + __gitcompadd
> fi
> ;;
> esac
> @@ -2183,14 +2186,14 @@ _git_stash ()
> __gitcomp "--index --quiet"
> ;;
> show,--*|drop,--*|branch,--*)
> - COMPREPLY=()
> + __gitcompadd
> ;;
> show,*|apply,*|drop,*|pop,*|branch,*)
> __gitcomp_nl "$(git --git-dir="$(__gitdir)" stash list \
> | sed -n -e 's/:.*//p')"
> ;;
> *)
> - COMPREPLY=()
> + __gitcompadd
> ;;
> esac
> fi
> @@ -2307,7 +2310,7 @@ _git_svn ()
> __gitcomp "--revision= --parent"
> ;;
> *)
> - COMPREPLY=()
> + __gitcompadd
> ;;
> esac
> fi
> @@ -2332,13 +2335,13 @@ _git_tag ()
>
> case "$prev" in
> -m|-F)
> - COMPREPLY=()
> + __gitcompadd
> ;;
> -*|tag)
> if [ $f = 1 ]; then
> __gitcomp_nl "$(__git_tags)"
> else
> - COMPREPLY=()
> + __gitcompadd
> fi
> ;;
> *)
> --
> 1.7.12.1
>
next prev parent reply other threads:[~2012-10-17 17:28 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-10-14 15:52 [PATCH v5 0/3] completion: refactor and zsh wrapper Felipe Contreras
2012-10-14 15:52 ` [PATCH v5 1/3] completion: add new __gitcompadd helper Felipe Contreras
2012-10-17 17:28 ` SZEDER Gábor [this message]
2012-10-22 0:41 ` Felipe Contreras
2012-10-30 23:18 ` SZEDER Gábor
2012-10-14 15:52 ` [PATCH v5 2/3] tests: use __gitcompadd to simplify completion tests Felipe Contreras
2012-10-16 0:24 ` Felipe Contreras
2012-10-17 17:50 ` SZEDER Gábor
2012-10-17 17:54 ` [PATCH] completion: clean up __gitcomp() tests SZEDER Gábor
2012-10-17 18:21 ` Felipe Contreras
2012-10-17 18:26 ` [PATCH v5 2/3] tests: use __gitcompadd to simplify completion tests Felipe Contreras
2012-10-14 15:52 ` [PATCH v5 3/3] completion: add new zsh completion Felipe Contreras
2012-10-15 6:38 ` Felipe Contreras
2012-10-15 16:45 ` [PATCH v5 0/3] completion: refactor and zsh wrapper Matthieu Moy
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20121017172808.GD2156@goldbirke \
--to=szeder@ira.uka.de \
--cc=felipe.contreras@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=matthieu.moy@grenoble-inp.fr \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).