[vim/vim] keep '[ and ] marks above line 1 (PR #19061)
glepnir
Problem: deleting lines could move '[ or '] to line 0
Solution: clamp op start/end lnum to at least 1
related neovim/neovim#30593
You can view, comment on, or merge this pull request online at:
https://github.com/vim/vim/pull/19061
Commit Summary
- 7b283d6 fix(mark): keep '[ and ] marks above line 1
File Changes
(2 files)
- M src/ex_cmds.c (5)
- M src/testdir/test_marks.vim (13)
Patch Links:
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
chrisbra left a comment (vim/vim#19061)that test does not currently fail. Also shouldn't it verify the mark positions?
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
Reopened #19061.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
dkearns
dkearns left a comment (vim/vim#19061)These marks should arguably remain unset when calling the command on an empty buffer as no text was changed. I say arguably because Vim's handling of commands on empty buffers is generally a bit suspect.
I would strongly consider leaving this for another day given the release schedule, rather than baking in a possibly incorrect fix.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
glepnir
glepnir left a comment (vim/vim#19061)The marks are already being set by op_colon (b_op_start/end = oap->start/end). do_filter then subtracts linecount without checking, making them invalid (lnum=0). Whether marks should be set on empty buffers is debatable, but once set, they
shouldn't become invalid. This just adds bounds checking to the adjustment.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
glepnir
glepnir left a comment (vim/vim#19061)that test does not currently fail.
sry WDHYM. before v:errmsg it's E20 now it's empty .
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
dkearns
dkearns left a comment (vim/vim#19061)I understand the PR I'm just concerned about committing to a fix that might be less than optimal and then getting hit with backward compatibility requirements when trying to normalise the behaviour with other commands in the future. It's broken now so it's unlikely anyone is relying on it. However, they might be relying on the error.
Having said that I had a quick look and while there's varying behaviour (d at least leaves the marks unset) most commands appear to set these marks to (1, 0) after an operation on an empty buffer without emitting an error, so it's probably reasonable.
Before your change only one mark was set, now they both are, and that should be confirmed by the test.
:marks []
mark line col file/text
[ 1 0
vs
:marks []
mark line col file/text
[ 1 0
] 1 0
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
glepnir
@glepnir pushed 1 commit.
- 785f99c fix(mark): keep '[ and ] marks above line 1
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
chrisbra left a comment (vim/vim#19061)If I run the new test right now, without your change to ex_cmds.c, it doesn't fail. That might be fixed now with your latest commit, let me retest it.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
glepnir
glepnir left a comment (vim/vim#19061)weird 🤔 I can get failed on master
image.png (view on web)
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
chrisbra left a comment (vim/vim#19061)There are a few issues here:
type(also I think formatprg should not be set tocmd /c) on MS-Windows does not work, because it does not handle stdin correctly. Funnily, it seemsmoreworks.- I think we should probably return early for an empty buffer in
do_filter()anyhow, this also avoids that the buffer is marked as modified (for a no-op)
The test should probably look like this:
func Test_mark_formatprg_on_empty() new if has('win32') setl formatprg=more else setl formatprg=cat endif call assert_equal([0, 0], [line("'["), col("'[")]) call assert_equal([0, 0], [line("']"), col("']")]) try norm! gqG catch call assert_report("gqG on empty buffer should not fail") endtry call assert_true(empty(v:errmsg)) call assert_equal([0, 0], [line("'["), col("'[")]) call assert_equal([0, 0], [line("']"), col("']")]) call assert_false(&modified) bwipe! endfunc
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
dkearns
dkearns left a comment (vim/vim#19061)
- I think we should probably return early for an empty buffer in
do_filter()anyhow, this also avoids that the buffer is marked as modified (for a no-op)
I thought this might be out of scope for the release but if it's not, there's some inconsistencies in command behaviour. <, for example, also sets 'modified' on an empty buffer. It really needs a full audit.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
chrisbra left a comment (vim/vim#19061)Yeah agree, let's hold off until after the release
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
