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