-----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@xxxxxxxxx> 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-----
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html