# [PATCH v3] git-completion.bash: add support for path completion 4 messages from 2012-12-19 to 2012-12-19. Participants: Manlio Perillo, Junio C Hamano. Thread: https://gitlist.dev/t/32409 ## Manlio Perillo, 2012-12-19 18:58 Subject: [PATCH v3] git-completion.bash: add support for path completion Message-ID: <1355943496-5533-1-git-send-email-manlio.perillo@gmail.com> URL: https://gitlist.dev/e/1355943496-5533-1-git-send-email-manlio.perillo%40gmail.com ``` The git-completion.bash script did not implemented full, git aware, support to complete paths, for git commands that operate on files within the current working directory or the index. As an example: git add will suggest all files in the current working directory, including ignored files and files that have not been modified. Support path completion, for git commands where the non-option arguments always refer to paths within the current working directory or the index, as the follow: * the path completion for the "git mv", "git rm" and "git ls-tree" commands will suggest all cached files. * the path completion for the "git add" command will suggest all untracked and modified files. Ignored files are excluded. * the path completion for the "git clean" command will suggest all untracked files. Ignored files are excluded. * the path completion for the "git commit" command will suggest all files that have been modified from the HEAD. For all affected commands, completion will always stop at directory boundary. Only standard ignored files are excluded, using the --exclude-standard option of the ls-files command. Signed-off-by: Manlio Perillo --- 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. contrib/completion/git-completion.bash | 127 ++++++++++++++++++++++++++++----- 1 file changed, 111 insertions(+), 16 deletions(-) diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index 0b77eb1..fc12d0f 100644 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -13,6 +13,7 @@ # *) .git/remotes file names # *) git 'subcommands' # *) tree paths within 'ref:path/to/file' expressions +# *) file paths within current working directory and index # *) common --long-options # # To use these routines: @@ -233,6 +234,58 @@ __gitcomp_nl () COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$1" -- "${3-$cur}")) } +# 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. +__git_index_file_list_filter () +{ + local path + + while read -r path; do + path=${path#$1} + + case "$path" in + ?*/*) echo "${path%%/*}/" ;; + *) echo $path ;; + esac + done +} + +# __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 + fi +} + +# __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 + fi +} + __git_heads () { local dir="$(__gitdir)" @@ -430,6 +483,46 @@ __git_complete_revlist_file () } +# __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 +} + +# __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_" "" + ;; + esac +} + __git_complete_file () { __git_complete_revlist_file @@ -770,8 +863,6 @@ _git_apply () _git_add () { - __git_has_doubledash && return - case "$cur" in --*) __gitcomp " @@ -780,7 +871,9 @@ _git_add () " return esac - COMPREPLY=() + + # XXX should we check for --update and --all options ? + __git_complete_index_file "om" } _git_archive () @@ -930,15 +1023,15 @@ _git_cherry_pick () _git_clean () { - __git_has_doubledash && return - case "$cur" in --*) __gitcomp "--dry-run --quiet" return ;; esac - COMPREPLY=() + + # XXX should we check for -x option ? + __git_complete_index_file "o" } _git_clone () @@ -969,8 +1062,6 @@ _git_clone () _git_commit () { - __git_has_doubledash && return - case "$cur" in --cleanup=*) __gitcomp "default strip verbatim whitespace @@ -997,8 +1088,10 @@ _git_commit () --verbose --quiet --fixup= --squash= " return + ;; esac - COMPREPLY=() + + __git_complete_diff_index_file "HEAD" } _git_describe () @@ -1216,8 +1309,6 @@ _git_init () _git_ls_files () { - __git_has_doubledash && return - case "$cur" in --*) __gitcomp "--cached --deleted --modified --others --ignored @@ -1230,7 +1321,10 @@ _git_ls_files () return ;; esac - COMPREPLY=() + + # XXX ignore options like --modified and always suggest all cached + # files. + __git_complete_index_file "c" } _git_ls_remote () @@ -1362,7 +1456,9 @@ _git_mv () return ;; esac - COMPREPLY=() + + # XXX needs more work + __git_complete_index_file "c" } _git_name_rev () @@ -2068,15 +2164,14 @@ _git_revert () _git_rm () { - __git_has_doubledash && return - case "$cur" in --*) __gitcomp "--cached --dry-run --ignore-unmatch --quiet" return ;; esac - COMPREPLY=() + + __git_complete_index_file "c" } _git_shortlog () -- 1.8.1.rc1.18.g9db0d25 ``` ## Junio C Hamano, 2012-12-19 19:57 Subject: Re: [PATCH v3] git-completion.bash: add support for path completion Message-ID: <7vzk1995mx.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vzk1995mx.fsf%40alter.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 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. ``` ## Manlio Perillo, 2012-12-19 21:54 Subject: Re: [PATCH v3] git-completion.bash: add support for path completion Message-ID: <50D2377D.90100@gmail.com> URL: https://gitlist.dev/e/50D2377D.90100%40gmail.com In-Reply-To: <7vzk1995mx.fsf@alter.siamese.dyndns.org> ``` -----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 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----- ``` ## Manlio Perillo, 2012-12-19 23:43 Subject: Re: [PATCH v3] git-completion.bash: add support for path completion Message-ID: <50D2511D.3020705@gmail.com> URL: https://gitlist.dev/e/50D2511D.3020705%40gmail.com In-Reply-To: <7vzk1995mx.fsf@alter.siamese.dyndns.org> ``` -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Il 19/12/2012 20:57, Junio C Hamano ha scritto: > [...] I just found a serious bug with "git commit" path completion. When doing the first commit on an empty repository, completion will cause an error: $git commit -m init fatal: ambiguous argument 'HEAD': unknown revision or path not in the working tree. Use '--' to separate paths from revisions, like this: 'git [...] -- [...]' The problem is that for a newly created repository there is no HEAD. If HEAD does not exists, code must use ls-files instead of diff-index to get the completion list. Another change is to always call git commands with stderr redirected to /dev/null. By the way, this is also a bug of current bash completion code: $ git show does-not-exists:fatal: Not a valid object name does-not-exists I will write a patch (based on master) to fix this. > [...] Regards Manlio -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.10 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ iEYEARECAAYFAlDSUR0ACgkQscQJ24LbaUSkRwCfVKk9JhSD4sKDFm4heAkVbN0o KAAAn3paTXyUiFlfY/UBpnkwiARegLsE =7Q5s -----END PGP SIGNATURE----- ```