Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RDY] :cquit to take an error code argument #7336

Closed
wants to merge 17 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@joshleeb
Copy link
Contributor

joshleeb commented Sep 28, 2017

Enhance :cquit to take an error code argument #2699.

Update the :cquit command to take an error code argument. By default the exit code is still 1, but when an exit code is provided then Neovim will exit with the specified code.

Josh Leeb-du Toit added some commits Sep 28, 2017

Josh Leeb-du Toit
ex_cmds.lua: Update cquit flags for exit value
Update `cquit` command flags to allow an exit value as an argument.
Josh Leeb-du Toit
ex_docmd.c: update cquit to exit with given value
Update the `cquit` command to execute with a given exit value.
Josh Leeb-du Toit
test: add test for cquit given an exit value
Add a test to check that `cquit` exits with the specified exit value
when an exit value is provided.
Josh Leeb-du Toit
doc: update quickfix docs for cquit command
Update the quickfix documentation for the `cquit` command to exit with a
specified exit value.
Josh Leeb-du Toit
doc: update formatting of cquit quickfix docs
Update the formatting of the `cquit` quickfix documentation.
@jamessan

This comment has been minimized.

Copy link
Member

jamessan commented Sep 28, 2017

There was a similar patch proposed upstream. I think it'd be worth prodding that discussion to see if it can get merged into Vim.

@marvim marvim added the WIP label Sep 28, 2017

Josh Leeb-du Toit
lint: fix linting errors for ex_cquit function
Fix linting errors for the `ex_cquit` function. The two errors were: the
format of the comment within the function; and the spacing surrounding a
type cast.
@joshleeb

This comment has been minimized.

Copy link
Contributor Author

joshleeb commented Sep 29, 2017

There was a similar patch proposed upstream.

Okay thanks, I'll take a look.

WARNING: All changes in files are lost! Also when the
[!] is not used. It works like ":qall!" |:qall|,
except that Vim returns a non-zero exit code.
:cq[uit][!] Quit Vim with an error code (When vim is called from another

This comment has been minimized.

@ZyX-I

ZyX-I Sep 29, 2017

Contributor

Here should be argument listed, or :[count]cq[uit], better the second.

@@ -614,7 +614,7 @@ return {
},
{
command='cquit',
flags=bit.bor(TRLBAR, BANG),
flags=bit.bor(TRLBAR, BANG, WORD1),

This comment has been minimized.

@ZyX-I

ZyX-I Sep 29, 2017

Contributor

It is more consistent with COUNT plus NOTADR and not WORD1 and argument: this would allow typing :2cq<CR> in addition to :cq2<CR> (which is consistent with e.g. :cnext), and simplify ex_cquit to a great extent: you just call getout(eap->addr_count > 0 ? eap->line2 : 1).

return ''
end
run(on_request, nil, on_setup)
end)

This comment has been minimized.

@ZyX-I

ZyX-I Sep 29, 2017

Contributor

Missing negative checks: what will happen if you supply it non-number argument? Number followed by non-number? Out-of-range for exit codes (e.g. 1024 while maximal exit code is 255) number? Out-of-range for regular integer number?

(BTW, COUNT takes care of that too: :cnext 1x and :cnext x are errors and you are absolutely missing checking for errors what you must do.)

return ''
end
run(on_request, nil, on_setup)
end)

This comment has been minimized.

@ZyX-I

ZyX-I Sep 29, 2017

Contributor

Also it is good idea to test whether executing if 0|cquit 5|endif throws some syntax errors (better use redir_exec, not exc_exec). With some combination of flags you need to bother populating eap->nextcmd, though I do not remember with which exactly (most likely not WORD1 though and definitely not with COUNT+NOTADR, but without WORD1).

This comment has been minimized.

@ZyX-I

ZyX-I Sep 29, 2017

Contributor

And check :0cquit and :cquit 0. It is logical that second will yield zero exit code and illogical, but consistent that first will yield exit code 1 (check :0cnext vs :cnext 0).

