Commit graph

274 commits

Author SHA1 Message Date
Fabian Homborg
0a379135a4 exec_job: Only call getpgrp() once
This reduces the syscall count for `fish -c exit` from 651 to 566.

We don't attempt to *cache* the pgrp or anything, we just call it once
when we're about to execute the job to see if we are in foreground and
to assign it to the job, instead of once for checking foreground and
once to give it to the job.

Caching it with a simple `static` would get the count down to 480, but
it's possible for fish to have its pgroup changed.
2020-01-05 09:41:06 +01:00
ridiculousfish
62302ee172 Properly print leading comments and indentation in functions
Store the entire function declaration, not just its job list.
This allows us to extract the body of the function complete with any
leading comments and indents.

Fixes #5285
2020-01-03 14:40:28 -08:00
Johannes Altmanninger
3de95038b0 Make "time" a job prefix
In particular, this allows `true && time true`, or `true; and time true`,
and both `time not true` as well as `not time true` (like bash).

time is valid only as job _prefix_, so `true | time true` could call
`/bin/time` (same in bash)

See discussion in #6442
2020-01-03 01:07:49 -06:00
ridiculousfish
b691d3130e Don't give job IDs to block processes either
Extend the commit 8e17d29e04 to block processes, for example:

    begin ; stuff ; end

or if/while blocks as well.

Note there's an existing optimization where we do not create a job for a
block if it has no redirections.
2019-12-31 13:12:24 -08:00
Dan Zimmerman
8e17d29e04 Introduce the internal jobs for functions
This PR is aimed at improving how job ids are assigned. In particular,
previous to this commit, a job id would be consumed by functions (and
thus aliases). Since it's usual to use functions as command wrappers
this results in awkward job id assignments.

