Merge #7193 from justinmk/cb-pathology

runtime/autoload/health.vim

@@ -90,7 +90,7 @@ endfunction
 
 " Changes ':h clipboard' to ':help |clipboard|'.
 function! s:help_to_link(s) abort
-  return substitute(a:s, '\v:h%[elp] ([^|][^"\r\n]+)', ':help |\1|', 'g')
+  return substitute(a:s, '\v:h%[elp] ([^|][^"\r\n ]+)', ':help |\1|', 'g')
 endfunction
 
 " Format a message for a specific report item

runtime/autoload/health/provider.vim

@@ -121,14 +121,14 @@ function! s:check_clipboard() abort
   call health#report_start('Clipboard (optional)')
 
   let clipboard_tool = provider#clipboard#Executable()
-  if empty(clipboard_tool)
+  if exists('g:clipboard') && empty(clipboard_tool)
+    call health#report_error(
+          \ provider#clipboard#Error(),
+          \ ["Use the example in :help g:clipboard as a template, or don't set g:clipboard at all."])
+  elseif empty(clipboard_tool)
     call health#report_warn(
-          \ 'No clipboard tool found. Clipboard registers will not work.',
+          \ 'No clipboard tool found. Clipboard registers (`"+` and `"*`) will not work.',
           \ [':help clipboard'])
-  elseif exists('g:clipboard') && (type({}) != type(g:clipboard)
-        \ || !has_key(g:clipboard, 'copy') || !has_key(g:clipboard, 'paste'))
-    call health#report_error(
-          \ 'g:clipboard exists but is malformed. It must be a dictionary with the keys documented at :help g:clipboard')
   else
     call health#report_ok('Clipboard tool found: '. clipboard_tool)
   endif

runtime/autoload/provider/clipboard.vim

@@ -3,6 +3,7 @@
 " available.
 let s:copy = {}
 let s:paste = {}
+let s:clipboard = {}
 
 " When caching is enabled, store the jobid of the xclip/xsel process keeping
 " ownership of the selection, so we know how long the cache is valid.
@@ -23,7 +24,7 @@ function! s:selection.on_exit(jobid, data, event) abort
   call provider#clear_stderr(a:jobid)
 endfunction
 
-let s:selections = { '*': s:selection, '+': copy(s:selection)}
+let s:selections = { '*': s:selection, '+': copy(s:selection) }
 
 function! s:try_cmd(cmd, ...) abort
   let argv = split(a:cmd, " ")
@@ -55,9 +56,15 @@ endfunction
 
 function! provider#clipboard#Executable() abort
   if exists('g:clipboard')
+    if type({}) isnot# type(g:clipboard)
+          \ || type({}) isnot# type(get(g:clipboard, 'copy', v:null))
+          \ || type({}) isnot# type(get(g:clipboard, 'paste', v:null))
+      let s:err = 'clipboard: invalid g:clipboard'
+      return ''
+    endif
     let s:copy = get(g:clipboard, 'copy', { '+': v:null, '*': v:null })
     let s:paste = get(g:clipboard, 'paste', { '+': v:null, '*': v:null })
-    let s:cache_enabled = get(g:clipboard, 'cache_enabled', 1)
+    let s:cache_enabled = get(g:clipboard, 'cache_enabled', 0)
     return get(g:clipboard, 'name', 'g:clipboard')
   elseif has('mac') && executable('pbcopy')
     let s:copy['+'] = 'pbcopy'
@@ -104,16 +111,17 @@ function! provider#clipboard#Executable() abort
     return 'tmux'
   endif
 
