[vim/vim] WIP: system()/systemlist(): accept List as command argument (PR #19791)
mattn
Allow system() and systemlist() to accept a List as the first argument, like job_start() does. When a List is given, the command is executed directly without invoking the shell.
This is still work in progress. Platform-specific implementations, documentation, and tests are not yet included.
Closes #19789
You can view, comment on, or merge this pull request online at:
https://github.com/vim/vim/pull/19791
Commit Summary
- 5da978b WIP: system()/systemlist(): accept List as command argument
File Changes
(2 files)
- M src/evalfunc.c (2)
- M src/misc1.c (63)
Patch Links:
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
mattn
@mattn pushed 1 commit.
- f47c008 WIP: system()/systemlist(): add Windows implementation
—
View it on GitHub.
You are receiving this because you are subscribed to this thread.
mattn
@mattn pushed 1 commit.
- 97cd256 WIP: system()/systemlist(): add comments to Windows implementation
—
View it on GitHub.
You are receiving this because you are subscribed to this thread.
mattn
@mattn pushed 1 commit.
- 5e56d5c WIP: system()/systemlist(): add verbose logging to Windows implementation
—
View it on GitHub.
You are receiving this because you are subscribed to this thread.
mattn
@mattn pushed 1 commit.
- 16c8996 WIP: system()/systemlist(): add ch_log to Windows implementation
—
View it on GitHub.
You are receiving this because you are subscribed to this thread.
mattn
@mattn pushed 1 commit.
- 6993664 WIP: system()/systemlist(): add Unix implementation
—
View it on GitHub.
You are receiving this because you are subscribed to this thread.
mattn
mattn left a comment (vim/vim#19791)I'm considering whether to add process tree termination (like job_start() does with JobObject on Windows and process groups on Unix), but not sure if it's needed for synchronous execution.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
mattn
mattn left a comment (vim/vim#19791)The current system() and systemlist() with a string argument don't have a mechanism to terminate the child process on CTRL-C either, so I'll leave this as-is for now.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
mattn
@mattn pushed 1 commit.
- 11e48bd WIP: system()/systemlist(): fix vim9 builtin test for new type check
—
View it on GitHub.
You are receiving this because you are subscribed to this thread.
mattn
mattn left a comment (vim/vim#19791)Note to self: if process tree termination is needed in the future, Unix would use kill(9) on a process group, and Windows would use JobObject like job_start() does. In that case, interrupt detection would also be needed.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
mattn
@mattn pushed 1 commit.
- 0309b25 WIP: system()/systemlist(): fix proto order in os_win32.pro
—
View it on GitHub.
You are receiving this because you are subscribed to this thread.
mattn
@mattn pushed 1 commit.
- a409a5b WIP: system()/systemlist(): fix preprocessor indentation in os_win32.c
—
View it on GitHub.
You are receiving this because you are subscribed to this thread.
mattn
@mattn pushed 1 commit.
- cc67414 system()/systemlist(): add tests for List argument
—
View it on GitHub.
You are receiving this because you are subscribed to this thread.
mattn
mattn left a comment (vim/vim#19791)Now that the List form bypasses the shell, environment variables like $HOME or %USERPROFILE% are no longer expanded. We may need to support a dict as the first argument to pass environment variables explicitly, like:
system({"cmd": ["foo", "bar"], "env": {"FOO": "BAR"}})
Since the second argument of system() is already used for stdin input, this would be the natural place to put it.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
mattn
mattn left a comment (vim/vim#19791)But for this PR, I'll keep it to List support only. Dict support can be added in a follow-up.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
mattn
@mattn pushed 1 commit.
- 17794e4 system()/systemlist(): fix test for CR on Windows
—
View it on GitHub.
You are receiving this because you are subscribed to this thread.
mattn
mattn left a comment (vim/vim#19791)Implemented list argument of system()/systemlist()
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
chrisbra left a comment (vim/vim#19791)Thanks for working on this. However this requires documentation updates that the list form works without a shell. We should also mention, that certain things won't work then (redirection, expansions, etc).
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
mattn
@mattn pushed 1 commit.
- 8fae2f8 system()/systemlist(): document List argument form
—
View it on GitHub.
You are receiving this because you are subscribed to this thread.
mattn
mattn left a comment (vim/vim#19791)Document updated
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
@chrisbra commented on this pull request.
In src/misc1.c:
> +
+ if (p_verbose > 3)
+ {
+ int i;
+ garray_T ga;
+
+ verbose_enter();
+ ga_init2(&ga, 1, 200);
+ for (i = 0; i < argc; ++i)
+ {
+ if (i > 0)
+ ga_append(&ga, ' ');
+ ga_concat(&ga, (char_u *)argv[i]);
+ }
+ ga_append(&ga, NUL);
+ smsg(_("Executing directly: \"%s\""), (char *)ga.ga_data);
okay, you are logging here.
In src/os_unix.c:
> +
+ if (p_verbose > 3)
+ {
+ garray_T log_ga;
+ int i;
+
+ verbose_enter();
+ ga_init2(&log_ga, 1, 200);
+ for (i = 0; argv[i] != NULL; ++i)
+ {
+ if (i > 0)
+ ga_append(&log_ga, ' ');
+ ga_concat(&log_ga, (char_u *)argv[i]);
+ }
+ ga_append(&log_ga, NUL);
+ smsg(_("Executing directly: \"%s\""), (char *)log_ga.ga_data);
now you are logging it again for the Unix implementation.
In src/os_win32.c:
> + break;
+ }
+
+ if (i > 0)
+ ga_append(&cmd_ga, ' ');
+
+ if (has_spaces)
+ {
+ ga_append(&cmd_ga, '"');
+ for (j = 0; arg[j] != NUL; j++)
+ {
+ if (arg[j] == '"')
+ ga_append(&cmd_ga, '\\');
+ ga_append(&cmd_ga, arg[j]);
+ }
+ ga_append(&cmd_ga, '"');
Hm, when cmd_ga ends with a backslash like C:\user\ this will escape the double quote and make the command invalid, no?
In src/os_win32.c:
> + }
+
+ ZeroMemory(&pi, sizeof(pi));
+ ZeroMemory(&si, sizeof(si));
+ si.cb = sizeof(si);
+ si.dwFlags = STARTF_USESTDHANDLES | STARTF_USESHOWWINDOW;
+ si.wShowWindow = SW_HIDE;
+ si.hStdOutput = hChildStdoutWr;
+ si.hStdError = hChildStdoutWr;
+ si.hStdInput = (hChildStdinRd != INVALID_HANDLE_VALUE)
+ ? hChildStdinRd : INVALID_HANDLE_VALUE;
+
+ if (p_verbose > 3)
+ {
+ verbose_enter();
+ smsg(_("Executing directly: \"%s\""), (char *)cmd_ga.ga_data);
logging again on the Windows implementation
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
mattn
@mattn pushed 1 commit.
- 2dedfef system()/systemlist(): remove duplicate verbose logging
—
View it on GitHub.
You are receiving this because you are subscribed to this thread.
mattn
@mattn pushed 1 commit.
- bfb5150 system()/systemlist(): fix backslash quoting on Windows
—
View it on GitHub.
You are receiving this because you are subscribed to this thread.
mattn
mattn left a comment (vim/vim#19791)ptal
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
@chrisbra commented on this pull request.
In src/os_unix.c:
> + int *ret_len)
+{
+ pid_t pid;
+ int fd_out[2] = {-1, -1};
+ int status = -1;
+ char_u *buffer = NULL;
+ garray_T ga;
+ SIGSET_DECL(curset)
+
+ ga_init2(&ga, 1, 4096);
+
+ ch_log(NULL, "directly executing: %s", argv[0]);
+
+ if (pipe(fd_out) < 0)
+ {
+ emsg(_("E5677: Cannot create pipes"));
sorry, I did not notice previously, can you make this a proper error message inside error.h?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
mattn
@mattn pushed 1 commit.
- 33526ce system()/systemlist(): move E5677 to errors.h
—
View it on GitHub.
You are receiving this because you are subscribed to this thread.
mattn
mattn left a comment (vim/vim#19791)thank you. fixed
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.