Improve dangerous/undefined PID expansion behavior

[lunixbochs]

Backstory

In Bash, % means something like 'last job backgrounded'. In upstream Fish, it means either "all backgrounded jobs" or "all processes owned by your user" if there are no processes backgrounded.

I usually run kill -9 % in Fish if I actually wanted to hard kill the last job backgrounded (useful if it wasn't responding). This actually matched for all jobs backgrounded, but I never noticed until today.

Even further, it turns out kill -9 % in Fish kills all processes owned by your user if you ever accidentally run it without backgrounding something first.

Go ahead, echo % to get a feel for how it works right now.

---

Change 1

Use Bash-like expansion for empty searches (when you just use a '%' by
itself).

'%' will now only match the last valid backgrounded process.
If there are no such processes, an expansion error will be generated.

'%' by itself would previously match either all backgrounded
processes, or failing that, all processes owned by your user. If you
ever tried to run kill -9 %, it would either kill all backgrounded
processes or all of your processes. I'm not sure why anyone would ever
want that to be a single keystroke away. You could almost typo it.

As a result of this change, fg %, bg %, kill %, etc will all now operate
on the last process touched by job control (or throw an expansion error).

---

Change 2

Don't run 'by-name' matches when the search term is numeric.

This prevents you from running a command like kill %1 and accidentally
killing a process named something like "1Command". Overloaded behavior
can be dangerous, and we probably shouldn't play fast and loose with
expansion characters that generate process IDs.

[zanchey]

That is terrific behaviour, not unlike Solaris' killall.

[zanchey]

I wonder if we should just error out on the expansion '%', or at least document it better.

[KamilaBorowska]

Indeed, it sounds dangerous. My suggestion would be to avoid current silly behavior of % (list all processes starting with something), but instead use globs as argument to add more flexibility. For example, current % would be %*, and current %cat would be %cat* (killing cat is dangerous if you run program like catchsegv), and possibly move %self to something like $fish_pid.

[zanchey]

fg % is a pretty common idiom in sh-land, but fg % is better written as fg.

[lunixbochs]

My biggest use case for solo % was kill -9 % as stated.

[KamilaBorowska]

@lunixbochs: Useless use of kill -9.

But aside of that – even normal kill would be dangerous (in fish), and sh users are confused, because % does way less dangerous thing in bash. I don't like % matching the latest process, you can use %1 for that after all – and it would introduce yet another special case. In my opinion, it should simply fail – there is no reason to continue, when it's not known what user wants, and suggest using %1 instead.

[lunixbochs]

What exactly does solo % do in bash that's less dangerous?

I found this article: http://web.mit.edu/gnu/doc/html/features_5.html#SEC33 which implies I was correct about the purpose of a standalone % in bash: "The symbols %% and %+ refer to the shell's notion of the current job, which is the last job stopped while it was in the foreground." unless there's some overloaded behavior of a standalone % in bash I haven't yet noticed. That's the behavior I was aiming to replicate with this pull request.

This is fish (with my patch):

~ vim
Job 1, 'vim' has stopped
~ kill %
~ kill -15 %
~ kill -9 %
fish: Job 1, 'vim' terminated by signal SIGKILL (Forced quit)
~ 

This is bash:

~ vim
[1]+  Stopped                 vim
~ kill %
[1]+  Stopped                 vim
~ kill -15 %
[1]+  Stopped                 vim
~ kill -9 %
[1]+  Stopped                 vim
~ 
[1]+  Killed: 9               vim

The entire point of my habit is to hard kill a badly behaving process that responds to ^Z but not ^D, ^C, ^\, and sometimes even kill 15, 2, 1. It's nice to theorize about the correct method of doing things, but in practice kill -9 on a non-critical process has never given me the slightest nasty side effect and saves 3+ steps. If you s/9/15/g in this discussion, the current behavior of % in fish is near as dangerous. This thread isn't a pull request on my shell habits. It's on fish.

[KamilaBorowska]

Yeah, sorry. I was referring to % being insecure in fish.

[zanchey]

OK, so this patch which leads to % (as a standalone expansion) expanding to the latest job iff it exists, and raising an error otherwise.

It sounds like @lunixbochs is opposed to entirely removing the expansion of % as a standalone token. In the absence of consensus, I think this is worth merging as the current behaviour is not ok.

[zanchey]

Merged with rebase:

To git@github.com:fish-shell/fish-shell.git
4ea92a9..f2a5237 master -> master

Thanks again!

[ridiculousfish]

We should update the docs to reflect this new behavior.

[zanchey]

The documentation is now less incorrect than it was - the current text reads

The following expansions are performed:

• If the string is the entire word self, the shell's PID is the result.
• Otherwise, if the string is the ID of a job, the result is the process group ID of the job.
• Otherwise, if any child processes match the specified string, their PIDs are the result of the expansion.
• Otherwise, if any processes owned by the user match the specified string, their PIDs are the result of the expansion.
• If none of these matches apply, an error is produced.

[ridiculousfish]

So it is. Good work!