-  let s:err = 'clipboard: No clipboard tool available. :help clipboard'
+  let s:err = 'clipboard: No clipboard tool. :help clipboard'
   return ''
 endfunction
 
 if empty(provider#clipboard#Executable())
+  " provider#clipboard#Call() *must not* be defined if the provider is broken.
+  " Otherwise eval_has_provider() thinks the clipboard provider is
+  " functioning, and eval_call_provider() will happily call it.
   finish
 endif
 
-let s:clipboard = {}
-
 function! s:clipboard.get(reg) abort
   if s:selections[a:reg].owner > 0
     return s:selections[a:reg].data
@@ -154,7 +162,9 @@ function! s:clipboard.set(lines, regtype, reg) abort
     echohl WarningMsg
     echomsg 'clipboard: failed to execute: '.(s:copy[a:reg])
     echohl None
+    return 0
   endif
+  return 1
 endfunction
 
 function! provider#clipboard#Call(method, args) abort

src/nvim/eval.c

@@ -22775,7 +22775,7 @@ typval_T eval_call_provider(char *provider, char *method, list_T *arguments)
 
 bool eval_has_provider(const char *name)
 {
-#define check_provider(name) \
+#define CHECK_PROVIDER(name) \
   if (has_##name == -1) { \
     has_##name = !!find_func((char_u *)"provider#" #name "#Call"); \
     if (!has_##name) { \
@@ -22791,17 +22791,17 @@ bool eval_has_provider(const char *name)
   static int has_python3 = -1;
   static int has_ruby = -1;
 
-  if (!strcmp(name, "clipboard")) {
-    check_provider(clipboard);
+  if (strequal(name, "clipboard")) {
+    CHECK_PROVIDER(clipboard);
     return has_clipboard;
-  } else if (!strcmp(name, "python3")) {
-    check_provider(python3);
+  } else if (strequal(name, "python3")) {
+    CHECK_PROVIDER(python3);
     return has_python3;
-  } else if (!strcmp(name, "python")) {
-    check_provider(python);
+  } else if (strequal(name, "python")) {
+    CHECK_PROVIDER(python);
     return has_python;
-  } else if (!strcmp(name, "ruby")) {
-    check_provider(ruby);
+  } else if (strequal(name, "ruby")) {
+    CHECK_PROVIDER(ruby);
     return has_ruby;
   }
 

src/nvim/ops.c

@@ -58,8 +58,8 @@ static yankreg_T *y_previous = NULL; /* ptr to last written yankreg */
 static bool clipboard_didwarn_unnamed = false;
 
 // for behavior between start_batch_changes() and end_batch_changes())
-static bool clipboard_delay_update = false;  // delay clipboard update
 static int batch_change_count = 0;           // inside a script
+static bool clipboard_delay_update = false;  // delay clipboard update
 static bool clipboard_needs_update = false;  // clipboard was updated
 
 /*
@@ -5524,7 +5524,7 @@ int get_default_register_name(void)
 }
 
 /// Determine if register `*name` should be used as a clipboard.
-/// In an unnammed operation, `*name` is `NUL` and will be adjusted to `'*'/'+'` if
+/// In an unnamed operation, `*name` is `NUL` and will be adjusted to */+ if
 /// `clipboard=unnamed[plus]` is set.
 ///
 /// @param name The name of register, or `NUL` if unnamed.
@@ -5535,44 +5535,59 @@ int get_default_register_name(void)
 /// if the register isn't a clipboard or provider isn't available.
 static yankreg_T *adjust_clipboard_name(int *name, bool quiet, bool writing)
 {
-  if (*name == '*' || *name == '+') {
-    if(!eval_has_provider("clipboard")) {
-      if (!quiet) {
-        EMSG("clipboard: No provider. Try \":CheckHealth\" or "
-             "\":h clipboard\".");
-      }
-      return NULL;
-    }
-    return &y_regs[*name == '*' ? STAR_REGISTER : PLUS_REGISTER];
-  } else if ((*name == NUL) && (cb_flags & CB_UNNAMEDMASK)) {
-    if(!eval_has_provider("clipboard")) {
-      if (!quiet && !clipboard_didwarn_unnamed) {
-        msg((char_u *)"clipboard: No provider. Try \":CheckHealth\" or "
-            "\":h clipboard\".");
-        clipboard_didwarn_unnamed = true;
-      }
-      return NULL;
+#define MSG_NO_CLIP "clipboard: No provider. " \
+  "Try \":CheckHealth\" or \":h clipboard\"."
+
+  yankreg_T *target = NULL;
+  bool explicit_cb_reg = (*name == '*' || *name == '+');
+  bool implicit_cb_reg = (*name == NUL) && (cb_flags & CB_UNNAMEDMASK);
+  int save_redir_off = redir_off;
+  if (!explicit_cb_reg && !implicit_cb_reg) {
+    goto end;
+  }
+
+  if (!eval_has_provider("clipboard")) {
+    if (batch_change_count == 1 && explicit_cb_reg && !quiet) {
+      redir_off = true;   // Avoid recursion from :redir + emsg().
+      // Do NOT error (emsg()) here--if it interrupts :redir we get into
+      // a weird state, stuck in "redirect mode".
+      msg((char_u *)MSG_NO_CLIP);
+    } else if (batch_change_count == 1 && implicit_cb_reg
+               && !quiet && !clipboard_didwarn_unnamed) {
+      redir_off = true;  // Avoid recursion from :redir + emsg().
+      msg((char_u *)MSG_NO_CLIP);
+      clipboard_didwarn_unnamed = true;
     }
+    // ... else, be silent (don't flood during :while, :redir, etc.).
+    goto end;
+  }
+
+  if (explicit_cb_reg) {
+    target = &y_regs[*name == '*' ? STAR_REGISTER : PLUS_REGISTER];
+    goto end;
+  } else {  // unnamed register: "implicit" clipboard
     if (writing && clipboard_delay_update) {
+      // For "set" (copy), defer the clipboard call.
       clipboard_needs_update = true;
-      return NULL;
+      goto end;
     } else if (!writing && clipboard_needs_update) {
-      // use the internal value
-      return NULL;
+      // For "get" (paste), use the internal value.
+      goto end;
     }
 
-    yankreg_T *target;
     if (cb_flags & CB_UNNAMEDPLUS) {
       *name = (cb_flags & CB_UNNAMED && writing) ? '"': '+';
       target = &y_regs[PLUS_REGISTER];
     } else {
       *name = '*';
       target = &y_regs[STAR_REGISTER];
     }
-    return target; // unnamed register
+    goto end;
   }
-  // don't do anything for other register names
-  return NULL;
+
+end:
+  redir_off = save_redir_off;
+  return target;
 }
 
 static bool get_clipboard(int name, yankreg_T **target, bool quiet)
@@ -5740,7 +5755,7 @@ static void set_clipboard(int name, yankreg_T *reg)
   (void)eval_call_provider("clipboard", "set", args);
 }
 
-/// Avoid clipboard (slow) during batch operations (i.e., a script).
+/// Avoid slow things (clipboard) during batch operations (while/for-loops).
 void start_batch_changes(void)
 {
   if (++batch_change_count > 1) {
@@ -5750,7 +5765,7 @@ void start_batch_changes(void)
   clipboard_needs_update = false;
 }
 
-/// Update the clipboard after batch changes finished.
+/// Counterpart to start_batch_changes().
 void end_batch_changes(void)
 {
   if (--batch_change_count > 0) {
@@ -5759,6 +5774,7 @@ void end_batch_changes(void)
   }
   clipboard_delay_update = false;
   if (clipboard_needs_update) {
+    // unnamed ("implicit" clipboard)
     set_clipboard(NUL, y_previous);
     clipboard_needs_update = false;
   }

test/functional/clipboard/clipboard_provider_spec.lua

@@ -4,6 +4,8 @@ local helpers = require('test.functional.helpers')(after_each)
 local Screen = require('test.functional.ui.screen')
 local clear, feed, insert = helpers.clear, helpers.feed, helpers.insert
 local feed_command, expect, eq, eval = helpers.feed_command, helpers.expect, helpers.eq, helpers.eval
+local command = helpers.command
+local meths = helpers.meths
 
 local function basic_register_test(noblock)
   insert("some words")
@@ -80,15 +82,47 @@ local function basic_register_test(noblock)
   expect("two and three and one")
 end
 
-describe('the unnamed register', function()
+describe('clipboard', function()
   before_each(clear)
-  it('works without provider', function()
+
+  it('unnamed register works without provider', function()
     eq('"', eval('v:register'))
     basic_register_test()
   end)
+
+  it('`:redir @+>` with invalid g:clipboard shows error exactly once', function()
+    local screen = Screen.new(72, 5)
+    screen:attach()
+    command("let g:clipboard = 'bogus'")
+    feed_command('redir @+> | :silent echo system("cat CONTRIBUTING.md") | redir END')
+    screen:expect([[
+      ^                                                                        |
+      ~                                                                       |
+      ~                                                                       |
+      ~                                                                       |
+      clipboard: No provider. Try ":CheckHealth" or ":h clipboard".           |
+    ]], nil, {{bold = true, foreground = Screen.colors.Blue}})
+  end)
+
+  it('invalid g:clipboard', function()
+    command("let g:clipboard = 'bogus'")
+    eq('', eval('provider#clipboard#Executable()'))
+    eq('clipboard: invalid g:clipboard', eval('provider#clipboard#Error()'))
+  end)
+
+  it('valid g:clipboard', function()
+    -- provider#clipboard#Executable() only checks the structure.
+    meths.set_var('clipboard', {
+      ['name'] = 'clippy!',
+      ['copy'] = { ['+'] = 'any command', ['*'] = 'some other' },
+      ['paste'] = { ['+'] = 'any command', ['*'] = 'some other' },
+    })
+    eq('clippy!', eval('provider#clipboard#Executable()'))
+    eq('', eval('provider#clipboard#Error()'))
+  end)
 end)
 
-describe('clipboard usage', function()
+describe('clipboard', function()
   local function reset(...)
     clear('--cmd', 'let &rtp = "test/functional/fixtures,".&rtp', ...)
   end
@@ -98,7 +132,23 @@ describe('clipboard usage', function()
     feed_command('call getreg("*")') -- force load of provider
   end)
 
-   it('has independent "* and unnamed registers per default', function()
+  it('`:redir @+>` invokes clipboard once-per-message', function()
+    eq(0, eval("g:clip_called_set"))
+    feed_command('redir @+> | :silent echo system("cat CONTRIBUTING.md") | redir END')
+    -- Assuming CONTRIBUTING.md has >100 lines.
+    assert(eval("g:clip_called_set") > 100)
+  end)
+
+  it('`:redir @">` does NOT invoke clipboard', function()
+    -- :redir to a non-clipboard register, with `:set clipboard=unnamed` does
+    -- NOT propagate to the clipboard. This is consistent with Vim.
+    command("set clipboard=unnamedplus")
+    eq(0, eval("g:clip_called_set"))
+    feed_command('redir @"> | :silent echo system("cat CONTRIBUTING.md") | redir END')
+    eq(0, eval("g:clip_called_set"))
+  end)
+
+  it('has independent "* and unnamed registers by default', function()
     insert("some words")
     feed('^"*dwdw"*P')
     expect('some ')
@@ -139,7 +189,7 @@ describe('clipboard usage', function()
     eq({'some\ntext', '\nvery binary\n'}, eval("getreg('*', 1, 1)"))
   end)
 
-  it('support autodectection of regtype', function()
+  it('autodetects regtype', function()
     feed_command("let g:test_clip['*'] = ['linewise stuff','']")
     feed_command("let g:test_clip['+'] = ['charwise','stuff']")
     eq("V", eval("getregtype('*')"))
@@ -169,7 +219,7 @@ describe('clipboard usage', function()
     eq({{' much', 'ktext', ''}, 'b'}, eval("g:test_clip['+']"))
   end)
 
-  it('supports setreg', function()
+  it('supports setreg()', function()
     feed_command('call setreg("*", "setted\\ntext", "c")')
     feed_command('call setreg("+", "explicitly\\nlines", "l")')
     feed('"+P"*p')
@@ -187,7 +237,7 @@ describe('clipboard usage', function()
         ]])
   end)
 
-  it('supports let @+ (issue #1427)', function()
+  it('supports :let @+ (issue #1427)', function()
     feed_command("let @+ = 'some'")
     feed_command("let @* = ' other stuff'")
     eq({{'some'}, 'v'}, eval("g:test_clip['+']"))
@@ -305,7 +355,7 @@ describe('clipboard usage', function()
 
   end)
 
-  describe('with clipboard=unnamedplus', function()
+  describe('clipboard=unnamedplus', function()
     before_each(function()
       feed_command('set clipboard=unnamedplus')
     end)
@@ -349,6 +399,7 @@ describe('clipboard usage', function()
         really unnamed
         the plus]])
     end)
+
     it('is updated on global changes', function()
       insert([[
 	text