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] Fix allowing arguments as part of shell for `:term` command #4504

Closed
wants to merge 11 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@FriedSock
Contributor

FriedSock commented Mar 29, 2016

Fixes the default behaviour of :term, especially when arguments have been set as part of the shell setting. Fixes #3999

@FriedSock FriedSock changed the title from Fix allowing arguments as part of shell to Fix allowing arguments as part of shell for `:term` command Mar 29, 2016

@FriedSock FriedSock force-pushed the FriedSock:fix-allowing-arguments-as-part-of-shell branch from 0e3351e to 0f77d32 Mar 29, 2016

@ZyX-I

This comment has been minimized.

Contributor

ZyX-I commented Mar 29, 2016

This is not a PR that fixes formatting, so first commit should not be here. You should not do changes to irrelevant functions because this fails lint tests (change to some line makes linter stop ignoring errors in two adjacent lines and in that line itself), makes review harder and specifically this change does not really make code look cleaner.

@FriedSock FriedSock force-pushed the FriedSock:fix-allowing-arguments-as-part-of-shell branch from 0f77d32 to 3765359 Mar 29, 2016

@FriedSock

This comment has been minimized.

Contributor

FriedSock commented Mar 29, 2016

sorry @ZyX-I, I have removed it now.

@FriedSock FriedSock changed the title from Fix allowing arguments as part of shell for `:term` command to [WIP] Fix allowing arguments as part of shell for `:term` command Mar 29, 2016

@marvim marvim added the WIP label Mar 29, 2016

@FriedSock FriedSock force-pushed the FriedSock:fix-allowing-arguments-as-part-of-shell branch 23 times, most recently from 1002a87 to dd960cf Apr 4, 2016

@FriedSock FriedSock force-pushed the FriedSock:fix-allowing-arguments-as-part-of-shell branch from 93d9d5f to 71d8d1b Mar 6, 2017

