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.
Commit 53d9b9b67 (Escaping tweak in git.kak, 2024-02-06) broke
blame-jump when the commit subject contains a single quote.
(Also on unbalanced "{" which is a rare edge case but we already have
it in our Git history.)
git.kak assumes that filenames don't contain ' or unbalanced {,
but we can't really make that assumption about people's names or
commit subjects.
Unfortunately the escaping here is very messy. We need to pass
arbitrary text to callbacks; maybe we should have closures that can
capture private temporary registers.
Our "git add" and "git rm" default to the current file. The shell
interprets globs in the filename, which can lead to surprising results,
for example if it's accidentally used on a scratch buffer like *git*.
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.
If I run
git blame
execute-keys 10o<esc>,j
git blame-jump
that'll jump to the wrong commit.
Use a flag-lines option to tell if a line still has blame information
cached.
Closes#5084
Today we can recursively search history with "git blame-jump". However
that command has some drawbacks, mainly that it's blocking. Making
it async without any progress indicator might be confusing. Better
to run plain "git blame"[1] and press Enter.
Also it might be nice to enable recursive searches using only "git
blame" and `<ret>` (since that is bound to "git blame-jump" while
blame annotations are displayed).
Make "git blame" in git-diff/git-log buffers run "git show
$commit:$file" for the commit and file at cursor, and decorate this
blob view with blame annotations. The latter allows to use `<ret>`
and repeat.
Unfortunately this relies on a hidden option "git_blob" to keep the
commit ID and filename. Maybe we can put this metadata somewhere
else like the buffer name or contents, ideally in a way that survives
serialization.
I'd still keep "git blame-jump" because it seems faster for the common
case of tracking down a single line.
[1]: In my testing, "git blame --incremental" is not any slower than
"git blame -L123,123" at finding that line.
Running git blame signals intent to view the blamed commit. Let's make
`<ret>` to go to that commit+line as long as blame info is shown.
In diff buffers we already use `<ret>` for "diff-jump".
Like blame annotations, the blame-jump mapping lives in window scope.
This means it will not collide with "diff-jump" which is mapped in
buffer scope.
Add the mapping synchronously (unlike the rest of the git-blame code)
to perhaps allow the user to override the mapping:
git blame; map window normal <ret> ...
Our ":git blame" annotates each line with the most recent commit.
However often a line has been modified by several commits.
Introduce ":git blame-jump" which shows the commit that added the
line at cursor. Crucially, it works also in Git diff buffers, so it
can be used recursively to find the full history of a line.
To do the recursive blame from a diff, I need to navigate to the
old (deleted) version of a line. Since old and new line are usually
neighbors. Speed up the common scenario of finding the old version
by making ":git blame-jump" jump to the new version. This means the
initial diff view might not include the commit message etc. Compensate
this by showing the commit's date+author+subject in the status line.
Here are some test cases.
- run blame-jump after "git blame"
- create an uncommitted or unsaved line, run "git blame" and
"blame-jump" on the uncommitted line
- run blame-jump without running "git blame"
- run blame-jump in "git show"
- run blame-jump in "git diff HEAD"
- run blame-jump in "git diff --cached"
- run blame-jump in "git diff" (YMMV if there are cached changes,
could fix that)
Naming: there are some similar commands in the wild [1];
they are usually called "show-blamed" or similar, but they
don't jump to the corresponding line. Also our list of git
commands is getting a bit messy (especially the undocumented
show-diff/hide-diff/next-hunk/prev-hunk; subject first naming seems
better).
[1]: f6e78ec4c0/kakrc (L423)
Future work: to go back to the previously-blamed commit we need to
have had the foresight to use "rename-buffer". Perhaps we want to
add some kind of buffer stack (like Magit does for example).
Most diff consumers we've written only care about the "final" state
after parsing through a diff. Let's extract the diff parsing part,
for reuse in several new commands.
In future we should try to use this (or better, a diff-parsing library)
for patch-range.pl. We'd add a callback argument that is invoked once
perl hunk (or line). Unfortunately I haven't found that library for
Perl yet.
Diff buffers created by ":git diff" differ from other filetype=diff
buffers in that they use "git rev-parse --show-toplevel" as root
directory for diff-jump. This makes sense because paths printed by
"git diff" are relative to that directory.
Today we handle the above difference by making ":git" override the
diff-jump mapping. This doesn't work for buffers that were read from
a file. Fix this by introducing a separate filetype, "git-diff",
which allows to move the mapping in the usual place.
This breaks existing filetype=diff hooks[1] which need to be adapted
to match git-diff (also git-log).
Another motivation for the separate filetype is that a following
patch wants to enable Git blame commands in git-diff buffers but
not in plain diff buffers -- those should keep being blamed like any
other file if tracked by Git.
Perhaps git-* buffers are for Git metadata, not files that are tracked
by Git.
The added hooks awkwardly include their hook parameter to work around
hook ordering issues when switching between filetypes. See also [2].
We could also use filetype=git-log instead of git-diff.
Our highlighting for "git log --graph" would have rare false positives.
Closes#5049
[1]: https://github.com/search?utf8=%E2%9C%93&q=filetype%3Ddiff+language%3Akakounescript+-repo%3Amawww%2Fkakoune+-is%3Afork&type=code
[2]: https://lists.sr.ht/~mawww/kakoune/%3C20240201091907.973508-1-aclopte@gmail.com%3E
When a buffer has unsaved deleted/added lines, then any blame
annotations below those lines may be off. Fix this by feeding the
latest buffer contents to Git. Unfortunately there is no easy way
to distinguish between "Unsaved" and "Saved but not committed yet"
so let's keep using the umbrella term.
We double-parse a command definition to figure out the location of
a support script at load time. This feels a bit dangerous and is not
really necessary, so use %val{runtime}/rc/tools/... instead.
Reference: https://lists.sr.ht/~mawww/kakoune/%3CZbOSCK2JjJvo-RTt@gmail.com%3E
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
When "edit -fifo" reads data without a trailing newline, the fifo
buffer will not have a trailing blank line. But if there is a trailing
newline, we will get a trailing blank line. This is weird because the
trailing blank line exists for scrolling, it should not be determined
by the data read.
Add a test case to demonstrates the inconsistency which is fixed by
the next patch.
I dedicate any and all copyright interest in this software to the
public domain. I make this dedication for the benefit of the public at
large and to the detriment of my heirs and successors. I intend this
dedication to be an overt act of relinquishment in perpetuity of all
present and future rights to this software under copyright law.
Patches as produced by "git format-patch" have a trailing signature
that is separated from the body by a line with "-- " on it. By default
it contains the Git version. We erroneously include this signature
in the diff we pipe to patch, which fails to apply as a result.
Add a targeted fix to suppress these signatures.
Sometimes a patch that fails to apply will apply cleanly after
adding -3. Also sometimes we do want to apply with conflict markers.
So this is another somewhat common option.
Sorry I did not test my earlier patch in production. It passes
blame flags via the environment. On a 5000 line file this results in
"execve failed: Argument list too long" errors.
Use a different way of checking whether blame info is shown.
Since :patch transforms its inputs into context-only lines, we can
easily get into a state where a file diff has only context lines.
git apply does not accept a "diff" without any hunk, so let's skip
that.
It is not an obviously better result than just displaying results
from each tag file, so remove sorting to take advantage of live
completion updates
As discussed in #5081
