[vim/vim] Adding register viminfo partial write warning (PR #14787)

54 views
Skip to first unread message

ElectricPulse

unread,
May 16, 2024, 10:15:47 AMMay 16
to vim/vim, Subscribed

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:

  1. Add a warning when exiting with register size bigger than viminfo
  2. 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)?


You can view, comment on, or merge this pull request online at:

  https://github.com/vim/vim/pull/14787

Commit Summary

  • 68f8cfb Adding register viminfo partial write warning

File Changes

(3 files)

Patch Links:


Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.Message ID: <vim/vim/pull/14787@github.com>

K.Takata

unread,
May 16, 2024, 10:27:34 AMMay 16
to vim/vim, Subscribed

@k-takata commented on this pull request.


In src/viminfo.c:

> @@ -1877,8 +1879,10 @@ write_viminfo_registers(FILE *fp)
 	    len = 0;
 	    for (j = 0; j < num_lines; j++)
 		len += (long)STRLEN(y_ptr->y_array[j]) + 1L;
-	    if (len > (long)max_kbyte * 1024L)
+	    if (len > (long)max_kbyte * 1024L) {
⬇️ Suggested change
-	    if (len > (long)max_kbyte * 1024L) {
+	    if (len > (long)max_kbyte * 1024L)
+	    {

In 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)
+	{

In src/viminfo.c:

> @@ -2949,7 +2958,12 @@ do_viminfo(FILE *fp_in, FILE *fp_out, int flags)
 	write_viminfo_search_pattern(fp_out);
 	write_viminfo_sub_string(fp_out);
 	write_viminfo_history(fp_out, merge);
-	write_viminfo_registers(fp_out);
+
+	if(write_viminfo_registers(fp_out) == FAIL) {
⬇️ Suggested change
-	if(write_viminfo_registers(fp_out) == FAIL) {
+	if (write_viminfo_registers(fp_out) == FAIL)
+	{


Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.Message ID: <vim/vim/pull/14787/review/2060964742@github.com>

Yegappan Lakshmanan

unread,
May 16, 2024, 11:19:06 AMMay 16
to vim/vim, Subscribed

@yegappan commented on this pull request.


In src/viminfo.c:

> @@ -2949,7 +2958,12 @@ do_viminfo(FILE *fp_in, FILE *fp_out, int flags)
 	write_viminfo_search_pattern(fp_out);
 	write_viminfo_sub_string(fp_out);
 	write_viminfo_history(fp_out, merge);
-	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?


Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.Message ID: <vim/vim/pull/14787/review/2061142077@github.com>

Restorer

unread,
May 19, 2024, 5:56:55 AMMay 19
to vim/vim, Subscribed

@RestorerZ commented on this pull request.


In 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:
https://github.com/vim/vim/blob/68f8cfb7efa79510a01dd55d35cb5c7456a7261f/src/buffer.c#L2278
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)?


Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.Message ID: <vim/vim/pull/14787/review/2065039390@github.com>

Restorer

unread,
May 19, 2024, 6:30:25 AMMay 19
to vim/vim, Subscribed

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


Reply to this email directly, view it on GitHub.

You are receiving this because you are subscribed to this thread.Message ID: <vim/vim/pull/14787/c2119184180@github.com>

ElectricPulse

unread,
May 21, 2024, 5:50:33 AMMay 21
to vim/vim, Push

@ElectricPulse pushed 1 commit.


View it on GitHub.
You are receiving this because you are subscribed to this thread.Message ID: <vim/vim/pull/14787/push/18516646103@github.com>

ElectricPulse

unread,
May 21, 2024, 5:50:48 AMMay 21
to vim/vim, Push

@ElectricPulse pushed 1 commit.


View it on GitHub.
You are receiving this because you are subscribed to this thread.Message ID: <vim/vim/pull/14787/push/18516649981@github.com>

ElectricPulse

unread,
May 21, 2024, 5:58:46 AMMay 21
to vim/vim, Subscribed

@ElectricPulse commented on this pull request.


In 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"));

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


Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.Message ID: <vim/vim/pull/14787/review/2068137093@github.com>

ElectricPulse

unread,
May 21, 2024, 9:10:48 AMMay 21
to vim/vim, Push

@ElectricPulse pushed 1 commit.


View it on GitHub.
You are receiving this because you are subscribed to this thread.Message ID: <vim/vim/pull/14787/push/18519892587@github.com>

ElectricPulse

unread,
May 21, 2024, 9:22:59 AMMay 21
to vim/vim, Push

@ElectricPulse pushed 1 commit.


View it on GitHub.
You are receiving this because you are subscribed to this thread.Message ID: <vim/vim/pull/14787/push/18520108509@github.com>

ElectricPulse

unread,
May 21, 2024, 9:54:14 AMMay 21
to vim/vim, Subscribed

Fixed the tests.


Reply to this email directly, view it on GitHub.

You are receiving this because you are subscribed to this thread.Message ID: <vim/vim/pull/14787/c2122692885@github.com>

Christian Brabandt

unread,
May 21, 2024, 5:56:25 PMMay 21
to vim/vim, Subscribed

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 quite 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?


Reply to this email directly, view it on GitHub.

You are receiving this because you are subscribed to this thread.Message ID: <vim/vim/pull/14787/c2123504020@github.com>

Christian Brabandt

unread,
May 21, 2024, 6:14:26 PMMay 21
to vim/vim, Subscribed

@chrisbra commented on this pull request.


In 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).


In src/viminfo.c:

> @@ -1827,6 +1827,8 @@ write_viminfo_registers(FILE *fp)
     long	len;
     yankreg_T	*y_ptr;
     yankreg_T	*y_regs_p = get_y_regs();;
+    // Signifies partial write
+    int		part_write = OK;
⬇️ Suggested change
-    int		part_write = OK;
+    int		retval = OK;

In src/testdir/test_viminfo.vim:

> @@ -315,6 +315,13 @@ func Test_cmdline_history_order()
   call delete('Xviminfo')
 endfunc
 
+func Force_write_viminfo()
+  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)


Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.Message ID: <vim/vim/pull/14787/review/2069703534@github.com>

ElectricPulse

unread,
May 22, 2024, 1:58:50 PMMay 22
to vim/vim, Push

@ElectricPulse pushed 1 commit.


View it on GitHub.
You are receiving this because you are subscribed to this thread.Message ID: <vim/vim/pull/14787/push/18542745996@github.com>

ElectricPulse

unread,
May 22, 2024, 1:59:13 PMMay 22
to vim/vim, Subscribed

@ElectricPulse commented on this pull request.


In src/testdir/test_viminfo.vim:

> @@ -315,6 +315,13 @@ func Test_cmdline_history_order()
   call delete('Xviminfo')
 endfunc
 
+func Force_write_viminfo()
+  redir => res
+  silent! wviminfo Xviminfo
+  redir END
+  call assert_match('W23:', res)

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


Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.Message ID: <vim/vim/pull/14787/review/2071969649@github.com>

ElectricPulse

unread,
May 22, 2024, 2:03:01 PMMay 22
to vim/vim, Subscribed

@ElectricPulse commented on this pull request.


In 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"}

fixed that


Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.Message ID: <vim/vim/pull/14787/review/2071976738@github.com>

ElectricPulse

unread,
May 22, 2024, 2:03:20 PMMay 22
to vim/vim, Push

@ElectricPulse pushed 1 commit.

  • 5b9e711 replaced :redir with :execute and changed defaults for MSWIN


View it on GitHub.
You are receiving this because you are subscribed to this thread.Message ID: <vim/vim/pull/14787/push/18542810204@github.com>

ElectricPulse

unread,
May 23, 2024, 7:00:32 AMMay 23
to vim/vim, Push

@ElectricPulse pushed 1 commit.


View it on GitHub.
You are receiving this because you are subscribed to this thread.Message ID: <vim/vim/pull/14787/push/18554002198@github.com>

K.Takata

unread,
May 23, 2024, 7:10:00 AMMay 23
to vim/vim, Subscribed

@k-takata commented on this pull request.


In 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"}

options.txt should be also updated accordingly:
https://github.com/vim/vim/blob/701ad50a9efcf0adfe6d787b606c4e4dbd31f26d/runtime/doc/options.txt#L9032-L9034


Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.Message ID: <vim/vim/pull/14787/review/2073632452@github.com>

Christian Brabandt

unread,
May 23, 2024, 7:30:37 AMMay 23
to vim/vim, Subscribed

@chrisbra commented on this pull request.


In 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?


Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.Message ID: <vim/vim/pull/14787/review/2073678138@github.com>

Christian Brabandt

unread,
May 23, 2024, 3:01:27 PMMay 23
to vim/vim, Subscribed

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


Reply to this email directly, view it on GitHub.

You are receiving this because you are subscribed to this thread.Message ID: <vim/vim/pull/14787/c2127839245@github.com>

Christian Brabandt

unread,
Jun 2, 2024, 2:04:47 PMJun 2
to vim/vim, Subscribed

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


Reply to this email directly, view it on GitHub.

You are receiving this because you are subscribed to this thread.Message ID: <vim/vim/pull/14787/c2143972111@github.com>

ElectricPulse

unread,
Jun 3, 2024, 3:15:31 PMJun 3
to vim/vim, Subscribed

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

Yes, it is a macos test failing and my macos vm is taking forever to install.


Reply to this email directly, view it on GitHub.

You are receiving this because you are subscribed to this thread.Message ID: <vim/vim/pull/14787/c2145938708@github.com>

Christian Brabandt

unread,
Jun 3, 2024, 3:49:31 PMJun 3
to vim/vim, Subscribed

This one:

Expected {'id': 15, 'jsonrpc': '2.0', 'result': 'delayed-payload'} but got {}

is a known flaky one.

Not sure about Test_popupwin_beval_3.dump for macos 12, huge. Let me try to re-trigger this one.


Reply to this email directly, view it on GitHub.

You are receiving this because you are subscribed to this thread.Message ID: <vim/vim/pull/14787/c2145991352@github.com>

D. Ben Knoble

unread,
Jun 16, 2024, 4:47:57 PM (10 days ago) Jun 16
to vim/vim, Subscribed

A late suggestion, but one I hope will be considered: perhaps this is better hooked into 'confirm'? I know current documentation says it's about fallible operations due to unsaved changes, but this feels like it's in that vein of problems (give me the opportunity to notice that something is wrong when quitting).

In fact, I suspect most of the warning + prompt code is already available in the confirm implementation; this would only need to perform the check from this PR in the right spot?


Reply to this email directly, view it on GitHub.

You are receiving this because you are subscribed to this thread.Message ID: <vim/vim/pull/14787/c2171873074@github.com>

Reply all
Reply to author
Forward
0 new messages