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

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')