Adding register viminfo partial write warning #14787

ElectricPulse · 2024-05-16T14:14:47Z

The following often happens to me:
I cut ("c" command) 100+ lines from file1, close file1, paste into file2, realize I had lost 50+ lines of text, which can't be recovered from file1 because it was cut out and the file closed.
I realize that this problem has many solutions (use one vim session, :set viminfo, use system clipboard &c) yet this still happens to me, especially on machines where I forgot to :set viminfo in vimrc and where there is no system clipboard.
For the sake of noobs I decided to atleast give a warning when such a thing is about to happen.

This feature request tries to address this in the following ways:

Add a warning when exiting with register size bigger than viminfo
Increasing viminfo register max line count from default 50 lines to 500 lines

Note 1: Regarding the warning, it doesn't prevent the user from exiting, it just displays the messages and after pressing Enter, exits. This is because even though I changed it so that do_viminfo now returns a FAIL or OK status, write_viminfo doesn't return such an error because I have no idea of how about implementing the more intrusive feature of an getout failing, ie. ex_exit failing.

Note 2: What error number should be applied to the new warning (I set it to 2000)?

src/viminfo.c

k-takata · 2024-05-16T14:24:49Z

src/viminfo.c

@@ -1906,8 +1910,10 @@ write_viminfo_registers(FILE *fp)
 	fprintf(fp, "\t%s\t%d\n", type, (int)y_ptr->y_width);

 	// If max_num_lines < 0, then we save ALL the lines in the register
