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

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

Hi,

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

[...]

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

[...]

> 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.

> 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.

>  #
>  # 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.

> +
> +       # 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[*]}"

> +}
> +
> +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?

> +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.

> +
> +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