`mmap` should fail when the length argument is 0. Checking this in advance
allows returning early, without performing unnecessary syscalls.
This also fixes an issue observed on FreeBSD 13.2 where `mmap` does not always
fail when the length is 0, resulting in the `assert!(len > 0)` in
`MmapRegion::new` failing.
https://github.com/fish-shell/fish-shell/issues/11595
Commit cd3da62d24 (fix(completion): unescape strings for __fish_complete_list,
2024-09-17) bravely addressed an issue that exists in a lot of completions.
It did so only for __fish_complete_list. Fair enough.
Unfortunately it unescaped more than just "$(commandline -t)".
This causes the problem described at
https://github.com/fish-shell/fish-shell/issues/11508#issuecomment-2889088934
where completion descriptions containing a backslash followed by "n" are
interpreted as newlines, breaking the completion parser. Fix that.
Given a command line like
foo --foo=bar=baz=qux\=argument
(the behavior is the same if '=' is substituted with ':').
fish completes arguments starting from the last unescaped separator, i.e.
foo --foo=bar=baz=qux\=argument
^
__fish_complete_list provides completions like
printf %s\n (commandline -t)(printf %s\n choice1 choice2 ...)
This means that completions include the "--foo=bar=baz=" prefix.
This is wrong. This wasn't a problem until commit f9febba (Fix replacing
completions with a -foo prefix, 2024-12-14), because prior to that, replacing
completions would replace the entire token.
This made it too hard to writ ecompletions like
complete -c foo -s s -l long -xa "hello-world goodbye-friend"
that would work with "foo --long fri" as well as "foo --long=frie".
Replacing the entire token would only work if the completion included that
prefix, but the above command is supposed to just work.
So f9febba made us replace only the part after the separator.
Unfortunately that caused the earlier problem. Work around this. The change
is not pretty, but it's a compromise until we have a better way of telling
which character fish considers to be the separator.
Fixes#11508
This was missed in the Rust port - C++ had statfs for MNT_LOCAL and not statvfs.
The effect of this is that fish never thought its filesystem was local on macOS
or BSDs (Linux was OK). This caused history race tests to fail, and also could
in rare cases result in history items being dropped with multiple concurrent
sessions.
This fixes the history race tests under macOS and FreeBSD - we weren't locking
because we thought the history was a remote file.
Within a linked worktree, `$GIT_DIR` and `$GIT_COMMON_DIR` have different
values (see [git-worktree docs](https://git-scm.com/docs/git-worktree#_details)).
The two serve different purposes, in case of stashes `$GIT_COMMON_DIR`
should be used, this way stash detection in the git prompt works also
when inside a `git worktree`.
Closes#11591
Systems like NixOS might not have "git-receive-pack" or any other "git-*"
executable in in $PATH -- instead they patch git to use absolute paths.
This is weird. But no reason for us to fail. Silence the error.
Fixes#11590
Eliminates some code duplication between the two different saving
implementations. These changes are based on
https://github.com/fish-shell/fish-shell/pull/11492#discussion_r2149438316.
One extra change is included here, namely the early return on an empty file
name, indicating private mode. Without this, `history_path.unwrap()` fails in
some tests. Returning early is probably what we want in such situations anyway.
The cached information might become outdated. It is important that all fish
processes use the same mutual exclusion logic (either `flock`-based or the
fallback), because the two methods do not provide mutual exclusion from one
another.
Avoiding caching makes the behavior independent on previous system states,
resulting in fish instances performing file operations at the same time to use
the same locking logic.
More detailed discussion in
https://github.com/fish-shell/fish-shell/pull/11492#discussion_r2134543447
If arguments always have the same value they do not need to be arguments.
For `HistoryImpl::add`, the comments are incorrect (assuming they should
indicate the value of the argument), since both arguments can be true or false
independently.
`History::add` is only called at one location in a test with a constant value of
`false` for `pending`. This might mean that the parameter could be deleted, or
maybe even the entire function, if testing can work without it.
Both history files and universal variables files are accessed by multiple
processes, which need a way to synchronize their accesses.
This synchronization logic is mixed in with the logic for reading and updating
the files' contents, which results in messy code, duplicated locking
implementations, and inconsistencies.
Moreover, the existing implementations are flawed which has resulted in file
corruption (e.g. https://github.com/fish-shell/fish-shell/issues/10300).
The new approach separates the synchronization logic from the rest.
There are two approaches to synchronization.
- The primary one is using `flock(2)` to lock the directory containing the file
which requires synchronized access. We do not lock the file holding the data
directly because this file might be replaced, which can result in locking
succeeding when it should block. Locking the directory solves this problem.
To avoid inconsistent file states, changes are first written to a temporary
file, which is then renamed into place while still holding the lock.
- In some situations `flock` locks are unavailable or take a long time. This
mostly applies to remote file systems. If we think that a directory is located
on a remote file system, we do not attempt to use `flock`.
As a fallback, we have a lockless approach, which uses file metadata (device,
inode, size, ctime, mtime) to identify file versions.
We then read from the file, write the updated data to a temporary file,
check if the file id for the path is the same as before we started reading,
and if so we rename the temporary file such that it replaces the old file.
Note that races are possible between the file id check and the rename syscall.
If we detect a file id mismatch, we retry, up to a predetermined number of
attempts.
The operations which should be performed are passed to the functions handling
synchronization as `Fn`s. Because we might have to run these operations
repeatedly when retrying, they should be executable arbitrarily often without
causing side-effects relevant to the program. This requires some changes to
functions accessing these files. In many cases, they have to work with
non-mutable references, which requires that they return the data which should be
updated in the program state, instead of directly assigning to the appropriate
location.
Locking via `O_EXLOCK`, which was used for the universal variable file, is no
longer supported. That version of locking locks the file operated on directly,
instead of its directory. According to the man pages of {Open,Free,Net}BSD
and macOS, these locks have flock semantics. So if flock is available, we can
use it, and if it is not, the `O_EXLOCK` flag does not help.
This is preparatory work for refactoring the file synchronization approach.
`read_exact` will fail if the file length does not match the expected one, which
means zero padding is useless. (On any reasonable OS it was also useless before,
because anonymous memory mapping would be zero-pages.)
Use `std::io::Result` instead of `Option` as the return type where appropriate,
to allow for better-informed error handling.
Remove explicit checks about the length to be mapped being 0.
From `mmap(3)`:
> If len is zero, mmap() shall fail and no mapping shall be established.
This allows for more informative error handling.
In some cases, the `?` operator can now be applied sensibly instead of more
verbose local error handling.
There is no need for separate implementations for history and uvar file
handling. Having a shared implementation ensures that creating temporary files
is handled consistently and removes clutter from the source files which want to
create temporary files.
This is another step towards making the `load_from_file` function callable
without modifying internal data of `self`. Instead, the required updates for
a successful load should be returned.
For now, `self.last_read_file_id` is still modified
within `load_from_file`, which means it still needs a mutable reference to
`self`.
This is done as a step towards enabling loading from the variables file without
affecting internal variables, such that retrying becomes possible without
issues.
Callbacks are generated by
`src/env_universal_common.rs:generate_callbacks_and_update_exports`.
This function only pushes to the `Vec`, so the content of the `Vec` passed to it
does not matter within this function.
The generated callbacks are used in `src/env/environment.rs:universal_sync`,
which calls `generate_callbacks_and_update_exports` via
`EnvUniversal::sync`,
(optionally `EnvUniversal::load_from_path_narrow`),
`EnvUniversal::load_from_file`.
The only other code making use of these callbacks is in tests.
Because the only real use passes an empty `Vec`, there is no need to to pass the
`Vec` as a mutable reference to the function at all. Instead, we create the `Vec`
in `generate_callbacks_and_update_exports` and return it from there, which is
what the new code does.
This change is made because we want loading from a file to be performed without
mutating data which does not come directly from the file.
Then, we can safely retry loading from the file as many times as we want,
without worrying about side-effects on our data structures.
We want to be able to do this in the case where we cannot properly lock the file
and fall back to lockless reading, where we check file metadata before and after
reading to detect modifications, and retry if modifications are detected.
This fallback logic is not in place yet, and further changes are required for
side-effect free loading.
This allows converting non-UTF-8-conforming bytes from their PUA encoding back
to the original bytes before outputting.
Due to the way Rust handles trait implementations, we cannot use
`impl<T: std::fmt::Display> FloggableDisplay for T`
anymore, as that would result in a conflicting implementation for the types
which get a custom implementation.
Instead, explicitly implement the trait for all types which need it.
The changes to enable terminal protocols (CSI-U, etc) also attempts to
re-disable these when fish exits. In particular it attempts to disable these
from a SIGTERM handler.
Unfortunately none of that machinery is async-signal safe. Indeed our SIGTERM
handler has gotten rather sketchy, with taking a mutex and some other stuff.
Remove the async-signal-unsafe stuff and make SIGTERM manifestly safe.
Unfortunately this means that terminal protocols will remain set after SIGTERM
but that's probably unavoidable.
Calls to redirect_tty_output were added in many places when certain tty-syscalls
returned EIO. See commit 396bf1235d
This was intended to work around a glibc bug in wide character output, but it
was never really justifiable or tested, and we no longer use glibc wide
character output.
Bravely remove these, except in the case where we got SIGHUP and we don't want
to trigger SIGTTIN or SIGTTOU.
Definitions of localizable strings should not be guarded by `cfg`, because then
they might not end up being exported, depending on the compilation config.
The PO file updates can now run in a normal test, eliminating the need for
special handling.
Rename the `check-translations.fish` script, to clarify which part of the checks
happens in it.
This is intended to allow translation updates in contexts where building within
the `fish_xgettext.fish` script is undesirable.
Specifically, this allows checking for PO file updates in the tests run by
`test_driver.py`. Because these use a tmpdir for `$HOME`, building within such a
test requires installing the entire Rust toolchain and doing a clean build,
which is a waste of resources.
With this argument, it is possible to build the template before running the
tests and passing the file path into the script.
When including the tests in the build, string literals passed to `wgettext!`
would be included in the gettext template file, which we do not want here,
because the strings should not be localized for this test.
This function does not need to run when the pager is not focused. Calling it
lazily eliminates the overhead of calling it when it is not needed.
This code runs on each keypress when entering a command, so it makes sense to
keep it as lean as possible. (At the very least it avoids spam when trying to
debug/analyze gettext behavior.)
This test is the one with the longest runtime. Splitting the two targets into
separate tests allows them to run in parallel, which can speed up the tests.
