The [disambiguate flag](https://sw.kovidgoyal.net/kitty/keyboard-protocol/#disambiguate) means that:
> In particular, ctrl+c will no longer generate the SIGINT signal,
> but instead be delivered as a CSI u escape code.
so cancellation only works while we turn off disambiguation.
Today we turn it off while running external commands that want to
claim the TTY. Also we do it (only as a workaround for this issue)
while expanding wildcards or while running builtin wait.
However there are other cases where we don't have a workaround,
like in trivial infinite loops or when opening a fifo.
Before we run "while true; end", we put the terminal back in ICANON
mode. This means it's line-buffered, so we won't be able to detect
if the user pressed ctrl-c.
Commit 8164855b7 (Disable terminal protocols throughout evaluation,
2024-04-02) had the right solution: simply disable terminal protocols
whenever we do computations that might take a long time.
eval_node() covers most of that; there are a few others.
As pointed out in #10494, the logic was fairly unsophisticated then:
it toggled terminal protocols many times. The fix in 29f2da8d1
(Toggle terminal protocols lazily, 2024-05-16) went to the extreme
other end of only toggling protocols when absolutely necessary.
Back out part of that commit by toggling in eval_node() again,
fixing cancellation. Fortunately, we can keep most of the benefits
of the lazy approach from 29f2da8d1: we toggle only 2 times instead
of 8 times for an empty prompt.
There are only two places left where we call signal_check_cancel()
without necessarily disabling the disambiguate flag
1. open_cloexec() we assume that the files we open outside eval_node()
are never blocking fifos.
2. fire_delayed(). Judging by commit history, this check is not
relevant for interactive sessions; we'll soon end up calling
eval_node() anyway.
In future, we can leave bracketed paste, modifyOtherKeys and
application keypad mode turned on again, until we actually run an
external command. We really only want to turn off the disambiguate
flag.
Since this is approach is overly complex, I plan to go with either
of these two alternatives in future:
- extend the kitty keyboard protocol to optionally support VINTR,
VSTOP and friends. Then we can drop most of these changes.
- poll stdin for ctrl-c. This promises a great simplification,
because it implies that terminal ownership (term_steal/term_donate)
will be perfectly synced with us enabling kitty keyboard protocol.
This is because polling requires us to turn off ICANON.
I started working on this change; I'm convinced it must work,
but it's not finished yet. Note that this will also want to
add stdin polling to builtin wait.
Closes#10864
The previous approach of "treat this field as an `Option<NonZeroU32>` and
remember to check `p.has_pid()` before accessing it" was a mix of C++ and rust
conventions and led to some bugs or incorrect behaviors.
* `jobs -p` would previously print both the (correct) external pid and the
(incorrect) internal value of `0` if a backgrounded command contained a
fish function (e.g. `function foo; end; cat | foo &; jobs`)
* Updating/calculating job cpu time and usage was incorrectly including all of
fish's cpu usage/time for each function/builtin member of the job pipeline.
Closes#10832
Commit ba67d20b7 (Refresh TTY timestamps after nextd/prevd, 2024-10-13)
wasn't quite right because it also needs to fix it for arbitrary commands.
While at it, do this only when needed:
1. It seems to be only relevant for multiline prompts.
Note that we can wait until after evaluation to check if the prompt is
multiline, because repaint events go through the queue, see 5ba21cd29
(Send repaint requests through the input queue again, 2024-04-19).
2. When the binding doesn't execute any external command, we probably don't
need to fix up whatever the user printed. If they actually wanted to show
output and print another prompt, they should currently use "__fish_echo",
to properly support multiline prompts. Bindings should produce no other
output. What distinguishes external programs is that they can trigger this
issue even if they don't produce any output that remains visible in fish,
namely by using the terminal's alternate screen.
Would be nice if we could get rid of __fish_echo; I'm not yet sure how.
Fixes#10800
Completion on ": {*," used to work but nowadays our attempt to wildcard-expand
it fails with a syntax error and we do nothing. This behavior probably only
makes sense for the overflow case, so do that.
Make fish-printf no longer depend on the widestring crate, as other clients
won't use it; instead this is an optional feature.
Make format strings a generic type, so that both narrow and wide strings can
serve. This removes a lot of the complexity around converting from narrow to
wide.
Add a README.md to this crate.
Since we have a mix of both 0-based and 1-based line numbers in the code base,
we can now distinguish between them by type alone. Also stop using 0 as a
placeholder value for "no line number available" except in explicit helper
functions such as `get_lineno_for_display()`.
The "principal" parser is the one and only today; in the future we hope to
have multiple parsers to execute fish script in parallel.
Having a globally accessible "principle" parser is suspicious; now we can
get rid of it.
The "principal" environment stack was the one that was associated with the
"principal" parser and would dispatch changes like to TZ, etc.
This was always very suspicious, as a global; now we can remove it.
This doesn't pull its weight. Block size is not a particularly big
problem,
and this both complicates the code a bit and would arbitrarily cause issues
if a fish script exceeded 65k lines.
This reverts commit edd6533a14a4c7beab18b054d0f970d04a2607aa.
This doesn't have any effect on the size of the struct (due to alignment
requirements and padding) but reduces the complexity by turning
Block::wants_pop_env into an emergent property dependent on the type rather than
something we have to manually manage.
We only increment it and check if it's non-zero, we never decrement or check the
actual count. As such, change it to a bool and bring the size of `Block` down
from 32 to 24 bytes.
We almost never access any of this and having it stored directly in the `Block`
struct increases its size (reducing how many we can fit in L1 and L2, and
increasing memory copy traffic).
Gets rid of BlockData::None so we can avoid allocating a Box at all when we have
no data (at the cost of yet-another-wrapper-type), which is the usual case.
This has a few advantages,
* We now statically assert that all fields used by a particular block type are
correctly initialized (i.e. you can't assign the function name but forget to
assign its arguments),
* Conversely, we can match directly on `BlockData` and be guaranteed that the
fields we want to access are initialized and present,
* We reduce the number of assertions, effectively "unwrapping" only once based
off the block type instead of each time we try to access a conditional field,
* We reduce the size of the `Block` struct by coalescing fields that cannot
co-exist, bringing it down from 104 bytes to 88 bytes.
It would be nice to make all of `Block` itself an enum, but it currently
requires `Copy` and we take advantage of that to copy it around everywhere.
Putting these fields directly in `Block` directly would mean a lot more memory
traffic just checking block types.
There's no need for two separate block types when one is merely a variant of the
other. This may have been required under C++ but thanks to sum types (rust's
enums) we don't need to do that any more.
`Parser` is a single-threaded `!Send`, `!Sync` type and does not need to use
`Arc` for anything. We were using it because that's all we had for the parser's
`EnvStack`, but though that is *technically* protected internally by a mutex
(shared with global EnvStack), there's nothing to say that other parsers with a
narrower scope/lifetime on other threads will be necessarily using the same
backing mutex.
We can safely marshal the existing `Arc<EnvStack>` we get from
`environment::principal()` into an `Rc<EnvStack>` since the underlying reference
is always valid. To prove this point, we could have PRINCIPAL_STACK be a static
`EnvStack` and have `environment::principal()` use `Arc::from_raw()` to turn
that into an `Arc<EnvStack>`, but there's no need to factorize this process.
By inverting the order of storage, we can use an `OnceCell`/`unsync::Lazy`
inside the Send/Sync `MainThread<T>` and remove the need for a lock altogether.
This allows running `set` without triggering any event handlers.
That is useful, for example, if you want to set a variable in an event
handler for that variable - we could do it, for example, in the
fish_user_path or fish_key_bindings handlers.
This is something the `block` builtin was supposed to be for, but it
never really worked because it only allows suppressing the event for
the duration, they would fire later. See #9030.
Because it is possible to abuse this, we only have a long-option so
that people see what is up.
[w]open_dir does not pass O_CREAT, so the mode argument to open is never used.
also, O_CREAT | O_DIRECTORY could not be used (portably) to create a directory.
(on POSIX does not specify what should happen, on Linux it is EINVAL.)
Remove the last non scoped place where we disable protocols (just before
exec(1)); it's not necessary with the current approach because we always
disable inside eval.
There is an edge case where we don't:
fish -ic "exec bash"
leaving bash with CSI u enabled. Disable that also in -ic mode where we
don't have a reader.
In future we should use the same approach for restore_term_mode() but I'm
not sure which one is better.
This is resistant to misuse by including O_DIRECTORY in the open flags and it is
a separate function from {w,}open_cloexec() in preparation for making that one
return a `File` instead of an `OwnedFd`.