eap->forceit ? "!" : "", name);
xfree(name);
} else {
char **argv = build_argv((char *)p_sh);

This comment has been minimized.

@oni-link

oni-link Mar 6, 2017

Contributor

Was there a specific reason why the new build_argv(p_sh) instead of shell_build_argv(NULL,NULL) was used?

if (*p_sh == 0) {
   EMSG("No shell");
   return;
}

char **argv = shell_build_argv(NULL,NULL);
...

This comment has been minimized.

@FriedSock

FriedSock Mar 7, 2017

Contributor

Yes, I do believe I tried that.

I have tried adding it back now, without the early validation check and return, it looks like there is a hung build. Which might be because the assertion on src/nvim/shell.c:71 would fail if shell was blank and cmd/extra args are both NULL.

Adding the early EMSG to avoid this seems to also cause test failures that could be due to early returning of ex_terminal. I'm going to revert back to using build_argv.

@@ -41,6 +41,9 @@ typedef struct {
/// Builds the argument vector for running the user-configured 'shell' (p_sh)
/// with an optional command prefixed by 'shellcmdflag' (p_shcf).
///
/// Creates ["shell", "-shellcommandflag", "-extra_args",

This comment has been minimized.

@oni-link

oni-link Mar 6, 2017

Contributor

The order needs to be changed for -shellcommandflag and -extra_args.

@FriedSock FriedSock force-pushed the FriedSock:fix-allowing-arguments-as-part-of-shell branch 3 times, most recently from 71d8d1b to 43b1690 Mar 7, 2017

@justinmk

This comment has been minimized.

Member

justinmk commented Mar 7, 2017

@FriedSock The build isn't going to hang if an assertion fails. It means nvim is waiting for input, e.g. a "Press ENTER" prompt.

I can look into it (later) if you leave the PR as-is.

@FriedSock

This comment has been minimized.

Contributor

FriedSock commented Mar 7, 2017

@justinmk Ah right, I will also take a look at it later today and won't change it until there is a definite fix 👍

@FriedSock FriedSock force-pushed the FriedSock:fix-allowing-arguments-as-part-of-shell branch from 43b1690 to ec6ee07 Mar 9, 2017

FriedSock added some commits Mar 29, 2016

Tokenize p_sh if used as default in ex_terminal
Previously p_sh was being used as the first
argument in a list when calling termopen(), this
would try to call an untokenized version of shell,
meaning if you had an argument passed to your
shell like so:

`set shell=/bin/bash\ --login`

The command will fail as “ --login” is interpreted
as part of a file path.
Expand and refactor ex terminal tests
Adds a new test that covers the case of calling a command on a shell
that has arguments.

Extract a `terminal_with_fake_shell_with_arguments` function to allow
keeping the two test cases within the original describe block.
Add comments to build_argv and shell_build_argv
There is a subtle difference between how these two methods work. Adding
comments based on PR discussions to clarify this.
Convert vim_strcat -> xstrlcat
Since the previous build, vim_strcat was deprecated, convert to use
xstrlcat
Optimize `ex_terminal`
We can remove an if statement by shifting the pointer for the shell_arg
string past the leading comma. We also do not need spaces in shellargv.

We can also remove some array initialisation and a branch for some late
memory freeing.

@FriedSock FriedSock force-pushed the FriedSock:fix-allowing-arguments-as-part-of-shell branch from ec6ee07 to 5b65951 Mar 11, 2017

justinmk added a commit that referenced this pull request Mar 17, 2017

terminal: Support extra arguments in 'shell'. #4504
Tokenize p_sh if used as default in ex_terminal(). Previously p_sh was
used as the first arg in a list when calling termopen(), this would try
to call an untokenized version of shell, meaning if you had an argument
in 'shell':
    set shell=/bin/bash\ --login
the command would fail.

Helped-by: oni-link <knil.ino@gmail.com>

Closes #3999
@justinmk

This comment has been minimized.

Member

justinmk commented Mar 17, 2017

Merged. Thanks @FriedSock and @oni-link !

@justinmk justinmk closed this Mar 17, 2017

@justinmk justinmk removed the RDY label Mar 17, 2017

justinmk added a commit to justinmk/neovim that referenced this pull request May 1, 2017

NVIM v0.2.0
FEATURES:
    bc4a2e1 help, man.vim: "outline" (TOC) feature neovim#5169
    58422f1 'guicursor' works in the TUI (and sends info to UIs) neovim#6423
    129f107 api: nvim_get_mode() neovim#6247
    0b59f98 api/ui: externalize tabline neovim#6583
    bc6d868 'listchars': `Whitespace` highlight group neovim#6367
    6afa7d6 writefile() obeys 'fsync' option neovim#6427
    c60e409 eval.c refactor (also improves some error messages) neovim#5119
    9d200cd getcompletion("cmdline") neovim#6376
    2ea7bfc terminal: Support extra arguments in 'shell'. neovim#4504
    bf51102 DirChanged autocmd neovim#5928 neovim#6262
    1743df8 'cpoptions': "_" flag to toggle `cw` behaviour neovim#6235
    22337b1 CTRL-R omits trailing ^M when pasting to cmdline neovim#6137
    0e44916 :edit allows unescaped spaces in filename neovim#6119
    abdbfd2 eval: Add id() function and make printf("%p") useful neovim#6095
    bdfa147 findfile(), :find, gf work in :terminal. neovim#6009
    2f38ed1 providers: Disable if `g:loaded_*` exists.
    b5560a6 setpos() can set lowercase marks in other buffers neovim#5753
    7c513d6 Throttle :! output, pulse "..." message. neovim#5396
    d2e8c76 v:exiting neovim#5651

    :terminal improvements neovim#6185 neovim#6142
      - cursor keeps position after leaving insert-mode.
      - 4ceec30 Follows output only if cursor is at end of buffer.
      - e7bbd35 new option: 'scrollback'
      - fedb844 quasi-support for undo and 'modifiable'
      - b45ddf7 disables 'list' by default
      - disables 'relativenumber' by default

    :help now contains full API documentation at `:help api`.

    man.vim saw numerous improvements.

    Windows support:
      - Windows is no longer "experimental", it is fully supported.
      - Windows package includes a GUI, curl.exe and other utilities.

    "Vim 8" features: partials, lambdas, packages.

FIXES:
    12fc1de ops: fix i<c-r> with multi-byte text neovim#6524
    dd391bf Windows: system() and friends neovim#6497
    13352c0 Windows: os_get_hostname() neovim#6413
    16babc6 tui: Less-noisy mouse seqs neovim#6411
    3a9dd13 (vim bug) folding edge-cases  neovim#6207
    f6946c6 job-control: set CLOEXEC on pty processes. neovim#5986
    d1afd43 rplugin: Call s:LoadRemotePlugins() on startup.
    1215084 backtick-expansion works with `shell=fish` neovim#6224
    e32ec03 tui: Improved behavior after resize. neovim#6202
    86c2adc edit.c: CTRL-SPC: Insert previously-inserted text. neovim#6090
    c318d8e b:changedtick now follows VimL rules neovim#6112
    34e24cb terminal: Initialize colors in reverse order neovim#6160
    e889917 undo: Don't set b_u_curhead in ex_undojoin() neovim#5869
    d25649f undo: :earlier, g-: Set b_u_seq_cur correctly. (neovim#6016)
    043d8ba 'Visual-mode put from @. register' neovim#5782
    42c922b open_buffer(): Do `BufEnter` for directories.
    50d0d89 inccommand: Preview :sub commands only after delimiter neovim#5932
    1420e10 CheckHealth improvements neovim#5519
    c8d5e92 jobstart(): Return -1 if cmd is not executable. neovim#5671

CHANGES:
    NVIM_TUI_ENABLE_CURSOR_SHAPE was removed. Use 'guicursor' instead.
        See https://github.com/neovim/neovim/wiki/Following-HEAD#20170402

    81525dc 'mouse=a' is no longer the default. (This will probably
                 change again after it is improved.) neovim#6022

    0c1f783 defaults: 'showcmd', 'belloff', 'ruler' neovim#6087
    eb0e94f api: {get,set}_option update local options as appropriate neovim#6405
    bdcb2a3 "Reading from stdin..." message was removed. neovim#6298

@justinmk justinmk referenced this pull request May 1, 2017

Merged

NVIM v0.2.0 #6634

justinmk added a commit to justinmk/neovim that referenced this pull request May 1, 2017

NVIM v0.2.0
FEATURES:
    bc4a2e1 help, man.vim: "outline" (TOC) feature neovim#5169
    58422f1 'guicursor' works in the TUI (and sends info to UIs) neovim#6423
    129f107 api: nvim_get_mode() neovim#6247
    0b59f98 api/ui: externalize tabline neovim#6583
    bc6d868 'listchars': `Whitespace` highlight group neovim#6367
    6afa7d6 writefile() obeys 'fsync' option neovim#6427
    c60e409 eval.c refactor (also improves some error messages) neovim#5119
    9d200cd getcompletion("cmdline") neovim#6376
    2ea7bfc terminal: Support extra arguments in 'shell'. neovim#4504
    bf51102 DirChanged autocmd neovim#5928 neovim#6262
    1743df8 'cpoptions': "_" flag to toggle `cw` behaviour neovim#6235
    22337b1 CTRL-R omits trailing ^M when pasting to cmdline neovim#6137
    0e44916 :edit allows unescaped spaces in filename neovim#6119
    abdbfd2 eval: Add id() function and make printf("%p") useful neovim#6095
    bdfa147 findfile(), :find, gf work in :terminal. neovim#6009
    2f38ed1 providers: Disable if `g:loaded_*` exists.
    b5560a6 setpos() can set lowercase marks in other buffers neovim#5753
    7c513d6 Throttle :! output, pulse "..." message. neovim#5396
    d2e8c76 v:exiting neovim#5651

    :terminal improvements neovim#6185 neovim#6142
      - cursor keeps position after leaving insert-mode.
      - 4ceec30 Follows output only if cursor is at end of buffer.
      - e7bbd35 new option: 'scrollback'
      - fedb844 quasi-support for undo and 'modifiable'
      - b45ddf7 disables 'list' by default
      - disables 'relativenumber' by default

    :help now contains full API documentation at `:help api`.

    man.vim saw numerous improvements.

    Windows support:
      - Windows is no longer "experimental", it is fully supported.
      - Windows package includes a GUI, curl.exe and other utilities.

    "Vim 8" features: partials, lambdas.

SECURITY FIXES:
    CVE-2017-5953 CVE-2017-6349 CVE-2017-6350 neovim#6485

CHANGES:
    NVIM_TUI_ENABLE_CURSOR_SHAPE was removed. Use 'guicursor' instead.
        See https://github.com/neovim/neovim/wiki/Following-HEAD#20170402

    81525dc 'mouse=a' is no longer the default. (This will probably
                 change again after it is improved.) neovim#6022

    0c1f783 defaults: 'showcmd', 'belloff', 'ruler' neovim#6087
    eb0e94f api: {get,set}_option update local options as appropriate neovim#6405
    bdcb2a3 "Reading from stdin..." message was removed. neovim#6298

FIXES:
    12fc1de ops: fix i<c-r> with multi-byte text neovim#6524
    dd391bf Windows: system() and friends neovim#6497
    13352c0 Windows: os_get_hostname() neovim#6413
    16babc6 tui: Less-noisy mouse seqs neovim#6411
    3a9dd13 (vim bug) folding edge-cases  neovim#6207
    f6946c6 job-control: set CLOEXEC on pty processes. neovim#5986
    d1afd43 rplugin: Call s:LoadRemotePlugins() on startup.
    1215084 backtick-expansion works with `shell=fish` neovim#6224
    e32ec03 tui: Improved behavior after resize. neovim#6202
    86c2adc edit.c: CTRL-SPC: Insert previously-inserted text. neovim#6090
    c318d8e b:changedtick now follows VimL rules neovim#6112
    34e24cb terminal: Initialize colors in reverse order neovim#6160
    e889917 undo: Don't set b_u_curhead in ex_undojoin() neovim#5869
    d25649f undo: :earlier, g-: Set b_u_seq_cur correctly. (neovim#6016)
    043d8ba 'Visual-mode put from @. register' neovim#5782
    42c922b open_buffer(): Do `BufEnter` for directories.
    50d0d89 inccommand: Preview :sub commands only after delimiter neovim#5932
    1420e10 CheckHealth improvements neovim#5519
    c8d5e92 jobstart(): Return -1 if cmd is not executable. neovim#5671

justinmk added a commit to justinmk/neovim that referenced this pull request May 1, 2017

NVIM v0.2.0
FEATURES:
    bc4a2e1 help, man.vim: "outline" (TOC) feature neovim#5169
    58422f1 'guicursor' works in the TUI (and sends info to UIs) neovim#6423
    129f107 api: nvim_get_mode() neovim#6247
    0b59f98 api/ui: externalize tabline neovim#6583
    bc6d868 'listchars': `Whitespace` highlight group neovim#6367
    6afa7d6 writefile() obeys 'fsync' option neovim#6427
    c60e409 eval.c refactor (also improves some error messages) neovim#5119
    9d200cd getcompletion("cmdline") neovim#6376
    2ea7bfc terminal: Support extra arguments in 'shell'. neovim#4504
    bf51102 DirChanged autocmd neovim#5928 neovim#6262
    1743df8 'cpoptions': "_" flag to toggle `cw` behaviour neovim#6235
    22337b1 CTRL-R omits trailing ^M when pasting to cmdline neovim#6137
    0e44916 :edit allows unescaped spaces in filename neovim#6119
    abdbfd2 eval: Add id() function and make printf("%p") useful neovim#6095
    bdfa147 findfile(), :find, gf work in :terminal. neovim#6009
    2f38ed1 providers: Disable if `g:loaded_*` exists.
    b5560a6 setpos() can set lowercase marks in other buffers neovim#5753
    7c513d6 Throttle :! output, pulse "..." message. neovim#5396
    d2e8c76 v:exiting neovim#5651

    :terminal improvements neovim#6185 neovim#6142
      - cursor keeps position after leaving insert-mode.
      - 4ceec30 Follows output only if cursor is at end of buffer.
      - e7bbd35 new option: 'scrollback'
      - fedb844 quasi-support for undo and 'modifiable'
      - b45ddf7 disables 'list' by default
      - disables 'relativenumber' by default

    :help now contains full API documentation at `:help api`.

    man.vim saw numerous improvements.

    Windows support:
      - Windows is no longer "experimental", it is fully supported.
      - Windows package includes a GUI, curl.exe and other utilities.

    "Vim 8" features: partials, lambdas.

SECURITY FIXES:
    CVE-2017-5953 CVE-2017-6349 CVE-2017-6350 neovim#6485

CHANGES:
    NVIM_TUI_ENABLE_CURSOR_SHAPE was removed. Use 'guicursor' instead.
        See https://github.com/neovim/neovim/wiki/Following-HEAD#20170402

    81525dc 'mouse=a' is no longer the default. (This will probably
                 change again after it is improved.) neovim#6022

    0c1f783 defaults: 'showcmd', 'belloff', 'ruler' neovim#6087
    eb0e94f api: {get,set}_option update local options as appropriate neovim#6405
    bdcb2a3 "Reading from stdin..." message was removed. neovim#6298

FIXES:
    12fc1de ops: fix i<c-r> with multi-byte text neovim#6524
    dd391bf Windows: system() and friends neovim#6497
    13352c0 Windows: os_get_hostname() neovim#6413
    16babc6 tui: Less-noisy mouse seqs neovim#6411
    3a9dd13 (vim bug) folding edge-cases  neovim#6207
    f6946c6 job-control: set CLOEXEC on pty processes. neovim#5986
    d1afd43 rplugin: Call s:LoadRemotePlugins() on startup.
    1215084 backtick-expansion works with `shell=fish` neovim#6224
    e32ec03 tui: Improved behavior after resize. neovim#6202
    86c2adc edit.c: CTRL-SPC: Insert previously-inserted text. neovim#6090
    c318d8e b:changedtick now follows VimL rules neovim#6112
    34e24cb terminal: Initialize colors in reverse order neovim#6160
    e889917 undo: Don't set b_u_curhead in ex_undojoin() neovim#5869
    d25649f undo: :earlier, g-: Set b_u_seq_cur correctly. (neovim#6016)
    043d8ba 'Visual-mode put from @. register' neovim#5782
    42c922b open_buffer(): Do `BufEnter` for directories.
    50d0d89 inccommand: Preview :sub commands only after delimiter neovim#5932
    1420e10 CheckHealth improvements neovim#5519
    c8d5e92 jobstart(): Return -1 if cmd is not executable. neovim#5671
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment