api: change nvim_command_output behavior #7821

justinmk · 2018-01-06T18:14:44Z

Implement nvim_command_output with execute({cmd},"silent").

Behavior changes:

does not provoke any hit-enter prompt
no longer prepends a newline char
does not capture some noise (like the "[New File]" message, see the
change to tabnewentered_spec.lua)

Technically ("bug-for-bug") this a breaking change. But the previous
behavior of nvim_command_output meant that it probably wasn't used for
anything outside of tests.

Also remove the undocumented v:command_output variable which was
a hack introduced only for the purposes of nvim_command_output.

closes #7726

bfredl · 2018-01-06T19:24:50Z

src/nvim/api/vim.c


+  Object rv = nvim_call_function(cstr_as_string("execute"), args, err);


are there much reason to not use f_execute directly, or (maybe even better) factor out common implementation function?

One reason is I've already spent too much time on this :)

Another reason is nvim_call_function is already taking care of the try_start dance, etc.

Creating typvals directly is simpler and cleared than first creating API values only to be converted to typvals. So you replaced a meaningful (and standard pattern) dance with a meaningless dance.

It's not clearer to me. And I thought we had planned to use the API internally?

Calling a vimscript function indirectly to implement an API function is definitely not "using the API internally" (that design pattern would be rather defining execute() in terms of an API function)
If one for some reason needs to construct an explicit typval there is no reason to construct an API value and immediately convecting it to typval. This just moves horizontally between different abstractions, at a performance cost, and not across whatever internal/external boundary. Not that performance matters matters in this particular case, but it is an anti-pattern to be avoided. ( i e "performance as a consequence of doing things right" and not optimization)

I agree, it would be cleaner to make f_execute call API nvim_command_output() and not the other way around. Though given how “much” code is actually needed I would rather go with calling do_cmdline directly with a custom LineGetter: basically you need

{ garray_T *const save_capture_ga = &capture_ga; garray_T capture_local; ga_init(&capture_local, 1, 80); capture_ga = &capture_local; try_start(); do_cmdline(NULL, get_string_line, &string, DOCMD_NOWAIT|DOCMD_VERBOSE|DOCMD_REPEAT|DOCMD_KEYTYPED); try_end(&err); Object ret = STRING_OBJ(cstr_as_string(capture_local.ga_data)); capture_ga = save_capture_ga; return ret; }

BTW, f_execute actually needs neither vim_strsave() nor ga_clear(), that is only useless waste of time.

Plus actual get_string_line, but that is not hard either.

@ZyX-I if we go this route, it might make sense to define nvim_source which sources an API list using custom reader and also has output/error handling controlled by parameter.

@ZyX-I Why is get_string_line variant needed? nvim_command_output only accepts String and returns String, so I guess do_cmdline_cmd(command.data) should be enough.

bfredl · 2018-01-06T19:26:19Z

src/nvim/api/vim.c

+/// Shell |:!| output is not captured.
+///
+/// On parse error: forwards the Vim error; does not update v:errmsg.
+/// On runtime error: forwards the Vim error; does not update v:errmsg.


Are these two intended to be different? Otherwise can be simplified

I plan to review all API functions again to nail down the differences.

bfredl · 2018-01-06T19:27:17Z

src/nvim/api/vim.c

-  return cstr_to_string((char *)get_vim_var_str(VV_COMMAND_OUTPUT));
+  assert(rv.type == kObjectTypeString);
+  // execute() always(?) prepends a newline; remove it.
+  if (rv.data.string.size > 1) {


This looks like cargo-cult magic. A common implementation can do the intended thing directly.

It's "I don't care why, but it's guarded by an assert()".

You can mark it with a TODO if noone cares to fix it right now. In the long run "the behaviour of the first error message inside a capture" doesn't sound like like something we should express uncertainty about, rather it should just do the intended thing directly.

The behavior of execute() is vim-compatible so I don't think it can be changed.

The assert(...=='\n') below here serves to document the assumption.

The behavior of execute() is already configurable, it wouldn't be strange for a common implementation (which can happen later) to also have this as a flag.

I might look into the relevant parts anyway when revisiting #7466 (which in some sense is command_output on steroids, but continuously). If we find a better way to treat newlines, it wouldn't be completely unrealistic for vim to also implement it? (either behind a new silent value, or change the default on the same theory of not bug-for-bug compatibility).

justinmk · 2018-01-09T09:42:56Z

Updated with capture_ga implementation.

Implement nvim_command_output with `execute({cmd},"silent")`. Behavior changes: - does not provoke any hit-enter prompt - no longer prepends a newline char - does not capture some noise (like the "[New File]" message, see the change to tabnewentered_spec.lua) Technically ("bug-for-bug") this a breaking change. But the previous behavior of nvim_command_output meant that it probably wasn't used for anything outside of tests. Also remove the undocumented `v:command_output` variable which was a hack introduced only for the purposes of nvim_command_output. closes neovim#7726

justinmk · 2018-01-11T09:38:06Z

travis macOS failure is unrelated. Looks like uv__stream_connect is called during teardown, @bfredl perhaps we should check exiting somewhere?

[ RUN      ] server -> client connecting to another (peer) nvim via ipv6 address: FAIL
...ovim/neovim/test/functional/api/server_requests_spec.lua:278: Expected objects to be the same.
Passed in:
(boolean) false
Expected:
(boolean) true
stack traceback:
	...ovim/neovim/test/functional/api/server_requests_spec.lua:278: in function 'connect_test'
	...ovim/neovim/test/functional/api/server_requests_spec.lua:329: in function <...ovim/neovim/test/functional/api/server_requests_spec.lua:320>
[ RUN      ] server -> client connecting to another (peer) nvim via hostname: 79.01 ms OK
========================= Core file /cores/core.19381 =========================
(lldb) target create "build/bin/nvim" --core "/cores/core.19381"
warning: (x86_64) /cores/core.19381 load command 73 LC_SEGMENT_64 has a fileoff + filesize (0x2742f000) that extends beyond the end of the file (0x2742e000), the segment will be truncated to match
warning: (x86_64) /cores/core.19381 load command 74 LC_SEGMENT_64 has a fileoff (0x2742f000) that extends beyond the end of the file (0x2742e000), ignoring this section
Core file '/cores/core.19381' (x86_64) was loaded.
(lldb) bt all
* thread #1: tid = 0x0000, 0x000000010796e584 nvim`uv__stream_io [inlined] uv__stream_connect + 156 at stream.c:1365, stop reason = signal SIGSTOP
  * frame #0: 0x000000010796e584 nvim`uv__stream_io [inlined] uv__stream_connect + 156 at stream.c:1365
    frame #1: 0x000000010796e4e8 nvim`uv__stream_io(loop=0x0000000000000014, w=<unavailable>, events=130865184) + 296 at stream.c:1289
    frame #2: 0x0000000107976942 nvim`uv__io_poll(loop=0x0000000107a9c5e8, timeout=<unavailable>) + 1538 at kqueue.c:307
    frame #3: 0x0000000107965cf8 nvim`uv_run(loop=0x0000000107a9c5e8, mode=UV_RUN_DEFAULT) + 456 at core.c:359
    frame #4: 0x000000010771db61 nvim`loop_close(loop=0x0000000107a9c5e8, wait=true) + 193 at loop.c:123
    frame #5: 0x00000001077bd224 nvim`event_teardown + 116 at main.c:178
    frame #6: 0x0000000107847bf1 nvim`mch_exit(r=0) + 49 at os_unix.c:144
    frame #7: 0x00000001077fbb43 nvim`exit_event(argv=0x00007fff585a0418) + 35 at channel.c:539
    frame #8: 0x000000010771e6c2 nvim`multiqueue_process_events(this=0x0000000007c18150) + 162 at multiqueue.c:150
    frame #9: 0x000000010771d538 nvim`loop_poll_events(loop=0x0000000107a9c5e8, ms=4000) + 248 at loop.c:63
    frame #10: 0x0000000107844367 nvim`input_poll(ms=4000) + 231 at input.c:349
    frame #11: 0x0000000107843330 nvim`inbuf_poll(ms=4000) + 32 at input.c:372
    frame #12: 0x0000000107843205 nvim`os_inchar(buf=0x0000000000000000, maxlen=0, ms=-1, tb_change_cnt=0) + 149 at input.c:110
    frame #13: 0x00000001078f0005 nvim`state_enter(s=0x00007fff585a0558) + 229 at state.c:55
    frame #14: 0x0000000107801127 nvim`normal_enter(cmdwin=false, noexmode=false) + 167 at normal.c:466
    frame #15: 0x00000001077bdeea nvim`main(argc=9, argv=0x00007fff585a0828) + 2666 at main.c:572
    frame #16: 0x00007fff8e4555ad libdyld.dylib`start + 1
    frame #17: 0x00007fff8e4555ad libdyld.dylib`start + 1
Tests covered by this check: 1
./test/helpers.lua:238: crash detected (see above)
stack traceback:
	./test/helpers.lua:238: in function 'check_cores'
	./test/functional/helpers.lua:752: in function <./test/functional/helpers.lua:747>

bfredl · 2018-01-11T09:47:25Z

Is uv__stream_connect really called? Is it not inlining that confuses the asm address inside uv__stream_io?

bfredl · 2018-01-11T09:52:13Z

Also it does not show the error, but I suppose segfault or use-after-free. One of the cases that would benefit from ASAN in libuv.
Let's first see if it reproduces with/after #7813 .

See neovim/neovim#7821

FEATURES: 3cc7ebf #7234 built-in VimL expression parser 6a7c904 #4419 implement <Cmd> key to invoke command in any mode b836328 #7679 'startup: treat stdin as text instead of commands' 58b210e :digraphs : highlight with hl-SpecialKey #2690 7a13611 #8276 'startup: Let `-s -` read from stdin' 1e71978 events: VimSuspend, VimResume #8280 1e7d5e8 #6272 'stdpath()' f96d99a #8247 server: introduce --listen e8c39f7 #8226 insert-mode: interpret unmapped META as ESC 98e7112 msg: do not scroll entire screen (#8088) f72630b #8055 let negative 'writedelay' show all redraws 5d2dd2e win: has("wsl") on Windows Subsystem for Linux #7330 a4f6cec cmdline: CmdlineEnter and CmdlineLeave autocommands (#7422) 207b7ca #6844 channels: support buffered output and bytes sockets/stdio API: f85cbea #7917 API: buffer updates 418abfc #6743 API: list information about all channels/jobs. 36b2e3f #8375 API: nvim_get_commands 273d2cd #8329 API: Make nvim_set_option() update `:verbose set …` 8d40b36 #8371 API: more reliable/descriptive VimL errors ebb1acb #8353 API: nvim_call_dict_function 9f994bb #8004 API: nvim_list_uis 3405704 #7520 API/UI: forward option updates to UIs 911b1e4 #7821 API: improve nvim_command_output WINDOWS OS: 9cefd83 #8084, #8516 build/win: support MSVC ee4e1fd win: Fix reading content from stdin (#8267) TUI: ffb8904 #8309 TUI: add support for mouse release events in urxvt 8d5a46e #8081 TUI: implement "standout" attribute 6071637 TUI: support TERM=konsole-256color 67848c0 #7653 TUI: report TUI info with -V3 ('verbose' >= 3) 3d0ee17 TUI/rxvt: enable focus-reporting d109f56 #7640 TUI: 'term' option: reflect effective terminal behavior FIXES: ed6a113 #8273 'job-control: avoid kill-timer race' 4e02f1a #8107 'jobs: separate process-group' 451c48a terminal: flush vterm output buffer on pty output #8486 5d6732f :checkhealth fixes #8335 53f11dc #8218 'Fix errors reported by PVS' d05712f inccommand: pause :terminal redraws (#8307) 51af911 inccommand: do not execute trailing commands #8256 84359a4 terminal: resize to the max dimensions (#8249) d49c1dd #8228 Make vim_fgets() return the same values as in Vim 60e96a4 screen: winhl=Normal:Background should not override syntax (#8093) 0c59ac1 #5908 'shada: Also save numbered marks' ba87a2c cscope: ignore EINTR while reading the prompt (#8079) b1412dc #7971 ':terminal Enter/Leave should not increment jumplist' 3a5721e TUI: libtermkey: force CSI driver for mouse input #7948 6ff13d7 #7720 TUI: faster startup 1c6e956 #7862 TUI: fix resize-related segfaults a58c909 #7676 TUI: always hide cursor when flushing, never flush buffers during unibilium output 303e1df #7624 TUI: disable BCE almost always 249bdb0 #7761 mark: Make sure that jumplist item will not have zero lnum 6f41ce0 #7704 macOS: Set $LANG based on the system locale a043899 #7633 'Retry fgets on EINTR' CHANGES: ad60927 #8304 default to 'nofsync' f3f1970 #8035 defaults: 'fillchars' a6052c7 #7984 defaults: sidescroll=1 b69fa86 #7888 defaults: enable cscopeverbose 7c4bb23 defaults: do :filetype stuff unless explicitly "off" 2aa308c #5658 'Apply :lmap in macros' 8ce6393 terminal: Leave 'relativenumber' alone (#8360) e46534b #4486 refactor: Remove maxmem, maxmemtot options 131aad9 win: defaults: 'shellcmdflag', 'shellxquote' #7343 c57d315 #8031 jobwait(): return -2 on interrupt also with timeout 6452831 clipboard: macOS: fallback to tmux if pbcopy is broken #7940 300d365 #7919 Make 'langnoremap' apply directly after a map ada1956 #7880 'lua/executor: Remove lightuserdata' INTERNAL: de0a954 #7806 internal statistics for list impl dee78a4 #7708 rewrite internal list impl

bfredl reviewed Jan 6, 2018

View reviewed changes

justinmk force-pushed the api-nvim_command_output branch from 923bf6e to cca0f42 Compare January 9, 2018 01:20

justinmk force-pushed the api-nvim_command_output branch from 406bcfa to ed3194c Compare January 10, 2018 08:38

justinmk force-pushed the api-nvim_command_output branch from 3e4468e to 68536a4 Compare January 10, 2018 22:51

api: nvim_command_output: direct impl

5055d4a

justinmk force-pushed the api-nvim_command_output branch from 68536a4 to 5055d4a Compare January 10, 2018 22:59

justinmk merged commit 911b1e4 into neovim:master Jan 11, 2018

justinmk deleted the api-nvim_command_output branch January 11, 2018 09:39

ghost pushed a commit to neovim/go-client that referenced this pull request Mar 17, 2018

Updatae test for change to nvim_command_output

2057b8c

See neovim/neovim#7821

ghost mentioned this pull request Mar 17, 2018

Update test for change to nvim_command_output neovim/go-client#33

Merged

ghost pushed a commit to neovim/go-client that referenced this pull request Mar 17, 2018

Update test for change to nvim_command_output

852e087

See neovim/neovim#7821

ghost pushed a commit to neovim/go-client that referenced this pull request Mar 17, 2018

Update test for change to nvim_command_output

e5c4b2f

See neovim/neovim#7821

ghost pushed a commit to neovim/go-client that referenced this pull request Mar 17, 2018

Update test for change to nvim_command_output

4ab7738

See neovim/neovim#7821

akinsho mentioned this pull request Jun 20, 2018

Marks Side Bar item causes Neovim Message Pop up onivim/oni#1461

Closed


		Object rv = nvim_call_function(cstr_as_string("execute"), args, err);

Uh oh!

api: change nvim_command_output behavior #7821

api: change nvim_command_output behavior #7821

Uh oh!

Conversation

justinmk commented Jan 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bfredl Jan 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bfredl Jan 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinmk Jan 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bfredl Jan 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinmk commented Jan 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

justinmk commented Jan 11, 2018

Uh oh!

bfredl commented Jan 11, 2018

Uh oh!

bfredl commented Jan 11, 2018

Uh oh!

Uh oh!

bfredl Jan 6, 2018 •

edited

Loading

bfredl Jan 6, 2018 •

edited

Loading

justinmk Jan 6, 2018 •

edited

Loading

bfredl Jan 6, 2018 •

edited

Loading

justinmk commented Jan 9, 2018 •

edited

Loading