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

[Manlio Perillo <manlio.perillo@gmail.com>]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Il 19/12/2012 20:57, Junio C Hamano ha scritto:
> [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.
> 

Thanks for the corrections.
As you can see, I'm not very expert in bash programming.
I will review the code to use proper escaping and correct optional
parameters handling, based on your review.

In this case, I incorrectly assumed that bash expands ${1} to an empty
string, in case no arguments are passed to the function.

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

Ok.

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

Ok.
Here I should use ${1-}.

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

No, I simply missed it.

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

Yes, this seems the better solution.

> [...]

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

No, it was not intended. But here it should probably be ${2-}.

One possible rule is:
    * ${n+"$n"} should be used by the _git_complete_xxx_file function;
    * ${n-} should be used by the _git_xxx_file functions

The alternative is for each function to use ${n-}, or {n+"$n"}.

But I'm not sure.  What is the best practice in bash for optional
parameters "propagation"?


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

I missed it.
Vim do not intent correctly this, and I forgot to dedent.


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

Currently it is always "HEAD", but in future it may contain an arbitrary
tree-ish (for my planned "git reset" path completion support).
This is the reason why in version 3 of the patch I added this new
__git_complete_diff_index_file function.

Better to quote it.

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

I missed it.


Thanks   Manlio
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAlDSN30ACgkQscQJ24LbaUQPRQCgkQaDyBeXjk5gMJsufGoe9FCe
yDkAn2M4d1xRYSkF6P0lQQmENlbYiCb8
=iml7
-----END PGP SIGNATURE-----