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

From: SZEDER Gábor <szeder@ira.uka.de>
Date: Oct 29, 2011, 12:47 UTC
Message-ID: <20111029124755.GE2345@goldbirke>
In-Reply-To: <20111028060517.GA3993@sigill.intra.peff.net>

Hi,

On Thu, Oct 27, 2011 at 11:05:20PM -0700, Jeff King wrote:

Show 21 quoted lines

> On Sun, Oct 23, 2011 at 11:29:28PM +0200, SZEDER Gábor wrote:
> 
> > On Fri, Oct 21, 2011 at 01:30:21PM -0400, Jeff King wrote:
> > > This incorporates the suggestions from Gábor's review, with one
> > > exception: it still looks only in the current directory for the "tags"
> > > files. I think that might have some performance implications, so I'd
> > > rather add it separately, if at all.
> > 
> > I agree that scanning through a whole working tree for tags files
> > would cost too much.  But I think that a tags file at the top of the
> > working tree is common enough to be supported, and checking its
> > existence is fairly cheap.
> 
> Actually, it's not too expensive. Asking git for the top of the working
> tree means it has to traverse up to there anyway. So the trick is just
> doing our search without invoking too many external tools which would
> cause unnecessary forks.
> 
> The patch is below, but I'm still not sure it's a good idea.
> 
> Grep only looks in the current subdirectory for matches.

Unless the user explicitly specifies the path(s)... But that comes at the end of the command line, so the completion script could have no idea about it at the time of 'git grep <TAB>'.

Show 10 quoted lines

> > So how about something like this for the case arm? (I didn't actually
> > tested it.)
> > 
> > 		local tagsfile
> > 		if test -r tags; then
> > 			tagsfile=tags
> > 		else
> > 			local dir="$(__gitdir)"
> 
> You don't want __gitdir here, but rather "git rev-parse --show-cdup".

Oh, yes, indeed.

But there was a point in using __gitdir() here: it respects the --git-dir= option. Which brings up the question: what should 'git --git-dir=/some/where/.git grep <TAB>' offer?

So in the end I agree that it's not a good idea.

Show 9 quoted lines

> > Btw, there is a bug in the case statement: 'git --no-pager grep <TAB>'
> > offers refs instead of symbols, because $cword is not 2 and $prev
> > doesn't start with a dash.  But it's not worse than the current
> > behavior, so I don't think this bug is a show-stopper for the patch.
> 
> Yeah. The intent of the "$cword is 2" thing is to know that we are the
> first non-option argument. Arguably, _get_comp_words_by_ref should
> somehow tell us which position we are completing relative to the actual
> command name.

_get_comp_words_by_ref() is a general completion function, the purpose of which is to provide a bash-version-independent equivalent of $COMP_WORDS and $COMP_CWORD by working around the different word splitting rules. It doesn't know about git and its commands at all.

But there is the while loop in _git() that looks for the git command (among other things) on the command line, which could store the index of the command name in $words in a variable. This variable could then be used in that case statement (and probably in a couple of other places, too).

Best, Gábor

Show 21 quoted lines

> ---
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index af283cb..b0ed657 100755
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -1429,6 +1429,39 @@ _git_gitk ()
>  	_gitk
>  }
>  
> +__git_cdup_dirs() {
> +	local prefix=$(git rev-parse --show-cdup 2>/dev/null)
> +	local oldifs=$IFS
> +	local dots
> +	local i
> +	IFS=/
> +	for i in $prefix; do
> +		dots=$dots../
> +		echo "$dots"
> +	done
> +	IFS=$oldifs
> +}

No need for $oldifs here; do a local IFS=/ instead, and then it just goes out of scope when returning from the function.

Show 36 quoted lines

> +
> +__git_find_in_cdup_one() {
> +	local dir=$1; shift
> +	for i in "$@"; do
> +		if test -r "$dir$i"; then
> +			echo "$dir$i"
> +			return 0
> +		fi
> +	done
> +	return 1
> +}
> +
> +__git_find_in_cdup() {
> +	__git_find_in_cdup_one "" "$@" && return
> +
> +	local dir
> +	for dir in $(__git_cdup_dirs); do
> +		__git_find_in_cdup_one "$dir" "$@" && return
> +	done
> +}
> +
>  __git_match_ctag() {
>  	awk "/^${1////\\/}/ { print \$1 }" "$2"
>  }
> @@ -1457,8 +1490,9 @@ _git_grep ()
>  
>  	case "$cword,$prev" in
>  	2,*|*,-*)
> -		if test -r tags; then
> -			__gitcomp "$(__git_match_ctag "$cur" tags)"
> +		local tags=$(__git_find_in_cdup tags)
> +		if test -n "$tags"; then
> +			__gitcomp "$(__git_match_ctag "$cur" "$tags")"
>  			return
>  		fi
>  		;;