This comment has been minimized.

@ZyX-I

ZyX-I Sep 29, 2017

Contributor

Ah, no, add ZEROR flag to the list and :0cquit should be the same as :cquit 0. It is just :cnext thinks that is incorrect.

This comment has been minimized.

@ZyX-I

ZyX-I Sep 29, 2017

Contributor

And COUNT goes with RANGE for desired behaviour. Basically what is expected is

diff --git a/src/nvim/ex_cmds.lua b/src/nvim/ex_cmds.lua
index 5a578cd08..6a1e6ceb3 100644
--- a/src/nvim/ex_cmds.lua
+++ b/src/nvim/ex_cmds.lua
@@ -614,7 +614,7 @@ return {
   },
   {
     command='cquit',
-    flags=bit.bor(TRLBAR, BANG),
+    flags=bit.bor(RANGE, NOTADR, COUNT, ZEROR, TRLBAR, BANG),
     addr_type=ADDR_LINES,
     func='ex_cquit',
   },

Josh Leeb-du Toit added some commits Sep 29, 2017

Josh Leeb-du Toit
doc: update command documentation for cquit
Update the documentation for the `cquit` command to add a count argument
and remove the bang.
Josh Leeb-du Toit
ex_cmds.lue: update flags for cquit command
Update flags for `cquit` command to be more consistent with similar
commands such as `cnext`.
Josh Leeb-du Toit
ex_docmd.c: ex_cquit function and add value checks
Update the `ex_cquit` function to work with the new command flags, and
add a check for an out of bounds exit value.
@ZyX-I

This comment has been minimized.

Copy link
Contributor

ZyX-I commented Sep 29, 2017

Original patch, BTW, has some strange code: it should not check *eap->arg, ex_docmd takes care of checking for trailing characters already, using [code] goes against more common convention of using [count] (and there are no tests). Though it appears that all what is needed for :cquit is indeed just

