Doing `set -U var` when a global named `var` exists can result in
confusing behavior. Try to limit the confusion by improving the warning
we write. Also, only write the warning if interactive.
Fixes#4267
The process_t pointer sent to setup_child_process can actually be 0
without it being failure, as that is what fish sends when `exec` is run
(in the case of INTERNAL_EXEC).
This was causing exec to fail.
There is no more race condition between parent and child with
regards to setting the process groups. Each child sets it for themselves
and then blocks indefinitely until the parent does what it needs to for
them (having waited for them to set their process groups). They are not
SIGCONT'd until the next process in the chain (if any) starts so that
that process can join their process group and open the pipes.
In the last commit, we introduced an indiscriminate if !EXTERNAL check
that unblocks a previously SIGSTOP'd command (if any) to allow the main
loop in exec_job to read from it without deadlocking (since builtins and
functions read directly from input as an optimization, sometimes).
Now only unblocking where a fork will not happen to ensure that if a
builtin ends up forking, that fork'd process is guaranteed to be able to
join the previous process' process group and access its output pipes.
Setting the process group in a fork/exec scenario is a well-documented
race condition in pretty much any job control mechanism [0] [1]. The
Wikipedia article contradicts the glibc article and suggests that the
best approach is for the parent to wait for the child to become the
process group leader, while the glibc article suggests that both should
make it so (which is what fish did previously). However, I'm running
into cases where tcsetpgrp is causing an EPERM error, which it isn't
documented to do except if the session id for the calling process
differs from that of the target process group (which is never the case
in fish since they are all part of the same session), which should cause
a _different_ error (SIGTTOU to be sent to all members of the calling
process' group).
In all cases, this is easily remedied by checking if the process group
in question is already in control of the terimnal. There's still the
off-chance that in the time between we check that and the time that the
command completes that situation may have changed, but the parent
process is supposed to ignore the result of this call if it errors out.
[0]: https://en.wikipedia.org/wiki/Process_group
[1]: https://www.gnu.org/software/libc/manual/html_node/Launching-Jobs.html
We were having child processes SIGSTOP themselves immediately after
setting their process group and before launching their intended targets,
but they were not necessarily stopped by the time the next command was
being executed (so the opposite of the original race condition where
they might have finished executing by the time the next command came
around), and as a result when we sent them SIGCONT, that could never
reach. Now using waitpid to synchronize the SIGSTOP/SIGCONT between the
two.
If we had a good, unnamed inter-process event/semaphore, we could use
that to have a child process conditionally stop itself if the next
command in the job chain hadn't yet been started / setup, but this is
probably a lot more straightforward and less-confusing, which isn't a
bad thing.
Additionally, there was a bug caused by the fact that the main exec_job
loop actually blocks to read from previous commands in the job if the
current command is a built-in that doesn't need to fork.
With this waitpid code, I was able to finally add the SIGSTOP code to
all the fork'd processes in the main exec_job loop without introducing
deadlocks; it turns out that they should be treated just like the main
EXTERNAL fork, but they tend to execute faster causing the same deadlock
described above to occur more readily.
The only thing I'm not sure about is whether we should execute
unblock_pid undconditionally for all !EXTERNAL commands. It makes more
sense to *only* do that if a blocking read were about to be done in the
main loop, otherwise the original race condition could still appear
(though it is probably mitigated by whatever duration the SIGSTOP lasted
for, even if it is SIGCONT'd before the next command tries to join the
process group).
I hadn't realized that the for loop is called multiple times for a given
"single input" (anything that doesn't include semicolons, etc) to fish,
and so processes were being blocked but blocked_pid was lost by the time
that the next job (which was reading from the last process in the
previous job) came around.
Now using a static variable to store the last blocked PID. AFAICT, this
main job control loop is always executed from the same process and
thread, so this shouldn't need to be wrapped in atomics/mutexes, etc.
This code should be more portable, and certainly cleaner. We are
currently always sending SIGCONT to the last process (if it was part of
a job chain) regardless of whether it called SIGSTOP on itself or not,
which should be fine.
Need to explore whether or not the other forks in src/exec.cpp need to
be SIGSTOP'd on run or only the one that we included in this patch.
I'm not sure if this happens on all platforms, but under WSL with the
existing codebase, processes in the job chain that pipe their
stdout/stderr to the next process in the job could terminate before the
next job started (on fast enough machines for quick enough jobs).
This caused issues like #4235 and possibly #3952, at least for external
commands. What was happening is that the first process was finishing
before the second process was fully set up. fish would then try to
assign (in both the child and the parent) the process group id belonging
to the process group leader to the new process; and if the first process
had already terminated, it would have ended its process group with it as
well before that happened.
I'm not sure if there was already a mechanism in place for ensuring that
a process remains running at least as long as it takes for the next
process in the chain to join its group, etc., but if that code was
there, it wasn't working in my test setup (WSL).
This patch definitely needs some review; I'm not sure how I should
handle non-external commands (and external commands executed via
posix_spawn). I don't know if they are affected by the race condition in
the first place, but when I tried to add the same "wait for next command
in chain to run before unblocking" that would cause black screens
requiring ctrl+c to bypass.
The "unblock previous command" code was originally run by the next child
to be forked, but was then moved to the shell code instead, making it
more-centrally located and less error-prone.
Note that additional headers may be required for the mmap system call on
other platforms.
This is the first step to implementing issue #4200 is to stop subclassing
env_var_t from wcstring. Not too surprisingly doing this identified
several places that were incorrectly treating env_var_t and wcstring as
interchangeable types. I'm not talking about those places that passed
an env_var_t instance to a function that takes a wcstring. I'm talking
about doing things like assigning the former to the latter type, relying
on the implicit conversion, and thus losing information.
We also rename `env_get_string()` to `env_get()` for symmetry with
`env_set()` and to make it clear the function does not return a string.
I decided this was just too useful not to include in our final fish 2.x
release. And since it does not modify any existing behavior it is safe
to include at this late date in the process of creating 2.7.
This makes command substitutions impose the same limit on the amount
of data they accept as the `read` builtin. It does not limit output of
external commands or builtins in other contexts.
Fixes#3822
This adds a new capability to the `set` command. It is similar to
running `set` with no other arguments but provides far more detail about
each variable. Such as whether it is set in each of the local, global,
and universal scopes. And the values in each scope. You can also ask for
specific variables to be shown.
Fixes#4265
Rewrite the `abbr` function to store each abbreviation in a separate
variable. This greatly improves the efficiency. For the common case
it is 5x faster. For pathological cases it is upwards of 100x faster.
Most people should be able to unconditionally define abbreviations in
their config.fish without a noticable slow down.
Fixes#4048
This takes a string that is then split upon like `string split`.
Unlike $IFS, the string is used as one piece, not a set of characters.
There is still a fallback to IFS if no delimiter is given, that
behaves exactly as before.
Fixes#4156.
This silences warnings from the compiler about ignoring return value of
‘ssize_t write(int, const void*, size_t)’, declared with attribute
warn_unused_result [-Wunused-result].
The class `completer_t` declares `complete_special_cd`, an unused method. I searched the entire source tree and this declaration seems to be the only instance of `complete_special_cd`. There is no definition or uses which likely means this is dead code.
PR #3691 made most calls to `signal_block()` and `signal_unblock()`
no-ops unless a magic env var is set when fish starts running. It's
been seven months since that change was made and no problems have been
reported. This finishes that work by removing those no-op function calls
and support for the magic env var in our next major release (which won't
happen till at least six months from now).
Introduce a -k/--keep-order switch to `complete` that can be used to
prevent fish from sorting/re-ordering the results provided by a completion
source.
In addition, this patch does so without doing away with deduplication
of completions by introducing a new unique_unsorted(..) helper function
that removes duplicates in-place without affecting the general order of
the vector/container.
Note that the code now uses a stable sort for completions, since the
behavior of is_naturally_less_than as of this patch now means that the
results are not necessarily _actually_ identical just because that function
repeatedly returns false for any ordering of any given two elements.
Fixes#361
This completes the refactoring of the `set` builtin. It also removes a
seemingly never used feature of the `set` command. It also eliminates all
the lint warnings about this module.
Fixes#4236
When reporting whether a boolean flag was seen report the actual flags
rather than a summary count. For example, if you have option spec `h/help`
and we parse `-h --help -h` don't do the equivalent of `set _flag_h 3`
do `set _flag_h -h --help -h`.
Partial fix for #4226
When executing a function, local-exported (`set -lx`) variables
previously were not accessible at all. This is weird e.g. in case of
aliases, since
```fish
set -lx PAGER cat
git something # which will call $PAGER
```
would not work if `git` were a function, even if that ends up calling
`command git`.
Now, we copy these variables, so functions get a local-exported copy.
```fish
function x
echo $var
set var wurst
echo $var
end
set -lx var banana
x # prints "banana" and "wurst"
echo $var # prints "banana"
```
One weirdness here is that, if a variable is both local and global,
the local-copy takes precedence:
```fish
set -gx var banana
set -lx var pineapple
echo $var # prints "pineapple"
x # from above, prints "pineapple" and "wurst"
echo $var # still prints "pineapple"
set -el var # deletes local version
echo $var # "banana" again
```
I don't think there is any more consistent way to handle this - the
local version is the one that is accessed first, so it should also be
written to first.
Global-exported variables are _not_ copied, instead they still offer
full read-write access.
When reporting whether a boolean flag was seen report the actual flags
rather than a summary count. For example, if you have option spec `h/help`
and we parse `-h --help -h` don't do the equivalent of `set _flag_h 3`
do `set _flag_h -h --help -h`.
Partial fix for #4226
In the rare case that we don't inherit $HOME _and_ can't read it from
/etc/passwd, this makes it so instead of triggering an assert() $HOME
is set to the empty list.
Tilde-expansion expands to nothing in such a case (and a string-empty
$HOME), `cd` errors out.
Fixes#4229.
Fish 2.6.0 introduced a regression that keeps setting
`fish_escape_delay_ms` as a uvar from working. This also fixes a related
problem: callbacks generated from the initial loading of universal vars
were not being acted on.
Fixes#4196
Also stop special-casing `printf` as if it were a syntactical keyword
with respect to handling `printf --help`. It should use the same pattern
as every other builtin command.
The code for reporting parser errors needs a major overhaul. But rather
than do that I'm going to add another hack in the hope that this doesn't
introduce yet another problem.
Fixes#4221
While updating the `history` function to use `argparse` I realized it is
useful to define an option that can be used in three ways. First by
using the short flag; e.g., `-n NNN`. Second by using the long flag;
e.g., `--max NNN`. Third, as an implicit int flag; e.g., `-NNN`. This
use case is now supported by a spec of the form `n#max`.
This implements support for numeric flags without an associated short or
long flag name. This pattern is used by many commands. For example `head
-3 /a/file` to emit the first three lines of the file.
Fixes#4214
We've needed a fishy way to parse flags and arguments given to scripts
and functions for a very long time. In particular a manner that provides
the same behavior implemented by builtin commands. The long term goal is
to support DocOpt. But since it is unclear when that will happen so this
implements a `argparse` command. So named as homage to the excellent
Python module of the same name.
Fixes#4190
This fixes a stupid bug in my previous commit to standardize on a new
`list_to_array_val()` function. This adds a unit test to keep this from
regressing.
