[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.
> +__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"?
> +# __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".
> +# __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.
case $value in
$pattern1)
action1
;;
...
;;
esac
> +# __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.
--
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