Thanks for the review.
On Tue, Nov 13, 2012 at 6:14 AM, SZEDER Gábor <szeder@xxxxxxxxxx> wrote:
> Hi,
>
> On Mon, Nov 12, 2012 at 03:07:46PM -0500, Marc Khouzam wrote:
>> Hi,
>
> [...]
>
>> Signed-off-by: Marc Khouzam <marc.khouzam@xxxxxxxxx>
>
> [...]
>
>> 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.
> 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.
>> + # 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 :)
>> + 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.
>> +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?
>> +# 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.
>> +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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html