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

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

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

Il 05/01/2013 21:23, Marc Khouzam ha scritto:
> [...]
> Thanks for this, it improves the situation dramatically.
> I did further testing with your patch and found some less obvious
> issues.  I didn't debug the script myself as I'm not that familiar with
> it either, but I think the testcases below should help Manlio or
> someone else look into some regressions.
> 
> 1- Using .. or . breaks completion when after the '/':
> [...]
> 2- Maybe related to problem 1.  Using .. breaks completion in other ways:
> [...]
> 3- Also probably related to problems 1 and 2.  Using absolute paths behaves wierdly and 
> worse than before:

> In my opinion, the above three cases are regressions.
>

Yes.
I did not considered this use case, thanks!
I have never done something like this when working with Mercurial.

The problem is caused by the __git_index_file_list_filter function.

The job of this function is to stop path completion at directory
boundary (in order to avoid to suggest files in child
directories [1]), and to make paths relative to current directory.

Unfortunately, what it does is to simply remove the prefix string from
the path name; of course this will not work when the prefix is a non
canonical path name.

The solution is quite simple: canonicalize both the prefix path and each
of the path name returned by git.

This can be done using `readlink -f "$path"` or `realpath $path`, but
the problem is that it is inefficient to execute an external command for
each of the path returned by git; moreover readlink and realpath are not
POSIX and may not be supported on all platforms where git works (but I
found a portable implementation using pushd, popd, `pwd`,  `dirname`,
`basename` -- not very efficient).

IMHO, the best solution is to recode __git_index_file_list_filter in Perl.

Another possible solution (as suggested by Junio) is to use the
- --relative option; unfortunately this is only supported by
`git diff-index` and not by `git ls-files`.
And it will not solve the problem when using absolute path names (but
this case can be handled by leaving path completion to bash).

> 4- Completion choices include their entire path, which is not what bash does by default.  For example:
>> cd git/contrib
>> ls completion/git-<tab>
> git-completion.bash  git-completion.tcsh  git-completion.zsh   git-prompt.sh
> but
>> git rm completion/git-<tab>
> completion/git-completion.bash  completion/git-completion.tcsh  completion/git-completion.zsh   completion/git-prompt.sh
> notice the extra 'completion/' before each completion.

This is another thing I missed.
The problem is that only the current directory is removed from the path
names returned by git.

>  This can get pretty large when completing with 
> many directory prefixes.  The current tcsh completion has the same problem which I couldn't fix.  However, I am 
> not sure if it can be fixed for bash.
> 

The fix was very easy, and it seems to work.
The problem is in the __git_complete_index_files and
__git_complete_diff_index_files function.

When calling the __git_index_files and git_index_files, the "$cur"
variable should be used, instead of the computed "$pfx".

Not sure if this is correct.
I will post the patch, so you can test it.

> I personally don't think this is regression, just an slight annoyance.
> 
> 5- With this feature git-completion.bash will return directories as completions.  This is something
> git-completion.tcsh is not handling very well.  I will post a patch to fix that.
> 

I'll pass on this, thanks.

> Below are two suggestions that are in line with this effort but that are not regressions.
> 
> A) It would be nice if 
> git commit -a <TAB>
> also completed with untracked files
> 

I agree.
And there are other places when it may be useful to check the passed
options (see the comments).

But I think it is better to leave these issues for the future.
I will just add a comment to take note of this use case.

> B) Now that, for example, 'git rm' completion is smart about path completion 
> it would be nice to somehow not trigger bash default file completion
> when 'git rm' does not report any completions.  
> 

Not sure how this can be done, but it is possible and should be easy.

> For example, if I have a file called zz.tar.gz (which is an ignored file) 
> and I do 'git rm <tab>', I will get the proper list of files that can be
> removed by git, excluding zz.tar.gz.  But if I complete
> 'git rm zz.tar.<tab>' then the completion script will return nothing,
> since git cannot remove that ignored file, but we will then fall-back
> to the bash default completion, which will complete the file to zz.tar.gz.
> 
> Although there are some issues, I think this feature will greatly benefit the user
> and is worth the time needed to fix.
> 
> Thanks!
> 
> Marc


Thanks to you for the review!

Regards   Manlio


[1] this is what the Mercurial bash completion script does
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAlDpxNEACgkQscQJ24LbaURZEgCcD2Uc+7/W+RCrMk3j+vrd5w36
6ogAn1ou4pOarBSMywaQ3zQKdZmofyKA
=iU13
-----END PGP SIGNATURE-----