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

[Junio C Hamano <gitster@pobox.com>]

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

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

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

> 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 ()