From 607e0d2202053a2db290f00d99364f4895917f39 Mon Sep 17 00:00:00 2001 From: ZyX Date: Wed, 4 Jan 2017 18:45:10 +0300 Subject: [PATCH 01/14] shada: Save numbered marks Problems so far: - Marks in the current instance are not adjusted. - Duplicates are not removed (not that it works in Vim either now, not at 8.0.134 at least). --- src/nvim/macros.h | 18 +++-- src/nvim/quickfix.c | 2 +- src/nvim/shada.c | 165 +++++++++++++++++++++++++++++++++++--------- 3 files changed, 147 insertions(+), 38 deletions(-) diff --git a/src/nvim/macros.h b/src/nvim/macros.h index 4e01265498..e802ba2aed 100644 --- a/src/nvim/macros.h +++ b/src/nvim/macros.h @@ -136,13 +136,21 @@ # define RESET_BINDING(wp) (wp)->w_p_scb = FALSE; (wp)->w_p_crb = FALSE -/// Calculate the length of a C array. +/// Calculate the length of a C array /// /// This should be called with a real array. Calling this with a pointer is an -/// error. A mechanism to detect many (though not all) of those errors at compile -/// time is implemented. It works by the second division producing a division by -/// zero in those cases (-Wdiv-by-zero in GCC). -#define ARRAY_SIZE(arr) ((sizeof(arr)/sizeof((arr)[0])) / ((size_t)(!(sizeof(arr) % sizeof((arr)[0]))))) +/// error. A mechanism to detect many (though not all) of those errors at +/// compile time is implemented. It works by the second division producing +/// a division by zero in those cases (-Wdiv-by-zero in GCC). +#define ARRAY_SIZE(arr) \ + ((sizeof(arr)/sizeof((arr)[0])) \ + / ((size_t)(!(sizeof(arr) % sizeof((arr)[0]))))) + +/// Get last array entry +/// +/// This should be called with a real array. Calling this with a pointer is an +/// error. +#define LAST_ARRAY_ENTRY(arr) (arr)[ARRAY_SIZE(arr) - 1] // Duplicated in os/win_defs.h to avoid include-order sensitivity. #define RGB_(r, g, b) ((r << 16) | (g << 8) | b) diff --git a/src/nvim/quickfix.c b/src/nvim/quickfix.c index 2d8c353f92..46124e0672 100644 --- a/src/nvim/quickfix.c +++ b/src/nvim/quickfix.c @@ -3029,7 +3029,7 @@ static void qf_fill_buffer(qf_info_T *qi, buf_T *buf, qfline_T *old_last) /* * Return TRUE if "buf" is the quickfix buffer. */ -int bt_quickfix(buf_T *buf) +int bt_quickfix(const buf_T *const buf) { return buf != NULL && buf->b_p_bt[0] == 'q'; } diff --git a/src/nvim/shada.c b/src/nvim/shada.c index 605d9c30a6..5014a88917 100644 --- a/src/nvim/shada.c +++ b/src/nvim/shada.c @@ -22,6 +22,7 @@ #include "nvim/globals.h" #include "nvim/memory.h" #include "nvim/mark.h" +#include "nvim/macros.h" #include "nvim/ops.h" #include "nvim/garray.h" #include "nvim/option.h" @@ -375,7 +376,8 @@ KHASH_MAP_INIT_STR(file_marks, FileMarks) /// Before actually writing most of the data is read to this structure. typedef struct { HistoryMergerState hms[HIST_COUNT]; ///< Structures for history merging. - PossiblyFreedShadaEntry global_marks[NGLOBALMARKS]; ///< All global marks. + PossiblyFreedShadaEntry global_marks[NMARKS]; ///< Named global marks. + PossiblyFreedShadaEntry numbered_marks[EXTRA_MARKS]; ///< Numbered marks. PossiblyFreedShadaEntry registers[NUM_SAVED_REGISTERS]; ///< All registers. PossiblyFreedShadaEntry jumps[JUMPLISTSIZE]; ///< All dumped jumps. size_t jumps_size; ///< Number of jumps occupied. @@ -2071,9 +2073,12 @@ static inline ShaDaWriteResult shada_read_when_writing( shada_free_shada_entry(&wms_entry->data); \ } \ } \ - wms_entry->can_free_entry = true; \ - wms_entry->data = (entry); \ + *wms_entry = pfs_entry; \ } while (0) + const PossiblyFreedShadaEntry pfs_entry = { + .can_free_entry = true, + .data = entry, + }; switch (entry.type) { case kSDItemMissing: { break; @@ -2125,13 +2130,46 @@ static inline ShaDaWriteResult shada_read_when_writing( break; } case kSDItemGlobalMark: { - const int idx = mark_global_index(entry.data.filemark.name); - if (idx < 0) { - ret = shada_pack_entry(packer, entry, 0); - shada_free_shada_entry(&entry); - break; + if (ascii_isdigit(entry.data.filemark.name)) { + bool processed_mark = false; + // Completely ignore numbered mark names, make a list sorted by + // timestamp. + for (size_t i = ARRAY_SIZE(wms->numbered_marks); i > 0; i--) { + ShadaEntry wms_entry = wms->numbered_marks[i - 1].data; + if (wms_entry.type != kSDItemGlobalMark) { + continue; + } + // Ignore duplicates. + if (wms_entry.timestamp == entry.timestamp + && (wms_entry.data.filemark.additional_data == NULL + && entry.data.filemark.additional_data == NULL) + && marks_equal(wms_entry.data.filemark.mark, + entry.data.filemark.mark) + && strcmp(wms_entry.data.filemark.fname, + entry.data.filemark.fname) == 0) { + processed_mark = true; + break; + } + if (wms_entry.timestamp >= entry.timestamp) { + processed_mark = true; + if (i < ARRAY_SIZE(wms->numbered_marks)) { + replace_numbered_mark(wms, i, pfs_entry); + } + break; + } + } + if (!processed_mark) { + replace_numbered_mark(wms, 0, pfs_entry); + } + } else { + const int idx = mark_global_index(entry.data.filemark.name); + if (idx < 0) { + ret = shada_pack_entry(packer, entry, 0); + shada_free_shada_entry(&entry); + break; + } + COMPARE_WITH_ENTRY(&wms->global_marks[idx], entry); } - COMPARE_WITH_ENTRY(&wms->global_marks[idx], entry); break; } case kSDItemChange: @@ -2175,8 +2213,7 @@ static inline ShaDaWriteResult shada_read_when_writing( shada_free_shada_entry(&wms_entry->data); } } - wms_entry->can_free_entry = true; - wms_entry->data = entry; + *wms_entry = pfs_entry; } } else { #define FREE_POSSIBLY_FREED_SHADA_ENTRY(entry) \ @@ -2216,6 +2253,20 @@ static inline ShaDaWriteResult shada_read_when_writing( return ret; } +/// Check whether buffer should be ignored +/// +/// @param[in] buf buf_T* to check. +/// @param[in] removable_bufs Cache of buffers ignored due to their location. +/// +/// @return true or false. +static bool ignore_buf(const buf_T *const buf, + khash_t(bufset) *const removable_bufs) + FUNC_ATTR_PURE FUNC_ATTR_WARN_UNUSED_RESULT FUNC_ATTR_ALWAYS_INLINE +{ + return (buf->b_ffname == NULL || !buf->b_p_bl || bt_quickfix(buf) \ + || in_bufset(removable_bufs, buf)); +} + /// Get list of buffers to write to the shada file /// /// @param[in] removable_bufs Buffers which are ignored @@ -2227,11 +2278,9 @@ static inline ShadaEntry shada_get_buflist( { int max_bufs = get_shada_parameter('%'); size_t buf_count = 0; -#define IGNORE_BUF(buf)\ - (buf->b_ffname == NULL || !buf->b_p_bl || bt_quickfix(buf) \ - || in_bufset(removable_bufs, buf)) // NOLINT(whitespace/indent) FOR_ALL_BUFFERS(buf) { - if (!IGNORE_BUF(buf) && (max_bufs < 0 || buf_count < (size_t)max_bufs)) { + if (!ignore_buf(buf, removable_bufs) + && (max_bufs < 0 || buf_count < (size_t)max_bufs)) { buf_count++; } } @@ -2249,7 +2298,7 @@ static inline ShadaEntry shada_get_buflist( }; size_t i = 0; FOR_ALL_BUFFERS(buf) { - if (IGNORE_BUF(buf)) { + if (ignore_buf(buf, removable_bufs)) { continue; } if (i >= buf_count) { @@ -2263,7 +2312,6 @@ static inline ShadaEntry shada_get_buflist( i++; } -#undef IGNORE_BUF return buflist_entry; } @@ -2365,6 +2413,34 @@ static inline void shada_initialize_registers(WriteMergerState *const wms, } while (reg_iter != NULL); } +/// Replace numbered mark in WriteMergerState +/// +/// Frees the last mark, moves (including adjusting mark names) marks from idx +/// to the last-but-one one and saves the new mark at given index. +/// +/// @param[out] wms Merger state to adjust. +/// @param[in] idx Index at which new mark should be placed. +/// @param[in] entry New mark. +static inline void replace_numbered_mark(WriteMergerState *const wms, + const size_t idx, + const PossiblyFreedShadaEntry entry) + FUNC_ATTR_NONNULL_ALL FUNC_ATTR_ALWAYS_INLINE +{ + if (LAST_ARRAY_ENTRY(wms->numbered_marks).can_free_entry) { + shada_free_shada_entry(&LAST_ARRAY_ENTRY(wms->numbered_marks).data); + } + for (size_t i = idx; i < ARRAY_SIZE(wms->numbered_marks) - 1; i++) { + if (wms->numbered_marks[i].data.type == kSDItemGlobalMark) { + wms->numbered_marks[i].data.data.filemark.name++; + assert(ascii_isdigit(wms->numbered_marks[i].data.data.filemark.name)); + } + } + memmove(wms->numbered_marks + idx + 1, wms->numbered_marks + idx, + sizeof(wms->numbered_marks[0]) + * (ARRAY_SIZE(wms->numbered_marks) - 1 - idx)); + wms->numbered_marks[idx] = entry; +} + /// Write ShaDa file /// /// @param[in] sd_writer Structure containing file writer definition. @@ -2619,21 +2695,24 @@ static ShaDaWriteResult shada_write(ShaDaWriteDef *const sd_writer, } fname = (const char *) buf->b_ffname; } - wms->global_marks[mark_global_index(name)] = (PossiblyFreedShadaEntry) { - .can_free_entry = false, - .data = { - .type = kSDItemGlobalMark, - .timestamp = fm.fmark.timestamp, - .data = { - .filemark = { - .mark = fm.fmark.mark, - .name = name, - .additional_data = fm.fmark.additional_data, - .fname = (char *) fname, - } - } - }, - }; + *(ascii_isdigit(name) + ? &wms->numbered_marks[name - '0'] + : &wms->global_marks[mark_global_index(name)]) = ( + (PossiblyFreedShadaEntry) { + .can_free_entry = false, + .data = { + .type = kSDItemGlobalMark, + .timestamp = fm.fmark.timestamp, + .data = { + .filemark = { + .mark = fm.fmark.mark, + .name = name, + .additional_data = fm.fmark.additional_data, + .fname = (char *)fname, + } + } + }, + }); } while (global_mark_iter != NULL); } @@ -2715,6 +2794,26 @@ static ShaDaWriteResult shada_write(ShaDaWriteDef *const sd_writer, } } + // Update numbered marks: '0' should be replaced with the current position, + // '9' should be removed and all other marks shifted. + if (!ignore_buf(curbuf, &removable_bufs)) { + replace_numbered_mark(wms, 0, (PossiblyFreedShadaEntry) { + .can_free_entry = false, + .data = { + .type = kSDItemGlobalMark, + .timestamp = os_time(), + .data = { + .filemark = { + .mark = curwin->w_cursor, + .name = '0', + .additional_data = NULL, + .fname = (char *)curbuf->b_ffname, + } + } + }, + }); + } + // Write the rest #define PACK_WMS_ARRAY(wms_array) \ do { \ @@ -2729,6 +2828,7 @@ static ShaDaWriteResult shada_write(ShaDaWriteDef *const sd_writer, } \ } while (0) PACK_WMS_ARRAY(wms->global_marks); + PACK_WMS_ARRAY(wms->numbered_marks); PACK_WMS_ARRAY(wms->registers); for (size_t i = 0; i < wms->jumps_size; i++) { if (shada_pack_pfreed_entry(packer, wms->jumps[i], max_kbyte) @@ -2823,6 +2923,7 @@ shada_write_exit: return ret; } +#undef IGNORE_BUF #undef PACK_STATIC_STR /// Write ShaDa file to a given location From 17ea0f2214f13b11305e380d9e93e7577b7675aa Mon Sep 17 00:00:00 2001 From: ZyX Date: Sun, 8 Jan 2017 05:23:45 +0300 Subject: [PATCH 02/14] functests: Fix existing functional tests --- test/functional/shada/merging_spec.lua | 27 +++++++++++++++----------- test/functional/shada/shada_spec.lua | 6 +++--- 2 files changed, 19 insertions(+), 14 deletions(-) diff --git a/test/functional/shada/merging_spec.lua b/test/functional/shada/merging_spec.lua index 7a15c8908b..1a289a2de7 100644 --- a/test/functional/shada/merging_spec.lua +++ b/test/functional/shada/merging_spec.lua @@ -563,13 +563,14 @@ describe('ShaDa marks support code', function() nvim_command('normal! `A') eq('-', funcs.fnamemodify(curbufmeths.get_name(), ':t')) eq(0, exc_exec('wshada ' .. shada_fname)) - local found = 0 + local found = {} for _, v in ipairs(read_shada_file(shada_fname)) do - if v.type == 7 and v.value.f == '' .. mock_file_path .. '-' then - found = found + 1 + if v.type == 7 and v.value.f == mock_file_path .. '-' then + local name = ('%c'):format(v.value.n) + found[name] = (found[name] or 0) + 1 end end - eq(1, found) + eq({['0']=1, A=1}, found) end) it('uses last A mark with eq timestamp from instance when writing', @@ -580,13 +581,14 @@ describe('ShaDa marks support code', function() nvim_command('normal! `A') eq('-', funcs.fnamemodify(curbufmeths.get_name(), ':t')) eq(0, exc_exec('wshada ' .. shada_fname)) - local found = 0 + local found = {} for _, v in ipairs(read_shada_file(shada_fname)) do if v.type == 7 and v.value.f == mock_file_path .. '-' then - found = found + 1 + local name = ('%c'):format(v.value.n) + found[name] = (found[name] or 0) + 1 end end - eq(1, found) + eq({['0']=1, A=1}, found) end) it('uses last A mark with gt timestamp from file when writing', @@ -597,13 +599,16 @@ describe('ShaDa marks support code', function() nvim_command('normal! `A') eq('-', funcs.fnamemodify(curbufmeths.get_name(), ':t')) eq(0, exc_exec('wshada ' .. shada_fname)) - local found = 0 + local found = {} for _, v in ipairs(read_shada_file(shada_fname)) do - if v.type == 7 and v.value.f == '' .. mock_file_path .. '?' then - found = found + 1 + if v.type == 7 then + local name = ('%c'):format(v.value.n) + local t = found[name] or {} + t[v.value.f] = (t[v.value.f] or 0) + 1 + found[name] = t end end - eq(1, found) + eq({['0']={['/a/b/-']=1}, A={['/a/b/?']=1}}, found) end) it('uses last a mark with gt timestamp from instance when reading', diff --git a/test/functional/shada/shada_spec.lua b/test/functional/shada/shada_spec.lua index ca44026852..720855860a 100644 --- a/test/functional/shada/shada_spec.lua +++ b/test/functional/shada/shada_spec.lua @@ -181,13 +181,13 @@ describe('ShaDa support code', function() nvim_command('set shada+=%') nvim_command('wshada! ' .. shada_fname) local readme_fname = funcs.resolve(paths.test_source_path) .. '/README.md' - eq({[7]=1, [8]=2, [9]=1, [10]=4, [11]=1}, find_file(readme_fname)) + eq({[7]=2, [8]=2, [9]=1, [10]=4, [11]=1}, find_file(readme_fname)) nvim_command('set shada+=r~') nvim_command('wshada! ' .. shada_fname) eq({}, find_file(readme_fname)) nvim_command('set shada-=r~') nvim_command('wshada! ' .. shada_fname) - eq({[7]=1, [8]=2, [9]=1, [10]=4, [11]=1}, find_file(readme_fname)) + eq({[7]=2, [8]=2, [9]=1, [10]=4, [11]=1}, find_file(readme_fname)) nvim_command('set shada+=r' .. funcs.escape( funcs.escape(paths.test_source_path, '$~'), ' "\\,')) nvim_command('wshada! ' .. shada_fname) @@ -206,7 +206,7 @@ describe('ShaDa support code', function() nvim_command('undo') nvim_command('set shada+=%') nvim_command('wshada! ' .. shada_fname) - eq({[7]=1, [8]=2, [9]=1, [10]=4, [11]=2}, find_file(fname)) + eq({[7]=2, [8]=2, [9]=1, [10]=4, [11]=2}, find_file(fname)) nvim_command('set shada+=r' .. pwd .. '/АБВ') nvim_command('wshada! ' .. shada_fname) eq({}, find_file(fname)) From 0bfc91be9642eae6a456d0bf88ca609dcc178fa5 Mon Sep 17 00:00:00 2001 From: ZyX Date: Wed, 11 Jan 2017 20:55:56 +0300 Subject: [PATCH 03/14] shada: Make ignore_buf also inline --- src/nvim/shada.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/nvim/shada.c b/src/nvim/shada.c index 5014a88917..f0ab1ff73f 100644 --- a/src/nvim/shada.c +++ b/src/nvim/shada.c @@ -2259,8 +2259,8 @@ static inline ShaDaWriteResult shada_read_when_writing( /// @param[in] removable_bufs Cache of buffers ignored due to their location. /// /// @return true or false. -static bool ignore_buf(const buf_T *const buf, - khash_t(bufset) *const removable_bufs) +static inline bool ignore_buf(const buf_T *const buf, + khash_t(bufset) *const removable_bufs) FUNC_ATTR_PURE FUNC_ATTR_WARN_UNUSED_RESULT FUNC_ATTR_ALWAYS_INLINE { return (buf->b_ffname == NULL || !buf->b_p_bl || bt_quickfix(buf) \ From 7941aaa3bf1acfff4fcf910c76186252e9ae3f6c Mon Sep 17 00:00:00 2001 From: ZyX Date: Wed, 11 Jan 2017 20:56:45 +0300 Subject: [PATCH 04/14] macros: Rename LAST_ARRAY_ENTRY to ARRAY_LAST_ENTRY --- src/nvim/macros.h | 2 +- src/nvim/shada.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/nvim/macros.h b/src/nvim/macros.h index e802ba2aed..348df2d9b6 100644 --- a/src/nvim/macros.h +++ b/src/nvim/macros.h @@ -150,7 +150,7 @@ /// /// This should be called with a real array. Calling this with a pointer is an /// error. -#define LAST_ARRAY_ENTRY(arr) (arr)[ARRAY_SIZE(arr) - 1] +#define ARRAY_LAST_ENTRY(arr) (arr)[ARRAY_SIZE(arr) - 1] // Duplicated in os/win_defs.h to avoid include-order sensitivity. #define RGB_(r, g, b) ((r << 16) | (g << 8) | b) diff --git a/src/nvim/shada.c b/src/nvim/shada.c index f0ab1ff73f..fbee9beacc 100644 --- a/src/nvim/shada.c +++ b/src/nvim/shada.c @@ -2426,8 +2426,8 @@ static inline void replace_numbered_mark(WriteMergerState *const wms, const PossiblyFreedShadaEntry entry) FUNC_ATTR_NONNULL_ALL FUNC_ATTR_ALWAYS_INLINE { - if (LAST_ARRAY_ENTRY(wms->numbered_marks).can_free_entry) { - shada_free_shada_entry(&LAST_ARRAY_ENTRY(wms->numbered_marks).data); + if (ARRAY_LAST_ENTRY(wms->numbered_marks).can_free_entry) { + shada_free_shada_entry(&ARRAY_LAST_ENTRY(wms->numbered_marks).data); } for (size_t i = idx; i < ARRAY_SIZE(wms->numbered_marks) - 1; i++) { if (wms->numbered_marks[i].data.type == kSDItemGlobalMark) { From 30e7fb2e32a636ba9713abc6545fab6f0ea6183c Mon Sep 17 00:00:00 2001 From: ZyX Date: Mon, 26 Mar 2018 00:45:38 +0300 Subject: [PATCH 05/14] shada: Also filter out invalid cursor position when writing '0' mark Based on https://github.com/neovim/neovim/pull/5908#issuecomment-375909903, but with adjusted condition as line number or column less then zero should not appear at all based on what I know. --- src/nvim/shada.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/nvim/shada.c b/src/nvim/shada.c index fbee9beacc..f726f09fad 100644 --- a/src/nvim/shada.c +++ b/src/nvim/shada.c @@ -2796,7 +2796,7 @@ static ShaDaWriteResult shada_write(ShaDaWriteDef *const sd_writer, // Update numbered marks: '0' should be replaced with the current position, // '9' should be removed and all other marks shifted. - if (!ignore_buf(curbuf, &removable_bufs)) { + if (!ignore_buf(curbuf, &removable_bufs) && curwin->w_cursor.lnum != 0) { replace_numbered_mark(wms, 0, (PossiblyFreedShadaEntry) { .can_free_entry = false, .data = { From aa728798b4bd89b59cee86103885c15d386f73ba Mon Sep 17 00:00:00 2001 From: ZyX Date: Tue, 27 Mar 2018 01:11:38 +0300 Subject: [PATCH 06/14] shada: In place of ignoring cursor position with lnum 0 save with 1 --- src/nvim/shada.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/nvim/shada.c b/src/nvim/shada.c index f726f09fad..552dd2dcf4 100644 --- a/src/nvim/shada.c +++ b/src/nvim/shada.c @@ -2796,7 +2796,7 @@ static ShaDaWriteResult shada_write(ShaDaWriteDef *const sd_writer, // Update numbered marks: '0' should be replaced with the current position, // '9' should be removed and all other marks shifted. - if (!ignore_buf(curbuf, &removable_bufs) && curwin->w_cursor.lnum != 0) { + if (!ignore_buf(curbuf, &removable_bufs)) { replace_numbered_mark(wms, 0, (PossiblyFreedShadaEntry) { .can_free_entry = false, .data = { @@ -2804,7 +2804,9 @@ static ShaDaWriteResult shada_write(ShaDaWriteDef *const sd_writer, .timestamp = os_time(), .data = { .filemark = { - .mark = curwin->w_cursor, + .mark = (curwin->w_cursor.lnum + ? curwin->w_cursor + : (pos_T) { 1, 0, 0 }), .name = '0', .additional_data = NULL, .fname = (char *)curbuf->b_ffname, From 920c582320085fa4bf0cf467fff572af47126ff7 Mon Sep 17 00:00:00 2001 From: ZyX Date: Tue, 27 Mar 2018 01:30:11 +0300 Subject: [PATCH 07/14] test/helpers: Support booleans --- test/helpers.lua | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/helpers.lua b/test/helpers.lua index 91ceed4df1..7cce6d8c55 100644 --- a/test/helpers.lua +++ b/test/helpers.lua @@ -469,6 +469,8 @@ format_luav = function(v, indent, opts) end elseif type(v) == 'nil' then ret = 'nil' + elseif type(v) == 'boolean' then + ret = (v and 'true' or 'false') else print(type(v)) -- Not implemented yet From 3df11cfbca3dc037974eb6c61d8f9413b3b7ca9c Mon Sep 17 00:00:00 2001 From: ZyX Date: Tue, 27 Mar 2018 03:03:02 +0300 Subject: [PATCH 08/14] Revert "shada: In place of ignoring cursor position with lnum 0 save with 1" This reverts commit aa728798b4bd89b59cee86103885c15d386f73ba. --- src/nvim/shada.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/nvim/shada.c b/src/nvim/shada.c index 552dd2dcf4..f726f09fad 100644 --- a/src/nvim/shada.c +++ b/src/nvim/shada.c @@ -2796,7 +2796,7 @@ static ShaDaWriteResult shada_write(ShaDaWriteDef *const sd_writer, // Update numbered marks: '0' should be replaced with the current position, // '9' should be removed and all other marks shifted. - if (!ignore_buf(curbuf, &removable_bufs)) { + if (!ignore_buf(curbuf, &removable_bufs) && curwin->w_cursor.lnum != 0) { replace_numbered_mark(wms, 0, (PossiblyFreedShadaEntry) { .can_free_entry = false, .data = { @@ -2804,9 +2804,7 @@ static ShaDaWriteResult shada_write(ShaDaWriteDef *const sd_writer, .timestamp = os_time(), .data = { .filemark = { - .mark = (curwin->w_cursor.lnum - ? curwin->w_cursor - : (pos_T) { 1, 0, 0 }), + .mark = curwin->w_cursor, .name = '0', .additional_data = NULL, .fname = (char *)curbuf->b_ffname, From 1ac1f520f02afc934523ce76a99a3d17d0d6a670 Mon Sep 17 00:00:00 2001 From: ZyX Date: Sun, 1 Apr 2018 20:04:35 +0300 Subject: [PATCH 09/14] functests: Add test for merging with file with only numeric mark Known to cause memory leak, but not an expected crash. --- test/functional/shada/merging_spec.lua | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/test/functional/shada/merging_spec.lua b/test/functional/shada/merging_spec.lua index 1a289a2de7..5d486bbd93 100644 --- a/test/functional/shada/merging_spec.lua +++ b/test/functional/shada/merging_spec.lua @@ -525,6 +525,22 @@ describe('ShaDa marks support code', function() eq('-', funcs.fnamemodify(curbufmeths.get_name(), ':t')) end) + it('can merge with file with mark 9 as the only numeric mark', function() + wshada('\007\001\018\131\162mX\195\161f\196\006' .. mock_file_path .. '-\161n9') + eq(0, exc_exec(sdrcmd())) + nvim_command('normal! `9oabc') + eq('-', funcs.fnamemodify(curbufmeths.get_name(), ':t')) + eq(0, exc_exec('wshada ' .. shada_fname)) + local found = {} + for _, v in ipairs(read_shada_file(shada_fname)) do + if v.type == 7 and v.value.f == mock_file_path .. '-' then + local name = ('%c'):format(v.value.n) + found[name] = (found[name] or 0) + 1 + end + end + eq({['0']=1, ['1']=1}, found) + end) + it('uses last A mark with gt timestamp from file when reading with !', function() wshada('\007\001\018\131\162mX\195\161f\196\006' .. mock_file_path .. '-\161nA') From 200898546ee81cc56b614274901cf392f1d462bd Mon Sep 17 00:00:00 2001 From: ZyX Date: Sun, 1 Apr 2018 20:05:19 +0300 Subject: [PATCH 10/14] shada: When storing numeric marks reset the numbers Attempt to fix observed crash. Crash currently not reproduced. --- src/nvim/shada.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/nvim/shada.c b/src/nvim/shada.c index f726f09fad..81261a183d 100644 --- a/src/nvim/shada.c +++ b/src/nvim/shada.c @@ -2431,14 +2431,14 @@ static inline void replace_numbered_mark(WriteMergerState *const wms, } for (size_t i = idx; i < ARRAY_SIZE(wms->numbered_marks) - 1; i++) { if (wms->numbered_marks[i].data.type == kSDItemGlobalMark) { - wms->numbered_marks[i].data.data.filemark.name++; - assert(ascii_isdigit(wms->numbered_marks[i].data.data.filemark.name)); + wms->numbered_marks[i].data.data.filemark.name = '0' + (char)i; } } memmove(wms->numbered_marks + idx + 1, wms->numbered_marks + idx, sizeof(wms->numbered_marks[0]) * (ARRAY_SIZE(wms->numbered_marks) - 1 - idx)); wms->numbered_marks[idx] = entry; + wms->numbered_marks[idx].data.data.filemark.name = '0' + (char)idx; } /// Write ShaDa file From f5373e2cdc6693a3353a37dd0652694834709739 Mon Sep 17 00:00:00 2001 From: ZyX Date: Sun, 1 Apr 2018 21:23:43 +0300 Subject: [PATCH 11/14] shada: Add functions to format ShaDa entries for debugging purposes To be used in debugging printfs. --- src/nvim/shada.c | 108 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 108 insertions(+) diff --git a/src/nvim/shada.c b/src/nvim/shada.c index 81261a183d..7b930d9f09 100644 --- a/src/nvim/shada.c +++ b/src/nvim/shada.c @@ -16,6 +16,7 @@ #include "nvim/os/os.h" #include "nvim/os/time.h" #include "nvim/vim.h" +#include "nvim/pos.h" #include "nvim/ascii.h" #include "nvim/shada.h" #include "nvim/message.h" @@ -2022,6 +2023,113 @@ shada_parse_msgpack_extra_bytes: return ret; } +/// Format shada entry for debugging purposes +/// +/// @param[in] entry ShaDa entry to format. +/// +/// @return string representing ShaDa entry in a static buffer. +static const char *shada_format_entry(const ShadaEntry entry) + FUNC_ATTR_WARN_UNUSED_RESULT FUNC_ATTR_UNUSED FUNC_ATTR_NONNULL_RET +{ + static char ret[1024]; + ret[0] = 0; + vim_snprintf(S_LEN(ret), "[ ] ts=%" PRIu64 " "); + // ^ Space for `can_free_entry` + switch (entry.type) { + case kSDItemMissing: { + vim_snprintf_add(S_LEN(ret), "Missing"); + break; + } + case kSDItemHeader: { + vim_snprintf_add(S_LEN(ret), "Header { TODO }"); + break; + } + case kSDItemBufferList: { + vim_snprintf_add(S_LEN(ret), "BufferList { TODO }"); + break; + } + case kSDItemUnknown: { + vim_snprintf_add(S_LEN(ret), "Unknown { TODO }"); + break; + } + case kSDItemSearchPattern: { + vim_snprintf_add(S_LEN(ret), "SearchPattern { TODO }"); + break; + } + case kSDItemSubString: { + vim_snprintf_add(S_LEN(ret), "SubString { TODO }"); + break; + } + case kSDItemHistoryEntry: { + vim_snprintf_add(S_LEN(ret), "HistoryEntry { TODO }"); + break; + } + case kSDItemRegister: { + vim_snprintf_add(S_LEN(ret), "Register { TODO }"); + break; + } + case kSDItemVariable: { + vim_snprintf_add(S_LEN(ret), "Variable { TODO }"); + break; + } +#define FORMAT_MARK_ENTRY(entry_name, name_fmt, name_fmt_arg) \ + do { \ + typval_T ad_tv = { \ + .v_type = VAR_DICT, \ + .vval.v_dict = entry.data.filemark.additional_data \ + }; \ + size_t ad_len; \ + char *const ad = encode_tv2string(&ad_tv, &ad_len); \ + vim_snprintf_add( \ + S_LEN(ret), \ + entry_name " {" name_fmt " file=[%zu]\"%.512s\", " \ + "pos={l=%" PRIdLINENR ",c=%" PRIdCOLNR ",a=%" PRIdCOLNR "}, " \ + "ad={%p:[%zu]%.64s} }", \ + name_fmt_arg, \ + strlen(entry.data.filemark.fname), \ + entry.data.filemark.fname, \ + entry.data.filemark.mark.lnum, \ + entry.data.filemark.mark.col, \ + entry.data.filemark.mark.coladd, \ + entry.data.filemark.additional_data, \ + ad_len, \ + ad); \ + } while (0) + case kSDItemGlobalMark: { + FORMAT_MARK_ENTRY("GlobalMark", " name='%c',", entry.data.filemark.name); + break; + } + case kSDItemChange: { + FORMAT_MARK_ENTRY("Change", "%s", ""); + break; + } + case kSDItemLocalMark: { + FORMAT_MARK_ENTRY("LocalMark", " name='%c',", entry.data.filemark.name); + break; + } + case kSDItemJump: { + FORMAT_MARK_ENTRY("Jump", "%s", ""); + break; + } +#undef FORMAT_MARK_ENTRY + } + return ret; +} + +/// Format possibly freed shada entry for debugging purposes +/// +/// @param[in] entry ShaDa entry to format. +/// +/// @return string representing ShaDa entry in a static buffer. +static const char *shada_format_pfreed_entry( + const PossiblyFreedShadaEntry pfs_entry) + FUNC_ATTR_WARN_UNUSED_RESULT FUNC_ATTR_UNUSED FUNC_ATTR_NONNULL_RET +{ + char *ret = (char *)shada_format_entry(pfs_entry.data); + ret[1] = (pfs_entry.can_free_entry ? 'T' : 'F'); + return ret; +} + /// Read and merge in ShaDa file, used when writing /// /// @param[in] sd_reader Structure containing file reader definition. From dd1b493f7560244162f4051443fb42dabaee0254 Mon Sep 17 00:00:00 2001 From: ZyX Date: Sun, 1 Apr 2018 21:29:47 +0300 Subject: [PATCH 12/14] shada: Fix some memory leaks and completely ignore numbered mark names Problems: - In two places in shada_read_when_writing() memory just was not freed. Both places were verified to cause test failures. - Numbered marks got assigned incorrect (off-by-one compared to position in the array) numbers in replace_numbered_mark. - It was possible to have non-continuously populated array of numbered marks which messed up code for merging them. (Note about tests: marks with additional data are always compared different when merging, that caused some confusion regarding why test did not work the way I expected.) --- src/nvim/shada.c | 44 +++++++++-------- test/functional/shada/merging_spec.lua | 65 +++++++++++++++++++++++++- 2 files changed, 89 insertions(+), 20 deletions(-) diff --git a/src/nvim/shada.c b/src/nvim/shada.c index 7b930d9f09..fd095b1ade 100644 --- a/src/nvim/shada.c +++ b/src/nvim/shada.c @@ -2255,6 +2255,7 @@ static inline ShaDaWriteResult shada_read_when_writing( entry.data.filemark.mark) && strcmp(wms_entry.data.filemark.fname, entry.data.filemark.fname) == 0) { + shada_free_shada_entry(&entry); processed_mark = true; break; } @@ -2262,6 +2263,8 @@ static inline ShaDaWriteResult shada_read_when_writing( processed_mark = true; if (i < ARRAY_SIZE(wms->numbered_marks)) { replace_numbered_mark(wms, i, pfs_entry); + } else { + shada_free_shada_entry(&entry); } break; } @@ -2539,7 +2542,7 @@ static inline void replace_numbered_mark(WriteMergerState *const wms, } for (size_t i = idx; i < ARRAY_SIZE(wms->numbered_marks) - 1; i++) { if (wms->numbered_marks[i].data.type == kSDItemGlobalMark) { - wms->numbered_marks[i].data.data.filemark.name = '0' + (char)i; + wms->numbered_marks[i].data.data.filemark.name = '0' + (char)i + 1; } } memmove(wms->numbered_marks + idx + 1, wms->numbered_marks + idx, @@ -2781,6 +2784,7 @@ static ShaDaWriteResult shada_write(ShaDaWriteDef *const sd_writer, // Initialize global marks if (dump_global_marks) { const void *global_mark_iter = NULL; + size_t digit_mark_idx = 0; do { char name = NUL; xfmark_T fm; @@ -2803,24 +2807,26 @@ static ShaDaWriteResult shada_write(ShaDaWriteDef *const sd_writer, } fname = (const char *) buf->b_ffname; } - *(ascii_isdigit(name) - ? &wms->numbered_marks[name - '0'] - : &wms->global_marks[mark_global_index(name)]) = ( - (PossiblyFreedShadaEntry) { - .can_free_entry = false, - .data = { - .type = kSDItemGlobalMark, - .timestamp = fm.fmark.timestamp, - .data = { - .filemark = { - .mark = fm.fmark.mark, - .name = name, - .additional_data = fm.fmark.additional_data, - .fname = (char *)fname, - } - } - }, - }); + const PossiblyFreedShadaEntry pf_entry = { + .can_free_entry = false, + .data = { + .type = kSDItemGlobalMark, + .timestamp = fm.fmark.timestamp, + .data = { + .filemark = { + .mark = fm.fmark.mark, + .name = name, + .additional_data = fm.fmark.additional_data, + .fname = (char *)fname, + } + } + }, + }; + if (ascii_isdigit(name)) { + replace_numbered_mark(wms, digit_mark_idx++, pf_entry); + } else { + wms->global_marks[mark_global_index(name)] = pf_entry; + } } while (global_mark_iter != NULL); } diff --git a/test/functional/shada/merging_spec.lua b/test/functional/shada/merging_spec.lua index 5d486bbd93..fdd35b70b3 100644 --- a/test/functional/shada/merging_spec.lua +++ b/test/functional/shada/merging_spec.lua @@ -526,7 +526,7 @@ describe('ShaDa marks support code', function() end) it('can merge with file with mark 9 as the only numeric mark', function() - wshada('\007\001\018\131\162mX\195\161f\196\006' .. mock_file_path .. '-\161n9') + wshada('\007\001\014\130\161f\196\006' .. mock_file_path .. '-\161n9') eq(0, exc_exec(sdrcmd())) nvim_command('normal! `9oabc') eq('-', funcs.fnamemodify(curbufmeths.get_name(), ':t')) @@ -541,6 +541,69 @@ describe('ShaDa marks support code', function() eq({['0']=1, ['1']=1}, found) end) + it('removes duplicates while merging', function() + wshada('\007\001\014\130\161f\196\006' .. mock_file_path .. '-\161n9' + .. '\007\001\014\130\161f\196\006' .. mock_file_path .. '-\161n9') + eq(0, exc_exec(sdrcmd())) + eq(0, exc_exec('wshada ' .. shada_fname)) + local found = 0 + for _, v in ipairs(read_shada_file(shada_fname)) do + if v.type == 7 and v.value.f == mock_file_path .. '-' then + print(require('test.helpers').format_luav(v)) + found = found + 1 + end + end + eq(1, found) + end) + + it('does not leak when no append is performed due to too many marks', + function() + wshada('\007\002\018\131\162mX\195\161f\196\006' .. mock_file_path .. 'a\161n0' + .. '\007\002\018\131\162mX\195\161f\196\006' .. mock_file_path .. 'b\161n1' + .. '\007\002\018\131\162mX\195\161f\196\006' .. mock_file_path .. 'c\161n2' + .. '\007\002\018\131\162mX\195\161f\196\006' .. mock_file_path .. 'd\161n3' + .. '\007\002\018\131\162mX\195\161f\196\006' .. mock_file_path .. 'e\161n4' + .. '\007\002\018\131\162mX\195\161f\196\006' .. mock_file_path .. 'f\161n5' + .. '\007\002\018\131\162mX\195\161f\196\006' .. mock_file_path .. 'g\161n6' + .. '\007\002\018\131\162mX\195\161f\196\006' .. mock_file_path .. 'h\161n7' + .. '\007\002\018\131\162mX\195\161f\196\006' .. mock_file_path .. 'i\161n8' + .. '\007\002\018\131\162mX\195\161f\196\006' .. mock_file_path .. 'j\161n9' + .. '\007\001\018\131\162mX\195\161f\196\006' .. mock_file_path .. 'k\161n9') + eq(0, exc_exec(sdrcmd())) + eq(0, exc_exec('wshada ' .. shada_fname)) + local found = {} + for _, v in ipairs(read_shada_file(shada_fname)) do + if v.type == 7 and v.value.f:sub(1, #mock_file_path) == mock_file_path then + found[#found + 1] = v.value.f:sub(#v.value.f) + end + end + eq({'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j'}, found) + end) + + it('does not leak when last mark in file removes some of the earlier ones', + function() + wshada('\007\002\018\131\162mX\195\161f\196\006' .. mock_file_path .. 'a\161n0' + .. '\007\002\018\131\162mX\195\161f\196\006' .. mock_file_path .. 'b\161n1' + .. '\007\002\018\131\162mX\195\161f\196\006' .. mock_file_path .. 'c\161n2' + .. '\007\002\018\131\162mX\195\161f\196\006' .. mock_file_path .. 'd\161n3' + .. '\007\002\018\131\162mX\195\161f\196\006' .. mock_file_path .. 'e\161n4' + .. '\007\002\018\131\162mX\195\161f\196\006' .. mock_file_path .. 'f\161n5' + .. '\007\002\018\131\162mX\195\161f\196\006' .. mock_file_path .. 'g\161n6' + .. '\007\002\018\131\162mX\195\161f\196\006' .. mock_file_path .. 'h\161n7' + .. '\007\002\018\131\162mX\195\161f\196\006' .. mock_file_path .. 'i\161n8' + .. '\007\002\018\131\162mX\195\161f\196\006' .. mock_file_path .. 'j\161n9' + .. '\007\003\018\131\162mX\195\161f\196\006' .. mock_file_path .. 'k\161n9') + eq(0, exc_exec(sdrcmd())) + eq(0, exc_exec('wshada ' .. shada_fname)) + local found = {} + for _, v in ipairs(read_shada_file(shada_fname)) do + if v.type == 7 and v.value.f:sub(1, #mock_file_path) == mock_file_path then + found[#found + 1] = v.value.f:sub(#v.value.f) + end + end + eq({'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'k'}, found) + end) + it('uses last A mark with gt timestamp from file when reading with !', function() wshada('\007\001\018\131\162mX\195\161f\196\006' .. mock_file_path .. '-\161nA') From bdf5f57989858436fed358310624a3ec3bc8c177 Mon Sep 17 00:00:00 2001 From: ZyX Date: Sun, 1 Apr 2018 23:37:23 +0300 Subject: [PATCH 13/14] shada: Fix conversion warnings --- src/nvim/shada.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/nvim/shada.c b/src/nvim/shada.c index fd095b1ade..15ac28e1bf 100644 --- a/src/nvim/shada.c +++ b/src/nvim/shada.c @@ -2542,14 +2542,14 @@ static inline void replace_numbered_mark(WriteMergerState *const wms, } for (size_t i = idx; i < ARRAY_SIZE(wms->numbered_marks) - 1; i++) { if (wms->numbered_marks[i].data.type == kSDItemGlobalMark) { - wms->numbered_marks[i].data.data.filemark.name = '0' + (char)i + 1; + wms->numbered_marks[i].data.data.filemark.name = (char)('0' + (int)i + 1); } } memmove(wms->numbered_marks + idx + 1, wms->numbered_marks + idx, sizeof(wms->numbered_marks[0]) * (ARRAY_SIZE(wms->numbered_marks) - 1 - idx)); wms->numbered_marks[idx] = entry; - wms->numbered_marks[idx].data.data.filemark.name = '0' + (char)idx; + wms->numbered_marks[idx].data.data.filemark.name = (char)('0' + (int)idx); } /// Write ShaDa file From 5d9bb16d66043f10dc14435c0594ca6f31b1795f Mon Sep 17 00:00:00 2001 From: ZyX Date: Mon, 2 Apr 2018 11:14:11 +0300 Subject: [PATCH 14/14] functests: Use proper path in `eq()` --- test/functional/shada/merging_spec.lua | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/test/functional/shada/merging_spec.lua b/test/functional/shada/merging_spec.lua index fdd35b70b3..a628baff53 100644 --- a/test/functional/shada/merging_spec.lua +++ b/test/functional/shada/merging_spec.lua @@ -670,8 +670,7 @@ describe('ShaDa marks support code', function() eq({['0']=1, A=1}, found) end) - it('uses last A mark with gt timestamp from file when writing', - function() + it('uses last A mark with gt timestamp from file when writing', function() wshada('\007\001\018\131\162mX\195\161f\196\006' .. mock_file_path .. '-\161nA') eq(0, exc_exec(sdrcmd())) wshada('\007\002\018\131\162mX\195\161f\196\006' .. mock_file_path .. '?\161nA') @@ -687,7 +686,7 @@ describe('ShaDa marks support code', function() found[name] = t end end - eq({['0']={['/a/b/-']=1}, A={['/a/b/?']=1}}, found) + eq({['0']={[mock_file_path .. '-']=1}, A={[mock_file_path .. '?']=1}}, found) end) it('uses last a mark with gt timestamp from instance when reading',