New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove or improve process expansion (`%something`) #4230

Closed
faho opened this Issue Jul 20, 2017 · 31 comments

Comments

Projects
None yet
10 participants
@faho
Copy link
Member

faho commented Jul 20, 2017

As discussed on gitter by @floam, @krader1961 and me:

We should either improve process expansion so that it is usable, or remove it ("juice it or lose it").

Improving it would entail:

  • Add some way to silence the error when no matching process is found (maybe the same as with globs - only allow with for, set and count)

  • Add some way to use it as part of a word (this would be a big incompatible change)

Removing it would remove a bunch of code. Note that there is also "job expansion", which isn't quite the same thing. E.g. %1 expands not to a process with "1" in its name, but the job with id 1. If that is to be removed as well, this would free up "%" as a special character.

Because of the annoying error, process expansion remains basically unused in fish scripts with one exception: %self. This should be replaced with a variable (see #1202).

In case we consider the POSIX standard way to get a PID from a part of a process COMM to be too odious (as far as I can tell it requires scraping ps output), it would be possible to provide a pidof command instead.

Now fight discuss.

@faho faho added this to the fish-3.0 milestone Jul 20, 2017

@krader1961

This comment has been minimized.

Copy link
Contributor

krader1961 commented Jul 20, 2017

I feel strongly that process expansion should be removed. We discussed this elsewhere recently. Besides the annoying error that can't be suppressed it is downright dangerous. Type something like echo %l (ell, not one). What pids did you get? How do you know those are the pids you want?

Removing job expansion is a tougher question. Outside of the bg, fg, disown and kill commands is it used? The first three could be changed to just take a bare integer which is interpreted as a job number if there is a job with that ID; i.e., fg 1 rather than fg %1. That leaves just kill as the only command I can think of where I regularly use job expansion and a succinct notation is nice.

@krader1961

This comment has been minimized.

Copy link
Contributor

krader1961 commented Jul 20, 2017

See also issue #1074.

@krader1961

This comment has been minimized.

Copy link
Contributor

krader1961 commented Jul 23, 2017

See also issue #2164 which I'm closing as a duplicate of this issue.

@floam

This comment has been minimized.

Copy link
Member

floam commented Jul 23, 2017

How do you know those are the pids you want?

I was going to say "Of course, hit tab after %l, or at least tab after %, and look at the process list in the pager." But was surprised to see there aren't even completions. I'd have to say the feature clearly isn't implemented wonderfully. I'm leaning towards scrapping it.

@derekstavis

This comment has been minimized.

Copy link

derekstavis commented Aug 1, 2017

I like using % to find processes. I usually find myself doing a kill %nvim or something like this. The only thing I'd be happy to see is improved matching against arguments too. Example:

$ ps aux
...
derek    14616  0.0  0.4 288548 32400 ?        Sl   17:19   0:00 /usr/bin/python /usr/bin/chrome-gnome-shell --gapplication-service
...

$ echo %'python /usr/bin/chrome'
fish: Unable to find a process “python /usr/bin/chrome”
echo %'python /usr/bin/chrome'

Yeah, this sounds odd to me too, but it's really useful to disambiguate processes.


edit: killall was meant to be kill

@krader1961

This comment has been minimized.

Copy link
Contributor

krader1961 commented Aug 1, 2017

@derekstavis, The killall and pkill commands already accept process names. In fact, the BSD killall doesn't take PIDs so something like killall %nvim doesn't work on BSD/macOS. Too, they don't need fish's process expansion. So that seems like a weak argument to retain the feature.

@derekstavis

This comment has been minimized.

Copy link

derekstavis commented Aug 1, 2017

Ok, just updated the comment, I meant kill instead of killall.

@zanchey

This comment has been minimized.

Copy link
Member

zanchey commented Aug 9, 2017

pgrep is also a useful utility here.

@mqudsi

This comment has been minimized.

Copy link
Contributor

mqudsi commented Oct 15, 2017

I'm of the opinion that any time we can avoid needing to escape a character, we should do so. If removing this feature means we don't have to escape %, then that's a huge motivation to drop this.

In all cases, % substitution is definitely not discoverable and is very much "magic," going against the fish ethos.

@ridiculousfish

This comment has been minimized.

Copy link
Member

ridiculousfish commented Oct 18, 2017

I'm in favor too. I guess that a bare % is job expansion, not process expansion.

@nafg

This comment has been minimized.

Copy link

nafg commented Apr 2, 2018

So... what's the replacement for kill %1?

@ridiculousfish

This comment has been minimized.

Copy link
Member

ridiculousfish commented Apr 2, 2018

kill $last_pid

@nafg

This comment has been minimized.

Copy link

nafg commented Apr 2, 2018

And for kill %2?

@nafg

This comment has been minimized.

Copy link

nafg commented Apr 2, 2018

Also, does that mean job numbers aren't or weren't stable? Meaning if I have two backgrounded jobs, is 1 the first one or the last one (thus changing numbers every time a new job is added)?

@mqudsi

This comment has been minimized.

Copy link
Contributor

mqudsi commented Apr 2, 2018

The general syntax is kill (jobs -p 1) where the subshell prints the pid of the job number in question. fish-native builtins fg, bg, and wait should have support for expanding %n as equivalent to (jobs -p n).

The job numbers are stable while the job is running.

@nafg

This comment has been minimized.

Copy link

nafg commented Apr 3, 2018

Neither of those are working for me

@ridiculousfish

This comment has been minimized.

Copy link
Member

ridiculousfish commented Apr 3, 2018

Reopening, let's be sure we have a canonical story for how to do job management before 3.0

@ridiculousfish ridiculousfish reopened this Apr 3, 2018

@zanchey

This comment has been minimized.

Copy link
Member

zanchey commented Apr 4, 2018

My suggestion is to at least partially restore the % syntax. There are real-world uses of %self (eg virtualfish) and job expansion is still important.

@nafg

This comment has been minimized.

Copy link

nafg commented Apr 4, 2018

As I suggested on another issue, IMO a shell language is no different than any sort of API: breaking changes should go through a deprecation cycle instead of breaking code (interactive or not) without warning.

@mqudsi

This comment has been minimized.

Copy link
Contributor

mqudsi commented Apr 15, 2018

I've brought back job expansion, after a fashion, in master.

% is still a regular character, but it is parsed by jobs as prefacing a job id. Additionally, a shim has been placed for job-related functions (fg, bg, and wait) to support the same, plus a wrapper function for kill.

Feedback welcome.

@zanchey

This comment has been minimized.

Copy link
Member

zanchey commented Apr 15, 2018

Could you teach function about still accepting %self as well?

@mqudsi

This comment has been minimized.

Copy link
Contributor

mqudsi commented Apr 17, 2018

@zanchey I'm afraid I'm not sure what you mean. Can you give me an example?

@zanchey

This comment has been minimized.

Copy link
Member

zanchey commented Apr 17, 2018

Sure:

function __vfsupport_remove_env_on_deactivate_or_exit --on-process %self
@zanchey

This comment has been minimized.

Copy link
Member

zanchey commented Apr 29, 2018

See #4952.

@faho

This comment has been minimized.

Copy link
Member

faho commented May 9, 2018

Since #4952, all places where %self made any amount of sense should understand it again, so I believe this can be closed because the impact has been minimized.

@faho faho closed this May 9, 2018

@zanchey

This comment has been minimized.

Copy link
Member

zanchey commented May 10, 2018

I mean, three of the first ten results from https://github.com/search?l=fish&q=%25self&type=Code are fixed (and one is in a comment only), so I'm still not certain we've solved this.

@faho faho reopened this May 12, 2018

@sabinem

This comment has been minimized.

Copy link
Contributor

sabinem commented Jun 20, 2018

Can someone please summarize about what is planned for fish 3.0 regarding process expansion.

I studied that topic in bash and compared to fish and I am still confused on that topic:

  • In bash jobs %sl will show you all jobs that start with sl as commands (as printed out in the process table)
  • Example: if I start a job such as sleep 100 & it will be found if I look for it with jobs %eep
  • In bash this will only work with the jobs command, but not with ps: so in bash this could not even be called process expansions but it is rather job expansions?

Process expansions are something quite different in bash:
http://tldp.org/LDP/abs/html/process-sub.html
I can't say that I understand this, but I feel this is something entirely else.

I really struggle with process expansion and don't even understand them with the documentation, but I also don't quite get what you plan for it in fish 3.0. Why is fish departing so much in this regard from bash. Isn't that very confusing to users?

@ridiculousfish

This comment has been minimized.

Copy link
Member

ridiculousfish commented Jun 23, 2018

@sabinem clarifying:

  1. Process substitution is when the output of a command becomes arguments to another command. bash supports this via backtick and $(..), fish supports it via (...). This isn't changing.
  2. Process expansion is syntax like %firefox that expands to the pid of Firefox, even if Firefox is not a child process of the shell. (It does require that it be the same user). bash does NOT support this, and it is currently removed in fish 3.0.
  3. Job expansion is syntax like %1 that expands to the pid of a job, where a job is a child process of the shell that is subject to job control. Both bash and fish support this but in different ways.

Point 3 is what we're talking about. bash has the notion of "jobspecs", but the aren't part of the bash grammar per se, but are interpreted by the jobs command (and others). But fish has these as par of the shell syntax. So for example echo %1 will output %1 in bash but the pid of job 1 in fish.

The goal is to make fish more like bash in this case, where job expansion is a DSL interpreted by jobs. This opens the door for richer syntax since it no longer affects the rest of the shell.

Currently this is removed in fish 3.0. However we think this is too much of a compatibility break so we're likely to restore at least %self. This is still in fish 3.0 because it's unresolved.

@cben

This comment has been minimized.

Copy link
Contributor

cben commented Oct 8, 2018

%self is useful in a wide range of places, like $$ in other shells. You can't cover it by a DSL in a fixed set of commands. (Arguably popular uses of $$ for pseudo-uniqueness are misguided but nevertheless...)
Yet keeping %self is unsatisfactory. Its expansion suffers from unintuitive limitations (#1074).

Perhaps re-consider introducing a var for it (#1202).
Or (status --pid) (though that's unpleasantly verbose for people used to $$).

@faho

This comment has been minimized.

Copy link
Member

faho commented Oct 8, 2018

Perhaps re-consider introducing a var for it (#1202).

@cben: We have introduced a var for it - $fish_pid (and $last_pid for %last).

This is still open for backwards-compatibility concerns - currently all %-expansion is removed, only some commands have been retro-fitted to still understand some of it (job expansion and %self, IIRC).

We might want to readd %self as a first-class thing.

acomagu added a commit to acomagu/fish-async-prompt that referenced this issue Oct 17, 2018

#2 Remove %self
fish-shell/fish-shell#4230
Fish unsupported %something expansion and $pid notation
will be introduced, but not yet so use another way to
get self PID
@faho

This comment has been minimized.

Copy link
Member

faho commented Oct 21, 2018

#5249 has been merged, so this seems done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment