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

2012-11-13 Thread SZEDER Gábor
Hi,

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

[...]

 Signed-off-by: Marc Khouzam marc.khou...@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 000..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.khou...@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 

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

2012-11-13 Thread Marc Khouzam
Thanks for the review.

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

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

 [...]

 Signed-off-by: Marc Khouzam marc.khou...@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.

 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 | 

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

2012-11-13 Thread SZEDER Gábor
Hi,

On Tue, Nov 13, 2012 at 03:12:44PM -0500, Marc Khouzam wrote:
  +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?

I see.  I read those comments and understood what it is about.  I was
just surprised that the code is there to make use of it, yet it's not
specified when invoking that function.

Since it's a trivial piece of code, I would say let's keep it.  Could
you please add a sentence about it (that it's for possible future
users and it's not used at the moment) to the commit message for
future reference?

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

Ok.  Then keep it for the time being, and we'll see what we can do to
avoid those duplicates.

  Does the git completion script returns any duplicates at all?
 
 It does.  'help' is returned twice for example.

Right.  Now that you mentioned it, I remember I noticed it a while
ago, too.  I even wrote a patch to fix it, but not sure what became of
it.  Will try to dig it up.

 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

Interesting, I can't reproduce.  Are the duplicates also there, if you
start a bash, source git-completion.bash, and run __git_refs ?

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2012-11-13 Thread SZEDER Gábor
Hi,


I've got two more comments.

On Mon, Nov 12, 2012 at 03:07:46PM -0500, Marc Khouzam wrote:
 @@ -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 ()

We differentiate between _git_whatever() and __git_whatever()
functions.  The former performs completion for the 'whatever' git
command/alias, the latter is a completion helper function.  This
is a helper function, so it should begin with double underscores.

 +{
 +   # Set COMP_WORDS to the command-line as bash would.
 +   COMP_WORDS=($1)
 +
 +   # Set COMP_CWORD to the cursor location as bash would.
 +   if [ -n $2 ]; then

A while ago the completion script was made 'set -u'-clean.  (If 'set
-u' is enabled, then it's an error to access undefined variables).
I'm not sure how many people are out there who'd use this script for
tcsh while having 'set -u' in their profile...  probably not that
many.  Still, I think it would be great to keep it up.

Here $2 would be undefined, so accessingit it would cause an error
under those semantincs.  Please use ${2-} instead (use empty string
when undefined).

 +if [ -n $1 ] ; then

Same here.

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

And here.

Thanks
Gábor

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2012-11-13 Thread Marc Khouzam
On Tue, Nov 13, 2012 at 6:46 PM, SZEDER Gábor sze...@ira.uka.de wrote:
 Hi,

 On Tue, Nov 13, 2012 at 03:12:44PM -0500, Marc Khouzam wrote:
  +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?

 I see.  I read those comments and understood what it is about.  I was
 just surprised that the code is there to make use of it, yet it's not
 specified when invoking that function.

 Since it's a trivial piece of code, I would say let's keep it.  Could
 you please add a sentence about it (that it's for possible future
 users and it's not used at the moment) to the commit message for
 future reference?

Will do.

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

 Ok.  Then keep it for the time being, and we'll see what we can do to
 avoid those duplicates.

Thanks.

  Does the git completion script returns any duplicates at all?

 It does.  'help' is returned twice for example.

 Right.  Now that you mentioned it, I remember I noticed it a while
 ago, too.  I even wrote a patch to fix it, but not sure what became of
 it.  Will try to dig it up.

Thanks for already posting the patch.

 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

 Interesting, I can't reproduce.  Are the duplicates also there, if you
 start a bash, source git-completion.bash, and run __git_refs ?

Running __git_refs does not show the duplicates, but running
__git refs '' 1
does show them.
That second parameter causes __git_refs to
use the guess heuristic employed by checkout for tracking branches

I don't quite understand this, but what I can see is that my remote
branches GitHub/master and origin/master each cause another
'master' to be listed:

$ __git_refs '' 1|grep master
master
GitHub/master
origin/master
master
master

All fixes are done and I'll post a second version of the patch
as soon as I can figure out the formatting properly.

Thanks again

Marc
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2012-11-12 Thread Marc Khouzam
Hi,

this patch allows tcsh-users to get the benefits of the awesome
git-completion.bash script.  It could also help other shells do the same.

==

The current tcsh-completion support for Git, as can be found on the
internet, takes the approach of defining the possible completions
explicitly.  This has the obvious draw-back to require constant
updating as the Git code base evolves.

The approach taken by this commit is to to re-use the advanced bash
completion script and use its result for tcsh completion.  This is
achieved by executing (versus sourcing) the bash script and
outputting the completion result for tcsh consumption.

Three solutions were looked at to implement this approach with (A)
being retained:

  A) Modifications:
  git-completion.bash and new git-completion.tcsh

 Modify the existing git-completion.bash script to support
 being sourced using bash (as now), but also executed using bash.
 When being executed, the script will output the result of the
 computed completion to be re-used elsewhere (e.g., in tcsh).
 Pros:
   1- allows the git-completion.bash script to easily be re-used
   2- tcsh support is mostly isolated in git-completion.tcsh
 Cons (for tcsh users only):
   1- requires the user to copy both git-completion.tcsh and
  git-completion.bash to ${HOME}
   2- requires bash script to have a fixed name and location:
  ${HOME}/.git-completion.bash

  B) Modifications:
  git-completion.bash

 Modify the existing git-completion.bash script to support
 being sourced using bash (as now), but also executed using bash,
 and sourced using tcsh.
 Pros:
   1- only requires the user to deal with a single file
   2- maintenance more obvious for tcsh since it is entirely part
  of the same git-completion.bash script.
 Cons:
   1- tcsh support could affect bash support as they share the
  same script
   2- small tcsh section must use syntax suitable for both tcsh
  and bash and must be at the beginning of the script
   3- requires the user to explicitly make the script executable
  when using tcsh (for tcsh users only)
   4- requires script to have a fixed name and location:
  ${HOME}/.git-completion.sh (for tcsh users only)

  C) Modifications:
  New git-completion.tcsh

 Provide a short tcsh script that converts git-completion.bash
 into an executable script suitable to be used by tcsh.
 Pros:
   1- tcsh support is entirely isolated in git-completion.tcsh
   2- new tcsh script can be as complex as needed
 Cons (for tcsh users only):
   1- requires the user to copy both git-completion.tcsh and
  git-completion.bash to ${HOME}
   2- requires bash script to have a fixed name and location:
  ${HOME}/.git-completion.bash
   3- sourcing the new script will generate a third script

Approach (A) was selected to keep the tcsh completion support well
isolated without introducing excessive complexity.

Signed-off-by: Marc Khouzam marc.khou...@gmail.com

==

With the changes applied, tcsh users should:

#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

The code can be found on GitHub.
Option (A):
https://github.com/marckhouzam/git/commit/86d3a8e740ae85b4b4462c997a0fd969b1b2d24c

Option (B):
https://github.com/marckhouzam/git/commit/e64606541682eaf66c0a56aceff279ca6e1d06cd

Option (C):
https://github.com/marckhouzam/git/commit/59792455f1e6a98d3ffeb828f4cff1ded0e4ed37

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

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