[vim/vim] Adding register viminfo partial write warning (PR #14787)
ElectricPulse
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)?
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)
- M src/errors.h (2)
- M src/optiondefs.h (2)
- M src/viminfo.c (32)
Patch Links:
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
K.Takata
@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.
Yegappan Lakshmanan
@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.
Restorer
@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.
Restorer
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.
ElectricPulse
@ElectricPulse pushed 1 commit.
- 26a4ccb Update src/viminfo.c
—
View it on GitHub.
You are receiving this because you are subscribed to this thread.
ElectricPulse
@ElectricPulse pushed 1 commit.
- 4672fca Update src/viminfo.c
—
View it on GitHub.
You are receiving this because you are subscribed to this thread.
ElectricPulse
@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.
ElectricPulse
@ElectricPulse pushed 1 commit.
- 1782976 fixed tests
—
View it on GitHub.
You are receiving this because you are subscribed to this thread.
ElectricPulse
@ElectricPulse pushed 1 commit.
- 522a1cb fixed codestyle :|
—
View it on GitHub.
You are receiving this because you are subscribed to this thread.
ElectricPulse
Fixed the tests.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
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.
Christian Brabandt
@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():
- 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.
ElectricPulse
@ElectricPulse pushed 1 commit.
- e0a3c1a Update src/viminfo.c
—
View it on GitHub.
You are receiving this because you are subscribed to this thread.
ElectricPulse
@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.
ElectricPulse
@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.
ElectricPulse
@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.
ElectricPulse
@ElectricPulse pushed 1 commit.
- 68abff5 fixed compilation
—
View it on GitHub.
You are receiving this because you are subscribed to this thread.
K.Takata
@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.
Christian Brabandt
@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.
Christian Brabandt
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.
Christian Brabandt
@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.
ElectricPulse
@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.
Christian Brabandt
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.
D. Ben Knoble
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.