Re: [PATCH v3 3/7] completion: add new __gitcompadd helper

2013-04-12 Thread Junio C Hamano
Felipe Contreras  writes:

> The idea is to never touch the COMPREPLY variable directly.
>
> This allows other completion systems (i.e. zsh) to override
> __gitcompadd, and do something different instead.
>
> Also, this allows further optimizations down the line.
>
> There should be no functional changes.
>
> Signed-off-by: Felipe Contreras 
> ---
>  contrib/completion/git-completion.bash | 13 -
>  1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/contrib/completion/git-completion.bash 
> b/contrib/completion/git-completion.bash
> index 2c87fd8..90b54ab 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -195,6 +195,11 @@ _get_comp_words_by_ref ()
>  }
>  fi
>  
> +__gitcompadd ()
> +{
> + COMPREPLY=($(compgen -W "$1" -P "$2" -S "$4" -- "$3"))

Making a mental note that this takes prefix ($2), suffix ($4) and
the actual word ($3) are given in addition to the list of expansions
($1)...

> +}
> +
>  # Generates completion reply with compgen, appending a space to possible
>  # completion words, if necessary.
>  # It accepts 1 to 4 arguments:
> @@ -211,9 +216,7 @@ __gitcomp ()
>   ;;
>   *)
>   local IFS=$'\n'
> - COMPREPLY=($(compgen -P "${2-}" \
> - -W "$(__gitcomp_1 "${1-}" "${4-}")" \
> - -- "$cur_"))
> + __gitcompadd "$(__gitcomp_1 "${1-}" "${4-}")" "${2-}" "$cur_" ""

This did not use to use suffix, but we pass an empty string as $4,
so it is an equivalent rewrite.

>   ;;
>   esac
>  }
> @@ -230,7 +233,7 @@ __gitcomp ()
>  __gitcomp_nl ()
>  {
>   local IFS=$'\n'
> - COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$1" -- "${3-$cur}"))
> + __gitcompadd "$1" "${2-}" "${3-$cur}" "${4- }"

This also is a straight-forward rewrite.

>  }
>  
>  # Generates completion reply with compgen from newline-separated possible
> @@ -1820,7 +1823,7 @@ _git_config ()
>   local remote="${prev#remote.}"
>   remote="${remote%.fetch}"
>   if [ -z "$cur" ]; then
> - COMPREPLY=("refs/heads/")
> + __gitcompadd "refs/heads/"

I am not sure about this one, though.

Other callers took pains to protet against triggering unset variable
references by using ${1-} instead of ${1}.  Shouldn't this caller be
passing three empty strings?

>   return
>   fi
>   __gitcomp_nl "$(__git_refs_remotes "$remote")"
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v3 3/7] completion: add new __gitcompadd helper

2013-04-12 Thread Felipe Contreras
On Fri, Apr 12, 2013 at 12:55 PM, Junio C Hamano  wrote:
> Felipe Contreras  writes:

>>  }
>>
>>  # Generates completion reply with compgen from newline-separated possible
>> @@ -1820,7 +1823,7 @@ _git_config ()
>>   local remote="${prev#remote.}"
>>   remote="${remote%.fetch}"
>>   if [ -z "$cur" ]; then
>> - COMPREPLY=("refs/heads/")
>> + __gitcompadd "refs/heads/"
>
> I am not sure about this one, though.
>
> Other callers took pains to protet against triggering unset variable
> references by using ${1-} instead of ${1}.  Shouldn't this caller be
> passing three empty strings?

Perhaps, or perhaps we were being too careful before: 'compgen -W foo'
is the same as 'compgen -W foo -S "" -P "" -- ""'.

--
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v3 3/7] completion: add new __gitcompadd helper

2013-04-12 Thread Junio C Hamano
Felipe Contreras  writes:

> On Fri, Apr 12, 2013 at 12:55 PM, Junio C Hamano  wrote:
>> Felipe Contreras  writes:
>
>>>  }
>>>
>>>  # Generates completion reply with compgen from newline-separated possible
>>> @@ -1820,7 +1823,7 @@ _git_config ()
>>>   local remote="${prev#remote.}"
>>>   remote="${remote%.fetch}"
>>>   if [ -z "$cur" ]; then
>>> - COMPREPLY=("refs/heads/")
>>> + __gitcompadd "refs/heads/"
>>
>> I am not sure about this one, though.
>>
>> Other callers took pains to protet against triggering unset variable
>> references by using ${1-} instead of ${1}.  Shouldn't this caller be
>> passing three empty strings?
>
> Perhaps, or perhaps we were being too careful before: 'compgen -W foo'
> is the same as 'compgen -W foo -S "" -P "" -- ""'.

Yes, they are the same (otherwise this patch would not be valid),
but that is not what i was wondering about.


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html