Re: [PATCH v2] Make use of git status when autocompleting git add, rm, checkout --, and reset HEAD

["SZEDER Gábor" <szeder@ira.uka.de>]

Hi,


On Wed, Aug 31, 2011 at 04:43:03AM +0700, Ron Panduwana wrote:
> Signed-off-by: Ron Panduwana <panduwana@gmail.com>

I agree with Junio that some explanation would be necessary here.

I'm not sure this change is in general a good idea.  I can imagine
that it would be useful for e.g. java projects, where you are forced
to have deep directory structures, but it will surely cause surprises
and confusion for users who trained their fingers to quickly enter
long paths with a few keystrokes and completion.  Furthermore, it
seems to cause considerable performance penalty, because it will fork
three processes and a subshell to connect them with a pipe for such a
"simple" thing as file name completion.  And one of those processes is
git status, which alone can take a while, especially for large
repositories, and as a side effect it refreshes the index (but that is
probably harmless).

> ---
> 
> On Fri, Aug 19, 2011 at 5:10 PM, Thomas Rast <trast@student.ethz.ch> wrote:
> > Some thoughts:
> >
> > * running git-status for . has some issues: it doesn't work in the
> >  case of
> >
> >    cd subdir
> >    git add ../some/file[TAB]
> >
> >  It's also inefficient if you are at the top level and
> >
> >    git add path/to/file/a/few/levels/down[TAB]
> >
> >  since it wouldn't actually have to look for untracked files in the
> >  entire repo.
> 
> Fixed by running git-status for $cur if $cur is a directory. Otherwise run on .

That won't do.  Imaginge you want to add ../some/file, and therefore
you do 'git add ../so<TAB>'.  In this case $cur will hold '../so',
which is not a directory (or at least not the directory you are
looking for).

> > * -uall is not required unless you are looking for untracked files.
> >   For the other commands you could speed up completion by passing
> >   -uno instead.
> 
> Fixed by adding second parameter to __git_files_having_status
> 
> 
>  contrib/completion/git-completion.bash |   84 ++++++++++++++++++++-----------
>  1 files changed, 54 insertions(+), 30 deletions(-)
> 
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 8648a36..9d44501 100755
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -1010,6 +1010,16 @@ __git_has_doubledash ()
>  	return 1
>  }
> 
> +# __git_files_having_status requires 2 arguments
> +__git_files_having_status ()
> +{
> +    local dir="."
> +    if [ -d "$cur" ]; then
> +        dir="$cur"
> +    fi

Please use tabs for indentation.

> +	echo "$(git status $2 -s "$dir" 2>/dev/null | egrep "^$1" | cut -c4-)"

Echoing a command substitution is unnecessary.

> +}
> +
>  __git_whitespacelist="nowarn warn error error-all fix"
> 
>  _git_am ()
> @@ -1058,17 +1068,17 @@ _git_apply ()
> 
>  _git_add ()
>  {
> -	__git_has_doubledash && return
> -
> -	case "$cur" in
> -	--*)
> -		__gitcomp "
> -			--interactive --refresh --patch --update --dry-run
> -			--ignore-errors --intent-to-add
> -			"
> -		return
> -	esac
> -	COMPREPLY=()
> +	if ! __git_has_doubledash; then
> +		case "$cur" in
> +		--*)
> +			__gitcomp "
> +				--interactive --refresh --patch --update --dry-run
> +				--ignore-errors --intent-to-add
> +				"
> +			return
> +		esac
> +	fi
> +	__gitcomp "$(__git_files_having_status "(.[MAU]|UD|\?\?)" -uall)"

Please do this and the others the other way around, as you did in
_git_checkout(), i.e.

	if __git_has_doubledash ; then
		__gitcomp "$(__git_files_having_status ...)"
		return
	fi

	case "$cur" in
	[...]

This will make the patch shorter and easier to review, it won't
increase the indentation unnecesarily, and it will be friendlier to
the users running 'git blame' in the future.

>  }
> 
>  _git_archive ()
> @@ -1171,7 +1181,12 @@ _git_bundle ()
> 
>  _git_checkout ()
>  {
> -	__git_has_doubledash && return
> +	if __git_has_doubledash; then
> +		if [[ ${words[2]} = "--" ]]; then
> +			__gitcomp "$(__git_files_having_status ".[MD]" -uno)"
> +		fi
> +		return
> +	fi
> 
>  	case "$cur" in
>  	--conflict=*)
> @@ -1469,7 +1484,7 @@ _git_help ()
>  	__gitcomp "$__git_all_commands $(__git_aliases)
>  		attributes cli core-tutorial cvs-migration
>  		diffcore gitk glossary hooks ignore modules
> -		namespaces repository-layout tutorial tutorial-2
> +		repository-layout tutorial tutorial-2

That's an independent change; I don't think you want to remove
namespaces here.

>  		workflows
>  		"
>  }
> @@ -2313,14 +2328,18 @@ _git_replace ()
> 
>  _git_reset ()
>  {
> -	__git_has_doubledash && return
> -
> -	case "$cur" in
> -	--*)
> -		__gitcomp "--merge --mixed --hard --soft --patch"
> +	if ! __git_has_doubledash; then
> +		case "$cur" in
> +		--*)
> +			__gitcomp "--merge --mixed --hard --soft --patch"
> +			return
> +			;;
> +		esac
> +	fi
> +	if [[ ${words[2]} = "HEAD" ]]; then
> +		__gitcomp "$(__git_files_having_status "[ADM]." -uno)"
>  		return
> -		;;
> -	esac
> +	fi
>  	__gitcomp "$(__git_refs)"
>  }
> 
> @@ -2337,15 +2356,20 @@ _git_revert ()
> 
>  _git_rm ()
>  {
> -	__git_has_doubledash && return
> -
> -	case "$cur" in
> -	--*)
> -		__gitcomp "--cached --dry-run --ignore-unmatch --quiet"
> -		return
> -		;;
> -	esac
> -	COMPREPLY=()
> +	if ! __git_has_doubledash; then
> +		case "$cur" in
> +		--*)
> +			__gitcomp "--cached --dry-run --ignore-unmatch --quiet"
> +			return
> +			;;
> +		esac
> +	fi
> +	# check if --cached was specified
> +	if [ "$(__git_find_on_cmdline "--cached")" ]; then
> +		COMPREPLY=()
> +	else
> +		__gitcomp "$(__git_files_having_status "(.D|DU|UA)" -uno)"

'git rm' can be used to delete any tracked files, but this limits
completion to only those files that are already deleted from the work
tree or are unmerged.

> +	fi
>  }
> 
>  _git_shortlog ()
> @@ -2640,7 +2664,6 @@ _git ()
>  			--exec-path
>  			--html-path
>  			--work-tree=
> -			--namespace=

Independent change.

>  			--help
>  			"
>  			;;
> @@ -2737,3 +2760,4 @@ else
>  		shopt "$@"
>  	}
>  fi
> +

Superfluous new line.



Best,
Gábor