Page MenuHomePhabricator

[Bash-autocompletion] Add support for older bash version.
ClosedPublic

Authored by yamaguchi on Jul 1 2017, 2:46 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

yamaguchi created this revision.Jul 1 2017, 2:46 AM
yamaguchi retitled this revision from [Bash-autocompletion] Check if bash-completion is installed to [Bash-autocompletion] Check if bash-completion is installed or not.Jul 1 2017, 2:46 AM
yamaguchi edited the summary of this revision. (Show Details)
yamaguchi updated this revision to Diff 104988.Jul 1 2017, 4:44 AM

Update patch and diff.

yamaguchi retitled this revision from [Bash-autocompletion] Check if bash-completion is installed or not to [Bash-autocompletion] Add support for older bash version..Jul 1 2017, 4:50 AM
yamaguchi edited the summary of this revision. (Show Details)
ruiu added inline comments.Jul 1 2017, 5:17 AM
clang/utils/bash-autocomplete.sh
3 ↗(On Diff #104988)

Is the output of compgen -f the same as _filedir? If so, can you always use compgen -f?

7 ↗(On Diff #104988)

Let's be consistent on spacing. Please add a space after -f.

15 ↗(On Diff #104988)

Likewise, add a space after 2>.

58 ↗(On Diff #104988)

Is it okay to use compopt here? (I wonder how does this line work on macOS.)

yamaguchi added inline comments.Jul 1 2017, 5:24 AM
clang/utils/bash-autocomplete.sh
3 ↗(On Diff #104988)

_filedir is better than compgen -f, because it honors spaces in filenames.

58 ↗(On Diff #104988)

I thought it will just emit errors and proceed to next line. This part doesn't make a big difference in completion, so I thought we can just ignore it when compopt is not supported.

yamaguchi updated this revision to Diff 104992.Jul 1 2017, 5:26 AM
yamaguchi marked 2 inline comments as done.

Update diff.

ruiu added inline comments.Jul 1 2017, 5:30 AM
clang/utils/bash-autocomplete.sh
3 ↗(On Diff #104988)

You want to describe it in a comment: _filedir function provided by recent versions of bash-completion package is better than "compgen -f" because the former honors spaces in pathnames while the latter doesn't. So we use compgen only when _filedir is not provided.

17–18 ↗(On Diff #104992)

It's not clear what this code does. Can you explain?

yamaguchi updated this revision to Diff 104993.Jul 1 2017, 5:41 AM

Update diff.

ruiu added inline comments.Jul 1 2017, 5:58 AM
clang/utils/bash-autocomplete.sh
8–9 ↗(On Diff #104993)

If this works, you can probably make it shorter

_filedir 2> /dev/null || COMPREPLY=...
yamaguchi marked 4 inline comments as done.Jul 1 2017, 5:58 AM
yamaguchi updated this revision to Diff 104994.Jul 1 2017, 6:01 AM
yamaguchi marked an inline comment as done.

Update patch.

ruiu accepted this revision.Jul 1 2017, 6:08 AM

LGTM

This revision is now accepted and ready to land.Jul 1 2017, 6:08 AM
v.g.vassilev accepted this revision.Jul 1 2017, 8:55 AM

It looks like this works with bash 3.2. I've tested this on mac and it works well.

yamaguchi updated this revision to Diff 104996.Jul 1 2017, 9:23 AM

_get_comp_words_by_ref still depends on older bash-completion package, so delete this and set cword and cur manualy.

ruiu accepted this revision.Jul 1 2017, 9:49 AM

LGTM. Confirmed that it works as expected on my Mac which doesn't have the bash-autocomplete package.

clang/utils/bash-autocomplete.sh
19 ↗(On Diff #104996)

I think you want to quote $COMP_WORD with "" so that it works even if $COMP_WORD contains space characters.
20

teemperor accepted this revision.Jul 1 2017, 10:16 AM

LGTM, everything still works in the latest bash on Linux.

Also thanks for the reviews Rui!

This revision was automatically updated to reflect the committed changes.