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

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 19, 2012, 19:57 UTC
Message-ID: <7vzk1995mx.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1355943496-5533-1-git-send-email-manlio.perillo@gmail.com>

[jch: again, adding area experts to Cc]

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

Show 6 quoted lines

> Changes from version 2:
>
> 	* Perl is no more used.
> 	* Fixed some coding style issues.
> 	* Refactorized code, to improve future path completion support for
> 	  the "git reset" command.

Thanks.  Will replace what was queued on 'pu'.

Show 5 quoted lines

> +# Process path list returned by "ls-files" and "diff-index --name-only"
> +# commands, in order to list only file names relative to a specified
> +# directory, and append a slash to directory names.
> +# It accepts 1 optional argument: a directory path.  The path must have
> +# a trailing slash.

The callsites that call this function, and the way the argument is used, do not make it look like it is an optional argument to me.

After reading later parts of the patch, I think the callers are wrong (see below) and you did intend to make "$1" optional.

Show 6 quoted lines

> +__git_index_file_list_filter ()
> +{
> +	local path
> +
> +	while read -r path; do
> +		path=${path#$1}

This will work correctly only if $1 does not have any shell metacharacter that removes prefix of $path that matches $1 as a pathname expansion pattern. As this file is for bash completion, using string-oriented Bash-isms like ${#1} (to get the length of the prefix) and ${path:$offset} (to get substring) are perfectly fine way to correct it.

Also, as $1 is optional, won't this barf if it is run under "set -u"?

Show 14 quoted lines

> +# __git_index_files accepts 1 or 2 arguments:
> +# 1: A string for file index status mode ("c", "m", "d", "o"), as
> +#    supported by git ls-files command.
> +# 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
> +		git --git-dir="$dir" ls-files --exclude-standard "-${1}" "${2}" |
> +			__git_index_file_list_filter ${2} | uniq

Even though everywhere else you seem to quote the variables with dq, but this ${2} is not written as "${2}", which looks odd. Deliberate?

If you wanted to call __git_index_file_list_filter without parameter when the caller did not give you the optional $2, then the above is not the way to write it. It would be ${2+"$2"}. The upstream of the pipe (ls-files) also is getting an empty string as the pathspec when $2 is omitted, and the call will break if this is run under "set -u".

Show 13 quoted lines

> +# __git_diff_index_files accepts 1 or 2 arguments:
> +# 1) The id of a tree object.
> +# 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_diff_index_files ()
> +{
> +	local dir="$(__gitdir)"
> +
> +	if [ -d "$dir" ]; then
> +		git --git-dir="$dir" diff-index --name-only "${1}" |
> +			__git_index_file_list_filter "${2}" | uniq

This one, when the optional $2 is absent, will call __git_index_file_list_filter with an empty string in its "$1". Intended, or is it also ${2+"$2"}?

Show 18 quoted lines

> +# __git_complete_index_file requires 1 argument: the file index
> +# status mode
> +__git_complete_index_file ()
> +{
> +	local pfx cur_="$cur"
> +
> +	case "$cur_" in
> +		?*/*)
> +			pfx="${cur_%/*}"
> +			cur_="${cur_##*/}"
> +			pfx="${pfx}/"
> +
> +			__gitcomp_nl "$(__git_index_files ${1} ${pfx})" "$pfx" "$cur_" ""
> +			;;
> +		*)
> +			__gitcomp_nl "$(__git_index_files ${1})" "" "$cur_" ""
> +			;;
> +	esac

Please dedent the case arms by one level, i.e.

	case $value in
        $pattern1)
        	action1
                ;;
                ...
		;;
	esac

Show 17 quoted lines

> +# __git_complete_diff_index_file requires 1 argument: the id of a tree
> +# object
> +__git_complete_diff_index_file ()
> +{
> +	local pfx cur_="$cur"
> +
> +	case "$cur_" in
> +		?*/*)
> +			pfx="${cur_%/*}"
> +			cur_="${cur_##*/}"
> +			pfx="${pfx}/"
> +
> +			__gitcomp_nl "$(__git_diff_index_files ${1} ${pfx})" "$pfx" "$cur_" ""
> +			;;
> +		*)
> +			__gitcomp_nl "$(__git_diff_index_files ${1})" "" "$cur_" ""
> +			;;

Unquoted $1 looks fishy, even if the only caller of this function always gives "HEAD" to it (in which case you can do without making it a parameter in the first place).

Unquoted ${pfx} given to __git_diff_index_files also looks fishy.