Re: [PATCH v2] git-completion: Add support for git submodule options

["SZEDER Gábor" <szeder@ira.uka.de>]

Hi,


On Mon, Apr 11, 2011 at 07:47:52PM +0200, Nicolas Morey-Chaisemartin wrote:
> Completion for git submodule only handled subcommands.
> Add support for options of each subcommand
> 
> Signed-off-by: Nicolas Morey-Chaisemartin <nicolas@morey-chaisemartin.com>
> ---
> Changes:
>  - Inlined local variables listing options.
>
>  contrib/completion/git-completion.bash |   32 +++++++++++++++++++++++++++++++-
>  1 files changed, 31 insertions(+), 1 deletions(-)

This change looks good overall, but I have a few nits, though.

> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 840ae38..20d0cf0 100755
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -2546,7 +2546,8 @@ _git_submodule ()
>  	__git_has_doubledash && return
>  
>  	local subcommands="add status init update summary foreach sync"
> -	if [ -z "$(__git_find_on_cmdline "$subcommands")" ]; then
> +	local subcommand="$(__git_find_on_cmdline "$subcommands")"
> +	if [ -z "$subcommand" ]; then
>  		local cur
>  		_get_comp_words_by_ref -n =: cur
>  		case "$cur" in
> @@ -2558,6 +2559,35 @@ _git_submodule ()
>  			;;
>  		esac
>  		return
> +	else
> +	    local cur
> +	    _get_comp_words_by_ref -n =: cur

Both branches of the if-else statement start with the same

	    local cur
	    _get_comp_words_by_ref -n =: cur

so these could be moved out in front of the if statement.

> +	    case "$subcommand,$cur" in
> +		 add,--*)
> +		     __gitcomp "--branch= --force --reference="
> +		     ;;
> +		 status,--*)
> +		     __gitcomp "--cached --recursive"
> +		     ;;
> +		 update,--*)
> +		     __gitcomp "--init --no-fetch --rebase --reference= --merge --recursive"
> +		     ;;
> +		 summary,--*)
> +		     __gitcomp "--cached --files --summary-limit="
> +		     ;;
> +		 summary,*)
> +		     __gitcomp "$(__git_refs)"
> +		     ;;
> +		 foreach,--*)
> +		     __gitcomp "--recursive"
> +		     ;;
> +		 sync,*)
> +		     COMPREPLY=()
> +		     ;;
> +		 *)
> +		     COMPREPLY=()
> +		     ;;

The 'sync,*' case is not necessary, because it has the same completion
reply as the '*' case, and that case would match it anyway.

And finally, please use tabs for indentation.


Best,
Gábor