Re: Fwd: [PATCH] Add tcsh-completion support to contrib by using git-completion.bash

From: SZEDER Gábor <szeder@ira.uka.de>
Date: Nov 13, 2012, 11:14 UTC
Message-ID: <20121113111448.GA3817@goldbirke>
In-Reply-To: <CAFj1UpFd9X8Jq5o7B4m35i=merBDvOo4NOtwth=UnG2S5X_rGw@mail.gmail.com>

Hi,

On Mon, Nov 12, 2012 at 03:07:46PM -0500, Marc Khouzam wrote:

> Hi,

[...]

> Signed-off-by: Marc Khouzam <marc.khouzam@gmail.com>

[...]

Show 9 quoted lines

> Thanks
> 
> Marc
> 
> ---
>  contrib/completion/git-completion.bash |   53 +++++++++++++++++++++++++++++++-
>  contrib/completion/git-completion.tcsh |   34 ++++++++++++++++++++
>  2 files changed, 86 insertions(+), 1 deletions(-)
>  create mode 100755 contrib/completion/git-completion.tcsh

Please have a look at Documentation/SubmittingPatches to see how to properly format the commit message, i.e. no greeting and sign-off in the commit message part, and the S-o-b line should be the last before the '---'.

Your patch seems to be severely line-wrapped. That document also contains a few MUA-specific tips to help avoid that.

Other than that, it's a good description of the changes and considerations. I agree that this approach seems to be the best from the three.

Show 10 quoted lines

> diff --git a/contrib/completion/git-completion.bash
> b/contrib/completion/git-completion.bash
> index be800e0..6d4b57a 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -1,4 +1,6 @@
> -#!bash
> +#!/bin/bash
> +# The above line is important as this script can be executed when used
> +# with another shell such as tcsh

See comment near the end.

Show 20 quoted lines

>  #
>  # bash/zsh completion support for core Git.
>  #
> @@ -2481,3 +2483,52 @@ __git_complete gitk __gitk_main
>  if [ Cygwin = "$(uname -o 2>/dev/null)" ]; then
>  __git_complete git.exe __git_main
>  fi
> +
> +# Method that will output the result of the completion done by
> +# the bash completion script, so that it can be re-used in another
> +# context than the bash complete command.
> +# It accepts 1 to 2 arguments:
> +# 1: The command-line to complete
> +# 2: The index of the word within argument #1 in which the cursor is
> +#    located (optional). If parameter 2 is not provided, it will be
> +#    determined as best possible using parameter 1.
> +_git_complete_with_output ()
> +{
> +       # Set COMP_WORDS to the command-line as bash would.
> +       COMP_WORDS=($1)

That comment is only true for older Bash versions. Since v4 Bash splits the command line at characters that the readline library treats as word separators when performing word completion. But the completion script has functions to deal with both, so this shouldn't be a problem.

Show 29 quoted lines

> +
> +       # Set COMP_CWORD to the cursor location as bash would.
> +       if [ -n "$2" ]; then
> +               COMP_CWORD=$2
> +       else
> +               # Assume the cursor is at the end of parameter #1.
> +               # We must check for a space as the last character which will
> +               # tell us that the previous word is complete and the cursor
> +               # is on the next word.
> +               if [ "${1: -1}" == " " ]; then
> +                       # The last character is a space, so our
> location is at the end
> +                       # of the command-line array
> +                       COMP_CWORD=${#COMP_WORDS[@]}
> +               else
> +                       # The last character is not a space, so our
> location is on the
> +                       # last word of the command-line array, so we
> must decrement the
> +                       # count by 1
> +                       COMP_CWORD=$((${#COMP_WORDS[@]}-1))
> +               fi
> +       fi
> +
> +       # Call _git() or _gitk() of the bash script, based on the first
> +       # element of the command-line
> +       _${COMP_WORDS[0]}
> +
> +       # Print the result that is stored in the bash variable ${COMPREPLY}

Really? ;)

I like the above comments about setting COMP_CWORD, because they explain why you do what you do, which would be otherwise difficult to figure out. But telling that an echo in a for loop over an array prints that array is, well, probably not necessary.

> +       for i in ${COMPREPLY[@]}; do
> +               echo "$i"
> +       done

There is no need for the loop here to print the array one element per line:

        local IFS=$'\n'
        echo "${COMPREPLY[*]}"

Show 6 quoted lines

> +}
> +
> +if [ -n "$1" ] ; then
> +  # If there is an argument, we know the script is being executed
> +  # so go ahead and run the _git_complete_with_output function
> +  _git_complete_with_output "$1" "$2"

Where does the second argument come from? Below you run this script as '${__git_tcsh_completion_script} "${COMMAND_LINE}"', i.e. $2 is never set. Am I missing something?

Show 41 quoted lines

> +fi
> diff --git a/contrib/completion/git-completion.tcsh
> b/contrib/completion/git-completion.tcsh
> new file mode 100755
> index 0000000..7b7baea
> --- /dev/null
> +++ b/contrib/completion/git-completion.tcsh
> @@ -0,0 +1,34 @@
> +#!tcsh
> +#
> +# tcsh completion support for core Git.
> +#
> +# Copyright (C) 2012 Marc Khouzam <marc.khouzam@gmail.com>
> +# Distributed under the GNU General Public License, version 2.0.
> +#
> +# This script makes use of the git-completion.bash script to
> +# determine the proper completion for git commands under tcsh.
> +#
> +# To use this completion script:
> +#
> +#    1) Copy both this file and the bash completion script to your
> ${HOME} directory
> +#       using the names ${HOME}/.git-completion.tcsh and
> ${HOME}/.git-completion.bash.
> +#    2) Add the following line to your .tcshrc/.cshrc:
> +#        source ${HOME}/.git-completion.tcsh
> +
> +# One can change the below line to use a different location
> +set __git_tcsh_completion_script = ${HOME}/.git-completion.bash
> +
> +# Check that the user put the script in the right place
> +if ( ! -e ${__git_tcsh_completion_script} ) then
> +       echo "ERROR in git-completion.tcsh script.  Cannot find:
> ${__git_tcsh_completion_script}.  Git completion will not work."
> +       exit
> +endif
> +
> +# Make the script executable if it is not
> +if ( ! -x ${__git_tcsh_completion_script} ) then
> +       chmod u+x ${__git_tcsh_completion_script}
> +endif

Not sure about this. If I source a script to provide completion for a command, then I definitely don't expect it to change file permissions.

However, even if the git completion script is not executable, you can still run it with 'bash ${__git_tcsh_completion_script}'. This way neither the user would need to set permissions, not the script would need to set it behind the users back. Furthermore, this would also make changing the shebang line unnecessary.

Show 5 quoted lines

> +
> +complete git  'p/*/`${__git_tcsh_completion_script} "${COMMAND_LINE}"
> | sort | uniq`/'
> +complete gitk 'p/*/`${__git_tcsh_completion_script} "${COMMAND_LINE}"
> | sort | uniq`/'

Is the 'sort | uniq' really necessary? After the completion function returns Bash automatically sorts the elements in COMPREPLY and removes any duplicates. Doesn't tcsh do the same? I have no idea about tcsh completion.

Does the git completion script returns any duplicates at all? Ambigious refs come to mind, but I just checked that refs completion, or rather 'git for-each-ref' (the command driving refs completion), is kind enough to make any ambigious ref names unique (i.e. a branch and a tag with the same name is listed as 'heads/name' and 'tags/name').

Thanks, Gábor