fix(jumplist): Ctrl+o, Ctrl+i weird behavior when deleting buffers #25461

Problem:
- Navigation is not always symmetric: pressing Ctrl+o n times followed
  by Ctrl+i n times does not always gets me back to where I started.
- Invalid buffers are not skipped by Ctrl+i/o, I have to press Ctrl+i/o
  multiple times to get to the next/previous buffer.

Solution:
- Remove all entries of a buffer from the jump list when deleting it.
- Don't add a new entry to the jump list if the next buffer to be
  displayed is already in the jump list.

Closes #25365

src/nvim/buffer.c

@@ -1183,6 +1183,32 @@ static int empty_curbuf(bool close_others, int forceit, int action)
   return retval;
 }
 
+/// Remove every jump list entry referring to a given buffer.
+/// This function will also adjust the current jump list index.
+void buf_remove_from_jumplist(buf_T *deleted_buf)
+{
+  // Remove all jump list entries that match the deleted buffer.
+  for (int i = curwin->w_jumplistlen - 1; i >= 0; i--) {
+    buf_T *buf = buflist_findnr(curwin->w_jumplist[i].fmark.fnum);
+
+    if (buf == deleted_buf) {
+      // Found an entry that we want to delete.
+      curwin->w_jumplistlen -= 1;
+
+      // If the current jump list index behind the entry we want to
+      // delete, move it back by one.
+      if (curwin->w_jumplistidx > i && curwin->w_jumplistidx > 0) {
+        curwin->w_jumplistidx -= 1;
+      }
+
+      // Actually remove the entry from the jump list.
+      for (int d = i; d < curwin->w_jumplistlen; d++) {
+        curwin->w_jumplist[d] = curwin->w_jumplist[d + 1];
+      }
+    }
+  }
+}
+
 /// Implementation of the commands for the buffer list.
 ///
 /// action == DOBUF_GOTO     go to specified buffer
