I saw a crash when running
git log --oneline %arg{@}
hook -once buffer NormalIdle .* %{
execute-keys -draft \
%{gk!} \
%{git diff --quiet || echo "Unstaged changes";} \
%{git diff --quiet --cached || echo "Staged changes";} \
<ret>
}
Backtrace (I still have GDB attached):
#4 0x00006502c740b13f in Kakoune::operator- (rhs=..., lhs=...) at /home/johannes/git/kakoune/src/units.hh:33
33 { return RealType(lhs.m_value - rhs.m_value); }
(gdb) up
#5 Kakoune::Buffer::next (coord=..., this=0x6502c90d7ff0) at /home/johannes/git/kakoune/src/buffer.inl.hh:18
18 if (coord.column < m_lines[coord.line].length() - 1)
(gdb) up
#6 FifoWatcher::read_fifo (this=0x6502c90d9e48) at buffer_utils.cc:252
252 m_buffer.erase(pos, m_buffer.next(pos));
This was introduced in 582c3c56b (Do not add trailing newline to
non-scrolling fifo buffers, 2024-01-28).
The problem seems to be that we call "m_buffer.next()" on a position
that is past-end the buffer, so m_lines[coord.line] is out-of-bounds.
Fix it.
For some reason I have not managed to reproduce the crash, not even
with sanitize=address.
There might be another problem: m_had_trailing_newline is intentionally
uninitialized because it is supposed to be read only on the second
read() with a positive return value. Unfortunately I think it's
possible that e.g. a NormalIdle hook inserts some text before the
first positive read(). Then, this line
const bool is_first = pos == BufferCoord{0,0};
if (not m_scroll and (is_first or m_had_trailing_newline))
pos = m_buffer.next(pos);
will read uninitialized "m_had_trailing_newline". Fix that too, to
be on the safe side. Sadly I don't have a test for this one either
so I'm not sure.
If the first byte in the multi-byte utf8 sequence does not match,
it means the "other" character is not set, so none of the sequence
byte will match (as they are all with the MSB set). This tightens
the critical loop which ends up running faster in most cases.
Avoid the costly shared object function call when most codepoints
will be ascii.
The regex benchmark gets a nice speedup:
Regex Before After
--------------------------------------+----------+---------
'Twain' | 25 ms | 15 ms
'(?i)Twain' | 74 ms | 57 ms
'[a-z]shing' | 323 ms | 303 ms
'Huck[a-zA-Z]+|Saw[a-zA-Z]+' | 26 ms | 17 ms
'\b\w+nn\b' | 424 ms | 393 ms
'[a-q][^u-z]{13}x' | 869 ms | 815 ms
'Tom|Sawyer|Huckleberry|Finn' | 33 ms | 24 ms
'(?i)Tom|Sawyer|Huckleberry|Finn' | 319 ms | 281 ms
'.{0,2}(Tom|Sawyer|Huckleberry|Finn)' | 1294 ms | 1293 ms
'.{2,4}(Tom|Sawyer|Huckleberry|Finn)' | 1470 ms | 1429 ms
'Tom.{10,25}river|river.{10,25}Tom' | 69 ms | 61 ms
'[a-zA-Z]+ing' | 447 ms | 408 ms
'\s[a-zA-Z]{0,12}ing\s' | 539 ms | 543 ms
'([A-Za-z]awyer|[A-Za-z]inn)\s' | 588 ms | 552 ms
'["'][^"']{0,30}[?!\.]["']' | 92 ms | 81 ms
For hash map, using fnv1a is faster as it is a much simpler algorithm
we can afford to inline. For files murmur3 should win as it processes
bytes 4 by 4.
Internally, all lines have a trailing "\n".
Buffers created empty (like fifo buffers) start with a single line.
When reading data into fifo buffers, we insert *before* the last line's
trailing newline ("last newline"). This enables autoscrolling (enabled
with "edit -scroll") as long as the cursor is on the last newline.
When autoscrolling is disabled, we have a special case to insert
*after* the last newline. This means that a cursor on that newline
won't be moved. Then we transplant the newline character from the
beginning to the end of the buffer. This special case happens only on
the very first fifo read; on subsequent reads, the cursor at position
1.1 will not be moved anway because insertions happen below 1.1.
Since we always insert (effectively) before the last newline, fifo
buffers have a trailing empty line.
For autoscrolling buffers this seems correct; it gives users an
obvious way to toggle autoscrolling.
For non-scrolling buffers the newline is redundant. Remove it.
This requires keeping track of whether the last newline comes from
the fifo, or was added by us. The shortest fix I could find
is to always append to the buffer if not scrolling, and then delete
the added newline character if applicable.
m_buffer.insert(m_scroll ? pos : m_buffer.next(pos), StringView(data, data+count));
if (not m_scroll and not m_had_trailing_newline)
m_buffer.erase(pos, m_buffer.next(pos));
maybe that's the best fix overall; but erasing at the end seems better
than erasing in the middle, so do that whenever possible.
Reported in https://lists.sr.ht/~mawww/kakoune/%3CZbTK7qit9nzvrMkx@gmail.com%3E
It turns out diffing was pretty fast, but applying the diff was
sub-optimal as it was constantly inserting/erasing lines which
led to lots of unnecessary shifting. Fix this by manually tracking
a read/write iterator and only shifting when necessary (on keeps,
and inserts).
In several places, we check for a control character with something like
char c;
[...]
if (c >= 0 and c <= 0x1F)
[...]
When char is signed (e.g. amd64) this is fine, but when char is unsigned
by default (e.g. arm32 and arm64) this generates warnings about the
tautologous check that an unsigned value is non-negative.
Write as
if ((unsigned char) c <= 0x1F)
[...]
which is both correct and not suspicious under both conventions.
In display_buffer.hh, the '->' operator is used on an iterator, but
(surprisingly) this is deprecated from C++20 because of x-value vs
l-value ambiguity. Now clang's -Wdeprecated-declarations warns about it
as we declare -std=c++2a. See
https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1252r2.pdf
which was adopted for 2019-03.
g++ 13.x is confused by the reinterpret_cast in Kakoune's memory.hh
allocator. Use -Wno-stringop-overflow to silence a large number of
verbose false alarms.
Users who rebind default keys and unmap the originals by binding them
to empty strings with empty docstrings end up with empty lines in the
autoinfo. For example, https://github.com/mawww/kakoune/issues/4918.
Hide completely null bindings which have both an empty mapping and an
empty docstring in the autoinfo, as an easy mechanism for these users to
eliminate the UI noise.
Fixes https://github.com/mawww/kakoune/issues/4918
When using either of
set-option g completers option=my_option
prompt -shell-script-candidates ...
While the search text is empty, the completions will be sorted
alphabetically.
This is bad because it means the most important entries are not listed
first, making them harder to select or even spot.
Let's apply input order before resorting to sorting alphabetically.
In theory there is a more elegant solution: sort candidates (except
if they're user input) before passing them to RankedMatch, and then
always use stable sort. However that doesn't work because we use a
heap which doesn't support stable sort.
Closes#1709, #4813
When doing :write -method replace, make sure we've set the correct mode,
uid and gid on the replacement file before attempting to rename it on
top of the original. This means that the original file is left in place
with correct permissions if anything fails, rather than ending up with
0700 permissions from mkstemp().
When a privileged :write is used with -method replace, it silently resets
the ownership of files to root:root. Restore the original owner and group
in the same way we restore the original permissions. Ownership needs to
be restored before permissions to avoid setuid and setgid bits being set
while the file is still owned by root, and to avoid them being subsequently
lost again on chmod(2).
If a user attempts to save a file without write permission for the
containing directory, with writemethod set as 'replace' or an explicit
':write -method replace' command, kak crashes with "terminating due to
uncaught exception of type Kakoune:runtime_error". (Note this doesn't
happen with a forced write, which fails earlier when it tries to enable
u+w permission.)
Don't raise another exception when already bailing out with a runtime
error for failing to create a temporary file or open the existing file.
Instead, make a best-efforts attempt to restore the file permissions
before raising the first exception, and only report the runtime chmod
exception if that step fails on the non-error path.
Some terminals misbehave when queried for output synchronization support,
such as Windows Terminal as reported in
https://github.com/mawww/kakoune/issues/5032
The relatively long response from a terminal which does support output-sync
is also prone to getting torn over a slow link such as a serial console,
causing stray input to the editor.
In ui_options, the terminal_synchronized option controls the use of this
feature, but unfortunately the query is unconditionally sent at startup
even when this is set false.
Skip the query at startup when terminal_synchronized is explicitly false.
We query at most once per terminal in set_ui_options so the behaviour
is correct both when kakoune is started with terminal_synchronized unset
and when it is started with terminal_synchronized set false but this is
later unset.
prompt has fuzzy filtering which is more discoverable than the menu
mode's regex filtering (because that one needs / to trigger it).
There are no important differences left, so replace the menu builtin
with a prompt-based command.
prompt does not support markup in the completion menu, so drop that
feature for now.
Remove FirstCharMatch which does not impact any of the test cases
and explicitely detect paths by using a BaseName flag when we match
the basename of the path.