For example if the user is like me and just made the jump from vim -> neovim
then the user might create the following alias:
```
alias vim=nvim
```
Previous to this commit if the user ran `vim` after setting up this
alias, backgrounded (^Z) and ran `jobs` then the output might be:
```
Job	Group	State	Command
2	60267	stopped	nvim  $argv
```
If the user subsequently opened another vim (nvim) session, backgrounded
and ran jobs then they might see what follows:
```
Job	Group	State	Command
4	70542	stopped	nvim  $argv
2	60267	stopped	nvim  $argv
```
These job ids feel unnatural, especially when transitioning away from
e.g. bash where job ids are sequentially incremented (and aliases/functions
don't consume a job id).

See #6053 for more details.

As @ridiculousfish pointed out in
https://github.com/fish-shell/fish-shell/issues/6053#issuecomment-559899400,
we want to elide a job's job id if it corresponds to a single function in the
foreground. This translates to the following prerequisites:

- A job must correspond to a single process (i.e. the job continuation
    must be empty)
- A job must be in the foreground (i.e. `&` wasn't appended)
- The job's single process must resolve to a function invocation

If all of these conditions are true then we should mark a job as
"internal" and somehow remove it from consideration when any
infrastructure tries to interact with jobs / job ids.

I saw two paths to implement these requirements:

- At the time of job creation calculate whether or not a job is
  "internal" and use a separate list of job ids to track their ids.
  Additionally introduce a new flag denoting that a job is internal so
  that e.g. `jobs` doesn't list internal jobs
  - I started implementing this route but quickly realized I was
    computing the same information that would be computed later on (e.g.
    "is this job a single process" and "is this jobs statement a
    function"). Specifically I was computing data that populate_job_process
    would end up computing later anyway. Additionally this added some
    weird complexities to the job system (after the change there were two
    job id lists AND an additional flag that had to be taken into
    consideration)
- Once a function is about to be executed we release the current jobs
  job id if the prerequisites are satisfied (which at this point have
  been fully computed).
  - I opted for this solution since it seems cleaner. In this
  implementation "releasing a job id" is done by both calling
  `release_job_id` and by marking the internal job_id member variable to
  -1. The former operation allows subsequent child jobs to reuse that
  same job id (so e.g. the situation described in Motivation doesn't
  occur), and the latter ensures that no other job / job id
  infrastructure will interact with these jobs because valid jobs have
  positive job ids. The second operation causes job_id to become
  non-const which leads to the list of code changes outside of `exec.c`
  (i.e. a codemod from `job_t::job_id` -> `job_t::job_id()` and moving the
   old member variable to a non-const private `job_t::job_id_`)

Note: Its very possible I missed something and setting the job id to -1
will break some other infrastructure, please let me know if so!

I tried to run `make/ninja lint`, but a bunch of non-relevant issues
appeared (e.g. `fatal error: 'config.h' file not found`). I did
successfully clang-format (`git clang-format -f`) and run tests, though.
This PR closes #6053.
2019-12-31 10:08:50 -08:00
Fabian Homborg
033a832687
Merge pull request #6447 from neheb/clang2
Several more small clang-tidy cleanups
2019-12-31 18:47:24 +01:00
ridiculousfish
c963442999 Collapse io_data switch statements
Now that each io_data knows its source and target fd, we don't need to switch
on its types any more.
2019-12-29 15:51:22 -08:00
ridiculousfish
0af5608ce8 io_data_t to store the source_fd directly
Now that all io_data_ts know their source fd, just store it directly in
the base class. This will simplify some uses of io_data_t.
2019-12-29 15:14:08 -08:00
ridiculousfish
b784a0caa3 dup2_list_t::resolve_chain to stop returning maybe
It can no longer fail.
2019-12-29 14:49:05 -08:00
Rosen Penev
06cb0bbe9a
[clang-tidy] Add several references
Found with performance-unnecessary-value-param

Signed-off-by: Rosen Penev <rosenp@gmail.com>
2019-12-26 21:55:53 -08:00
Johannes Altmanninger
9f48fc6285 Fix status when function/block evaluation is cancelled
It looks like the last status already contains the signal that cancelled
execution.

Also make `fish -c something` always return the last exit status of
"something", instead of hardcoded 127 if exited or signalled.

Fixes #6444
2019-12-23 17:38:19 +01:00
ridiculousfish
c19407ab0f Default parser_t::eval()'s block type to top
This is the parameter value at every call site except one. Just make it the
default.
2019-12-22 16:27:03 -08:00
ridiculousfish
a59f35a378 Make block_type_t an enum class 2019-12-22 15:37:14 -08:00
ridiculousfish
97dd5ece26 Remove redirection_is_to_real_file
This was previously required so that, if there was a redirection to a
file, we would fork a process to create the file even if there was no
output. For example `echo -n >/tmp/file.txt` would have to create
file.txt even though it would be empty.

However now we open the file before fork, so we no longer need special
logic around this.
2019-12-20 14:40:57 -08:00
ridiculousfish
0531c02ce4 Remove 'user_supplied' flag for io_fd_t
user_supplied was used to distinguish IO redirections which were
explicit, vs those that came about through "transmogrphication." But
transmogrification is no more. Remove the flag.
2019-12-19 14:14:23 -08:00
ridiculousfish
e52433d6fe Fix gcc warnings harder 2019-12-17 19:51:00 -08:00
ridiculousfish
7ab373fbbd Fix some gcc warnings 2019-12-17 19:22:43 -08:00
ridiculousfish
d4daa28690 Correctly set the exit status in block and function processes
Previously, if the user control-C'd out of a process, we would set a
bogus exit status in the process, but it was difficult to observe this
because we would be cancelling anyways. But set it properly.
2019-12-17 18:19:38 -08:00
ridiculousfish
09f8e05b80 Clean up the return type of parser_t::eval
parser_t::eval indicates whether there was a parse error. It can be
easily confused with the status of the execution. Use a real type to
make it more clear.
2019-12-17 16:16:43 -08:00
ridiculousfish
9be77d1f9c Correctly handle "self fd redirections"
This adds a test for the obscure case where an fd is redirected to
itself. This is tricky because the dup2 will not clear the CLO_EXEC bit.
So do it manually; also posix_spawn can't be used in this case.
2019-12-13 16:51:49 -08:00
ridiculousfish
1f83fb47ce Finish the IO cleanup.
Remove some dead code and add missing dtors.
2019-12-12 17:47:08 -08:00
ridiculousfish
5cd9de1049 Eliminate resolve_file_redirections_to_fds
Now that file redirections store FDs and not paths, this function has
nothing to do and can be removed.
2019-12-12 17:34:44 -08:00
ridiculousfish
33aff87c10 Switch io_file_t to store an fd, not a path
Prior to this fix, a file redirection was turned into an io_file_t. This is
annoying because every place where we want to apply the redirection, we
might fail due to open() failing. Switch to opening the file at the point
we resolve the redirection spec. This will simplify a lot of code.
2019-12-12 17:34:40 -08:00
ridiculousfish
af473d4d0c Introduce redirection_spec_t
Prior to this change, a process after it has been constructed by
parse_execution, but before it is executed, was given a list of
io_data_t redirections. The problem is that redirections have a
sensitive ownership policy because they hold onto fds. This made it
rather hard to reason about fd lifetime.

Change these to redirection_spec_t. This is a textual description
of a redirection after expansion. It does not represent an open file and
so its lifetime is no longer important.

This enables files to be held only on the stack, and are no longer owned
by a process of indeterminate lifetime.
2019-12-12 16:44:24 -08:00
ridiculousfish
be685faeb8 Clean up how pipe fd avoidance works
fish has to ensure that the pipes it creates do not conflict with any
explicit fds named in redirections. Switch this code to using
autoclose_fd_t to make the ownership logic more explicit, and also
introduce fd_set_t to reduce the dependence on io_chain_t.
2019-12-12 14:58:18 -08:00
ridiculousfish
c0b3be9fb4 Stop storing block_io in job_t
Prior to this fix, a job would hold onto any IO redirections from its
parent. For example:

    begin
        echo a
    end < file.txt

The "echo a" job would hold a reference to the I/O redirection.
The problem is that jobs then extend the life of pipes until the job is
cleaned up. This can prevent pipes from closing, leading to hangs.

Fix this by not storing the block IO; this ensures that jobs do not
prolong the life of pipes.

Fixes #6397
2019-12-11 16:34:20 -08:00
ridiculousfish
970288c854 Migrate the read limit into parser_t::libdata
It is more natural here than "on the last bufferfill."
2019-12-11 11:50:52 -08:00
ridiculousfish
0b1af1ace4 Correct the use of the constructed pointer in job lineage
This was always being set to a different pointer. Ensure the root job
shares its constructed pointer with its children.
2019-12-10 18:32:56 -08:00
ridiculousfish
f136d634eb Collapse a job's "parent stuff" into a new type job_lineage_t
Currently a job needs to know three things about its "parents:"

1. Any IO redirections for the block or function containing this job
2. The pgid for the parent job
3. Whether the parent job has been fully constructed (to defer self-disown)

These are all tracked in somewhat separate awkward ways. Collapse them
into a single new type job_lineage_t.
2019-12-08 15:03:07 -08:00
ridiculousfish
2ec8cd3bca Remove a dead variable 2019-12-07 11:28:22 -08:00
ridiculousfish
f575c55f5b Migrate the logic to make empty functions succeed into the performer
This is a more natural place for this logic.
2019-12-07 11:06:54 -08:00
ridiculousfish
a7f6fd22d4 Factor block and fish function execution into a "performer" std::function
In preparation for concurrent execution, invert the control of function and
block execution. Allow a process to return an std::function that performs the
the execution. This can be run on either the main or a background thread
(eventually).
2019-12-07 10:29:06 -08:00
Rosen Penev
7d1cc992e5 [clang-tidy] Simplify boolean expressions
Found with readability-simplify-boolean-expr

Signed-off-by: Rosen Penev <rosenp@gmail.com>
2019-11-29 23:46:51 -08:00
Rosen Penev
4087b2ee15 [clang-tidy] Use bool literals
Found with modernize-use-bool-literals

Signed-off-by: Rosen Penev <rosenp@gmail.com>
2019-11-29 23:46:50 -08:00
Rosen Penev
1055ff321c [clang-tidy] Replace NULL with nullptr
Found with modernize-use-nullptr

Signed-off-by: Rosen Penev <rosenp@gmail.com>
2019-11-25 14:23:03 -08:00
Rosen Penev
0dfa7421f3 [clang-tidy] Convert C casts to C++ ones
Found with google-readability-casting

Signed-off-by: Rosen Penev <rosenp@gmail.com>
2019-11-25 14:17:49 -08:00
Johannes Altmanninger
7d5b44e828 Support FOO=bar syntax for passing variables to individual commands
This adds initial support for statements with prefixed variable assignments.
Statments like this are supported:

a=1 b=$a echo $b        # outputs 1

Just like in other shells, the left-hand side of each assignment must
be a valid variable identifier (no quoting/escaping).  Array indexing
(PATH[1]=/bin ls $PATH) is *not* yet supported, but can be added fairly
easily.

The right hand side may be any valid string token, like a command
substitution, or a brace expansion.

Since `a=* foo` is equivalent to `begin set -lx a *; foo; end`,
the assignment, like `set`, uses nullglob behavior, e.g. below command
can safely be used to check if a directory is empty.

x=/nothing/{,.}* test (count $x) -eq 0

Generic file completion is done after the equal sign, so for example
pressing tab after something like `HOME=/` completes files in the
root directory
Subcommand completion works, so something like
`GIT_DIR=repo.git and command git ` correctly calls git completions
(but the git completion does not use the variable as of now).

The variable assignment is highlighted like an argument.

Closes #6048
2019-11-25 09:20:51 +01:00
ridiculousfish
a74fc7ef6d Remove the wait_for_threads_to_die parameter to execute_fork
This is always set to false so we can get rid of it.
2019-11-23 12:36:44 -08:00
ridiculousfish
64ce1088fd Make function_prepare_environment take argv instead of the process
This will help concurrent execution.
2019-11-14 17:58:39 -08:00
ridiculousfish
b51edcfcac Simplify function_info_t and function_data_t
Work towards cleaning up function definition. Migrate inherit_vars into
props and capture their values at the point of definition.
2019-11-12 09:53:10 -08:00
ridiculousfish
b0cf94e3ba Refactor function_prepare_environment
Migrate it into exec.cpp to reduce the complexity of
exec_block_or_func_process.
2019-11-10 14:46:21 -08:00
ridiculousfish
ac2eed2ffa Make io_chain_t store const io_data_t
This will make it easier to reason about with concurrent execution.
2019-11-10 14:00:30 -08:00
ridiculousfish
521d0e84f5 Remove non-const get_io_for_fd
These could be made unused.
2019-11-10 13:41:12 -08:00
ridiculousfish
2555ecf757 Remove the forbidden function stack
Detect forbidden functions directly from the associated block_t.
Also unify where we do stack overflow detection.
2019-11-10 12:36:46 -08:00
ridiculousfish
a7f1d2c0c7 Add support for fish_trace variable to trace execution
This adds support for `fish_trace`, a new variable intended to serve the
same purpose as `set -x` as in bash. Setting this variable to anything
non-empty causes execution to be traced. In the future we may give more
specific meaning to the value of the variable.

The user's prompt is not traced unless you run it explicitly. Events are
also not traced because it is noisy; however autoloading is.

Fixes #3427
2019-11-02 14:40:57 -07:00
ridiculousfish
cc1c973025 Remove job_flags as an enum, just use a struct
This removes an over-complicated flag implementation, replacing it with
just a plain struct.
2019-10-15 14:40:58 -07:00
ridiculousfish
82eca4bc86 Run clang-format on all files
The main change here is to reorder headers.
2019-10-13 15:50:48 -07:00
Aaron Gyes
22811ebcf6 Remove unused macros
Most just left behind after actual code was moved into different files.
2019-09-18 01:31:03 -07:00
ridiculousfish
73bf60754c Don't assign jobs to fish's pgrp if there's no external process
Fixes #6011
2019-08-03 14:43:55 -07:00
ridiculousfish
a33f0eb636 Clean up some logic around when process exit events are sent 2019-07-28 14:36:57 -07:00