diff --git a/src/nvim/ex_docmd.c b/src/nvim/ex_docmd.c
index 6e7938046..825d93a95 100644
--- a/src/nvim/ex_docmd.c
+++ b/src/nvim/ex_docmd.c
@@ -5995,7 +5995,7 @@ static void ex_quit(exarg_T *eap)
  */
 static void ex_cquit(exarg_T *eap)
 {
-  getout(1);
+  getout((int)eap->line2);
 }
 
 /*
@joshleeb

This comment has been minimized.

Copy link
Contributor Author

joshleeb commented Sep 29, 2017

Thanks a lot @ZyX-I. I've updated the patch based on your comments.

I've confirmed that if 0|cquit 5|endif doesn't throw a syntax error and works as expected. Also that :cquit 0 and :0 cquit exit with a status code of 0. For no exit value, the status code remains as the default, which is 1. And when a value out of the exit value range is provided then it will set the exit code to 255.

int exitval = eap->addr_count > 0 ? eap->line2 : 1;

// Exit status out of range.
if (exitval > 255) {

This comment has been minimized.

@ZyX-I

ZyX-I Sep 29, 2017

Contributor

I think that should too great exit values cause any issues the situation should be handled by mch_exit and not here and not even getout. AFAIK on linux it just works like a cast to uint8_t. BTW, it looks like according to the C99 standard EXIT_FAILURE is the only proper default: 0 or EXIT_SUCCESS indicates success (standard explicitly mentions both), EXIT_FAILURE indicates failure and everything else is “implementation-defined” with no particular meaning attached by the standard, but without claiming that any other value yields undefined behaviour either.

So do not limit values by yourself, and use EXIT_FAILURE. May need an addition to os/win_defs.h: import stdlib.h there, define EXIT_FAILURE to 1 if it is not already defined: Windows headers have a habit of not containing some values they must according to the standard.

This comment has been minimized.

@ZyX-I

ZyX-I Sep 29, 2017

Contributor
diff --git a/src/nvim/ex_docmd.c b/src/nvim/ex_docmd.c
index 6e7938046..d4ce78aa3 100644
--- a/src/nvim/ex_docmd.c
+++ b/src/nvim/ex_docmd.c
@@ -9,6 +9,7 @@
 #include <string.h>
 #include <stdbool.h>
 #include <stdint.h>
+#include <stdlib.h>
 #include <inttypes.h>
 
 #include "nvim/vim.h"
@@ -5995,7 +5996,7 @@ static void ex_quit(exarg_T *eap)
  */
 static void ex_cquit(exarg_T *eap)
 {
-  getout(1);
+  getout(eap->addr_count > 0 ? (int)eap->line2 : EXIT_FAILURE);
 }
 
 /*

This comment has been minimized.

@ZyX-I

ZyX-I Sep 29, 2017

Contributor

The good thing is that Vim help was nice enough to not specify what “error code” is exactly, so EXIT_FAILURE will not go against the help even should it be not one (and I have it defined to 1).

:[count]cq[uit] Quit Vim with an error code (When vim is called from
another program, the caller might behave in a special way if the
error code is not 0; e.g. `git commit`, `fc` in bash, some
compilers.)

This comment has been minimized.

@ZyX-I

ZyX-I Sep 29, 2017

Contributor

Missing documentation on what “count” means, also this does not have to have parenthesis:

Quit Vim with an error code, or the code specified in [count]. This may make a difference when Vim is called from another program which needs editor: e.g. git commit will abort the comitting process, fc (built-in from various shells including bash and zsh) will not execute the command, …

No need to mention “some compilers”, it is not useful without some specifics (like what was in original message). Not suggesting to add “compiler will not compile …” because of me never hearing the compiler which bothers to call editor at all.

@joshleeb joshleeb changed the title [WIP] :cquit to take an error code argument [RFC] :cquit to take an error code argument Sep 29, 2017

@marvim marvim added RFC and removed WIP labels Sep 29, 2017

return ''
end
run(on_request, nil, on_setup)
end)

This comment has been minimized.

@ZyX-I

ZyX-I Sep 29, 2017

Contributor

Still missing most of the tests I listed. I would also highly advise on creating a “test generator” function called like test_cq('cquit 0', 0, nil):

local function test_cq(cmdline, exit_code, redir_msg)
  if redir_msg then
    eq('\n' .. redir_msg, redir_exec(cmdline))
    wait()
    eq(1, eval('1'))  -- Check that Neovim did not crash or exit
  else
    funcs.system({nvim_prog, '-u', 'NONE', '-i', 'NONE', '--headless', '--cmd', cmdline})
    eq(exit_code, eval('v:shell_error'))
    local function on_setup()
       command('autocmd VimLeavePre * call rpcrequest('..cid..', "")')
       command('autocmd VimLeave    * call rpcrequest('..cid..', "")')
       command(cmdline)
     end
     local function on_request()
       eq(exit_code, eval('v:exiting'))
       return ''
     end
     run(on_request, nil, on_setup)
  end
end

Note the significant difference: I do not suggest checking only v:exiting, this test does not really serve its purpose because you can’t observe exit code before Neovim actually exits and you can’t have v:exiting available after it exits: test is only valid as long as there is connection between v:exiting and exit value and it is not necessary the case if somebody added a bug. Actually since I mentioned it: what you are testing is :cquit, not v:exiting, so while checking for it sideways is fine, setting v:exiting is by no means a purpose of :cquit: it is purpose of v:exiting to contain exit value Neovim will use, purpose of :cq is quit Neovim with specified exit value which is checked by system() check only.

Thus remove :cq out of the current describe() block, it is not correct.

Josh Leeb-du Toit
test: add tests for cquit command
Add tests for edge and error cases for the `cquit` command. These tests
were moved into a separate `describe` block within the same file
`exit_spec.lua`.

@joshleeb joshleeb force-pushed the joshleeb:cquit-exitval branch from 79546bd to 8b0a38c Sep 30, 2017

Josh Leeb-du Toit
test: remove windows specific test for cquit
Remove windows specific test for the command `cquit` which performs a
check for out of range exit codes.

@joshleeb joshleeb force-pushed the joshleeb:cquit-exitval branch from 8b0a38c to cd9593d Sep 30, 2017

@ZyX-I

This comment has been minimized.

Copy link
Contributor

ZyX-I commented Sep 30, 2017

AppVeyor MINGW_32 failure looks unrelated: it fails building dependencies:

[ 92%] Generating usr/lib/luarocks/rocks/mpack
call usr\2.4\luarocks.bat build mpack CC=C:/msys64/mingw32/bin/gcc.exe LD=C:/msys64/mingw32/bin/gcc.exe
lmpack.c:21:17: fatal error: lua.h: No such file or directory
 #include <lua.h>
                 ^
compilation terminated.
C:/msys64/mingw32/bin/gcc.exe -O2 -c -o lmpack.o -IC:/projects/neovim/.deps/usr/include/luajit-2.0/ lmpack.c
Error: Build error: Failed compiling object lmpack.o
mingw32-make[2]: *** [CMakeFiles\mpack.dir\build.make:60: usr/lib/luarocks/rocks/mpack] Error 1
mingw32-make[2]: Leaving directory 'C:/projects/neovim/.deps'
mingw32-make[1]: *** [CMakeFiles\Makefile2:99: CMakeFiles/mpack.dir/all] Error 2
mingw32-make[1]: Leaving directory 'C:/projects/neovim/.deps'
mingw32-make: *** [Makefile:83: all] Error 2
@ZyX-I

ZyX-I approved these changes Sep 30, 2017

@joshleeb joshleeb changed the title [RFC] :cquit to take an error code argument [RDY] :cquit to take an error code argument Sep 30, 2017

@marvim marvim added RDY and removed RFC labels Sep 30, 2017

justinmk added a commit that referenced this pull request Oct 22, 2017

:cquit : take an error code argument #7336
closes #2699

ex_cmds.lua: use flags consistent with similar commands such as `cnext`.

upstream discussion:
"[patch] :qcuit can take exit code"
https://groups.google.com/d/msg/vim_dev/_PjyNbUKyRc/oPgr5_ZXc6AJ
@justinmk

This comment has been minimized.

Copy link
Member

justinmk commented Oct 22, 2017

Merged. @joshleeb thanks for the attention to detail.

Granular commits are appreciated if they explain something that's not obvious by the code change, otherwise just name them "fixup" and squash them later.

@justinmk justinmk closed this Oct 22, 2017

@justinmk justinmk removed the RDY label Oct 22, 2017

@justinmk justinmk added this to the 0.2.1 milestone Oct 22, 2017

sameedali added a commit to sameedali/neovim that referenced this pull request Oct 24, 2017

Merge branch 'master' of https://github.com/neovim/neovim
* 'master' of https://github.com/neovim/neovim: (554 commits)
  test: tabstop=<big-number> neovim#2838
  plines_win_nofold(): Ignore virtcols after 32000th computation neovim#3527
  :cquit : take an error code argument neovim#7336
  vim-patch:8.0.0140 (neovim#7428)
  cmake,bsd: Fix mandir to saner defaults. (neovim#7417)
  help, man.vim: change "outline" map to gO (neovim#7405)
  build: set MIN_LOG_LEVEL correctly (neovim#7419)
  vim-patch:8.0.1019
  vim-patch:8.0.0962
  bufhl: support creating new groups
  lint
  refactor/single-include: undo.h
  refactor/single-include: undo_defs.h
  refactor/single-include: syntax_defs.h
  refactor/single-include: regexp_defs.h
  refactor/single-include: terminal.h
  vim-patch:8.0.0118
  :checkhealth : validate $VIM
  ex_checkhealth: call health#check() directly
  doc: E5009 "Invalid $VIMRUNTIME"
  ...

justinmk added a commit to justinmk/neovim that referenced this pull request Nov 8, 2017

NVIM v0.2.1
FEATURES:
0e873a3 Lua(Jit) built-in neovim#4411
5b32bce Windows: `:terminal` neovim#7007
7b0ceb3 UI/API: externalize cmdline neovim#7173
b67f58b UI/API: externalize wildmenu neovim#7454
b23aa1c UI: 'winhighlight' neovim#6597
17531ed UI: command-line coloring (`:help input()-highlight`) neovim#6364
244a1f9 API: execute lua directly from the remote api neovim#6704
45626de API: `get_keymap()` neovim#6236
db99982 API: `nvim_get_hl_by_name()`, `nvim_get_hl_by_id()` neovim#7082
dc68538 menu_get() function neovim#6322
9db42d4 :cquit : take an error code argument neovim#7336
9cc185d job-control: serverstart(): support ipv6 neovim#6680
1b7a9bf job-control: sockopen() neovim#6594
6efe84a clipboard: fallback to tmux clipboard neovim#6894
6016ac2 clipboard: customize clipboard with `g:clipboard` neovim#6030
3a86dd5 ruby: override ruby host via `g:ruby_host_prog` neovim#6841
16cce1a debug: $NVIM_LOG_FILE neovim#6827
0cba3da `:checkhealth` built-in, validates $VIMRUNTIME neovim#7399

FIXES:
105d680 TUI: more terminals, improve scroll/resize neovim#6816
cb912a3 :terminal : handle F1-F12, other keys neovim#7241
619838f inccommand: improve performance neovim#6949
04b3c32 inccommand: Fix matches for zero-width neovim#7487
60b1e8a inccommand: multiline, other fixes neovim#7315
f1f7f3b inccommand: Ignore leading modifiers in the command neovim#6967
1551f71 inccommand: fix 'gdefault' lockup neovim#7262
6338199 API: bufhl: support creating new groups neovim#7414
541dde3 API: allow K_EVENT during operator-pending
8c732f7 terminal: adjust for 'number' neovim#7440
5bec946 UI: preserve wildmenu during jobs/events neovim#7110
c349083 UI: disable 'lazyredraw' during ui_refresh. neovim#6259
51808a2 send FocusGained/FocusLost event instead of pseudokey neovim#7221
133f8bc shada: preserve unnamed register on restart neovim#4700
1b70a1d shada: avoid assertion on corrupt shada file neovim#6958
9f534f3 mksession: Restore tab-local working directory neovim#6859
de1084f fix buf_write() crash neovim#7140
7f76986 syntax: register 'Normal' highlight group neovim#6973
6e7a8c3 RPC: close channel if stream was closed neovim#7081
85f3084 clipboard: disallow recursion; show hint only once neovim#7203
8d1ccb6 clipboard: performance, avoid weird edge-cases neovim#7193
01487d4 'titleold' neovim#7358
01e53a5 Windows: better path-handling, separator (slash) hygiene neovim#7349
0f2873c Windows: multibyte startup arguments neovim#7060

CHANGES:
9ff0cc7 :terminal : start in normal-mode neovim#6808
032b088 lower priority of 'cursorcolumn', 'colorcolumn' neovim#7364
2a3bcd1 RPC: Don't delay notifications when request is pending neovim#6544
023f67c :terminal : Do not change 'number', 'relativenumber' neovim#6796
1ef2d76 socket.c: Disable Nagle's algorithm on TCP sockets neovim#6915
6720fe2 help: `K` tries Vim help instead of manpage neovim#3104
7068370 help, man.vim: change "outline" map to `gO` neovim#7405

@justinmk justinmk referenced this pull request Nov 8, 2017

Merged

NVIM v0.2.1 #7505

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.