From: Jonathan Nieder <jrnieder@gmail.com>
Date: Wed, 27 Oct 2010 17:31:32 GMT
Subject: Re: [PATCH v2/RFC] Make git-completion Bash 4 compatible.
Message-ID: <20101027173132.GA15657@burratino>
In-Reply-To: <20101027131506.4da06c6d@MonteCarlo>

Hi Peter,

Peter van der Does wrote:

> The completion script does not work as expected under Bash 4.

Thanks for your work fixing this.  That's awesome.

It would be ideal if someone could write or find a nice summary of the
problem and the chosen solution, for inclusion in the commit message.

Could some zsh user perhaps test that the new zsh support is not
broken?

>  1 files changed, 355 insertions(+), 62 deletions(-)

Kind of unfortunate.  There are a lot of comments, but still...

> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -76,12 +76,251 @@
>  #
>  #       git@vger.kernel.org
>  #
> +# Updated for Bash 4.0

I don't think this comment will be so important for posterity (e.g., once
bash 5 comes around ;-)).

[...]
> +# If the function _get_comp_words_by_ref does not exists, we can assume the
> +# bash_completion 1.2 script isn't loaded and therefor we're defining the
> +# necessary functions ourselves.

Probably this explanation belongs in the commit message?  A comment
could provide a brief reminder, like:

	if ! type _get_comp_words_by_ref &>/dev/null ; then
		# The bash_completion 1.2 library was not loaded,
		# so we have to define some functions from it ourselves.

Are the implementations taken from bash_completion?  If so, that would
be very useful information for the log message: future readers may
want to know where to look for a more recent version.

> +	# Assign variable one scope above the caller
[... I'm assuming this is all written correctly, etc ...]

> @@ -331,7 +570,8 @@ __gitcomp_1 ()
>  # generates completion reply with compgen
>  __gitcomp ()
>  {
> -	local cur="${COMP_WORDS[COMP_CWORD]}"
> +	local cur
> +	_get_comp_words_by_ref -n "=" cur
[...]

The rest looks sane.  Maybe it would make sense to split this into two
patches for readability:

 - one to introduce the _get_comp_words_by_ref function
 - one to use it

?