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.peri...@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

>> 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).


>> 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
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to