Page MenuHomePhabricator

[Wilds] Shell complete files with spaces in them correctly

Authored by epriestley on Sep 25 2018, 12:53 PM.



Fixes T9116. Ref T13098. Fix shell completion handling of filenames with spaces in them. This is a big mess -- see T9116 for discussion and I'll walk through things below.

Test Plan

These all now seem to do the right thing:

arc we<tab> -> arc weld (+space)
arc weld fo<tab> -> arc weld foo\ bar (+space)
arc weld 'fo<tab> -> arc weld 'foo bar' (+space)
arc weld src<tab> -> arc weld src/ (no space)
arc weld src/w<tab> -> arc weld src/work (no space)
arc weld src/work<tab><tab> -> suggests "workflow/", "workingcopy/"
arc shell-complete --gen<tab> -> arc shell-complete --generate

These also work, which I think is nice-to-have:

arc WEL<tab> -> arc weld
arc shell-complete --GEN<tab> -> arc shell-complete --generate

Diff Detail

rARC Arcanist
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

epriestley requested review of this revision.Sep 25 2018, 12:53 PM
epriestley created this revision.
epriestley marked 7 inline comments as done.Sep 25 2018, 1:00 PM
epriestley added inline comments.

We now do need to do this ourselves (see below) so I threw this comment away.


I've replaced "FILE\n" with slightly more explicit magic, but it's still magic.


Instead of returning magic, we can just return nothing, so this is a slight simplification.


We're no longer using compgen -W ... (my goal in removing it is to reduce our reliance on Bash magic) so we now need to do our own matching in all cases. We were doing it in most cases anyway already so this isn't a large change.


We need to quote ${COMP_WORDS[@]} or foo\ bar gets passed to shell-complete as two separate arguments. Otherwise, I just wrapped this so it looks a little nicer.


See T9116 for more details on this. The new code is basically the same as the old code, except I've replaced -f with the more explicit -A file (both have the same meaning, -A file is just more explicit and easier to find in the documentation) and am replacing the result instead of turning it into an array here, so we can do the "turn it into an array" part later on in one place.

At least on my machine, I can't get the correct behavior from this command unless we run compgen from the script instead of from PHP. I'd prefer to just run compgen ... from PHP over returning "<compgen:file>" as a magic string, but couldn't get it to work.


This is probably the most important part in getting the behavior correct, and means:

$COMPREPLY = explode("\n", $RESULT);

The previous behavior was to explode on any whitespace, without regard for quotes, so a\ b would become two tokens.

IFS is "internal field separator", a piece of bash dark magic.

amckinley accepted this revision.Sep 25 2018, 7:56 PM

I definitely exhaustively tested this code and 100% understand it completely.

But seriously, from reading the comments on T9116, this looks like brain poison and I'm not prepared to forget all of college calculus just to understand the depths of how bash shell completion works. Your test plan looks pretty exhaustive, and I will bookmark this revision for a long plane flight.

This revision is now accepted and ready to land.Sep 25 2018, 7:56 PM

Yeah, I basically spent six hours prodding at Bash to ultimately add one line of local IFS=$'\n' and a pair of quotation marks. I feel no wiser for the experience.

This revision was automatically updated to reflect the committed changes.