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

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 21, 2012, 17:59 UTC
Message-ID: <7vmwx71e2y.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1356108872-5881-1-git-send-email-manlio.perillo@gmail.com>

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

> +		case "$path" in
> +		?*/*) echo "${path%%/*}/" ;;
> +		*) echo $path ;;

$path unquoted???

Show 14 quoted lines

> +# __git_index_files accepts 1 or 2 arguments:
> +# 1: Options to pass to ls-files (required).
> +#    Supported options are --cached, --modified, --deleted, --others,
> +#    and --directory.
> +# 2: A directory path (optional).
> +#    If provided, only files within the specified directory are listed.
> +#    Sub directories are never recursed.  Path must have a trailing
> +#    slash.
> +__git_index_files ()
> +{
> +	local dir="$(__gitdir)"
> +
> +	if [ -d "$dir" ]; then
> +		# NOTE: $1 is not quoted in order to support multiple options

Good thinking to document this.  Thanks.

I take it that $1 never comes from the end user and it is known that it is correct to split them at $IFS? That is the way I read callers of this function in this patch, but I am just double-checking.

Show 7 quoted lines

> @@ -998,7 +1093,13 @@ _git_commit ()
>  			"
>  		return
>  	esac
> -	COMPREPLY=()
> +
> +	if git rev-parse --verify --quiet HEAD 1>/dev/null; then

s/1>/>/;

> +		__git_complete_diff_index_file "HEAD"

As this runs "git diff-index" without --cached,

The completion will give only for paths that have difference between the working tree and the HEAD. If the user has a bogus contents that was "git add"ed earlier, (i.e. the index is different from HEAD), then realizes the mistake and fixes it in the working tree with his editor to match "HEAD" (i.e. the working tree is the same as HEAD):

	git commit the-prefix-to-that-file<TAB>

to complete the filename will not give that file. I do not think it is a show-stopper, but it may puzzle the users when they encounter the situation.

I am wondering if reading from "git status --porcelain" might be a better alternative, or if it is too much trouble and slow things down to cover such a corner case.

Show 13 quoted lines

> @@ -1362,7 +1464,14 @@ _git_mv ()
>  		return
>  		;;
>  	esac
> -	COMPREPLY=()
> +
> +	if [ $cword -gt 2 ]; then
> +		# We need to show both cached and untracked files (including
> +		# empty directories) since this may not be the last argument.
> +		__git_complete_index_file "--cached --others --directory"
> +	else
> +		__git_complete_index_file "--cached"
> +	fi

Is $cword affected by the presense of "-f" in "git mv [-f] foo bar"? Just being curious.

Other than that, I do not see anything majorly wrong from the coding and semantics point of view in the patch. As to the interaction with the rest of the completion machinery, I'll leave the review to the area experts CC'ed and wait for their comments.

Thanks.