[vim/vim] Prevent TextPutPre from calling clipboard provider callback twice (PR #20215)

[Foxe Chen]

---

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

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

Commit Summary

• 7bddb05 prevent textputpre from calling clipboard provider callback twice

File Changes

(2 files)

• M src/register.c (27)
• M src/testdir/test_eval_stuff.vim (39)

Patch Links:

• https://github.com/vim/vim/pull/20215.patch
• https://github.com/vim/vim/pull/20215.diff

—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.Message ID: <vim/vim/pull/20215@github.com>

[h_east]

h-east left a comment (vim/vim#20215)

Nice fix — the suppression mechanism via inc/dec_clip_provider() works as expected (I confirmed locally that put + goes from 3 paste calls down to 2 with the patch).

Two suggestions for follow-up: consolidating the dec_clip_provider() calls in do_put, and tightening the test. Both keep the existing behavior (the Test_clipboard_provider_accessed_once assertions still pass with these changes).

---

1. Consolidate dec_clip_provider() in do_put

Currently the patch sprinkles six dec_clip_provider() calls across do_put (one per early-return / goto end site, plus one in each branch of the insert_string if/else). That makes future maintenance fragile — any new error path needs to remember to call dec to balance the inc at the top of the function.

Most of those exits already funnel through the end: label. By placing a single dec_clip_provider() right after end: (and before the TextPutPost block, so the autocmd is allowed to query the clipboard), the four main-path exits and the normal-flow exits all collapse into one site. Only two dec calls really need to stay inline: the regname == '.' branch (which returns early without touching end:, and must dec between TextPutPre and TextPutPost), and the get_spec_reg failure early-return (which also bypasses end:).

Net result: 6 dec_clip_provider() calls → 3.

Suggested diff for src/register.c:

@@ -1787,12 +1787,7 @@ do_put(
     // Autocommands may be executed when saving lines for undo.  This might
     // make "y_array" invalid, so we start undo now to avoid that.
     if (u_save(curwin->w_cursor.lnum, curwin->w_cursor.lnum + 1) == FAIL)
-    {
-#ifdef FEAT_CLIPBOARD_PROVIDER
-	dec_clip_provider();
-#endif
 	goto end;
-    }
 
     if (insert_string.string != NULL)
     {
@@ -1846,12 +1841,7 @@ do_put(
 		    break;
 		y_array = ALLOC_MULT(string_T, y_size);
 		if (y_array == NULL)
-		{
-# ifdef FEAT_CLIPBOARD_PROVIDER
-	dec_clip_provider();
-# endif
 		    goto end;
-		}
 	    }
 	}
 	else
@@ -1863,9 +1853,6 @@ do_put(
 #ifdef FEAT_EVAL
 	if (has_textputpre())
 	    put_do_autocmd(regname, NULL, &insert_string, false, dir);
-#endif
-#ifdef FEAT_CLIPBOARD_PROVIDER
-	dec_clip_provider();
 #endif
     }
     else
@@ -1878,9 +1865,6 @@ do_put(
 	    get_yank_register(regname, FALSE);
 	    put_do_autocmd(regname, y_current, NULL, false, dir);
 	}
-#endif
-#ifdef FEAT_CLIPBOARD_PROVIDER
-	dec_clip_provider();
 #endif
 	get_yank_register(regname, FALSE);
 
@@ -2548,6 +2532,11 @@ end:
 	curbuf->b_op_end = orig_end;
     }
 
+#ifdef FEAT_CLIPBOARD_PROVIDER
+    // Release before TextPutPost so the autocmd can query the clipboard again.
+    dec_clip_provider();
+#endif
+
 #ifdef FEAT_EVAL
     if (has_textputpost())
     {

A small side benefit: the awkward broken-indentation block inside if (y_array == NULL) { ... } — and the nested # ifdef FEAT_CLIPBOARD_PROVIDER with an unusual leading space — go away.

One subtle behavior difference to flag: the u_save failure path now reaches end: with the suppression still active until dec_clip_provider() runs. The actual put body (lines 1894-2542) is no longer entered in that case, so this is fine. For the normal-flow exit, the put body itself doesn't touch the clipboard provider, so the move is also safe in practice.

---

2. Test: tighten assertions and clean up augroup

A few small things in the new Test_clipboard_provider_accessed_once block:

• The comment claims TextPutPost "will always cause a second clipboard access". After tracing through the code, the second call actually originates from add_regtype_to_dict() → get_reg_type() → call_clip_provider_request(), invoked while building v:event for the autocmd. The current wording overstates a guarantee (it depends on internals of put_do_autocmd, not on TextPutPre having modified the clipboard). Suggested rewording below.
• g:putpost / g:putpre / g:yankpost are set but never asserted, so we don't actually verify the autocmds fired. Counting them gives a stronger check.
• autocmd! TextAutocmd empties the group but leaves the augroup itself defined. Using augroup! TextAutocmd after the empty avoids leaving state behind.

Suggested diff for src/testdir/test_eval_stuff.vim:

@@ -1128,16 +1128,13 @@ func Test_clipboard_provider_accessed_once()
   bw!
 
   new
-  " Emitting TextPutPre/TextPutPost/TextYankPost may cause a clipboard access
-  "
-  " Note that TextPutPost will always cause a second clipboard access, since a
-  " TextPutPre may have changed the clipboard, meaning another "paste" call is
-  " needed to make sure everything is up to date.
+  " The "paste" callback runs once for the initial register lookup and once
+  " for the TextPutPost autocmd; the TextPutPre lookup is suppressed.
   augroup TextAutocmd
     autocmd!
-    autocmd TextPutPost * let g:putpost = 1
-    autocmd TextPutPre * let g:putpre = 1
-    autocmd TextYankPost * let g:yankpost = 1
+    autocmd TextPutPost * let g:putpost += 1
+    autocmd TextPutPre * let g:putpre += 1
+    autocmd TextYankPost * let g:yankpost += 1
   augroup END
 
   let g:putpost = 0
@@ -1163,8 +1160,15 @@ func Test_clipboard_provider_accessed_once()
   call assert_equal(2, g:vim_paste_count['*'])
   call assert_equal(1, g:vim_copy_count['*'])
 
+  call assert_equal(2, g:putpost)
+  call assert_equal(2, g:putpre)
+  call assert_equal(2, g:yankpost)
+
   bw!
-  autocmd! TextAutocmd
+  augroup TextAutocmd
+    autocmd!
+  augroup END
+  augroup! TextAutocmd
 
   set clipmethod&
   set clipboard&

I verified locally that make test_eval_stuff passes with both changes applied together. Patch counts (vim_paste_count etc.) are unchanged from the current PR.

—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.

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

[Foxe Chen]

@64-bitman pushed 1 commit.

• 5423628 prevent textputpre from calling clipboard provider callback twice

—
View it on GitHub or unsubscribe.

Triage notifications on the go with GitHub Mobile for iOS or Android.

You are receiving this because you are subscribed to this thread.Message ID: <vim/vim/pull/20215/before/7bddb056ef344651866e9cd6a0dd14cdd008ce02/after/5423628f493c69693f26f4897b6352cc200dbd1b@github.com>

[Foxe Chen]

64-bitman left a comment (vim/vim#20215)

Resolved

—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.

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

[Christian Brabandt]

chrisbra left a comment (vim/vim#20215)

thanks

—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.

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

[Christian Brabandt]

Closed #20215 via bec23ef.

—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.

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