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

From: Marc Khouzam <marc.khouzam@gmail.com>
Date: Nov 13, 2012, 20:12 UTC
Message-ID: <CAFj1UpGxx_9GHSnJRpe8hDGB6OTio1mcN71LKcR0pxhSVx2xDw@mail.gmail.com>
In-Reply-To: <20121113111448.GA3817@goldbirke>

Thanks for the review.

On Tue, Nov 13, 2012 at 6:14 AM, SZEDER Gábor <szeder@ira.uka.de> wrote:

Show 25 quoted lines

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

Sorry about that, since I should have noticed it in the doc. I will do my best to address all submission issues mentioned when I post the next version of the patch.

Show 19 quoted lines

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

Great suggestion below.  I removed the above change.

Show 8 quoted lines

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

I've updated the comment to be more general and left the code the same since it is supported by the script.

>
>> +       # Print the result that is stored in the bash variable ${COMPREPLY}
>
> Really? ;)

Removed :)

Show 9 quoted lines

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

Better.  Thanks.

Show 8 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?

This second argument is optional and, if present, will be put in $COMP_CWORD. If not present, $COMP_CWORD must be computed from $1. Also see comment above _git_complete_with_output (). tcsh does not provide me with this information, so I cannot make use of it. However, I thought it would be more future-proof to allow it for other shells which may have that information.

It is not necessary for tcsh, so I can remove if you prefer?

Show 13 quoted lines

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

Very nice!  Done.

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

On my machine, tcsh does not remove duplicates. It does sort the results but that is done after I've run 'uniq', which is too late. I'm not happy about this either, but the other option is to improve git-completion.bash to avoid duplicates, which seemed less justified.

> Does the git completion script returns any duplicates at all?

It does. 'help' is returned twice for example. Also, when completing 'git checkout ' in the git repo, I can see multiple 'todo' branches, as well as 'master', 'pu', 'next', etc.

You can actually try it without tcsh by running my proposed version of git-completion.bash like this:

cd git/contrib/completion bash git-completion.bash "git checkout " | sort | uniq --repeated

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

I will post a new version of the patch after looking at Felipe's patch for zsh, which I was not aware of.

Thanks!

Marc