-	if (max_num_lines > 0 && num_lines > max_num_lines)
+	if (max_num_lines > 0 && num_lines > max_num_lines) {


Suggested change

if (max_num_lines > 0 && num_lines > max_num_lines) {

if (max_num_lines > 0 && num_lines > max_num_lines)

{

src/viminfo.c

yegappan · 2024-05-16T15:18:07Z

src/viminfo.c

-	write_viminfo_registers(fp_out);
+
+	if(write_viminfo_registers(fp_out) == FAIL) {
+		emsg(_(e_warning_registers_partially_written_to_viminfo));


Can you add tests for these changes to the testdir/test_viminfo.vim file?

RestorerZ · 2024-05-19T09:56:03Z

src/errors.h

+EXTERN char e_warning_registers_partially_written_to_viminfo[]
+	INIT(= N_("E2000: Warning: registers only partially written to viminfo, increase viminfo size"));


If I understood your description correctly, then this is not an error, but a warning.
Like this:

vim/src/buffer.c

Line 2278 in 68f8cfb

emsg(_("W14: Warning: List of file names overflow"));

Accordingly, the number should not start with the letter "E", but with the letter "W". The last warning number was "W22".

Note 2: What error number should be applied to the new warning (I set it to 2000)?

Oh okay and why are there Warnings under src/errors.h?

RestorerZ · 2024-05-19T10:29:34Z

It might be a good idea to add a description of this warning.
For example, here: https://vimhelp.org/options.txt.html#%27viminfo%27

Co-authored-by: K.Takata <kentkt@csc.jp>

ElectricPulse · 2024-05-21T13:53:19Z

Fixed the tests.

chrisbra · 2024-05-21T21:55:35Z

Note 1: Regarding the warning, it doesn't prevent the user from exiting, it just displays the messages and after pressing Enter, exits. This is because even though I changed it so that do_viminfo now returns a FAIL or OK status, write_viminfo doesn't return such an error because I have no idea of how about implementing the more intrusive feature of an getout failing, ie. ex_exit failing.

I checked this and this got me wondering. I see, that there is already some error handling, that simply shows an error message but does still exit Vim after the user presses Enter. So I am wondering how useful this is? It's certainly too late for the user to do anything against this and fix the warning. But on the other side, with the newly increased defaults, perhaps this is something not to worry about it for now?

chrisbra · 2024-05-21T22:01:15Z

src/optiondefs.h

@@ -2769,7 +2769,7 @@ static struct vimoption options[] =
 			    {(char_u *)"",
 				 (char_u *)"'100,<50,s10,h,rdf0:,rdf1:,rdf2:"}
 #else
-			    {(char_u *)"", (char_u *)"'100,<50,s10,h"}
+			    {(char_u *)"", (char_u *)"'100,<500,s100,h"}


If we change the default, I think we should also increase it for at least Windows (see above).

options.txt should be also updated accordingly:

vim/runtime/doc/options.txt

Lines 9032 to 9034 in 701ad50

MS-Windows: '100,<50,s10,h,rA:,rB:,

for Amiga: '100,<50,s10,h,rdf0:,rdf1:,rdf2:

for others: '100,<50,s10,h)

src/viminfo.c

chrisbra · 2024-05-21T22:12:52Z

src/testdir/test_viminfo.vim

+  redir => res
+  silent! wviminfo Xviminfo
+  redir END
+  call assert_match('W23:', res)


instead of :redir, you can use execute():

Suggested change

call assert_match('W23:', res)

let res = execute('wviminfo Xviminfo')

call assert_match('W23:', res)

I commited it along with the silent! since that allows the capturing of the warning

Co-authored-by: Christian Brabandt <cb@256bit.org>

chrisbra · 2024-05-23T11:28:51Z

src/viminfo.c

@@ -2895,17 +2905,18 @@ read_viminfo_up_to_marks(
 /*
 * do_viminfo() -- Should only be called from read_viminfo() & write_viminfo().
 */
-    static void
+    static int


What is the reason for returning a return value here? It seems like all callers of do_viminfo() do not check the return value, so I guess it is no needed?

chrisbra · 2024-05-23T19:00:29Z

I did check the vim-history repo and the s flag (max size of registers) has been added to the viminfo option as of patch 6.2.393 (from around Mar 2004). So I think, increasing the limit by 10 times to 100 KBytes should be okay after more than 20 years. I didn't try to find when the </" item has been set initially. So all in all I think this is good change

chrisbra · 2024-06-02T18:03:59Z

@ElectricPulse are you going to address those few points please?

Adding register viminfo partial write warning

68f8cfb

k-takata reviewed May 16, 2024

View reviewed changes

yegappan reviewed May 16, 2024

View reviewed changes

RestorerZ reviewed May 19, 2024

View reviewed changes

ElectricPulse and others added 4 commits May 21, 2024 11:49

Update src/viminfo.c

26a4ccb

Co-authored-by: K.Takata <kentkt@csc.jp>

Update src/viminfo.c

4672fca

Co-authored-by: K.Takata <kentkt@csc.jp>

fixed tests

1782976

fixed codestyle :|

522a1cb

chrisbra reviewed May 21, 2024

View reviewed changes

ElectricPulse and others added 3 commits May 22, 2024 19:57

Update src/viminfo.c

e0a3c1a

Co-authored-by: Christian Brabandt <cb@256bit.org>

replaced :redir with :execute and changed defaults for MSWIN

5b9e711

fixed compilation

68abff5

chrisbra reviewed May 23, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding register viminfo partial write warning #14787

Adding register viminfo partial write warning #14787

ElectricPulse commented May 16, 2024

k-takata May 16, 2024

yegappan May 16, 2024

RestorerZ May 19, 2024

ElectricPulse May 21, 2024

RestorerZ commented May 19, 2024

ElectricPulse commented May 21, 2024

chrisbra commented May 21, 2024 •

edited

chrisbra May 21, 2024

ElectricPulse May 22, 2024

k-takata May 23, 2024

chrisbra May 21, 2024

ElectricPulse May 22, 2024

chrisbra May 23, 2024

chrisbra commented May 23, 2024

chrisbra commented Jun 2, 2024

	if (max_num_lines > 0 && num_lines > max_num_lines) {
	if (max_num_lines > 0 && num_lines > max_num_lines)
	{

		EXTERN char e_warning_registers_partially_written_to_viminfo[]
		INIT(= N_("E2000: Warning: registers only partially written to viminfo, increase viminfo size"));

	MS-Windows: '100,<50,s10,h,rA:,rB:,
	for Amiga: '100,<50,s10,h,rdf0:,rdf1:,rdf2:
	for others: '100,<50,s10,h)

	call assert_match('W23:', res)
	let res = execute('wviminfo Xviminfo')
	call assert_match('W23:', res)

Adding register viminfo partial write warning #14787

Are you sure you want to change the base?

Adding register viminfo partial write warning #14787

Conversation

ElectricPulse commented May 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RestorerZ commented May 19, 2024

ElectricPulse commented May 21, 2024

chrisbra commented May 21, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbra commented May 23, 2024

chrisbra commented Jun 2, 2024

chrisbra commented May 21, 2024 •

edited