Re: [PATCH] git-completion.bash: add support for path completion

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 17, 2012, 04:50 UTC
Message-ID: <7vy5gxnuy1.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1355693080-4765-1-git-send-email-manlio.perillo@gmail.com>

Manlio Perillo <manlio.perillo@gmail.com> writes:

Show 5 quoted lines

> The git-completion.bash script did not implemented full support for
> completion, for git commands that operate on files from the current
> working directory or the index.
>
> For these commands, only options completion was available.

Hrm, "git mv CO<TAB>" completes it to "COPYING" for me.  Same for:

    git rm CO<TAB>
    git clean Doc<TAB>
    git commit --edit Doc<TAB>

There must be something missing from the above description, and the claim the above two paragraphs make does no make sense without that something that is missing.

> * the path completion for the "git mv" and "git rm" commands is provided
>   using "git ls-files --exclude-standard"

Does the above mean "git mv COPYING Doc<TAB>" would complete the command line to move it to Documentation/ directory?

I think "using X" is of secondary importance. Reviewers and future developers (who are reading "git log" output) can read it from the patch. What is expected in the log message is why the implemenation was chosen, and in order to achieve what effect.

For example, something like:

    Each parameter between the first one and the one before the last
    one to 'git mv' is completed to a tracked path or a leading
    directory of a tracked path

should come first to explain what your patch wanted to do. It of course is helpful to explain how you implemented that behaviour, by appending ", using 'ls-files --exclude-standard'" at the end.

> * the path completion for the "git add" command is provided using
>   "git ls-files --exclude-standard -o -m"

Likewise. You are adding "either a modified, or a new and unignored" and use "ls-files --exclude-standard -o -m" to achieve it.

Show 5 quoted lines

> * the path completion for the "git clean" command is provided using
>   "git ls-files --exclude-standard -o"
>
> * the path completion for the "git commit" command is provides using
>   "git diff-index --name-only HEAD"

As long as all of the above stops completion at directory boundary, I think the above sounds like a sensible thing to do. e.g. when "ls-files" gives Documentation/Makefile and Documentation/git.txt, "git cmd Doc<TAB>" first would give "git cmd Documentation/" and then the second <TAB> would offer these two paths as choices. That way, the user can choose to just execute "git cmd Documentation/" without even looking at these individual paths.

I am not sure how you would handle the last parameter to "git mv", though. That is by definition a path that does not exist, i.e. cannot be completed.

Show 116 quoted lines

> Signed-off-by: Manlio Perillo <manlio.perillo@gmail.com>
> ---
>  contrib/completion/git-completion.bash | 39 ++++++++++++++++++++++------------
>  1 file changed, 26 insertions(+), 13 deletions(-)
>
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 0b77eb1..8b348c3 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -233,6 +233,25 @@ __gitcomp_nl ()
>  	COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$1" -- "${3-$cur}"))
>  }
>  
> +__git_files ()
> +{
> +	local dir="$(__gitdir)"
> +	if [ -d "$dir" ]; then
> +		git --git-dir="$dir" ls-files --exclude-standard $*
> +		return
> +	fi
> +}
> +
> +# Return all staged files with modification from current HEAD
> +__git_commit_files ()
> +{
> +	local dir="$(__gitdir)"
> +	if [ -d "$dir" ]; then
> +		git --git-dir="$dir" diff-index --name-only HEAD
> +		return
> +	fi
> +}
> +
>  __git_heads ()
>  {
>  	local dir="$(__gitdir)"
> @@ -770,8 +789,6 @@ _git_apply ()
>  
>  _git_add ()
>  {
> -	__git_has_doubledash && return
> -
>  	case "$cur" in
>  	--*)
>  		__gitcomp "
> @@ -780,7 +797,8 @@ _git_add ()
>  			"
>  		return
>  	esac
> -	COMPREPLY=()
> +	# XXX should we care for --update and --all options ?
> +	__gitcomp_nl "$(__git_files -o -m)" "" "$cur" ""
>  }
>  
>  _git_archive ()
> @@ -930,15 +948,14 @@ _git_cherry_pick ()
>  
>  _git_clean ()
>  {
> -	__git_has_doubledash && return
> -
>  	case "$cur" in
>  	--*)
>  		__gitcomp "--dry-run --quiet"
>  		return
>  		;;
>  	esac
> -	COMPREPLY=()
> +	# TODO: check for -x option
> +	__gitcomp_nl "$(__git_files -o)" "" "$cur" ""
>  }
>  
>  _git_clone ()
> @@ -969,8 +986,6 @@ _git_clone ()
>  
>  _git_commit ()
>  {
> -	__git_has_doubledash && return
> -
>  	case "$cur" in
>  	--cleanup=*)
>  		__gitcomp "default strip verbatim whitespace
> @@ -998,7 +1013,7 @@ _git_commit ()
>  			"
>  		return
>  	esac
> -	COMPREPLY=()
> +	__gitcomp_nl "$(__git_commit_files)" "" "$cur" ""
>  }
>  
>  _git_describe ()
> @@ -1362,7 +1377,7 @@ _git_mv ()
>  		return
>  		;;
>  	esac
> -	COMPREPLY=()
> +	__gitcomp_nl "$(__git_files)" "" "$cur" ""
>  }
>  
>  _git_name_rev ()
> @@ -2068,15 +2083,13 @@ _git_revert ()
>  
>  _git_rm ()
>  {
> -	__git_has_doubledash && return
> -
>  	case "$cur" in
>  	--*)
>  		__gitcomp "--cached --dry-run --ignore-unmatch --quiet"
>  		return
>  		;;
>  	esac
> -	COMPREPLY=()
> +	__gitcomp_nl "$(__git_files)" "" "$cur" ""
>  }
>  
>  _git_shortlog ()