[RFC] Fix for #2773: 'sxe' and 'sxq' not respected in system() call #2789
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| if (*p_sxe != NUL && STRCMP(p_sxq, "(") == 0) { | ||
| ecmd = (char*)vim_strsave_escaped_ext((char_u*)cmd, p_sxe, '^', FALSE); | ||
| } | ||
| ncmd = xmalloc(STRLEN(ecmd) + STRLEN(p_sxq) * 2 + 1); |
There was a problem hiding this comment.
Can use strlen(ecmd) here, as it's already char *.
brglng
added a commit
to brglng/neovim
that referenced
this pull request
Jun 5, 2015
brglng
added a commit
to brglng/neovim
that referenced
this pull request
Jun 5, 2015
| char *ncmd; | ||
|
|
||
| if (cmd == NULL || *p_sxq == NUL) { | ||
| ncmd = (cmd == NULL) ? NULL : xstrdup(cmd); |
There was a problem hiding this comment.
What about:
if (cmd == NULL) {
ncmd = NULL;
} else if (*p_sxq == NUL) {
ncmd = xstrdup(cmd);
} else {
// ...
}Should be equivalent, but a bit more readable IMO.
brglng
added a commit
to brglng/neovim
that referenced
this pull request
Jun 5, 2015
| @@ -37,6 +37,50 @@ typedef struct { | |||
| #endif | |||
|
|
|||
|
|
|||
| /// Process command string with 'shellxescape' (p_sxe) and 'shellxquote' | |||
| /// (p_sxq) | |||
There was a problem hiding this comment.
Should describe what happens if cmd is null.
There was a problem hiding this comment.
It is added at the @return line now.
brglng
added a commit
to brglng/neovim
that referenced
this pull request
Jun 8, 2015
brglng
added a commit
to brglng/neovim
that referenced
this pull request
Jun 8, 2015
| size_t ncmd_size; | ||
|
|
||
| if (*p_sxe != NUL && STRCMP(p_sxq, "(") == 0) { | ||
| ecmd = (char *)vim_strsave_escaped_ext((char_u *)cmd, p_sxe, '^', false); |
There was a problem hiding this comment.
That's weird, Nevermind...p_sxq == "(" implies that '^' should be escaped? Why?
:help 'sxe'
| describe('shell_build_argv', function() | ||
| setup(function() | ||
| shell.p_sxq = to_cstr('(') | ||
| shell.p_sxe = to_cstr('"&|<>()@^') |
There was a problem hiding this comment.
Could you add another test (it(...)) that exercises the unix default values:
shell.p_sxq = to_cstr('"')
shell.p_sxe = to_cstr('')Then move this setup to the beginning of the respective tests. The setup() won't be needed, and the after_each I mentioned in the other coment will clean up each it() test.
|
Thanks for this! I restarted the failed builds (one was a timeout, the other was a gcov error; both invalid failures). |
brglng
added a commit
to brglng/neovim
that referenced
this pull request
Jun 9, 2015
brglng
added a commit
to brglng/neovim
that referenced
this pull request
Jun 9, 2015
|
Haven't had a chance to look carefully at this. Did you look at Vim's implementation to make sure you covered all edge cases? |
|
Hi @brglng. Any chance we can get a rebase here? |
|
@equalsraf OK, I will do the rebase these days. |
|
If it helps here is a squashed commit I was using for testing ca4d21f |
|
@equalsraf I have done the rebase, and thanks for your help. |
| @@ -59,7 +104,8 @@ char **shell_build_argv(const char *cmd, const char *extra_args) | |||
|
|
|||
| if (cmd) { | |||
| i += tokenize(p_shcf, rv + i); // Split 'shellcmdflag' | |||
| rv[i++] = xstrdup(cmd); // Push a copy of the command. | |||
| rv[i++] = shell_escape(cmd); // Process command string with | |||
There was a problem hiding this comment.
Doing this here will affect tv_to_argv which is used by f_jobstart--but only if the first arg is String, so that seems fine. It also affects f_termopen(), not sure about that (and I don't care too much because we should deprecate that function and enhance jobstart() to replace it).
So, 👍
justinmk
pushed a commit
to justinmk/neovim
that referenced
this pull request
Sep 11, 2016
Merged
justinmk
added a commit
that referenced
this pull request
Sep 11, 2016
|
Merged after rework |
justinmk
added a commit
to justinmk/neovim
that referenced
this pull request
Oct 27, 2016
FEATURES: 0b5a7e4 neovim#4432 API: external UIs can render custom popupmenu c6ac4f8 neovim#4934 API: call any API method from vimscript 31df051 neovim#4568 API: nvim_call_atomic(): multiple calls in a single request b268ba3 neovim#5424 API: nvim_win_get_number(), nvim_tabpage_get_number() e7e2844 has("nvim-1.2.3") checks for a specific Nvim version 522b885 neovim#5295, neovim#5493 `:CheckHealth` checks tmux, terminfo, performance 719dae2 neovim#5384 events: allow event processing in getchar() f25797f neovim#5386 API: metadata: Nvim version & API level 22dfe69 neovim#5389 API: metadata: "since", "deprecated_since" 605e743 Added QuickFixLine highlight group CHANGES: 4af6ec7 neovim#5253 perf: Disable clipboard in do_cmdline() 6e9f329 neovim#5299 perf: Skip foldUpdate() in insert-mode. 9d4fcec neovim#5426 perf: Do not auto-update folds for some foldmethods. eeec0ca neovim#5419 tui: Default to normal-mode cursor shape. FIXES: e838452 neovim#5436 tui: Fix "weird characters" / "bleeding termcodes" 10a54ad neovim#5243 signal_init: Always unblock SIGCHLD. bccb49b neovim#5316 eval.c: Fix memory leak for detached pty job 626065d neovim#5227 tchdir: New tab should inherit CWD. cd321b7 neovim#5292 getcwd(): Return empty string if CWD is invalid. 6127eae shada: Fix non-writeable ShaDa directory handling ca65514 neovim#2789 system(): Respect shellxescape, shellxquote 2daf54e neovim#4874 Restore vim-like tab dragging 0c536b5 neovim#5319 syntax.c: Support bg/fg special color-names. 3c53371 neovim#4972 from justinmk/schedule-ui_refresh 68bcb32 neovim#4789 tui.c: Do not wait for tui loop on teardown. c8b6ec2 neovim#5409 v:count broken in command-line window 6bc3bce neovim#5461 fix emoji display 51937e1 neovim#5470 fix :terminal with :argadd, :argu 79d77da neovim#5481 external UIs: opening multiple files from command-line 657ba62 neovim#5501 rplugin: resolve paths in manifest file 6a6f188 neovim#5502 system('foo &', 'bar'): Show error, don't crash. 1ff162c neovim#5515 os_nodetype: open fd with O_NONBLOCK 2a6c5bb neovim#5450 modeline: Handle version number overflow. 0ade1bb neovim#5225 CI tests now run against Windows!
justinmk
added a commit
to justinmk/neovim
that referenced
this pull request
Oct 28, 2016
FEATURES: 0b5a7e4 neovim#4432 API: external UIs can render custom popupmenu c6ac4f8 neovim#4934 API: call any API method from vimscript 31df051 neovim#4568 API: nvim_call_atomic(): multiple calls in a single request b268ba3 neovim#5424 API: nvim_win_get_number(), nvim_tabpage_get_number() e7e2844 has("nvim-1.2.3") checks for a specific Nvim version 522b885 neovim#5295, neovim#5493 `:CheckHealth` checks tmux, terminfo, performance 719dae2 neovim#5384 events: allow event processing in getchar() f25797f neovim#5386 API: metadata: Nvim version & API level 22dfe69 neovim#5389 API: metadata: "since", "deprecated_since" 605e743 Added QuickFixLine highlight group CHANGES: 4af6ec7 neovim#5253 perf: Disable clipboard in do_cmdline() 6e9f329 neovim#5299 perf: Skip foldUpdate() in insert-mode. 9d4fcec neovim#5426 perf: Do not auto-update folds for some foldmethods. eeec0ca neovim#5419 tui: Default to normal-mode cursor shape. FIXES: e838452 neovim#5436 tui: Fix "weird characters" / "bleeding termcodes" 10a54ad neovim#5243 signal_init: Always unblock SIGCHLD. bccb49b neovim#5316 eval.c: Fix memory leak for detached pty job 626065d neovim#5227 tchdir: New tab should inherit CWD. cd321b7 neovim#5292 getcwd(): Return empty string if CWD is invalid. 6127eae shada: Fix non-writeable ShaDa directory handling ca65514 neovim#2789 system(): Respect shellxescape, shellxquote 2daf54e neovim#4874 Restore vim-like tab dragging 0c536b5 neovim#5319 syntax.c: Support bg/fg special color-names. 3c53371 neovim#4972 from justinmk/schedule-ui_refresh 68bcb32 neovim#4789 tui.c: Do not wait for tui loop on teardown. c8b6ec2 neovim#5409 v:count broken in command-line window 6bc3bce neovim#5461 fix emoji display 51937e1 neovim#5470 fix :terminal with :argadd, :argu 79d77da neovim#5481 external UIs: opening multiple files from command-line 657ba62 neovim#5501 rplugin: resolve paths in manifest file 6a6f188 neovim#5502 system('foo &', 'bar'): Show error, don't crash. 1ff162c neovim#5515 os_nodetype: open fd with O_NONBLOCK 2a6c5bb neovim#5450 modeline: Handle version number overflow. 0ade1bb neovim#5225 CI tests now run against Windows!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously the 'shellxescape' and 'shellxquote' were only respected in the
!commandcall. This PR adds support of 'shellxescape' and 'shellxquote' in thesystem()call. The unit test is added astest/unit/os/shell_sxe_sxq_spec.lua.