{"thread":{"id":"30531","subject":"[PATCH v5] completion: add new __git_complete helper","startedAt":"2012-05-14T14:38:10Z","lastAt":"2012-05-16T15:25:50Z","messageCount":11,"participants":["Felipe Contreras","Junio C Hamano"],"isPatch":true,"patchVersion":5,"patchTotal":null},"messages":[{"id":"191491","messageId":"CAMP44s380rQtpDesBA5N8EWd5TUZDFMKaXgyTVoZb390aKqdGg@mail.gmail.com","threadId":"30531","inReplyTo":"1337009718-1164-1-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH v5] completion: add new __git_complete helper","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-05-14T14:38:10Z","receivedAt":"2012-05-14T14:38:10Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Mon, May 14, 2012 at 5:35 PM, Felipe Contreras\n<felipe.contreras@gmail.com> wrote:\n> This simplifies the completions, and would make it easier to define\n> aliases in the future.\n\nSince last version:\n\nI renamed the wrapper function so they would end up as __git_wrap_foo,\nand got rid of all the convenience to avoid specifying the prefix of\nthe functions; this function is not intended to be used outside git\nanyway.\n\n-- \nFelipe Contreras\n"},{"id":"191490","messageId":"1337009718-1164-1-git-send-email-felipe.contreras@gmail.com","threadId":"30531","inReplyTo":null,"subject":"[PATCH v5] completion: add new __git_complete helper","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-05-14T15:35:18Z","receivedAt":"2012-05-14T15:35:18Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"This simplifies the completions, and would make it easier to define\naliases in the future.\n\nSigned-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n---\n contrib/completion/git-completion.bash |   70 +++++++++++++++-----------------\n t/t9902-completion.sh                  |    2 +-\n 2 files changed, 34 insertions(+), 38 deletions(-)\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 9f56ec7..d60bb8a 100755\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -2603,21 +2603,6 @@ _git ()\n {\n \tlocal i c=1 command __git_dir\n \n-\tif [[ -n ${ZSH_VERSION-} ]]; then\n-\t\temulate -L bash\n-\t\tsetopt KSH_TYPESET\n-\n-\t\t# workaround zsh's bug that leaves 'words' as a special\n-\t\t# variable in versions < 4.3.12\n-\t\ttypeset -h words\n-\n-\t\t# workaround zsh's bug that quotes spaces in the COMPREPLY\n-\t\t# array if IFS doesn't contain spaces.\n-\t\ttypeset -h IFS\n-\tfi\n-\n-\tlocal cur words cword prev\n-\t_get_comp_words_by_ref -n =: cur words cword prev\n \twhile [ $c -lt $cword ]; do\n \t\ti=\"${words[c]}\"\n \t\tcase \"$i\" in\n@@ -2667,22 +2652,6 @@ _git ()\n \n _gitk ()\n {\n-\tif [[ -n ${ZSH_VERSION-} ]]; then\n-\t\temulate -L bash\n-\t\tsetopt KSH_TYPESET\n-\n-\t\t# workaround zsh's bug that leaves 'words' as a special\n-\t\t# variable in versions < 4.3.12\n-\t\ttypeset -h words\n-\n-\t\t# workaround zsh's bug that quotes spaces in the COMPREPLY\n-\t\t# array if IFS doesn't contain spaces.\n-\t\ttypeset -h IFS\n-\tfi\n-\n-\tlocal cur words cword prev\n-\t_get_comp_words_by_ref -n =: cur words cword prev\n-\n \t__git_has_doubledash && return\n \n \tlocal g=\"$(__gitdir)\"\n@@ -2703,16 +2672,43 @@ _gitk ()\n \t__git_complete_revlist\n }\n \n-complete -o bashdefault -o default -o nospace -F _git git 2>/dev/null \\\n-\t|| complete -o default -o nospace -F _git git\n-complete -o bashdefault -o default -o nospace -F _gitk gitk 2>/dev/null \\\n-\t|| complete -o default -o nospace -F _gitk gitk\n+__git_func_wrap ()\n+{\n+\tif [[ -n ${ZSH_VERSION-} ]]; then\n+\t\temulate -L bash\n+\t\tsetopt KSH_TYPESET\n+\n+\t\t# workaround zsh's bug that leaves 'words' as a special\n+\t\t# variable in versions < 4.3.12\n+\t\ttypeset -h words\n+\n+\t\t# workaround zsh's bug that quotes spaces in the COMPREPLY\n+\t\t# array if IFS doesn't contain spaces.\n+\t\ttypeset -h IFS\n+\tfi\n+\tlocal cur words cword prev\n+\t_get_comp_words_by_ref -n =: cur words cword prev\n+\t$1\n+}\n+\n+# Setup completion for certain functions defined above by setting common\n+# variables and workarounds.\n+# This is NOT a public function; use at your own risk.\n+__git_complete ()\n+{\n+\tlocal wrapper=\"__git_wrap${2}\"\n+\teval \"$wrapper () { __git_func_wrap $2 ; }\"\n+\tcomplete -o bashdefault -o default -o nospace -F $wrapper $1 2>/dev/null \\\n+\t\t|| complete -o default -o nospace -F $wrapper $1\n+}\n+\n+__git_complete git _git\n+__git_complete gitk _gitk\n \n # The following are necessary only for Cygwin, and only are needed\n # when the user has tab-completed the executable name and consequently\n # included the '.exe' suffix.\n #\n if [ Cygwin = \"$(uname -o 2>/dev/null)\" ]; then\n-complete -o bashdefault -o default -o nospace -F _git git.exe 2>/dev/null \\\n-\t|| complete -o default -o nospace -F _git git.exe\n+__git_complete git.exe _git\n fi\ndiff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\nindex 5bda6b6..0f09fd6 100755\n--- a/t/t9902-completion.sh\n+++ b/t/t9902-completion.sh\n@@ -63,7 +63,7 @@ run_completion ()\n \tlocal _cword\n \t_words=( $1 )\n \t(( _cword = ${#_words[@]} - 1 ))\n-\t_git && print_comp\n+\t__git_wrap_git && print_comp\n }\n \n test_completion ()\n-- \n1.7.10.1\n"},{"id":"191510","messageId":"7vvcjyhd5n.fsf@alter.siamese.dyndns.org","threadId":"30531","inReplyTo":"1337009718-1164-1-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH v5] completion: add new __git_complete helper","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-05-14T17:43:00Z","receivedAt":"2012-05-14T17:43:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras <felipe.contreras@gmail.com> writes:\n\n> This simplifies the completions, and would make it easier to define\n> aliases in the future.\n>\n> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n> ---\n>  contrib/completion/git-completion.bash |   70 +++++++++++++++-----------------\n>  t/t9902-completion.sh                  |    2 +-\n>  2 files changed, 34 insertions(+), 38 deletions(-)\n>\n> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\n> index 9f56ec7..d60bb8a 100755\n> --- a/contrib/completion/git-completion.bash\n> +++ b/contrib/completion/git-completion.bash\n> @@ -2603,21 +2603,6 @@ _git ()\n>  {\n>  \tlocal i c=1 command __git_dir\n>  \n> -\tif [[ -n ${ZSH_VERSION-} ]]; then\n> -\t\temulate -L bash\n> -\t\tsetopt KSH_TYPESET\n> -\n> -\t\t# workaround zsh's bug that leaves 'words' as a special\n> -\t\t# variable in versions < 4.3.12\n> -\t\ttypeset -h words\n> -\n> -\t\t# workaround zsh's bug that quotes spaces in the COMPREPLY\n> -\t\t# array if IFS doesn't contain spaces.\n> -\t\ttypeset -h IFS\n> -\tfi\n> -\n> -\tlocal cur words cword prev\n> -\t_get_comp_words_by_ref -n =: cur words cword prev\n>  \twhile [ $c -lt $cword ]; do\n>  \t\ti=\"${words[c]}\"\n>  \t\tcase \"$i\" in\n> @@ -2667,22 +2652,6 @@ _git ()\n>  \n>  _gitk ()\n>  {\n> -\tif [[ -n ${ZSH_VERSION-} ]]; then\n> -\t\temulate -L bash\n> -\t\tsetopt KSH_TYPESET\n> -\n> -\t\t# workaround zsh's bug that leaves 'words' as a special\n> -\t\t# variable in versions < 4.3.12\n> -\t\ttypeset -h words\n> -\n> -\t\t# workaround zsh's bug that quotes spaces in the COMPREPLY\n> -\t\t# array if IFS doesn't contain spaces.\n> -\t\ttypeset -h IFS\n> -\tfi\n> -\n> -\tlocal cur words cword prev\n> -\t_get_comp_words_by_ref -n =: cur words cword prev\n> -\n> @@ -2703,16 +2672,43 @@ _gitk ()\n>  \t__git_complete_revlist\n>  }\n>  \n> -complete -o bashdefault -o default -o nospace -F _git git 2>/dev/null \\\n> -\t|| complete -o default -o nospace -F _git git\n> -complete -o bashdefault -o default -o nospace -F _gitk gitk 2>/dev/null \\\n> -\t|| complete -o default -o nospace -F _gitk gitk\n\nNice code reduction by moving the duplicated code into the new helpers ;-)\n\n> +__git_func_wrap ()\n> +{\n> +\tif [[ -n ${ZSH_VERSION-} ]]; then\n> +\t\temulate -L bash\n> +\t\tsetopt KSH_TYPESET\n> +\n> +\t\t# workaround zsh's bug that leaves 'words' as a special\n> +\t\t# variable in versions < 4.3.12\n> +\t\ttypeset -h words\n> +\n> +\t\t# workaround zsh's bug that quotes spaces in the COMPREPLY\n> +\t\t# array if IFS doesn't contain spaces.\n> +\t\ttypeset -h IFS\n> +\tfi\n> +\tlocal cur words cword prev\n> +\t_get_comp_words_by_ref -n =: cur words cword prev\n> +\t$1\n> +}\n> +\n> +# Setup completion for certain functions defined above by setting common\n> +# variables and workarounds.\n> +# This is NOT a public function; use at your own risk.\n> +__git_complete ()\n> +{\n> +\tlocal wrapper=\"__git_wrap${2}\"\n> +\teval \"$wrapper () { __git_func_wrap $2 ; }\"\n> +\tcomplete -o bashdefault -o default -o nospace -F $wrapper $1 2>/dev/null \\\n> +\t\t|| complete -o default -o nospace -F $wrapper $1\n> +}\n> +\n> +__git_complete git _git\n> +__git_complete gitk _gitk\n\nIt makes my stomach queasy whenever I see $var not in double quotes that\nforces me to guess (and trace to verify if the codepath is what I really\ncare about) if any value with $IFS in it could be used there, so even when\nthey are known to be safe, I'd prefer to see either explicit quotes or\ncomment that says what are expected in $1 and $2.\n\nThe quoting of all the added lines looks correct, though.\n\n>  # The following are necessary only for Cygwin, and only are needed\n>  # when the user has tab-completed the executable name and consequently\n>  # included the '.exe' suffix.\n>  #\n>  if [ Cygwin = \"$(uname -o 2>/dev/null)\" ]; then\n> -complete -o bashdefault -o default -o nospace -F _git git.exe 2>/dev/null \\\n> -\t|| complete -o default -o nospace -F _git git.exe\n> +__git_complete git.exe _git\n>  fi\n> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\n> index 5bda6b6..0f09fd6 100755\n> --- a/t/t9902-completion.sh\n> +++ b/t/t9902-completion.sh\n> @@ -63,7 +63,7 @@ run_completion ()\n>  \tlocal _cword\n>  \t_words=( $1 )\n>  \t(( _cword = ${#_words[@]} - 1 ))\n> -\t_git && print_comp\n> +\t__git_wrap_git && print_comp\n>  }\n>  \n>  test_completion ()\n"},{"id":"191512","messageId":"CAMP44s1pb+J_SAzZ66QVcWq4V=LauUQ2VmzMD8KBtnhjubkkVg@mail.gmail.com","threadId":"30531","inReplyTo":"7vvcjyhd5n.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v5] completion: add new __git_complete helper","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-05-14T17:55:09Z","receivedAt":"2012-05-14T17:55:09Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Mon, May 14, 2012 at 7:43 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Felipe Contreras <felipe.contreras@gmail.com> writes:\n>\n>> This simplifies the completions, and would make it easier to define\n>> aliases in the future.\n>>\n>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n>> ---\n>>  contrib/completion/git-completion.bash |   70 +++++++++++++++-----------------\n>>  t/t9902-completion.sh                  |    2 +-\n>>  2 files changed, 34 insertions(+), 38 deletions(-)\n>>\n>> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\n>> index 9f56ec7..d60bb8a 100755\n>> --- a/contrib/completion/git-completion.bash\n>> +++ b/contrib/completion/git-completion.bash\n>> @@ -2603,21 +2603,6 @@ _git ()\n>>  {\n>>       local i c=1 command __git_dir\n>>\n>> -     if [[ -n ${ZSH_VERSION-} ]]; then\n>> -             emulate -L bash\n>> -             setopt KSH_TYPESET\n>> -\n>> -             # workaround zsh's bug that leaves 'words' as a special\n>> -             # variable in versions < 4.3.12\n>> -             typeset -h words\n>> -\n>> -             # workaround zsh's bug that quotes spaces in the COMPREPLY\n>> -             # array if IFS doesn't contain spaces.\n>> -             typeset -h IFS\n>> -     fi\n>> -\n>> -     local cur words cword prev\n>> -     _get_comp_words_by_ref -n =: cur words cword prev\n>>       while [ $c -lt $cword ]; do\n>>               i=\"${words[c]}\"\n>>               case \"$i\" in\n>> @@ -2667,22 +2652,6 @@ _git ()\n>>\n>>  _gitk ()\n>>  {\n>> -     if [[ -n ${ZSH_VERSION-} ]]; then\n>> -             emulate -L bash\n>> -             setopt KSH_TYPESET\n>> -\n>> -             # workaround zsh's bug that leaves 'words' as a special\n>> -             # variable in versions < 4.3.12\n>> -             typeset -h words\n>> -\n>> -             # workaround zsh's bug that quotes spaces in the COMPREPLY\n>> -             # array if IFS doesn't contain spaces.\n>> -             typeset -h IFS\n>> -     fi\n>> -\n>> -     local cur words cword prev\n>> -     _get_comp_words_by_ref -n =: cur words cword prev\n>> -\n>> @@ -2703,16 +2672,43 @@ _gitk ()\n>>       __git_complete_revlist\n>>  }\n>>\n>> -complete -o bashdefault -o default -o nospace -F _git git 2>/dev/null \\\n>> -     || complete -o default -o nospace -F _git git\n>> -complete -o bashdefault -o default -o nospace -F _gitk gitk 2>/dev/null \\\n>> -     || complete -o default -o nospace -F _gitk gitk\n>\n> Nice code reduction by moving the duplicated code into the new helpers ;-)\n>\n>> +__git_func_wrap ()\n>> +{\n>> +     if [[ -n ${ZSH_VERSION-} ]]; then\n>> +             emulate -L bash\n>> +             setopt KSH_TYPESET\n>> +\n>> +             # workaround zsh's bug that leaves 'words' as a special\n>> +             # variable in versions < 4.3.12\n>> +             typeset -h words\n>> +\n>> +             # workaround zsh's bug that quotes spaces in the COMPREPLY\n>> +             # array if IFS doesn't contain spaces.\n>> +             typeset -h IFS\n>> +     fi\n>> +     local cur words cword prev\n>> +     _get_comp_words_by_ref -n =: cur words cword prev\n>> +     $1\n>> +}\n>> +\n>> +# Setup completion for certain functions defined above by setting common\n>> +# variables and workarounds.\n>> +# This is NOT a public function; use at your own risk.\n>> +__git_complete ()\n>> +{\n>> +     local wrapper=\"__git_wrap${2}\"\n>> +     eval \"$wrapper () { __git_func_wrap $2 ; }\"\n>> +     complete -o bashdefault -o default -o nospace -F $wrapper $1 2>/dev/null \\\n>> +             || complete -o default -o nospace -F $wrapper $1\n>> +}\n>> +\n>> +__git_complete git _git\n>> +__git_complete gitk _gitk\n>\n> It makes my stomach queasy whenever I see $var not in double quotes that\n> forces me to guess (and trace to verify if the codepath is what I really\n> care about) if any value with $IFS in it could be used there, so even when\n> they are known to be safe, I'd prefer to see either explicit quotes or\n> comment that says what are expected in $1 and $2.\n\nWhich ones? These?\n\n>> +     $1\n>> +     complete -o bashdefault -o default -o nospace -F $wrapper $1 2>/dev/null \\\n>> +             || complete -o default -o nospace -F $wrapper $1\n\nSo you want:\n\n  \"$1\"\n\n  complete -o bashdefault -o default -o nospace -F \"$wrapper\" \"$1\" 2>/dev/null \\\n    || complete -o default -o nospace -F \"$wrapper\" \"$1\"\n\nI wouldn't object to the arguments of complete, but the function call\nseems totally unintuitive to me like that.\n\n-- \nFelipe Contreras\n"},{"id":"191514","messageId":"7vmx5ahbrm.fsf@alter.siamese.dyndns.org","threadId":"30531","inReplyTo":"CAMP44s1pb+J_SAzZ66QVcWq4V=LauUQ2VmzMD8KBtnhjubkkVg@mail.gmail.com","subject":"Re: [PATCH v5] completion: add new __git_complete helper","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-05-14T18:13:01Z","receivedAt":"2012-05-14T18:13:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras <felipe.contreras@gmail.com> writes:\n\n>>> +__git_func_wrap ()\n>>> +{\n>>> +     if [[ -n ${ZSH_VERSION-} ]]; then\n>>> +             emulate -L bash\n>>> +             setopt KSH_TYPESET\n>>> +\n>>> +             # workaround zsh's bug that leaves 'words' as a special\n>>> +             # variable in versions < 4.3.12\n>>> +             typeset -h words\n>>> +\n>>> +             # workaround zsh's bug that quotes spaces in the COMPREPLY\n>>> +             # array if IFS doesn't contain spaces.\n>>> +             typeset -h IFS\n>>> +     fi\n>>> +     local cur words cword prev\n>>> +     _get_comp_words_by_ref -n =: cur words cword prev\n>>> +     $1\n>>> +}\n>>> +\n>>> +# Setup completion for certain functions defined above by setting common\n>>> +# variables and workarounds.\n>>> +# This is NOT a public function; use at your own risk.\n>>> +__git_complete ()\n>>> +{\n>>> +     local wrapper=\"__git_wrap${2}\"\n>>> +     eval \"$wrapper () { __git_func_wrap $2 ; }\"\n>>> +     complete -o bashdefault -o default -o nospace -F $wrapper $1 2>/dev/null \\\n>>> +             || complete -o default -o nospace -F $wrapper $1\n>>> +}\n>>> +\n>>> +__git_complete git _git\n>>> +__git_complete gitk _gitk\n>>\n>> It makes my stomach queasy whenever I see $var not in double quotes that\n>> forces me to guess (and trace to verify if the codepath is what I really\n>> care about) if any value with $IFS in it could be used there, so even when\n>> they are known to be safe, I'd prefer to see either explicit quotes or\n>> comment that says what are expected in $1 and $2.\n>\n> Which ones?\n\nAll of them ;-)\n\nHere is my attempt to explain why none of them needs to be quoted:\n\n # Setup completion for certain functions defined above by setting common\n # variables and workarounds.\n # It takes two parameters:\n #  - the first is the command name on the command line to complete its\n #    arguments for the user;\n #  - the second is a name of the completion function\n # This is NOT a public function; use at your own risk.\n #\n # Note that none of the variable reference in the implementation of this\n # function needs dq around it.\n #\n # wrapper: the name of an internal shell function that wraps the\n #          completion function $2, formed by prefixing \"__git_wrap\"\n #          in front of it.  As it has to be usable as a name of a\n #          shell function, by definition there won't be $IFS characters\n #          in it.\n # $1:      the command name on the command line---ditto.\n # $2:      the shell function name that implements the completion-ditto.\n\nOnce there is such an explanation, the answer to the next question would\nbecome...\n\n>>> +     $1\n>>> +     complete -o bashdefault -o default -o nospace -F $wrapper $1 2>/dev/null \\\n>>> +             || complete -o default -o nospace -F $wrapper $1\n>\n> So you want:\n>\n>   \"$1\"\n\n... Not really.\n"},{"id":"191516","messageId":"CAMP44s1h=MPT8vx6JrGjMZWJzRjndxYKoYgo+1Y_Mmv+gWXzaQ@mail.gmail.com","threadId":"30531","inReplyTo":"7vmx5ahbrm.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v5] completion: add new __git_complete helper","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-05-14T18:30:45Z","receivedAt":"2012-05-14T18:30:45Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Mon, May 14, 2012 at 8:13 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Felipe Contreras <felipe.contreras@gmail.com> writes:\n>\n>>>> +__git_func_wrap ()\n>>>> +{\n>>>> +     if [[ -n ${ZSH_VERSION-} ]]; then\n>>>> +             emulate -L bash\n>>>> +             setopt KSH_TYPESET\n>>>> +\n>>>> +             # workaround zsh's bug that leaves 'words' as a special\n>>>> +             # variable in versions < 4.3.12\n>>>> +             typeset -h words\n>>>> +\n>>>> +             # workaround zsh's bug that quotes spaces in the COMPREPLY\n>>>> +             # array if IFS doesn't contain spaces.\n>>>> +             typeset -h IFS\n>>>> +     fi\n>>>> +     local cur words cword prev\n>>>> +     _get_comp_words_by_ref -n =: cur words cword prev\n>>>> +     $1\n>>>> +}\n>>>> +\n>>>> +# Setup completion for certain functions defined above by setting common\n>>>> +# variables and workarounds.\n>>>> +# This is NOT a public function; use at your own risk.\n>>>> +__git_complete ()\n>>>> +{\n>>>> +     local wrapper=\"__git_wrap${2}\"\n>>>> +     eval \"$wrapper () { __git_func_wrap $2 ; }\"\n>>>> +     complete -o bashdefault -o default -o nospace -F $wrapper $1 2>/dev/null \\\n>>>> +             || complete -o default -o nospace -F $wrapper $1\n>>>> +}\n>>>> +\n>>>> +__git_complete git _git\n>>>> +__git_complete gitk _gitk\n>>>\n>>> It makes my stomach queasy whenever I see $var not in double quotes that\n>>> forces me to guess (and trace to verify if the codepath is what I really\n>>> care about) if any value with $IFS in it could be used there, so even when\n>>> they are known to be safe, I'd prefer to see either explicit quotes or\n>>> comment that says what are expected in $1 and $2.\n>>\n>> Which ones?\n>\n> All of them ;-)\n>\n> Here is my attempt to explain why none of them needs to be quoted:\n>\n>  # Setup completion for certain functions defined above by setting common\n>  # variables and workarounds.\n>  # It takes two parameters:\n>  #  - the first is the command name on the command line to complete its\n>  #    arguments for the user;\n>  #  - the second is a name of the completion function\n>  # This is NOT a public function; use at your own risk.\n>  #\n>  # Note that none of the variable reference in the implementation of this\n>  # function needs dq around it.\n\nI don't understand that.\n\n>  # wrapper: the name of an internal shell function that wraps the\n>  #          completion function $2, formed by prefixing \"__git_wrap\"\n>  #          in front of it.  As it has to be usable as a name of a\n>  #          shell function, by definition there won't be $IFS characters\n>  #          in it.\n>  # $1:      the command name on the command line---ditto.\n>  # $2:      the shell function name that implements the completion-ditto.\n\nSo in short; because the variables are used as function/command names.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"191519","messageId":"7vaa1ah95p.fsf@alter.siamese.dyndns.org","threadId":"30531","inReplyTo":"CAMP44s1h=MPT8vx6JrGjMZWJzRjndxYKoYgo+1Y_Mmv+gWXzaQ@mail.gmail.com","subject":"Re: [PATCH v5] completion: add new __git_complete helper","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-05-14T19:09:22Z","receivedAt":"2012-05-14T19:09:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras <felipe.contreras@gmail.com> writes:\n\n>> Here is my attempt to explain why none of them needs to be quoted:\n>> ...\n> I don't understand that.\n\nThat was my attempt to give the readers some context to understand what\ncomes after that.  If the original patch author does not understand it,\nperhaps it is an indication that the function is somewhat underdocumented.\n\n>>  # wrapper: the name of an internal shell function that wraps the\n> ...\n> So in short; because the variables are used as function/command names.\n\nYes, and the fact that this is internal and we won't listen to clueless\npeople who attempt \"__git_complete 'foo bar' 'bar boz'\" and complain are\nwhat make all of them safe to be used without any quotes around it.\n"},{"id":"191524","messageId":"CAMP44s2L=s4bd-sDcY3RWLWe9=YzLgC7UavgnE6K0akMCJSK4w@mail.gmail.com","threadId":"30531","inReplyTo":"7vaa1ah95p.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v5] completion: add new __git_complete helper","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-05-14T19:53:10Z","receivedAt":"2012-05-14T19:53:10Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Mon, May 14, 2012 at 9:09 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Felipe Contreras <felipe.contreras@gmail.com> writes:\n>\n>>> Here is my attempt to explain why none of them needs to be quoted:\n>>> ...\n>> I don't understand that.\n>\n> That was my attempt to give the readers some context to understand what\n> comes after that.  If the original patch author does not understand it,\n> perhaps it is an indication that the function is somewhat underdocumented.\n\nI mean I don't understand this \"Note that none of the variable\nreference in the implementation of this function needs dq around it.\"\nI suppose you meant 'references' (as that would make it grammatically\ncorrect AFAICS), but I still don't understand what you are trying so\nsay. That the way the arguments are used in the __git_complete\nfunction makes it so double quotes are irrelevant?\n\nIf so, then yeah, I agree.\n\nIs there something actionable?\n\n-- \nFelipe Contreras\n"},{"id":"191556","messageId":"7v62bxfkeh.fsf@alter.siamese.dyndns.org","threadId":"30531","inReplyTo":"CAMP44s2L=s4bd-sDcY3RWLWe9=YzLgC7UavgnE6K0akMCJSK4w@mail.gmail.com","subject":"Re: [PATCH v5] completion: add new __git_complete helper","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-05-15T17:01:42Z","receivedAt":"2012-05-15T17:01:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras <felipe.contreras@gmail.com> writes:\n\n> On Mon, May 14, 2012 at 9:09 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>> Felipe Contreras <felipe.contreras@gmail.com> writes:\n>>\n>>>> Here is my attempt to explain why none of them needs to be quoted:\n>>>> ...\n>>> I don't understand that.\n>>\n>> That was my attempt to give the readers some context to understand what\n>> comes after that....\n>\n> I mean I don't understand this \"Note that none of the variable\n> reference in the implementation of this function needs dq around it.\"\n\nYou later say \"then yeah, I agree.\", so I am interpreting this \"I don't\nunderstand\" to mean that you do agree that missing dq around $1, $2 and\n$wrapper in the function is _not_ a problem, and you do not understand why\nit needs to be explained.  See below.\n\n> I suppose you meant 'references' (as that would make it grammatically\n> correct AFAICS),...\n\nCorrect.\n\n> but I still don't understand what you are trying so\n> say. That the way the arguments are used in the __git_complete\n> function makes it so double quotes are irrelevant?\n\nCorrect.\n\n> If so, then yeah, I agree.\n>\n> Is there something actionable?\n\nLike adding an explanation, preferrably a better one, there, so that the\nnext person do not have to wonder and waste time like I did?\n"},{"id":"191557","messageId":"7v1umlfkaq.fsf@alter.siamese.dyndns.org","threadId":"30531","inReplyTo":"7v62bxfkeh.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v5] completion: add new __git_complete helper","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-05-15T17:03:57Z","receivedAt":"2012-05-15T17:03:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Felipe Contreras <felipe.contreras@gmail.com> writes:\n> ...\n>> If so, then yeah, I agree.\n>>\n>> Is there something actionable?\n>\n> Like adding an explanation, preferrably a better one, there, so that the\n> next person do not have to wonder and waste time like I did?\n\ns/a better one/& than my attempt in the message you are replying to/;\n"},{"id":"191593","messageId":"CAMP44s0F04HZc5+MJQrzDJ0qJNjogBSpnWPdiS9MRWrUZx4Bpg@mail.gmail.com","threadId":"30531","inReplyTo":"7v62bxfkeh.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v5] completion: add new __git_complete helper","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-05-16T15:25:50Z","receivedAt":"2012-05-16T15:25:50Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Tue, May 15, 2012 at 7:01 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Felipe Contreras <felipe.contreras@gmail.com> writes:\n\n>> Is there something actionable?\n>\n> Like adding an explanation, preferrably a better one, there, so that the\n> next person do not have to wonder and waste time like I did?\n\nThe next person that does what? Review the code while keeping an eye\non the way the variable arguments are used inside the implementation\nof the function? I don't think there will *ever* be such a person\nafter the code is committed.\n\nEither way, I believe in the notion of self-documenting code, so I\nwould rather add a few double quotes rather than add an essay on top\nof the function just for you.\n\nNot everything in the code needs to be documented, and most\ndocumentation in the code is ignored anyway, and for a good example\ntake a look at the code in the Linux kernel.\n\nCheers.\n\n-- \nFelipe Contreras\n"}]}