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

From: SZEDER Gábor <szeder@ira.uka.de>
Date: Sep 6, 2011, 17:43 UTC
Message-ID: <20110906174324.GA8211@goldbirke>
In-Reply-To: <1314740583-14567-1-git-send-email-panduwana@gmail.com>

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

Show 19 quoted lines

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

Show 25 quoted lines

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

Show 31 quoted lines

> +}
> +
>  __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.

Show 23 quoted lines

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

Show 55 quoted lines

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

Show 9 quoted lines

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

Independent change.

Show 8 quoted lines

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

Superfluous new line.

Best, Gábor