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

> [jch: again, adding area experts to Cc]
> 
> Manlio Perillo <manlio.perillo@gmail.com> writes:
> 
>> 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'.
> 
>> +# 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.
> 

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

>> +# __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?
> 

>> +# __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"}?

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

> [...]
>> +# __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).
>