Page MenuHomePhabricator

[bash-completion] Fix tab separation on macOS
ClosedPublic

Authored by benlangmuir on May 23 2018, 11:28 AM.

Details

Summary

We have a regex that needs to match a tab character in the command output, but on macOS sed doesn't support '\t', causing it to split on the 't' character instead. Some options:

  • use perl -p -e 's/\t.*//', which does handle '\t'
  • put a literal tab character in the string passed to sed
  • use sed -e $'s/\t.*//', which will cause bash to expand the '\t' before passing it to sed

Diff Detail

Event Timeline

benlangmuir created this revision.May 23 2018, 11:28 AM
ruiu added a comment.May 23 2018, 11:45 AM

Even though Perl may be installed to 99.99% of machines that use this autocomplete script, using perl instead of sed is too much. If we could use perl, we'd have wrote this script entirely in perl in the first place. We shouldn't add a dependency to perl.

I wonder if you could replace \t with \0x09. At least it works on my machine which has GNU sed.

benlangmuir added a comment.EditedMay 23 2018, 11:49 AM

I wonder if you could replace \t with \0x09. At least it works on my machine which has GNU sed.

Unfortunately that doesn't work either on mac :-\

EDIT: nor does \x9, which I think is what you meant? Or maybe gnu sed support \0x as well as \x?

ruiu added a comment.May 23 2018, 12:04 PM

How about

sed -e "$(echo -e 's/\t.*//')"

?

sed -e "$(echo -e 's/\t.*//')"

Yep, that works! Is there a reason you prefer that over the more succinct $'s/\t.*//'?

ruiu added a comment.May 23 2018, 12:30 PM

Oh, I didn't know the existence of $''. Thank you for the suggestion! I wonder which version of bash started supporting that notation. Do you know if all recent versions of bash support it? Unfortunately $'' bash is very hard query to google...

ruiu added a comment.May 23 2018, 12:33 PM

Looks like $'' is there since bash 2.0 which should be pretty safe to use in the script. So feel free to use $'' instead of $(echo ...) in this patch.

benlangmuir edited the summary of this revision. (Show Details)

Thanks for looking up the supported bash version! Updated diff.

ruiu accepted this revision.May 23 2018, 1:35 PM

LGTM

This revision is now accepted and ready to land.May 23 2018, 1:35 PM