The clang warning for pending_signals_t was about the operator=
return type being wrong (misc-unconventional-assign-operator).
Signed-off-by: Rosen Penev <rosenp@gmail.com>
We don't want to convert the input to a "wcstring &" because
"stage_variables" needs to have the same type as other stages, so we
can use it in a loop. Communicate that to clang-tidy.
We also don't want to take "wcstring &&". As the Google style guide
states, it's not really beneficial here, and it potentially hurts
readability because it's a relatively obscure feature.
The rest of our code contains a bunch of && parameters. We might
want to get rid of some of them.
Closes#8227
clang-tidy wrongly sees an std::move to a const ref parameter and
believes it to be pointless. The copy constructor however is deleted.
Signed-off-by: Rosen Penev <rosenp@gmail.com>
This disables job control inside command substitutions. Prior to this
change, a cmdsub might get its own process group. This caused it to fail
to cancel loops properly. For example:
while true ; echo (sleep 5) ; end
could not be control-C cancelled, because the signal would go to sleep,
and so the loop would continue on. The simplest way to fix this is to
match other shells and not use job control in cmdsubs.
Related is #1362
The previous layout confused me for a minute as it suggested it was
possible for `pipe_next_read` to be moved twice (once in the first
conditional block, then again when the deferred process conditional
called `continue` - if and only if the deferred process *was* the last
process in the job. This patch clarifies that can't be the case.
`pipe_next_read` is moved in the body of the loop, and not
re-initialized the last go around. However, we call
`pipe_next_read.close()` after the loop, which is undefined behavior (as
it's been moved).
Best case scenario, the compiler passed the address of our copy of the
struct to `exec_process_in_job` and beyond, it went out of scope there,
the value of `fd` was set to closed (minus one), and we explicitly call
`.close()` again, in which case it does nothing.
Worst case scenario, the compiler re-uses the storage for the now-moved
struct for something else and our call to `.close()` ends up closing
some other value of `fd` (valid or invalid) and things break.
Aside from the fact that we obviously don't need to close it since it's
not assigned for the last process in the job, it's a RAII object so we
don't have to worry about manually closing it in the first place.
`escape_code_length()` was converted from returning a `size_t` to
returning a `maybe_t<size_t>` but that subtly broke all existing call
sites by forcing all input to go through the slow path of assuming a
zero-length escape sequence was found.
This is because all callers predicated their next action on what amounts
to `if (escape_code_length(...))` which would correctly skip the slow
path when `escape_code_length` returned zero, but after the conversion
to `maybe_t` contained not `maybe_t::none()` but rather
`maybe_t::some(0)` due to coercion of the result from the `size_t` local
`esc_seq_len` to the `maybe_t<size_t>` return value - which, when
coerced to a boolean returns *true* for `maybe_t::some(0)` rather than
false.
The regression was introduced in 7ad855a844a271770ad659622a10f9fe273566b4
and did not ship in any released versions so no harm, no foul.
This is required for the usage of placement new. Not an issue for fish
as it gets picked up from elsewhere, but it lets one use it in a C++
test directly this way.
* commandline: Add --is-valid option to query whether it's syntactically complete
This means querying when the commandline is in a state that it could
be executed. Because our `execute` bind function also inserts a
newline if it isn't.
One case that's not handled right now: `execute` also expands
abbreviations, those can technically make the commandline invalid
again.
Unfortunately we have no real way to *check* without doing the
replacement.
Also since abbreviations are only available in command position when
you _execute_ them the commandline will most likely be valid.
This is enough to make transient prompts work:
```fish
function reset-transient --on-event fish_postexec
set -g TRANSIENT 0
end
function maybe_execute
if commandline --is-valid
set -g TRANSIENT 1
commandline -f repaint
else
set -g TRANSIENT 0
end
commandline -f execute
end
bind \r maybe_execute
```
and then in `fish_prompt` react to $TRANSIENT being set to 1.
Because we are, ultimately, interested in how many cells a string
occupies, we *have* to handle carriage return (`\r`) and line
feed (`\n`).
A carriage return sets the current tally to 0, and only the longest
tally is kept. The idea here is that the last position is the same as
the last position of the longest string. So:
abcdef\r123
ends up looking like
123def
which is the same width as abcdef, 6.
A line feed meanwhile means we flush the current tally and start a new
one. Every line is printed separately, even if it's given as one.
That's because, well, counting the width over multiple lines
doesn't *help*.
As a sidenote: This is necessarily imperfect, because, while we may
know the width of the terminal ($COLUMNS), we don't know the current
cursor position. So we can only give the width, and the user can then
figure something out on their own.
But for the common case of figuring out how wide the prompt is, this
should do.
Without escapes.
The new option is a bit cheesy, but "width" isn't as expressive and
requires an argument.
Maybe we want "pad" to also require --visible?
* Add `set --function`
This makes the function's scope available, even inside of blocks. Outside of blocks it's the toplevel local scope.
This removes the need to declare variables locally before use, and will probably end up being the main way variables get set.
E.g.:
```fish
set -l thing
if condition
set thing one
else
set thing two
end
```
could be written as
```fish
if condition
set -f thing one
else
set -f thing two
end
```
Note: Many scripts shipped with fish use workarounds like `and`/`or`
instead of `if`, so it isn't easy to find good examples.
Also, if there isn't an else-branch in that above, just with
```fish
if condition
set -f thing one
end
```
that means something different from setting it before! Now, if
`condition` isn't true, it would use a global (or universal) variable of
te same name!
Some more interesting parts:
Because it *is* a local scope, setting a variable `-f` and
`-l` in the toplevel of a function ends up the same:
```fish
function foo2
set -l foo bar
set -f foo baz # modifies the *same* variable!
end
```
but setting it locally inside a block creates a new local variable
that shadows the function-scoped variable:
```fish
function foo3
set -f foo bar
begin
set -l foo banana
# $foo is banana
end
# $foo is bar again
end
```
This is how local variables already work. "Local" is actually "block-scoped".
Also `set --show` will only show the closest local scope, so it won't
show a shadowed function-level variable. Again, this is how local
variables already work, and could be done as a separate change.
As a fun tidbit, functions with --no-scope-shadowing can now use this to set variables in the calling function. That's probably okay given that it's already an escape hatch (but to be clear: if it turns out to problematic I reserve the right to remove it).
Fixes#565
Fixes some regressions from 35ca42413 ("Simplify some parse_util functions").
The tmux tests are not beautiful but I find them easy to write.
Probably a pexpect test would also be enough here?
The names in the implementation differed from those in the header, but
the header names were definitely better (because they correlated across
function calls).
This doesn't work.
The real thing that tells if something is read-only is
electric_var_t::readonly().
This wasn't used, and we provide no way to make a variable read-only,
which makes this an unnecessary footgun.
for PWD in foo; true; end
prints:
>..src/parse_execution.cpp:461: end_execution_reason_t parse_execution_context_t::run_for_statement(const ast::for_header_t&, const ast::job_list_t&): Assertion `retval == ENV_OK' failed.
because this used the wrong way to see if something is read-only.
env_var_t::read_only() is basically broken.
It doesn't work for $PWD, as best as I can tell no variable is
read-only except for a hardcoded list of some of the electric ones.
So we should probably remove the entire read_only and
setting_read_only mechanism.
This breaks in comma-using locales (like my own de_DE.UTF-8), because
it still uses the locale-dependent strtod, which will then refuse to
read
1234.567
Using strtod_l (not in POSIX, I think?) might help, but might also be
a lot slower. Let's revert this for now and figure out if that is
workable.
This reverts commit fba86fb82105e65b0b8bef8f1e1d258cfc2f08dd.
fish_wcstod had a "fast path" which looked for all digits, otherwise
falling back to wcstod_l. However we now pass the C locale to wcstod_l,
so it is safe to extend the fast path to all ASCII characters.
In practice math parsing would pass strings here like "123 + 456" and
the space and + were knocking us off the fast path. benchmarks/math.fish
goes from 2.3 to 1.4 seconds with this change.
is_block is a field which supports 'status is-block', and also controls
whether notifications get posted. However there is no reason to store
this as a distinct field since it is trivially computed from the block
list. Stop storing it. No functional changes in this commit.
Through a mechanism I don't entirely understand, $PWD is sometimes
writable (so that `cd` can change it) and sometimes not.
In this case we ended up with it writable, which is wrong.
See #8179.
This didn't do all the syntax checks, so something like
fish -c 'echo foo; and $status'
complained of a missing command `0` (i.e. $status), and
fish -c 'echo foo | exec grep'
hit an assert!
So we do what read_ni does, parse each command into an ast, run
parse_util_detect_errors on it if it worked and then eval the ast.
It is possible to do this neater by modifying parser::eval, but I
can't find where.
This is slightly unclean. Even tho it would otherwise be syntactically
valid, using $status as a command is very very very likely to be an
error, like
if not $status
We have reports of this surprisingly regularly, including #2773.
Because $status can only ever be a value from 0 to 255, it is also
very unlikely to be an actual command, and that command is very
unlikely to do what you want.
So we simply point the user towards the "conditions" help section,
that should explain things.
