Re: [PATCH 3/3] completion: match ctags symbol names in grep patterns

On Fri, Oct 21, 2011 at 03:25:45PM +0200, SZEDER Gábor wrote:

Interesting idea.  I reckon most of the time I do 'git grep' in order
to find callsites of a function or usage of a global variable.  I
usually do that by copy-pasting the symbol name, but this change could
likely spare me that copy-paste.

Thanks. Glad somebody else finds it useful. :)

quoted

+_git_grep_ctag_match() {

This is a helper function, therefore it should have two underscores as
prefix, i.e. __git_grep_ctag_match().  Single underscore prefixes are
"reserved" for completion functions of the corresponding git command,
i.e. if someone ever creates a git command or alias called
'grep_ctag_match', then 'git grep_ctag_match <TAB>' will invoke this
function to offer possible completion words.

Good point. Will change.

quoted

+	case "$COMP_CWORD,$prev" in

Depending on what is on the command line before the current word,
$COMP_CWORD might have different value in Bash 3 and in Bash 4; see
da48616f (bash: get --pretty=m<tab> completion to work with bash v4,
2010-12-02).  Please use $cword instead.

Thanks, I was completely unaware of this issue.

quoted

+	2,*|*,-*)
+		test -r tags || return

1. This checks for the tags file in the current directory, so it would
   only work at the top of the working tree, but not in any of the
   subdirectories.

Yeah. I didn't want to spend a lot of effort looking up through the
repository directories for a 'tags' file. Especially for people who
aren't using ctags at all, and for whom that would just be unnecessary
work.

OTOH, it is triggered only when they try to complete a pattern, which is
currently pointless. So maybe it's not worth worrying about existing
users, as they won't do this completion anyway.

2. The return in case of no tags file would cause file name
   completion.  This is different from the current behavior, when the
   completion script would offer refs.  Now, I don't think that refs
   as grep pattern are any more useful than file names...  but neither
   do I think that offering file names is an improvement over current
   behavior.  Anyway, this is a side effect not mentioned in the
   commit message.

Good point. Will fix.

quoted

+		COMPREPLY=( $(compgen -W "`_git_grep_ctag_match "$cur" tags`") )

1. Nit: $() around _git_grep_ctag_match().
   This would be the first backticks usage in the completion script.

Functionally irrelevant, I think, but style-wise, I agree it should
match the rest of the script.

2. When the completion script offers possible completion words, then
   usually a space is appended, e.g. 'git grep --e<TAB>' would
   complete to '--extended-regexp ', unless the offered option
   requires an argument, e.g. 'git commit --m<TAB>' would complete to
   '--message='.  I think function names extracted from the tags file
   should also get that trailing space.  No big deal, the easiest way
   to do that is to pass the output of _git_grep_ctag_match() to
   __gitcomp(), and it will take care of that.

Thanks, I had wanted to add the space at one point, but forgot about it
and got used to the current behavior. I agree adding it is better, and
it's nice that it's easy to do.

   However, this change will make 'git grep <TAB>' offer 9868 possible
   completion words in my git repository, which is quite a lot.

Hmm. I never thought of that as much, but after converting to use
__gitcomp, there is a noticeable delay. I guess it's the loop of printfs
in __gitcomp_1.

...Ah, yes, reading your bd4139caa, it seems that is exactly the
problem.

   I posted some completion optimizations recently, currently cooking
   in pu, which make processing that many possible completion words
   faster (sg/complete-refs, tip currently at 175901a5; the important
   commit is bd4139ca (completion: optimize refs completion,
   2011-10-15)).  To be able to use that optimization possible
   completion words must be separated by a newline, but your
   _git_grep_ctag_match() lists symbol names separated by a space.  It
   would be great if you could tweak _git_grep_ctag_match()'s awk
   script to list newline-separated symbols, so that when both
   branches are merged, then we could just change the __gitcomp() call
   to __gitcomp_nl().

Easy enough (just drop the ORS setting).

Thanks very much for your review. I have a fix for the first patch in
the series, too, so I'll send a whole new series in a moment.

-Peff