Download raw body.
tog: don't mark incorrect base commit in nested log views
We reuse the work tree base commit index in nested log views, which can
result in an incorrect commit being painted with the base commit marker
because the index is based on the initial commit queue, which might be
different in nested log views. As such, below are three related diffs.
The first is the test case and one line fix, which resets the base
commit index when opening a nested log view so that it is recomputed
again (provided tog was invoked with the log view in a work tree so we
have the base commit id cached). I'd like to look at improving this so
that we get the base commit id whenever opening a nested log view in a
work tree irrespective of the view in which tog was started--but for now
I want to fix marking incorrect commits.
The test required increasing the mock terminal height from 3 to 4 lines
so that we can actually scroll down in the tree view. Running the test
with a binary built from HEAD will fail as the base commit marker
incorrectly prefixes a commit that is not the base commit. The test
passes as expected when using a binary built with the tog.c fix applied.
The second and third diffs contain two small albeit related fixes for
bugs discovered during the process of writing the test: the first fixes
a NULL pointer deref from passing a NULL s->last_displayed_entry to
got_tree_entry_get_next() when scrolling down in a somewhat contrived
tree view that is less than 4 lines in height; and the other diff clamps
an s->selected assignment to 0 rather than becoming negative in the
similarly contrived case of views less than their header and border
lines in height (i.e., [2,5] depending on the view), which can result in
bogus indexes displayed in the tree view header. Both diffs include a
test covering these bugs: without the fixes, the added log test fails
with a [-1/4] index displayed in the nested tree view header; and the
tree test segfaults when scrolling down in the 3-line high tree view.
-----------------------------------------------
commit ece1c9be5bbce981c493f47522d70ed1a1355a03
from: Mark Jamsek <mark@jamsek.dev>
date: Tue Dec 31 11:16:12 2024 UTC
tog: don't mark incorrect base commit in nested log views
If a nested log view is opened, don't reuse the work tree's base commit
index from the initial `tog log` invocation because it is based on the
initial commit queue. If it has become invalid, an incorrect log view
commit entry is painted with the base commit marker. Reset the index
whenever opening nested log views so that it is recomputed, and write
a regression test to cover this case.
M regress/tog/log.sh | 28+ 1-
M tog/tog.c | 1+ 0-
2 files changed, 29 insertions(+), 1 deletion(-)
diff 35c7ffa4d97796f8a99059d3e45a7aa100352c0a ece1c9be5bbce981c493f47522d70ed1a1355a03
commit - 35c7ffa4d97796f8a99059d3e45a7aa100352c0a
commit + ece1c9be5bbce981c493f47522d70ed1a1355a03
blob - 5329319eb952b701fc799caf6f0648c80bd83b52
blob + 01a33201435af4ba4c618c10db784372be30bea2
--- regress/tog/log.sh
+++ regress/tog/log.sh
@@ -542,7 +542,7 @@ test_log_commit_keywords()
test_log_show_base_commit()
{
# make view wide enough to show full headline
- test_init log_show_base_commit 80 3
+ test_init log_show_base_commit 80 4
local repo="$testroot/repo"
local id=$(git_show_head "$repo")
@@ -574,6 +574,7 @@ test_log_show_base_commit()
$(trim 80 "commit $head_id [1/2] master")
$ymd flan_hacker *[master] base commit
$ymd flan_hacker adding the test tree
+
EOF
tog log
@@ -590,6 +591,7 @@ test_log_show_base_commit()
$(trim 80 "commit $head_id [1/2] master")
$ymd flan_hacker [master] base commit
$ymd flan_hacker adding the test tree
+
EOF
tog log -r "$repo"
@@ -615,6 +617,7 @@ test_log_show_base_commit()
$(trim 80 "commit $head_id [1/3] master")
$ymd flan_hacker ~[master] new base mixed-commit
$ymd flan_hacker base commit
+ $ymd flan_hacker adding the test tree
EOF
tog log
@@ -626,6 +629,30 @@ test_log_show_base_commit()
return 1
fi
+ # check marker is not incorrectly drawn when opening a nested log view
+ cat <<-EOF >$TOG_TEST_SCRIPT
+ T # open tree view of base commit
+ j # select beta tree entry
+ L # load log view of commits involving beta
+ SCREENDUMP
+ EOF
+
+ cat <<-EOF >$testroot/view.expected
+ $(trim 80 "commit $id /beta [1/1]")
+ $ymd flan_hacker adding the test tree
+
+
+ EOF
+
+ tog log
+ cmp -s "$testroot/view.expected" "$testroot/view"
+ ret=$?
+ if [ $ret -ne 0 ]; then
+ diff -u "$testroot/view.expected" "$testroot/view"
+ test_done "$testroot" "$ret"
+ return 1
+ fi
+
test_done "$testroot" "$ret"
}
blob - fe799cdf0299588e54f5654e65a0af63994dedf3
blob + e31dd5f281a3090496215529ba601df0b1b29849
--- tog/tog.c
+++ tog/tog.c
@@ -11305,6 +11305,7 @@ view_dispatch_request(struct tog_view **new_view, stru
"parent/child view pair not supported");
break;
case TOG_VIEW_LOG:
+ tog_base_commit.idx = -1;
if (view->type == TOG_VIEW_BLAME)
err = log_annotated_line(new_view, y, x,
view->state.blame.repo, view->state.blame.id_to_log);
-----------------------------------------------
commit df7caa1a7ae745e41ecee2ae1d3420a536796715
from: Mark Jamsek <mark@jamsek.dev>
date: Tue Dec 31 12:45:28 2024 UTC
fix NULL deref when scrolling small tog tree views down
In the rare case a tree view is smaller than four lines in height,
the last_displayed_entry is NULL. Check this condition on a scroll
down event with j/down arrow or ^f/pgdn to guard a NULL got_tree_entry
from being passed to got_tree_entry_get_next() where it is dereferenced.
And add a test case covering this path.
M regress/tog/tree.sh | 33+ 0-
M tog/tog.c | 4+ 2-
2 files changed, 37 insertions(+), 2 deletions(-)
diff ece1c9be5bbce981c493f47522d70ed1a1355a03 df7caa1a7ae745e41ecee2ae1d3420a536796715
commit - ece1c9be5bbce981c493f47522d70ed1a1355a03
commit + df7caa1a7ae745e41ecee2ae1d3420a536796715
blob - 0fcbbd78a8afe17bd2f12a187cecec268453d4ea
blob + bf044c0e1211d8e7b452d4f9d02d71b7a8bcf18b
--- regress/tog/tree.sh
+++ regress/tog/tree.sh
@@ -341,9 +341,42 @@ test_tree_commit_keywords()
test_done "$testroot" "$ret"
}
+test_tree_insufficient_height()
+{
+ test_init tree_basic 120 3
+
+ local id=$(git_show_head $testroot/repo)
+
+ # Cover the path that guards a NULL dereference when scrolling
+ # down in a tree view too small to display any tree entries.
+ cat <<-EOF >$TOG_TEST_SCRIPT
+ j attempt to scroll down
+ f
+ SCREENDUMP
+ EOF
+
+ cat <<-EOF >$testroot/view.expected
+ commit $id
+ [1/4] /
+
+ EOF
+
+ cd $testroot/repo && tog tree
+ cmp -s $testroot/view.expected $testroot/view
+ ret=$?
+ if [ $ret -ne 0 ]; then
+ diff -u $testroot/view.expected $testroot/view
+ test_done "$testroot" "$ret"
+ return 1
+ fi
+
+ test_done "$testroot" "$ret"
+}
+
test_parseargs "$@"
run_test test_tree_basic
run_test test_tree_vsplit_blame
run_test test_tree_hsplit_blame
run_test test_tree_symlink
run_test test_tree_commit_keywords
+run_test test_tree_insufficient_height
blob - e31dd5f281a3090496215529ba601df0b1b29849
blob + e8031edb015e8f848ba06221e89e864108afdd00
--- tog/tog.c
+++ tog/tog.c
@@ -9671,7 +9671,8 @@ input_tree_view(struct tog_view **new_view, struct tog
s->selected++;
break;
}
- if (got_tree_entry_get_next(s->tree, s->last_displayed_entry)
+ if (s->last_displayed_entry == NULL ||
+ got_tree_entry_get_next(s->tree, s->last_displayed_entry)
== NULL) {
/* can't scroll any further */
view->count = 0;
@@ -9687,7 +9688,8 @@ input_tree_view(struct tog_view **new_view, struct tog
case CTRL('f'):
case 'f':
case ' ':
- if (got_tree_entry_get_next(s->tree, s->last_displayed_entry)
+ if (s->last_displayed_entry == NULL ||
+ got_tree_entry_get_next(s->tree, s->last_displayed_entry)
== NULL) {
/* can't scroll any further; move cursor down */
if (s->selected < s->ndisplayed - 1)
-----------------------------------------------
commit 72e562c6a8faa2f41bdd83956217700d87c6155b
from: Mark Jamsek <mark@jamsek.dev>
date: Tue Dec 31 12:45:28 2024 UTC
tog: clamp selected idx to avoid showing negative indexes
In the rare case views are smaller in height than the first line number
after the view's header lines (i.e., [2,5] depending on the view), we can
decrement the selected entry index into negative values; clamp it to 0.
Add a log view test case covering this path by opening a child tree view
and toggling fullscreen.
M regress/tog/log.sh | 33+ 0-
M tog/tog.c | 1+ 1-
2 files changed, 34 insertions(+), 1 deletion(-)
diff df7caa1a7ae745e41ecee2ae1d3420a536796715 72e562c6a8faa2f41bdd83956217700d87c6155b
commit - df7caa1a7ae745e41ecee2ae1d3420a536796715
commit + 72e562c6a8faa2f41bdd83956217700d87c6155b
blob - 01a33201435af4ba4c618c10db784372be30bea2
blob + 77e67c8632bd152393715a611f8d74a820e0e3c5
--- regress/tog/log.sh
+++ regress/tog/log.sh
@@ -1102,6 +1102,38 @@ test_log_worktree_entries()
test_done "$testroot" 0
}
+test_log_tiny_child_tree_view()
+{
+ test_init log_tiny_child_tree_view 120 3
+
+ local id=$(git_show_head $testroot/repo)
+
+ # This test covers the offset_selection_down() path ensuring
+ # indexes are properly clamped to prevent negative values.
+ cat <<-EOF >$TOG_TEST_SCRIPT
+ T open tree view in vsplit
+ F toggle fullscreen to hit offset_selection_down()
+ SCREENDUMP
+ EOF
+
+ cat <<-EOF >$testroot/view.expected
+ commit $id
+ [1/4] /
+
+ EOF
+
+ cd $testroot/repo && tog log
+ cmp -s $testroot/view.expected $testroot/view
+ ret=$?
+ if [ $ret -ne 0 ]; then
+ diff -u $testroot/view.expected $testroot/view
+ test_done "$testroot" "$ret"
+ return 1
+ fi
+
+ test_done "$testroot" "$ret"
+}
+
test_parseargs "$@"
run_test test_log_hsplit_diff
run_test test_log_vsplit_diff
@@ -1116,3 +1148,4 @@ run_test test_log_limit_view
run_test test_log_search
run_test test_log_mark_keymap
run_test test_log_worktree_entries
+run_test test_log_tiny_child_tree_view
blob - e8031edb015e8f848ba06221e89e864108afdd00
blob + 8d227eff283fc8287e36b66f666eb4a49d584018
--- tog/tog.c
+++ tog/tog.c
@@ -11465,7 +11465,7 @@ offset_selection_down(struct tog_view *view)
view->offset = offset;
if (scrolld && offset) {
err = scrolld(view, offset);
- *selected -= offset;
+ *selected -= MIN(*selected, offset);
}
}
--
Mark Jamsek <https://bsdbox.org>
GPG: F2FF 13DE 6A06 C471 CA80 E6E2 2930 DC66 86EE CF68
tog: don't mark incorrect base commit in nested log views