@@ -1205,6 +1231,7 @@ int do_buffer(int action, int start, int dir, int count, int forceit)
 {
   buf_T *buf;
   buf_T *bp;
+  bool update_jumplist = true;
   bool unload = (action == DOBUF_UNLOAD || action == DOBUF_DEL
                  || action == DOBUF_WIPE);
 
@@ -1362,7 +1389,11 @@ int do_buffer(int action, int start, int dir, int count, int forceit)
 
     // If the buffer to be deleted is not the current one, delete it here.
     if (buf != curbuf) {
+      // Remove the buffer to be deleted from the jump list.
+      buf_remove_from_jumplist(buf);
+
       close_windows(buf, false);
+
       if (buf != curbuf && bufref_valid(&bufref) && buf->b_nwindows <= 0) {
         close_buffer(NULL, buf, action, false, false);
       }
@@ -1382,40 +1413,53 @@ int do_buffer(int action, int start, int dir, int count, int forceit)
     if (au_new_curbuf.br_buf != NULL && bufref_valid(&au_new_curbuf)) {
       buf = au_new_curbuf.br_buf;
     } else if (curwin->w_jumplistlen > 0) {
-      int jumpidx = curwin->w_jumplistidx - 1;
+      // Remove the current buffer from the jump list.
-      if (jumpidx < 0) {
+      buf_remove_from_jumplist(curbuf);
-        jumpidx = curwin->w_jumplistlen - 1;
-      }
 
-      forward = jumpidx;
+      // It's possible that we removed all jump list entries, in that case we need to try another
-      while (jumpidx != curwin->w_jumplistidx) {
+      // approach
-        buf = buflist_findnr(curwin->w_jumplist[jumpidx].fmark.fnum);
+      if (curwin->w_jumplistlen > 0) {
-        if (buf != NULL) {
+        // If the index is the same as the length, the current position was not yet added to the jump
-          // Skip current and unlisted bufs.  Also skip a quickfix
+        // list. So we can safely go back to the last entry and search from there.
-          // buffer, it might be deleted soon.
+        if (curwin->w_jumplistidx == curwin->w_jumplistlen) {
-          if (buf == curbuf || !buf->b_p_bl || bt_quickfix(buf)) {
+          curwin->w_jumplistidx = curwin->w_jumplistlen - 1;
-            buf = NULL;
+        }
-          } else if (buf->b_ml.ml_mfp == NULL) {
+
-            // skip unloaded buf, but may keep it for later
+        int jumpidx = curwin->w_jumplistidx;
-            if (bp == NULL) {
+
-              bp = buf;
+        forward = jumpidx;
+        do {
+          buf = buflist_findnr(curwin->w_jumplist[jumpidx].fmark.fnum);
+
+          if (buf != NULL) {
+            // Skip unlisted bufs.  Also skip a quickfix
+            // buffer, it might be deleted soon.
+            if (!buf->b_p_bl || bt_quickfix(buf)) {
+              buf = NULL;
+            } else if (buf->b_ml.ml_mfp == NULL) {
+              // skip unloaded buf, but may keep it for later
+              if (bp == NULL) {
+                bp = buf;
+              }
+              buf = NULL;
             }
-            buf = NULL;
           }
-        }
+          if (buf != NULL) {         // found a valid buffer: stop searching
-        if (buf != NULL) {         // found a valid buffer: stop searching
+            curwin->w_jumplistidx = jumpidx;
-          break;
+            update_jumplist = false;
-        }
+            break;
-        // advance to older entry in jump list
+          }
-        if (!jumpidx && curwin->w_jumplistidx == curwin->w_jumplistlen) {
+          // advance to older entry in jump list
-          break;
+          if (!jumpidx && curwin->w_jumplistidx == curwin->w_jumplistlen) {
-        }
+            break;
-        if (--jumpidx < 0) {
+          }
-          jumpidx = curwin->w_jumplistlen - 1;
+          if (--jumpidx < 0) {
-        }
+            jumpidx = curwin->w_jumplistlen - 1;
-        if (jumpidx == forward) {               // List exhausted for sure
+          }
-          break;
+          if (jumpidx == forward) {               // List exhausted for sure
-        }
+            break;
+          }
+        } while (jumpidx != curwin->w_jumplistidx);
       }
     }
 
@@ -1511,7 +1555,7 @@ int do_buffer(int action, int start, int dir, int count, int forceit)
   }
 
   // Go to the other buffer.
-  set_curbuf(buf, action);
+  set_curbuf(buf, action, update_jumplist);
 
   if (action == DOBUF_SPLIT) {
     RESET_BINDING(curwin);      // reset 'scrollbind' and 'cursorbind'
@@ -1533,14 +1577,17 @@ int do_buffer(int action, int start, int dir, int count, int forceit)
 ///                DOBUF_UNLOAD     unload it
 ///                DOBUF_DEL        delete it
 ///                DOBUF_WIPE       wipe it out
-void set_curbuf(buf_T *buf, int action)
+void set_curbuf(buf_T *buf, int action, bool update_jumplist)
 {
   buf_T *prevbuf;
   int unload = (action == DOBUF_UNLOAD || action == DOBUF_DEL
                 || action == DOBUF_WIPE);
   OptInt old_tw = curbuf->b_p_tw;
 
-  setpcmark();
+  if (update_jumplist) {
+    setpcmark();
+  }
+
   if ((cmdmod.cmod_flags & CMOD_KEEPALT) == 0) {
     curwin->w_alt_fnum = curbuf->b_fnum;     // remember alternate file
   }
@@ -3675,7 +3722,7 @@ void ex_buffer_all(exarg_T *eap)
 
       // Open the buffer in this window.
       swap_exists_action = SEA_DIALOG;
-      set_curbuf(buf, DOBUF_GOTO);
+      set_curbuf(buf, DOBUF_GOTO, false);
       if (!bufref_valid(&bufref)) {
         // Autocommands deleted the buffer.
         swap_exists_action = SEA_NONE;

src/nvim/ex_cmds2.c

@@ -405,7 +405,7 @@ buf_found:
 
   // Open the changed buffer in the current window.
   if (buf != curbuf) {
-    set_curbuf(buf, unload ? DOBUF_UNLOAD : DOBUF_GOTO);
+    set_curbuf(buf, unload ? DOBUF_UNLOAD : DOBUF_GOTO, true);
   }
 
 theend:

test/functional/editor/jump_spec.lua

@@ -3,6 +3,7 @@ local Screen = require('test.functional.ui.screen')
 
 local clear = helpers.clear
 local command = helpers.command
+local dedent = helpers.dedent
 local eq = helpers.eq
 local fn = helpers.fn
 local feed = helpers.feed
@@ -192,6 +193,131 @@ describe("jumpoptions=stack behaves like 'tagstack'", function()
   end)
 end)
 
+describe('buffer deletion', function()
+  local base_file = 'Xtest-functional-buffer-deletion'
+  local file1 = base_file .. '1'
+  local file2 = base_file .. '2'
+  local file3 = base_file .. '3'
+  local base_content = 'text'
+  local content1 = base_content .. '1'
+  local content2 = base_content .. '2'
+  local content3 = base_content .. '3'
+
+  local function format_jumplist(input)
+    return dedent(input)
+      :gsub('%{file1%}', file1)
+      :gsub('%{file2%}', file2)
+      :gsub('%{file3%}', file3)
+      :gsub('%{content1%}', content1)
+      :gsub('%{content2%}', content2)
+      :gsub('%{content3%}', content3)
+  end
+
+  before_each(function()
+    clear()
+    command('clearjumps')
+
+    write_file(file1, content1, false, false)
+    write_file(file2, content2, false, false)
+    write_file(file3, content3, false, false)
+
+    command('edit ' .. file1)
+    command('edit ' .. file2)
+    command('edit ' .. file3)
+  end)
+
+  it('deletes jump list entries when the current buffer is deleted', function()
+    command('edit ' .. file1)
+
+    eq(
+      format_jumplist([[
+       jump line  col file/text
+         3     1    0 {content1}
+         2     1    0 {file2}
+         1     1    0 {file3}
+      >]]),
+      exec_capture('jumps')
+    )
+
+    command('bwipeout')
+
+    eq(
+      format_jumplist([[
+       jump line  col file/text
+         1     1    0 {file2}
+      >  0     1    0 {content3}]]),
+      exec_capture('jumps')
+    )
+  end)
+
+  it('deletes jump list entries when another buffer is deleted', function()
+    eq(
+      format_jumplist([[
+       jump line  col file/text
+         2     1    0 {file1}
+         1     1    0 {file2}
+      >]]),
+      exec_capture('jumps')
+    )
+
+    command('bwipeout ' .. file2)
+
+    eq(
+      format_jumplist([[
+       jump line  col file/text
+         1     1    0 {file1}
+      >]]),
+      exec_capture('jumps')
+    )
+  end)
+
+  it('sets the correct jump index when the current buffer is deleted', function()
+    feed('<C-O>')
+
+    eq(
+      format_jumplist([[
+       jump line  col file/text
+         1     1    0 {file1}
+      >  0     1    0 {content2}
+         1     1    0 {file3}]]),
+      exec_capture('jumps')
+    )
+
+    command('bw')
+
+    eq(
+      format_jumplist([[
+       jump line  col file/text
+         1     1    0 {file1}
+      >  0     1    0 {content3}]]),
+      exec_capture('jumps')
+    )
+  end)
+
+  it('sets the correct jump index when the another buffer is deleted', function()
+    feed('<C-O>')
+
+    eq(
+      format_jumplist([[
+       jump line  col file/text
+         1     1    0 {file1}
+      >  0     1    0 {content2}
+         1     1    0 {file3}]]),
+      exec_capture('jumps')
+    )
+
+    command('bwipeout ' .. file1)
+
+    eq(
+      format_jumplist([[
+       jump line  col file/text
+      >  0     1    0 {content2}
+         1     1    0 {file3}]]),
+      exec_capture('jumps')
+    )
+  end)
+end)
+
 describe('jumpoptions=view', function()
   local file1 = 'Xtestfile-functional-editor-jumps'
   local file2 = 'Xtestfile-functional-editor-jumps-2'