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

From: Jeff King <peff@peff.net>
Date: Oct 21, 2011, 17:22 UTC
Message-ID: <20111021172239.GA22289@sigill.intra.peff.net>
In-Reply-To: <20111021132545.GA27385@goldbirke>

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

Show 8 quoted lines

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

Show 6 quoted lines

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

Show 6 quoted lines

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

Show 7 quoted lines

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

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

Show 8 quoted lines

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

Show 11 quoted lines

>    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