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

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

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

Il 18/12/2012 18:53, Junio C Hamano ha scritto:
> [jch: cc'ed git-completion experts to review implementation details]
> 
> Manlio Perillo <manlio.perillo@gmail.com> writes:
> 
>> The git-completion.bash script did not implemented full, git aware,
>> support for completion, for git commands that operate on files within
>> the current working directory or the index.
>>
>> For these commands, only long options completion was available.
> 
> I find the "long options completion" is a misleading phrase.  It
> sounds as if you changed the current completion that does not
> complete "git commit -<TAB>" but does "git commit --<TAB>" to
> complete the short options (e.g. "git commit -c"), but I do not
> think that is the topic of this patch.
> 

It does not sound misleading to me.
I'm saying that the git-completion.bash script only implemented
completion for long options, but not for file names in the current
working directory.

Do you think I should rewrite the subject and the log message introduction?

As an example, something like this in the subject:

  git-completion.bash: improve some git commands completion

and in the message:

  The git-completion.bash script did not implemented full, git aware,
  support for completion, for git commands that operate on files within
  the current working directory or the index.

  As an example:

 ...

I'm still not fully satisfied with it, however.
It still requires reading the full message to understand the changes
implemented.

>> As an example:
>>
>> 	git add <TAB>
>>
>> will suggest all files in the current working directory, including
>> ignored files and files that have not been modified.
>>
>> Full support for completion is now implemented, for git commands where
> 
> s/Full.*implemented/Support more comprehensive completion/; or
> something, talking in the imperative mood (i.e. as if you are giving
> the order to the codebase to do something).
> 

Ok.

>> 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.
> 
> I thought you dropped "git mv" in this round.
> 

Well, no.
But the current implementation should not cause problems.
Also note that I added support for ls-files, too.

There are some XXX marks in the code, but I think that the changes
always improve the old behaviour.

> [...]
>> 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.
> 
> I read "always stop at directory boundary" to mean that
> 
> 	git cmd t<TAB>
> 
> will give us "t/ tag.c" (assuming there is a new or modified file in
> t/ and tag.c is the only modified file at the root level that begins
> with "t") and then
> 
> 	git cmd t/<TAB>
> 
> will likewise show the files and top-level subdirectories within t/
> directory.  That would be great.
> 

Yes, this is how it works, bugs excluded (I'm not a bash/perl expert).


> [...]
>> +# Perl filter used to process path list returned by ls-files and
>> +# diff-index --name-only commands, in order to list file names
>> +# relative to a specified directory, and append a slash to directory
>> +# names.
>> +# The script expects the prefix path in the "pfx" environ variable.
>> +# The output must be processed with the uniq filter, to remove
>> +# duplicate directories.
>> +# XXX remove duplicates in the Perl script ?
> 
> Surely, that will remove one fork/exec with pipeline.  I am not sure
> what the performance implication of using Perl here, but because we
> do not have to stick to POSIX shell in this file, the completion
> experts would be able to help rewriting this logic as a pure bash
> script.
> 

Ok. I'll wait for a review from git-completion experts.

Note that the performance is the reason why I suggested, in a previous
email, that git should have some more options to format data in custom ways.
As an example, there is no way to tell ls-files to not recurse
directories, and there is no way to also get the file type.

A --no-recurse option, and a change in the code to make, as an example

  git ls-files --stage --modified

to honor the --modified option,  will probably make it possible to use a
simple sed filter (there is still the problem that, unlike ls-tree,
ls-files shows the complete file path).

> [...]
>> +__git_files ()
>> +{
>> +	local dir="$(__gitdir)" flags="-${1}"
>> +
>> +	if [ -d "$dir" ]; then
>> +		git --git-dir="$dir" ls-files --exclude-standard ${flags} ${pfx} \
>> +			| pfx=$2 perl -ne "${__git_index_file_list_filter}" \
>> +			| uniq
> 
> This is purely a style thing (note that style suggestions are not
> optional), but
> 
>         the data source command |
>         a filter command |
>         another filter command
> 
> is easier to read and can be spelled without the backslash.  The
> same comment applies to git-commit-files as well.
> 

I agree.

But I was copying the style currently used in the script
(see the __git_complete_revlist_file function).

Note that I plan to do a small code refactorization, since I need the
ls-tree support code from __git_complete_revlist_file function for a
future change. I can fix these style issues in that patch.

I plan to improve completion support for checkout and reset commands,
too (currently only the commit/tree-ish argument is autocompleted, but
not the path).


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

iEYEARECAAYFAlDQvDIACgkQscQJ24LbaUTT9wCgh6jtbhdQ7GNIkqCq34QjXdIs
w9QAnjz2VjPFm4n2ICkcWWEsqWDWM+Hm
=8/Ad
-----END PGP SIGNATURE-----