From c92bb703dc5dfce2a8cdc036f747dd570f5bf813 Mon Sep 17 00:00:00 2001 From: Benoit Hamon Date: Fri, 26 Jan 2018 17:36:18 +0100 Subject: [PATCH 01/58] add few bindings for vi mode --- share/functions/fish_vi_key_bindings.fish | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/share/functions/fish_vi_key_bindings.fish b/share/functions/fish_vi_key_bindings.fish index 5033b23c9..76cbfcee3 100644 --- a/share/functions/fish_vi_key_bindings.fish +++ b/share/functions/fish_vi_key_bindings.fish @@ -157,6 +157,10 @@ function fish_vi_key_bindings --description 'vi-like key bindings for fish' bind dB backward-kill-bigword bind dge backward-kill-word bind dgE backward-kill-bigword + bind df begin-selection forward-jump kill-selection end-selection + bind dt begin-selection forward-jump backward-char kill-selection end-selection + bind dF begin-selection backward-jump kill-selection end-selection + bind dT begin-selection backward-jump forward-char kill-selection end-selection bind -m insert s delete-char force-repaint bind -m insert S kill-whole-line force-repaint @@ -240,6 +244,11 @@ function fish_vi_key_bindings --description 'vi-like key bindings for fish' bind -M visual E forward-bigword bind -M visual o swap-selection-start-stop force-repaint + bind -M visual f forward-jump + bind -M visual t forward-jump backward-char + bind -M visual F backward-jump + bind -M visual T backward-jump forward-char + for key in $eol_keys bind -M visual $key end-of-line end From 5b3729842cacdf0566743e2327c99beed8f0cc6c Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sun, 28 Jan 2018 15:07:19 -0800 Subject: [PATCH 02/58] tnode_t::try_get_child() to properly implement null check. try_get_child() was taking the address of a reference; clang was thereby assuming it could not be null and so was dropping the null check. Ensure we do not dereference a null pointer. Fixes #4678 --- src/tnode.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tnode.h b/src/tnode.h index c8544c33f..c3658f606 100644 --- a/src/tnode.h +++ b/src/tnode.h @@ -143,7 +143,7 @@ class tnode_t { static_assert(child_type_possible_at_index(), "Cannot contain a child of this type"); const parse_node_t *child = nullptr; - if (nodeptr) child = &get_child_node(); + if (nodeptr) child = tree->get_child(*nodeptr, Index); if (child && child->type == ChildType::token) return {tree, child}; return {}; } From e3a2eadb49ac4ebafadebe3b9d2f15dcc998bd14 Mon Sep 17 00:00:00 2001 From: Fabian Homborg Date: Mon, 29 Jan 2018 19:30:34 +0100 Subject: [PATCH 03/58] Remove obsolete "--gui" option from gradle completions Fixes #4691. [ci skip] --- share/completions/gradle.fish | 1 - 1 file changed, 1 deletion(-) diff --git a/share/completions/gradle.fish b/share/completions/gradle.fish index e0e67480d..7adc60e02 100644 --- a/share/completions/gradle.fish +++ b/share/completions/gradle.fish @@ -13,7 +13,6 @@ complete -c gradle -l debug -s d -d 'Log in debug mode' complete -c gradle -l daemon -d 'Uses Gradle Daemon to run build' complete -c gradle -l foreground -d 'Uses Gradle Daemon in foreground' complete -c gradle -l gradle-user-home -s g -r -d 'Specify gradle user home directory' -complete -c gradle -l gui -d 'Launch Gradle GUI' complete -c gradle -l initscript -s I -r -d 'Specify an initialization script' complete -c gradle -l info -s i -d 'Set log level to info' complete -c gradle -l include-build -r -d 'Include specified build in composite' From 9ce3ac5b933d0c74b54c906c210afc7bf7baca57 Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Mon, 29 Jan 2018 11:30:11 -0800 Subject: [PATCH 04/58] Remove R_SENTINAL It was unused and misspelled. --- src/input_common.h | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/input_common.h b/src/input_common.h index e48b32d91..618b30e7c 100644 --- a/src/input_common.h +++ b/src/input_common.h @@ -73,10 +73,7 @@ enum { R_AND, R_CANCEL, R_TIMEOUT, // we didn't get interactive input within wait_on_escape_ms - R_MAX = R_CANCEL, - // This is a special psuedo-char that is not used other than to mark the end of the the special - // characters so we can sanity check the enum range. - R_SENTINAL + R_MAX = R_CANCEL }; /// Init the library. From 43c839ab0e22ebf9ed2b5cf764302b4d8b92cd0e Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Mon, 29 Jan 2018 11:52:55 -0800 Subject: [PATCH 05/58] Rename R_MIN and R_MAX to R_BEGIN/END_INPUT_FUNCTIONS This makes the names more obvious. We also make the range half-open as is the convention. --- src/input.cpp | 10 ++++++---- src/input_common.cpp | 2 +- src/input_common.h | 13 +++++++------ 3 files changed, 14 insertions(+), 11 deletions(-) diff --git a/src/input.cpp b/src/input.cpp index b759c1982..5cba29738 100644 --- a/src/input.cpp +++ b/src/input.cpp @@ -329,7 +329,8 @@ void input_function_push_args(int code) { wchar_t arg; // Skip and queue up any function codes. See issue #2357. - while ((arg = input_common_readch(0)) >= R_MIN && arg <= R_MAX) { + while ((arg = input_common_readch(0)) >= R_BEGIN_INPUT_FUNCTIONS && + arg < R_END_INPUT_FUNCTIONS) { skipped.push_back(arg); } @@ -483,7 +484,8 @@ static wchar_t input_read_characters_only() { wchar_t char_to_return; for (;;) { char_to_return = input_common_readch(0); - bool is_readline_function = (char_to_return >= R_MIN && char_to_return <= R_MAX); + bool is_readline_function = + (char_to_return >= R_BEGIN_INPUT_FUNCTIONS && char_to_return < R_END_INPUT_FUNCTIONS); // R_NULL and R_EOF are more control characters than readline functions, so check specially // for those. if (!is_readline_function || char_to_return == R_NULL || char_to_return == R_EOF) { @@ -509,7 +511,7 @@ wint_t input_readch(bool allow_commands) { while (1) { wchar_t c = input_common_readch(0); - if (c >= R_MIN && c <= R_MAX) { + if (c >= R_BEGIN_INPUT_FUNCTIONS && c < R_END_INPUT_FUNCTIONS) { switch (c) { case R_EOF: // if it's closed, then just return { @@ -525,7 +527,7 @@ wint_t input_readch(bool allow_commands) { return input_readch(); } c = input_common_readch(0); - while (c >= R_MIN && c <= R_MAX) { + while (c >= R_BEGIN_INPUT_FUNCTIONS && c < R_END_INPUT_FUNCTIONS) { c = input_common_readch(0); } input_common_next_ch(c); diff --git a/src/input_common.cpp b/src/input_common.cpp index 19ad41e2e..05041bd99 100644 --- a/src/input_common.cpp +++ b/src/input_common.cpp @@ -194,7 +194,7 @@ wchar_t input_common_readch(int timed) { while (1) { wint_t b = readb(); - if (b >= R_NULL && b <= R_MAX) return b; + if (b >= R_NULL && b < R_END_INPUT_FUNCTIONS) return b; if (MB_CUR_MAX == 1) { // return (unsigned char)b; // single-byte locale, all values are legal diff --git a/src/input_common.h b/src/input_common.h index 618b30e7c..896c820dc 100644 --- a/src/input_common.h +++ b/src/input_common.h @@ -14,11 +14,7 @@ enum { // delivered, or when an exception happened. R_NULL = R_MIN, R_EOF, - // Key codes for inputrc-style keyboard functions that are passed on to the caller of - // input_read(). - // - // NOTE: If you modify this sequence of symbols you must update the name_arr, code_arr and - // desc_arr variables in input.cpp to match! + R_BEGINNING_OF_LINE, R_END_OF_LINE, R_FORWARD_CHAR, @@ -72,8 +68,13 @@ enum { R_BACKWARD_JUMP, R_AND, R_CANCEL, + R_TIMEOUT, // we didn't get interactive input within wait_on_escape_ms - R_MAX = R_CANCEL + + // The range of key codes for inputrc-style keyboard functions that are passed on to the caller + // of input_read(). + R_BEGIN_INPUT_FUNCTIONS = R_BEGINNING_OF_LINE, + R_END_INPUT_FUNCTIONS = R_CANCEL + 1 }; /// Init the library. From d03aff87422694e101a1f42bbf2f159669e4eec9 Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Mon, 29 Jan 2018 19:15:16 -0800 Subject: [PATCH 06/58] Encapsulate input function name and code into a single struct Reduces the reliance in ugly parallel arrays. --- src/input.cpp | 199 ++++++++++++++++++++------------------------------ 1 file changed, 80 insertions(+), 119 deletions(-) diff --git a/src/input.cpp b/src/input.cpp index 5cba29738..4dd9c768f 100644 --- a/src/input.cpp +++ b/src/input.cpp @@ -58,125 +58,82 @@ struct terminfo_mapping_t { const char *seq; // character sequence generated on keypress }; -/// Names of all the input functions supported. -static const wchar_t *const name_arr[] = {L"beginning-of-line", - L"end-of-line", - L"forward-char", - L"backward-char", - L"forward-word", - L"backward-word", - L"forward-bigword", - L"backward-bigword", - L"history-search-backward", - L"history-search-forward", - L"delete-char", - L"backward-delete-char", - L"kill-line", - L"yank", - L"yank-pop", - L"complete", - L"complete-and-search", - L"beginning-of-history", - L"end-of-history", - L"backward-kill-line", - L"kill-whole-line", - L"kill-word", - L"kill-bigword", - L"backward-kill-word", - L"backward-kill-path-component", - L"backward-kill-bigword", - L"history-token-search-backward", - L"history-token-search-forward", - L"self-insert", - L"transpose-chars", - L"transpose-words", - L"upcase-word", - L"downcase-word", - L"capitalize-word", - L"vi-arg-digit", - L"vi-delete-to", - L"execute", - L"beginning-of-buffer", - L"end-of-buffer", - L"repaint", - L"force-repaint", - L"up-line", - L"down-line", - L"suppress-autosuggestion", - L"accept-autosuggestion", - L"begin-selection", - L"swap-selection-start-stop", - L"end-selection", - L"kill-selection", - L"forward-jump", - L"backward-jump", - L"and", - L"cancel"}; +static constexpr size_t input_function_count = R_END_INPUT_FUNCTIONS - R_BEGIN_INPUT_FUNCTIONS; + +/// Input function metadata. This list should be kept in sync with the key code list in +/// input_common.h. +struct input_function_metadata_t { + wchar_t code; + const wchar_t *name; +}; +static const input_function_metadata_t input_function_metadata[] = { + {R_BEGINNING_OF_LINE, L"beginning-of-line"}, + {R_END_OF_LINE, L"end-of-line"}, + {R_FORWARD_CHAR, L"forward-char"}, + {R_BACKWARD_CHAR, L"backward-char"}, + {R_FORWARD_WORD, L"forward-word"}, + {R_BACKWARD_WORD, L"backward-word"}, + {R_FORWARD_BIGWORD, L"forward-bigword"}, + {R_BACKWARD_BIGWORD, L"backward-bigword"}, + {R_HISTORY_SEARCH_BACKWARD, L"history-search-backward"}, + {R_HISTORY_SEARCH_FORWARD, L"history-search-forward"}, + {R_DELETE_CHAR, L"delete-char"}, + {R_BACKWARD_DELETE_CHAR, L"backward-delete-char"}, + {R_KILL_LINE, L"kill-line"}, + {R_YANK, L"yank"}, + {R_YANK_POP, L"yank-pop"}, + {R_COMPLETE, L"complete"}, + {R_COMPLETE_AND_SEARCH, L"complete-and-search"}, + {R_BEGINNING_OF_HISTORY, L"beginning-of-history"}, + {R_END_OF_HISTORY, L"end-of-history"}, + {R_BACKWARD_KILL_LINE, L"backward-kill-line"}, + {R_KILL_WHOLE_LINE, L"kill-whole-line"}, + {R_KILL_WORD, L"kill-word"}, + {R_KILL_BIGWORD, L"kill-bigword"}, + {R_BACKWARD_KILL_WORD, L"backward-kill-word"}, + {R_BACKWARD_KILL_PATH_COMPONENT, L"backward-kill-path-component"}, + {R_BACKWARD_KILL_BIGWORD, L"backward-kill-bigword"}, + {R_HISTORY_TOKEN_SEARCH_BACKWARD, L"history-token-search-backward"}, + {R_HISTORY_TOKEN_SEARCH_FORWARD, L"history-token-search-forward"}, + {R_SELF_INSERT, L"self-insert"}, + {R_TRANSPOSE_CHARS, L"transpose-chars"}, + {R_TRANSPOSE_WORDS, L"transpose-words"}, + {R_UPCASE_WORD, L"upcase-word"}, + {R_DOWNCASE_WORD, L"downcase-word"}, + {R_CAPITALIZE_WORD, L"capitalize-word"}, + {R_VI_ARG_DIGIT, L"vi-arg-digit"}, + {R_VI_DELETE_TO, L"vi-delete-to"}, + {R_EXECUTE, L"execute"}, + {R_BEGINNING_OF_BUFFER, L"beginning-of-buffer"}, + {R_END_OF_BUFFER, L"end-of-buffer"}, + {R_REPAINT, L"repaint"}, + {R_FORCE_REPAINT, L"force-repaint"}, + {R_UP_LINE, L"up-line"}, + {R_DOWN_LINE, L"down-line"}, + {R_SUPPRESS_AUTOSUGGESTION, L"suppress-autosuggestion"}, + {R_ACCEPT_AUTOSUGGESTION, L"accept-autosuggestion"}, + {R_BEGIN_SELECTION, L"begin-selection"}, + {R_SWAP_SELECTION_START_STOP, L"swap-selection-start-stop"}, + {R_END_SELECTION, L"end-selection"}, + {R_KILL_SELECTION, L"kill-selection"}, + {R_FORWARD_JUMP, L"forward-jump"}, + {R_BACKWARD_JUMP, L"backward-jump"}, + {R_AND, L"and"}, + {R_CANCEL, L"cancel"}}; + +static_assert(sizeof(input_function_metadata) / sizeof(input_function_metadata[0]) == + input_function_count, + "input_function_metadata size mismatch with input_common. Did you forget to update " + "input_function_metadata?"); wcstring describe_char(wint_t c) { - wint_t initial_cmd_char = R_BEGINNING_OF_LINE; - long name_count = sizeof(name_arr) / sizeof(*name_arr); - if (c >= initial_cmd_char && c < initial_cmd_char + name_count) { - return format_string(L"%02x (%ls)", c, name_arr[c - initial_cmd_char]); + if (c >= R_BEGIN_INPUT_FUNCTIONS && c < R_END_INPUT_FUNCTIONS) { + size_t idx = c - R_BEGIN_INPUT_FUNCTIONS; + return format_string(L"%02x (%ls)", c, input_function_metadata[idx].name); } return format_string(L"%02x", c); } -/// Internal code for each supported input function. -static const wchar_t code_arr[] = {R_BEGINNING_OF_LINE, - R_END_OF_LINE, - R_FORWARD_CHAR, - R_BACKWARD_CHAR, - R_FORWARD_WORD, - R_BACKWARD_WORD, - R_FORWARD_BIGWORD, - R_BACKWARD_BIGWORD, - R_HISTORY_SEARCH_BACKWARD, - R_HISTORY_SEARCH_FORWARD, - R_DELETE_CHAR, - R_BACKWARD_DELETE_CHAR, - R_KILL_LINE, - R_YANK, - R_YANK_POP, - R_COMPLETE, - R_COMPLETE_AND_SEARCH, - R_BEGINNING_OF_HISTORY, - R_END_OF_HISTORY, - R_BACKWARD_KILL_LINE, - R_KILL_WHOLE_LINE, - R_KILL_WORD, - R_KILL_BIGWORD, - R_BACKWARD_KILL_WORD, - R_BACKWARD_KILL_PATH_COMPONENT, - R_BACKWARD_KILL_BIGWORD, - R_HISTORY_TOKEN_SEARCH_BACKWARD, - R_HISTORY_TOKEN_SEARCH_FORWARD, - R_SELF_INSERT, - R_TRANSPOSE_CHARS, - R_TRANSPOSE_WORDS, - R_UPCASE_WORD, - R_DOWNCASE_WORD, - R_CAPITALIZE_WORD, - R_VI_ARG_DIGIT, - R_VI_DELETE_TO, - R_EXECUTE, - R_BEGINNING_OF_BUFFER, - R_END_OF_BUFFER, - R_REPAINT, - R_FORCE_REPAINT, - R_UP_LINE, - R_DOWN_LINE, - R_SUPPRESS_AUTOSUGGESTION, - R_ACCEPT_AUTOSUGGESTION, - R_BEGIN_SELECTION, - R_SWAP_SELECTION_START_STOP, - R_END_SELECTION, - R_KILL_SELECTION, - R_FORWARD_JUMP, - R_BACKWARD_JUMP, - R_AND, - R_CANCEL}; - /// Mappings for the current input mode. static std::vector mapping_list; @@ -818,15 +775,19 @@ wcstring_list_t input_terminfo_get_names(bool skip_null) { return result; } -wcstring_list_t input_function_get_names(void) { - size_t count = sizeof name_arr / sizeof *name_arr; - return wcstring_list_t(name_arr, name_arr + count); +wcstring_list_t input_function_get_names() { + wcstring_list_t result; + result.reserve(input_function_count); + for (const auto &md : input_function_metadata) { + result.push_back(md.name); + } + return result; } wchar_t input_function_get_code(const wcstring &name) { - for (size_t i = 0; i < sizeof code_arr / sizeof *code_arr; i++) { - if (name == name_arr[i]) { - return code_arr[i]; + for (const auto &md : input_function_metadata) { + if (name == md.name) { + return md.code; } } return INPUT_CODE_NONE; From ac8fe1e4b928b2109e67d7e134c905c126bbd4df Mon Sep 17 00:00:00 2001 From: Donovan Glover Date: Sun, 28 Jan 2018 13:09:52 -0500 Subject: [PATCH 07/58] Various fixes to CONTRIBUTING.md --- CONTRIBUTING.md | 44 ++++++++++++++++++++++---------------------- 1 file changed, 22 insertions(+), 22 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 5defc3631..d7b4241d3 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -1,6 +1,6 @@ # Guidelines For Developers -This document provides guidelines for making changes to the fish-shell project. This includes rules for how to format the code, naming conventions, etcetera. Generally known as the style of the code. It also includes recommended best practices such as creating a Travis-CI account so you can verify your changes pass all the tests before making a pull-request. +This document provides guidelines for making changes to the fish-shell project. This includes rules for how to format the code, naming conventions, etcetera. Generally known as the style of the code. It also includes recommended best practices such as creating a Travis CI account so you can verify that your changes pass all the tests before making a pull request. See the bottom of this document for help on installing the linting and style reformatting tools discussed in the following sections. @@ -145,7 +145,7 @@ However, as I write this there are no places in the code where we use this and I 1. All fish scripts, such as those in the *share/functions* and *tests* directories, should be formatted using the `fish_indent` command. -1. Function names should be all lowercase with undescores separating words. Private functions should begin with an underscore. The first word should be `fish` if the function is unique to fish. +1. Function names should be in all lowercase with words separated by underscores. Private functions should begin with an underscore. The first word should be `fish` if the function is unique to fish. 1. The first word of global variable names should generally be `fish` for public vars or `_fish` for private vars to minimize the possibility of name clashes with user defined vars. @@ -155,7 +155,7 @@ However, as I write this there are no places in the code where we use this and I 1. The `clang-format` command is authoritative with respect to indentation, whitespace around operators, etc. -1. All names in code should be `small_snake_case`. No Hungarian notation is used. Classes and structs names should be followed by `_t`. +1. All names in code should be `small_snake_case`. No Hungarian notation is used. The names for classes and structs should be followed by `_t`. 1. Always attach braces to the surrounding context. @@ -163,13 +163,13 @@ However, as I write this there are no places in the code where we use this and I 1. Comments should always use the C++ style; i.e., each line of the comment should begin with a `//` and should be limited to 100 characters. Comments that do not begin a line should be separated from the previous text by two spaces. -1. Comments that document the purpose of a function or class should begin with three slashes, `///`, so that OS X Xcode (and possibly other IDE's) will extract the comment and show it in the "Quick Help" window when the cursor is on the symbol. +1. Comments that document the purpose of a function or class should begin with three slashes, `///`, so that OS X Xcode (and possibly other IDEs) will extract the comment and show it in the "Quick Help" window when the cursor is on the symbol. ## Testing -The source code for fish includes a large collection of tests. If you are making any changes to fish, running these tests is mandatory to make sure the behaviour remains consistent and regressions are not introduced. Even if you don't run the tests they will be run via the [Travis CI](https://travis-ci.org/fish-shell/fish-shell) service. +The source code for fish includes a large collection of tests. If you are making any changes to fish, running these tests is mandatory to make sure the behaviour remains consistent and regressions are not introduced. Even if you don't run the tests on your machine, they will still be run via the [Travis CI](https://travis-ci.org/fish-shell/fish-shell) service. -You are strongly encouraged to add tests when changing the functionality of fish. Especially if you are fixing a bug to help ensure there are no regressions in the future (i.e., we don't reintroduce the bug). +You are strongly encouraged to add tests when changing the functionality of fish, especially if you are fixing a bug to help ensure there are no regressions in the future (i.e., we don't reintroduce the bug). ### Local testing @@ -183,12 +183,12 @@ Running the tests is only supported from the autotools build and not xcodebuild. ### Travis CI Build and Test -The Travis Continuous Integration services can be used to test your changes using multiple configurations. This is the same service that the fish shell project uses to ensure new changes haven't broken anything. Thus it is a really good idea that you leverage Travis CI before making a pull-request to avoid embarrasment at breaking the build. +The Travis Continuous Integration services can be used to test your changes using multiple configurations. This is the same service that the fish shell project uses to ensure new changes haven't broken anything. Thus it is a really good idea that you leverage Travis CI before making a pull request to avoid potential embarrassment at breaking the build. -You will need to [fork the fish-shell repository on GitHub](https://help.github.com/articles/fork-a-repo/). Then setup Travis to test your changes before you make a pull-request: +You will need to [fork the fish-shell repository on GitHub](https://help.github.com/articles/fork-a-repo/), then setup Travis to test your changes before making a pull request. 1. [Sign in to Travis CI](https://travis-ci.org/auth) with your GitHub account, accepting the GitHub access permissions confirmation. -1. Once you're signed in, and your repositories are synchronised, go to your [profile page](https://travis-ci.org/profile) and enable the fish-shell repository. +1. Once you're signed in and your repositories are synchronised, go to your [profile page](https://travis-ci.org/profile) and enable the fish-shell repository. 1. Push your changes to GitHub. You'll receive an email when the tests are complete telling you whether or not any tests failed. @@ -228,9 +228,9 @@ fi exit 0 ``` -This will check if the push is to the master branch and, if it is, will run `make test` and only allow the push if that succeeds. In some circumstances it might be advisable to circumvent it with `git push --no-verify`, but usually that should not be necessary. +This will check if the push is to the master branch and, if it is, only allow the push if running `make test` succeeds. In some circumstances it may be advisable to circumvent this check with `git push --no-verify`, but usually that isn't necessary. -To install the hook, put it in .git/hooks/pre-push and make it executable. +To install the hook, place the code in a new file `.git/hooks/pre-push` and make it executable. ### Coverity Scan @@ -240,7 +240,7 @@ We use Coverity's static analysis tool which offers free access to open source p ### Installing the Linting Tools -To install the lint checkers on Mac OS X using HomeBrew: +To install the lint checkers on Mac OS X using Homebrew: ``` brew tap oclint/formulae @@ -248,7 +248,7 @@ brew install oclint brew install cppcheck ``` -To install the lint checkers on Linux distros that use Apt: +To install the lint checkers on Debian-based Linux distributions: ``` sudo apt-get install clang @@ -258,19 +258,19 @@ sudo apt-get install cppcheck ### Installing the Reformatting Tools -To install the reformatting tool on Mac OS X using HomeBrew: +Mac OS X: ``` brew install clang-format ``` -To install the reformatting tool on Linux distros that use Apt: +Debian-based: ``` apt-cache install clang-format ``` -That will list the versions available. Pick the newest one available (3.9 for Ubuntu 16.10 as I write this) and install it: +Above will list all the versions available. Pick the newest one available (3.9 for Ubuntu 16.10 as I write this) and install it: ``` sudo apt-get install clang-format-3.9 @@ -279,24 +279,24 @@ sudo ln -s /usr/bin/clang-format-3.9 /usr/bin/clang-format ## Message Translations -Fish uses the GNU gettext library to translate messages from english to other languages. To create or update a translation run `make po/[LANGUAGE CODE].po`. Where `LANGUAGE CODE` is the two letter ISO 639-1 language code of the language you are translating to. For example, `de` for German. You'll need to have the `xgettext`, `msgfmt` and `msgmerge` commands installed to do this. +Fish uses the GNU gettext library to translate messages from English to other languages. To create or update a translation run `make po/[LANGUAGE CODE].po` where `LANGUAGE CODE` is the two letter ISO 639-1 language code of the language you are translating to (e.g. `de` for German). Make sure that you have the `xgettext`, `msgfmt` and `msgmerge` commands installed in order to do this. -All messages in fish script must be enclosed in single or double quote characters. They must also be translated via a subcommand. This means that the following are not valid: +All messages in fish script must be enclosed in single or double quote characters. They must also be translated via a subcommand. This means that the following are **not** valid: ``` echo (_ hello) _ "goodbye" ``` -Those should be written like this: +Above should be written like this instead: ``` echo (_ "hello") echo (_ "goodbye") ``` -Note that you can use either single or double quotes to enclose the message to be translated. You can also optionally include spaces after the opening parentheses and before the closing paren. +Note that you can use either single or double quotes to enclose the message to be translated. You can also optionally include spaces after the opening parentheses and once again before the closing parentheses. -Be cautious about blindly updating an existing translation file. Trivial changes to an existing message (e.g., changing the punctuation) will cause existing translations to be removed. That is because the tools do literal string matching. Which means that in general you need to carefully review any recommended deletions. +Be cautious about blindly updating an existing translation file. Trivial changes to an existing message (e.g., changing the punctuation) will cause existing translations to be removed, since the tools do literal string matching. Therefore, in general, you need to carefully review any recommended deletions. -See the [wiki](https://github.com/fish-shell/fish-shell/wiki/Translations) for more details. +Read the [translations wiki](https://github.com/fish-shell/fish-shell/wiki/Translations) for more information. From f135c53196bdca84281e358c93e4749486b37337 Mon Sep 17 00:00:00 2001 From: David Adam Date: Tue, 30 Jan 2018 21:58:13 +0800 Subject: [PATCH 08/58] [cmake] move all CheckFunctionExists to CheckCXXSymbolExists CheckFunctionExists checks for C linkage only, and recommends the use of CheckSymbolExists in the documentation. This improves the detection of C++ features, as opposed to C features. --- cmake/ConfigureChecks.cmake | 42 ++++++++++++++++++------------------- 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/cmake/ConfigureChecks.cmake b/cmake/ConfigureChecks.cmake index 4eb8e64fc..6e1168e73 100644 --- a/cmake/ConfigureChecks.cmake +++ b/cmake/ConfigureChecks.cmake @@ -17,38 +17,38 @@ ENDIF() # Set up the config.h file. SET(PACKAGE_NAME "fish") SET(PACKAGE_TARNAME "fish") -INCLUDE(CheckFunctionExists) INCLUDE(CheckCXXSymbolExists) INCLUDE(CheckIncludeFileCXX) INCLUDE(CheckIncludeFiles) INCLUDE(CheckStructHasMember) INCLUDE(CheckCXXSourceCompiles) INCLUDE(CheckTypeSize) -CHECK_FUNCTION_EXISTS(backtrace_symbols HAVE_BACKTRACE_SYMBOLS) -CHECK_FUNCTION_EXISTS(clock_gettime HAVE_CLOCK_GETTIME) -CHECK_FUNCTION_EXISTS(ctermid_r HAVE_CTERMID_R) +CHECK_CXX_SYMBOL_EXISTS(backtrace_symbols execinfo.h HAVE_BACKTRACE_SYMBOLS) +CHECK_CXX_SYMBOL_EXISTS(clock_gettime time.h HAVE_CLOCK_GETTIME) +CHECK_CXX_SYMBOL_EXISTS(ctermid_r stdio.h HAVE_CTERMID_R) CHECK_STRUCT_HAS_MEMBER("struct dirent" d_type dirent.h HAVE_STRUCT_DIRENT_D_TYPE LANGUAGE CXX) -CHECK_FUNCTION_EXISTS(dirfd HAVE_DIRFD) +CHECK_CXX_SYMBOL_EXISTS(dirfd "sys/types.h;dirent.h" HAVE_DIRFD) CHECK_INCLUDE_FILE_CXX(execinfo.h HAVE_EXECINFO_H) -CHECK_FUNCTION_EXISTS(flock HAVE_FLOCK) +CHECK_CXX_SYMBOL_EXISTS(flock sys/file.h HAVE_FLOCK) # futimens is new in OS X 10.13 but is a weak symbol. # Don't assume it exists just because we can link - it may be null. CHECK_CXX_SYMBOL_EXISTS(futimens sys/stat.h HAVE_FUTIMENS) -CHECK_FUNCTION_EXISTS(futimes HAVE_FUTIMES) -CHECK_FUNCTION_EXISTS(getifaddrs HAVE_GETIFADDRS) -CHECK_FUNCTION_EXISTS(getpwent HAVE_GETPWENT) -CHECK_FUNCTION_EXISTS(gettext HAVE_GETTEXT) -CHECK_FUNCTION_EXISTS(killpg HAVE_KILLPG) -CHECK_FUNCTION_EXISTS(lrand48_r HAVE_LRAND48_R) -CHECK_FUNCTION_EXISTS(mkostemp HAVE_MKOSTEMP) +CHECK_CXX_SYMBOL_EXISTS(futimes sys/time.h HAVE_FUTIMES) +CHECK_CXX_SYMBOL_EXISTS(getifaddrs ifaddrs.h HAVE_GETIFADDRS) +CHECK_CXX_SYMBOL_EXISTS(getpwent pwd.h HAVE_GETPWENT) +CHECK_CXX_SYMBOL_EXISTS(gettext libintl.h HAVE_GETTEXT) +CHECK_CXX_SYMBOL_EXISTS(killpg "sys/types.h;signal.h" HAVE_KILLPG) +CHECK_CXX_SYMBOL_EXISTS(lrand48_r stdlib.h HAVE_LRAND48_R) +# mkostemp is in stdlib in glibc and FreeBSD, but unistd on macOS +CHECK_CXX_SYMBOL_EXISTS(mkostemp "stdlib.h;unistd.h" HAVE_MKOSTEMP) SET(HAVE_NCURSES_CURSES_H ${CURSES_HAVE_NCURSES_CURSES_H}) SET(HAVE_NCURSES_H ${CURSES_HAVE_NCURSES_H}) CHECK_INCLUDE_FILE_CXX("ncurses/term.h" HAVE_NCURSES_TERM_H) CHECK_INCLUDE_FILE_CXX(siginfo.h HAVE_SIGINFO_H) CHECK_INCLUDE_FILE_CXX(spawn.h HAVE_SPAWN_H) -CHECK_FUNCTION_EXISTS(std::wcscasecmp HAVE_STD__WCSCASECMP) -CHECK_FUNCTION_EXISTS(std::wcsdup HAVE_STD__WCSDUP) -CHECK_FUNCTION_EXISTS(std::wcsncasecmp HAVE_STD__WCSNCASECMP) +CHECK_CXX_SYMBOL_EXISTS(std::wcscasecmp wchar.h HAVE_STD__WCSCASECMP) +CHECK_CXX_SYMBOL_EXISTS(std::wcsdup wchar.h HAVE_STD__WCSDUP) +CHECK_CXX_SYMBOL_EXISTS(std::wcsncasecmp wchar.h HAVE_STD__WCSNCASECMP) CHECK_STRUCT_HAS_MEMBER("struct stat" st_ctime_nsec "sys/stat.h" HAVE_STRUCT_STAT_ST_CTIME_NSEC LANGUAGE CXX) CHECK_STRUCT_HAS_MEMBER("struct stat" st_mtimespec.tv_nsec "sys/stat.h" @@ -61,11 +61,11 @@ CHECK_INCLUDE_FILE_CXX(sys/select.h HAVE_SYS_SELECT_H) CHECK_INCLUDE_FILES("sys/types.h;sys/sysctl.h" HAVE_SYS_SYSCTL_H) CHECK_INCLUDE_FILE_CXX(termios.h HAVE_TERMIOS_H) # Needed for TIOCGWINSZ CHECK_INCLUDE_FILE_CXX(term.h HAVE_TERM_H) -CHECK_FUNCTION_EXISTS(wcscasecmp HAVE_WCSCASECMP) -CHECK_FUNCTION_EXISTS(wcsdup HAVE_WCSDUP) -CHECK_FUNCTION_EXISTS(wcslcpy HAVE_WCSLCPY) -CHECK_FUNCTION_EXISTS(wcsncasecmp HAVE_WCSNCASECMP) -CHECK_FUNCTION_EXISTS(wcsndup HAVE_WCSNDUP) +CHECK_CXX_SYMBOL_EXISTS(wcscasecmp wchar.h HAVE_WCSCASECMP) +CHECK_CXX_SYMBOL_EXISTS(wcsdup wchar.h HAVE_WCSDUP) +CHECK_CXX_SYMBOL_EXISTS(wcslcpy wchar.h HAVE_WCSLCPY) +CHECK_CXX_SYMBOL_EXISTS(wcsncasecmp wchar.h HAVE_WCSNCASECMP) +CHECK_CXX_SYMBOL_EXISTS(wcsndup wchar.h HAVE_WCSNDUP) CHECK_CXX_SYMBOL_EXISTS(_sys_errs stdlib.h HAVE__SYS__ERRS) From d0d7bb75cdcb2aae692c719de41ee7f5d4736c77 Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Tue, 30 Jan 2018 09:39:04 -0800 Subject: [PATCH 09/58] Add new pager-toggle-search input function This adds a new input binding pager-toggle-search which toggles the search field on and off when the pager is showing. --- CHANGELOG.md | 1 + doc_src/bind.txt | 2 ++ src/input.cpp | 1 + src/input_common.h | 1 + src/reader.cpp | 9 +++++++++ 5 files changed, 14 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5f074b791..652b84076 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,6 +33,7 @@ This section is for changes merged to the `major` branch that are not also merge - `funced` now has a `-s` and `--save` option to automatically save the edited function after successfully editing (#4668). - Arguments to `end` are now errors, instead of being silently ignored. - Pager navigation has been improved. Most notably, moving down now wraps around, moving up from the commandline now jumps to the last element and moving right and left now reverse each other even when wrapping around (#4680). +- A new input binding `pager-toggle-search` toggles the search field in the completions pager on and off. ## Other significant changes - Command substitution output is now limited to 10 MB by default (#3822). diff --git a/doc_src/bind.txt b/doc_src/bind.txt index c3ec3e367..94ee19b6d 100644 --- a/doc_src/bind.txt +++ b/doc_src/bind.txt @@ -120,6 +120,8 @@ The following special input functions are available: - `kill-word`, move the next word to the killring +- `pager-toggle-search`, toggles the search field if the completions pager is visible. + - `suppress-autosuggestion`, remove the current autosuggestion - `swap-selection-start-stop`, go to the other end of the highlighted text without changing the selection diff --git a/src/input.cpp b/src/input.cpp index 4dd9c768f..9c2464509 100644 --- a/src/input.cpp +++ b/src/input.cpp @@ -84,6 +84,7 @@ static const input_function_metadata_t input_function_metadata[] = { {R_YANK_POP, L"yank-pop"}, {R_COMPLETE, L"complete"}, {R_COMPLETE_AND_SEARCH, L"complete-and-search"}, + {R_PAGER_TOGGLE_SEARCH, L"pager-toggle-search"}, {R_BEGINNING_OF_HISTORY, L"beginning-of-history"}, {R_END_OF_HISTORY, L"end-of-history"}, {R_BACKWARD_KILL_LINE, L"backward-kill-line"}, diff --git a/src/input_common.h b/src/input_common.h index 896c820dc..617ff4157 100644 --- a/src/input_common.h +++ b/src/input_common.h @@ -32,6 +32,7 @@ enum { R_YANK_POP, R_COMPLETE, R_COMPLETE_AND_SEARCH, + R_PAGER_TOGGLE_SEARCH, R_BEGINNING_OF_HISTORY, R_END_OF_HISTORY, R_BACKWARD_KILL_LINE, diff --git a/src/reader.cpp b/src/reader.cpp index 21af60de7..a8b5282f3 100644 --- a/src/reader.cpp +++ b/src/reader.cpp @@ -2591,6 +2591,15 @@ const wchar_t *reader_readline(int nchars) { } break; } + case R_PAGER_TOGGLE_SEARCH: { + if (data->is_navigating_pager_contents()) { + bool sfs = data->pager.is_search_field_shown(); + data->pager.set_search_field_shown(!sfs); + data->pager.set_fully_disclosed(true); + reader_repaint_needed(); + } + break; + } case R_KILL_LINE: { editable_line_t *el = data->active_edit_line(); const wchar_t *buff = el->text.c_str(); From c4a12f90c16eb3754a42315348013c82088f32fe Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Tue, 30 Jan 2018 09:40:17 -0800 Subject: [PATCH 10/58] Bind pager-toggle-search to control-s by default. --- CHANGELOG.md | 2 +- share/functions/__fish_shared_key_bindings.fish | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 652b84076..c13ba458a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,7 +33,7 @@ This section is for changes merged to the `major` branch that are not also merge - `funced` now has a `-s` and `--save` option to automatically save the edited function after successfully editing (#4668). - Arguments to `end` are now errors, instead of being silently ignored. - Pager navigation has been improved. Most notably, moving down now wraps around, moving up from the commandline now jumps to the last element and moving right and left now reverse each other even when wrapping around (#4680). -- A new input binding `pager-toggle-search` toggles the search field in the completions pager on and off. +- A new input binding `pager-toggle-search` toggles the search field in the completions pager on and off. By default this is bound to control-s. ## Other significant changes - Command substitution output is now limited to 10 MB by default (#3822). diff --git a/share/functions/__fish_shared_key_bindings.fish b/share/functions/__fish_shared_key_bindings.fish index df662f559..6b46be177 100644 --- a/share/functions/__fish_shared_key_bindings.fish +++ b/share/functions/__fish_shared_key_bindings.fish @@ -31,6 +31,7 @@ function __fish_shared_key_bindings -d "Bindings shared between emacs and vi mod bind $argv \e cancel bind $argv \t complete + bind $argv \cs pager-toggle-search # shift-tab does a tab complete followed by a search. bind $argv --key btab complete-and-search From 5c2e6734c1e547da172b3fe0615e070ba84451e3 Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Tue, 30 Jan 2018 09:49:46 -0800 Subject: [PATCH 11/58] Normal text input to disable paging instead of search Prior to this fix, if the user typed normal characters while the completion pager was shown, it would begin searching. This feature was not well liked, so we are going to instead just append the characters as normal and disable paging. Control-S can be used to toggle the search field. Fixes #2249 --- CHANGELOG.md | 1 + src/reader.cpp | 7 +------ 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c13ba458a..06cd51b3a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,6 +33,7 @@ This section is for changes merged to the `major` branch that are not also merge - `funced` now has a `-s` and `--save` option to automatically save the edited function after successfully editing (#4668). - Arguments to `end` are now errors, instead of being silently ignored. - Pager navigation has been improved. Most notably, moving down now wraps around, moving up from the commandline now jumps to the last element and moving right and left now reverse each other even when wrapping around (#4680). +- Typing normal characters while the completion pager is active no longer shows the search field. Instead it enters them into the command line, and ends paging (#2249). - A new input binding `pager-toggle-search` toggles the search field in the completions pager on and off. By default this is bound to control-s. ## Other significant changes diff --git a/src/reader.cpp b/src/reader.cpp index a8b5282f3..7ac94394c 100644 --- a/src/reader.cpp +++ b/src/reader.cpp @@ -3195,15 +3195,10 @@ const wchar_t *reader_readline(int nchars) { // Other, if a normal character, we add it to the command. if (!fish_reserved_codepoint(c) && (c >= L' ' || c == L'\n' || c == L'\r') && c != 0x7F) { - bool allow_expand_abbreviations = false; - if (data->is_navigating_pager_contents()) { - data->pager.set_search_field_shown(true); - } else { - allow_expand_abbreviations = true; - } // Regular character. editable_line_t *el = data->active_edit_line(); + bool allow_expand_abbreviations = (el == &data->command_line); insert_char(data->active_edit_line(), c, allow_expand_abbreviations); // End paging upon inserting into the normal command line. From f02526919580f972d901761c5c6e65fed0d1b8fb Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Tue, 30 Jan 2018 12:36:50 -0800 Subject: [PATCH 12/58] env_var_t to forget its name Store properties associated with the name via flags instead --- src/builtin_cd.cpp | 2 +- src/builtin_set.cpp | 3 +-- src/env.cpp | 34 ++++++++++++++++++++-------------- src/env.h | 36 ++++++++++++++++++++++++++---------- src/env_universal_common.cpp | 18 +++++++++--------- src/fish_tests.cpp | 2 +- src/highlight.cpp | 8 +++----- 7 files changed, 61 insertions(+), 42 deletions(-) diff --git a/src/builtin_cd.cpp b/src/builtin_cd.cpp index 542019a5d..b86981e5c 100644 --- a/src/builtin_cd.cpp +++ b/src/builtin_cd.cpp @@ -37,7 +37,7 @@ int builtin_cd(parser_t &parser, io_streams_t &streams, wchar_t **argv) { wcstring dir; if (argv[optind]) { - dir_in = env_var_t(L"", argv[optind]); // unamed var + dir_in = env_var_t(wcstring(argv[optind]), 0); // unamed var } else { auto maybe_dir_in = env_get(L"HOME"); if (maybe_dir_in.missing_or_empty()) { diff --git a/src/builtin_set.cpp b/src/builtin_set.cpp index fd2b2beb7..ac064aaea 100644 --- a/src/builtin_set.cpp +++ b/src/builtin_set.cpp @@ -532,8 +532,7 @@ static void show_scope(const wchar_t *var_name, int scope, io_streams_t &streams return; } - - const wchar_t *exportv = var->exportv ? _(L"exported") : _(L"unexported"); + const wchar_t *exportv = var->exports() ? _(L"exported") : _(L"unexported"); wcstring_list_t vals = var->as_list(); streams.out.append_format(_(L"$%ls: set in %ls scope, %ls, with %d elements\n"), var_name, scope_name, exportv, vals.size()); diff --git a/src/env.cpp b/src/env.cpp index 8ef67dc63..03bca8782 100644 --- a/src/env.cpp +++ b/src/env.cpp @@ -218,7 +218,7 @@ void var_stack_t::push(bool new_scope) { // "--no-scope-shadowing". if (new_scope && top_node != this->global_env) { for (const auto &var : top_node->env) { - if (var.second.exportv) node->env.insert(var); + if (var.second.exports()) node->env.insert(var); } } @@ -265,7 +265,7 @@ void var_stack_t::pop() { for (const auto &entry_pair : old_top->env) { const env_var_t &var = entry_pair.second; - if (var.exportv) { + if (var.exports()) { this->mark_changed_exported(); break; } @@ -323,8 +323,6 @@ static bool is_read_only(const wcstring &key) { return env_read_only.find(key) != env_read_only.end(); } -bool env_var_t::read_only() const { return is_read_only(name); } - /// Table of variables whose value is dynamically calculated, such as umask, status, etc. static const_string_set_t env_electric; @@ -1090,7 +1088,7 @@ static int env_set_internal(const wcstring &key, env_mode_flags_t var_mode, wcst var_table_t::const_iterator result = preexisting_node->env.find(key); assert(result != preexisting_node->env.end()); const env_var_t &var = result->second; - if (var.exportv) { + if (var.exports()) { preexisting_entry_exportv = true; has_changed_new = true; } @@ -1137,7 +1135,7 @@ static int env_set_internal(const wcstring &key, env_mode_flags_t var_mode, wcst // Set the entry in the node. Note that operator[] accesses the existing entry, or // creates a new one. env_var_t &var = node->env[key]; - if (var.exportv) { + if (var.exports()) { // This variable already existed, and was exported. has_changed_new = true; } @@ -1146,11 +1144,11 @@ static int env_set_internal(const wcstring &key, env_mode_flags_t var_mode, wcst if (var_mode & ENV_EXPORT) { // The new variable is exported. - var.exportv = true; + var.set_exports(true); node->exportv = true; has_changed_new = true; } else { - var.exportv = false; + var.set_exports(false); // Set the node's exported when it changes something about exports // (also when it redefines a variable to not be exported). node->exportv = has_changed_old != has_changed_new; @@ -1201,7 +1199,7 @@ static bool try_remove(env_node_t *n, const wchar_t *key, int var_mode) { var_table_t::iterator result = n->env.find(key); if (result != n->env.end()) { - if (result->second.exportv) { + if (result->second.exports()) { vars_stack().mark_changed_exported(); } n->env.erase(result); @@ -1272,7 +1270,7 @@ const wcstring_list_t &env_var_t::as_list() const { return vals; } wcstring env_var_t::as_string(void) const { if (this->vals.empty()) return wcstring(ENV_NULL); - wchar_t sep = variable_is_colon_delimited_var(this->name) ? L':' : ARRAY_SEP; + wchar_t sep = (flags & flag_colon_delimit) ? L':' : ARRAY_SEP; auto it = this->vals.cbegin(); wcstring result(*it); while (++it != vals.end()) { @@ -1286,6 +1284,14 @@ void env_var_t::to_list(wcstring_list_t &out) const { out = vals; } +env_var_t::env_var_flags_t env_var_t::flags_for(const wchar_t *name) { + env_var_flags_t result = 0; + wcstring tmp(name); // todo: eliminate + if (is_read_only(tmp)) result |= flag_read_only; + if (variable_is_colon_delimited_var(tmp)) result |= flag_colon_delimit; + return result; +} + maybe_t env_get(const wcstring &key, env_mode_flags_t mode) { const bool has_scope = mode & (ENV_LOCAL | ENV_GLOBAL | ENV_UNIVERSAL); const bool search_local = !has_scope || (mode & ENV_LOCAL); @@ -1334,7 +1340,7 @@ maybe_t env_get(const wcstring &key, env_mode_flags_t mode) { var_table_t::const_iterator result = env->env.find(key); if (result != env->env.end()) { const env_var_t &var = result->second; - if (var.exportv ? search_exported : search_unexported) { + if (var.exports() ? search_exported : search_unexported) { return var; } } @@ -1387,7 +1393,7 @@ static void add_key_to_string_set(const var_table_t &envs, std::set *s for (iter = envs.begin(); iter != envs.end(); ++iter) { const env_var_t &var = iter->second; - if ((var.exportv && show_exported) || (!var.exportv && show_unexported)) { + if ((var.exports() && show_exported) || (!var.exports() && show_unexported)) { // Insert this key. str_set->insert(iter->first); } @@ -1454,7 +1460,7 @@ static void get_exported(const env_node_t *n, var_table_t &h) { const wcstring &key = iter->first; const env_var_t var = iter->second; - if (var.exportv) { + if (var.exports()) { // Export the variable. Don't use std::map::insert here, since we need to overwrite // existing values from previous scopes. h[key] = var; @@ -1570,7 +1576,7 @@ maybe_t env_vars_snapshot_t::get(const wcstring &key) const { } auto iter = vars.find(key); if (iter == vars.end()) return none(); - return env_var_t(iter->second); + return iter->second; } const wchar_t *const env_vars_snapshot_t::highlighting_keys[] = {L"PATH", L"CDPATH", diff --git a/src/env.h b/src/env.h index 88e8949f7..543b4dd11 100644 --- a/src/env.h +++ b/src/env.h @@ -70,28 +70,34 @@ void misc_init(); class env_var_t { private: - wcstring name; // name of the var + using env_var_flags_t = uint8_t; wcstring_list_t vals; // list of values assigned to the var + env_var_flags_t flags; public: - bool exportv = false; // whether the variable should be exported + enum { + flag_export = 1 << 0, // whether the variable is exported + flag_colon_delimit = 1 << 1, // whether the variable is colon delimited + flag_read_only = 1 << 2 // whether the variable is read only + }; // Constructors. env_var_t(const env_var_t &) = default; env_var_t(env_var_t &&) = default; - env_var_t(wcstring name, wcstring_list_t vals) : name(std::move(name)), vals(std::move(vals)) {} + env_var_t(wcstring_list_t vals, env_var_flags_t flags) : vals(std::move(vals)), flags(flags) {} + env_var_t(wcstring val, env_var_flags_t flags) + : env_var_t(wcstring_list_t{std::move(val)}, flags) {} - env_var_t(wcstring name, wcstring val) - : name(std::move(name)), - vals({ - std::move(val), - }), - exportv(false) {} + // Constructors that infer the flags from a name. + env_var_t(const wchar_t *name, wcstring_list_t vals) + : env_var_t(std::move(vals), flags_for(name)) {} + env_var_t(const wchar_t *name, wcstring val) : env_var_t(std::move(val), flags_for(name)) {} env_var_t() = default; bool empty() const { return vals.empty() || (vals.size() == 1 && vals[0].empty()); }; - bool read_only() const; + bool read_only() const { return flags & flag_read_only; } + bool exports() const { return flags & flag_export; } wcstring as_string() const; void to_list(wcstring_list_t &out) const; @@ -99,6 +105,16 @@ class env_var_t { void set_vals(wcstring_list_t v) { vals = std::move(v); } + void set_exports(bool exportv) { + if (exportv) { + flags |= flag_export; + } else { + flags &= ~flag_export; + } + } + + static env_var_flags_t flags_for(const wchar_t *name); + env_var_t &operator=(const env_var_t &var) = default; env_var_t &operator=(env_var_t &&) = default; diff --git a/src/env_universal_common.cpp b/src/env_universal_common.cpp index 42703146c..1bce23c75 100644 --- a/src/env_universal_common.cpp +++ b/src/env_universal_common.cpp @@ -268,7 +268,7 @@ bool env_universal_t::get_export(const wcstring &name) const { bool result = false; var_table_t::const_iterator where = vars.find(name); if (where != vars.end()) { - result = where->second.exportv; + result = where->second.exports(); } return result; } @@ -282,9 +282,9 @@ void env_universal_t::set_internal(const wcstring &key, wcstring_list_t vals, bo } env_var_t &entry = vars[key]; - if (entry.exportv != exportv || entry.as_list() != vals) { + if (entry.exports() != exportv || entry.as_list() != vals) { entry.set_vals(std::move(vals)); - entry.exportv = exportv; + entry.set_exports(exportv); // If we are overwriting, then this is now modified. if (overwrite) { @@ -319,7 +319,7 @@ wcstring_list_t env_universal_t::get_names(bool show_exported, bool show_unexpor for (iter = vars.begin(); iter != vars.end(); ++iter) { const wcstring &key = iter->first; const env_var_t &var = iter->second; - if ((var.exportv && show_exported) || (!var.exportv && show_unexported)) { + if ((var.exports() && show_exported) || (!var.exports() && show_unexported)) { result.push_back(key); } } @@ -357,11 +357,11 @@ void env_universal_t::generate_callbacks(const var_table_t &new_vars, // See if the value has changed. const env_var_t &new_entry = iter->second; var_table_t::const_iterator existing = this->vars.find(key); - if (existing == this->vars.end() || existing->second.exportv != new_entry.exportv || + if (existing == this->vars.end() || existing->second.exports() != new_entry.exports() || existing->second != new_entry) { // Value has changed. - callbacks.push_back( - callback_data_t(new_entry.exportv ? SET_EXPORT : SET, key, new_entry.as_string())); + callbacks.push_back(callback_data_t(new_entry.exports() ? SET_EXPORT : SET, key, + new_entry.as_string())); } } } @@ -448,7 +448,7 @@ bool env_universal_t::write_to_fd(int fd, const wcstring &path) { // variable; soldier on. const wcstring &key = iter->first; const env_var_t &var = iter->second; - append_file_entry(var.exportv ? SET_EXPORT : SET, key, var.as_string(), &contents, + append_file_entry(var.exports() ? SET_EXPORT : SET, key, var.as_string(), &contents, &storage); // Go to next. @@ -830,7 +830,7 @@ void env_universal_t::parse_message_internal(const wcstring &msgstr, var_table_t wcstring val; if (unescape_string(tmp + 1, &val, 0)) { env_var_t &entry = (*vars)[key]; - entry.exportv = exportv; + entry.set_exports(exportv); entry.set_vals(decode_serialized(val)); } } else { diff --git a/src/fish_tests.cpp b/src/fish_tests.cpp index ceec2ef2b..6d86c07d6 100644 --- a/src/fish_tests.cpp +++ b/src/fish_tests.cpp @@ -2634,7 +2634,7 @@ static void test_universal() { if (j == 0) { expected_val = none(); } else { - expected_val = env_var_t(L"", format_string(L"val_%d_%d", i, j)); + expected_val = env_var_t(format_string(L"val_%d_%d", i, j), 0); } const maybe_t var = uvars.get(key); if (j == 0) assert(!expected_val); diff --git a/src/highlight.cpp b/src/highlight.cpp index e419230ad..fa10aa516 100644 --- a/src/highlight.cpp +++ b/src/highlight.cpp @@ -224,11 +224,9 @@ static bool is_potential_cd_path(const wcstring &path, const wcstring &working_d } else { // Get the CDPATH. auto cdpath = env_get(L"CDPATH"); - if (cdpath.missing_or_empty()) cdpath = env_var_t(L"CDPATH", L"."); + std::vector pathsv = + cdpath.missing_or_empty() ? wcstring_list_t{L"."} : cdpath->as_list(); - // Tokenize it into directories. - std::vector pathsv; - cdpath->to_list(pathsv); for (auto next_path : pathsv) { if (next_path.empty()) next_path = L"."; // Ensure that we use the working directory for relative cdpaths like ".". @@ -355,7 +353,7 @@ bool autosuggest_validate_from_history(const history_item_t &item, string_prefixes_string(cd_dir, L"--help") || string_prefixes_string(cd_dir, L"-h"); if (!is_help) { wcstring path; - env_var_t dir_var(L"n/a", cd_dir); + env_var_t dir_var(cd_dir, 0); bool can_cd = path_get_cdpath(dir_var, &path, working_directory.c_str(), vars); if (can_cd && !paths_are_same_file(working_directory, path)) { suggestionOK = true; From 39a02f8ead85eb0323772197a46c1fe14aeef174 Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Tue, 30 Jan 2018 13:22:36 -0800 Subject: [PATCH 13/58] Turn the set of read-only variables into a const array --- src/env.cpp | 39 ++++++++++++++++++++++++--------------- 1 file changed, 24 insertions(+), 15 deletions(-) diff --git a/src/env.cpp b/src/env.cpp index 03bca8782..61f57f0e4 100644 --- a/src/env.cpp +++ b/src/env.cpp @@ -30,6 +30,7 @@ #endif #include +#include #include #include #include @@ -316,13 +317,26 @@ static env_universal_t *uvars() { return s_universal_variables; } // Comparer for const string set. typedef std::unordered_set const_string_set_t; -/// Table of variables that may not be set using the set command. -static const_string_set_t env_read_only; +// A typedef for a set of constant strings. Note our sets are typically on the order of 6 elements, +// so we don't bother to sort them. +using string_set_t = const wchar_t *const[]; -static bool is_read_only(const wcstring &key) { - return env_read_only.find(key) != env_read_only.end(); +template +bool string_set_contains(const T &set, const wchar_t *val) { + for (const wchar_t *entry : set) { + if (!wcscmp(val, entry)) return true; + } + return false; } +/// Check if a variable may not be set using the set command. +static bool is_read_only(const wchar_t *val) { + const string_set_t env_read_only = {L"PWD", L"SHLVL", L"_", L"history", L"status", L"version"}; + return string_set_contains(env_read_only, val); +} + +static bool is_read_only(const wcstring &val) { return is_read_only(val.c_str()); } + /// Table of variables whose value is dynamically calculated, such as umask, status, etc. static const_string_set_t env_electric; @@ -845,9 +859,6 @@ static void setup_var_dispatch_table() { void env_init(const struct config_paths_t *paths /* or NULL */) { setup_var_dispatch_table(); - // These variables can not be altered directly by the user. - env_read_only.insert({L"status", L"history", L"_", L"PWD", L"version"}); - // Names of all dynamically calculated variables. env_electric.insert({L"history", L"status", L"umask"}); @@ -909,19 +920,18 @@ void env_init(const struct config_paths_t *paths /* or NULL */) { wcstring version = str2wcstring(get_fish_version()); env_set_one(L"version", ENV_GLOBAL, version); - // Set up SHLVL variable. - const auto shlvl_var = env_get(L"SHLVL"); + // Set up SHLVL variable. Not we can't use env_get because SHLVL is read-only, and therefore was + // not inherited from the environment. wcstring nshlvl_str = L"1"; - if (!shlvl_var.missing_or_empty()) { + if (const char *shlvl_var = getenv("SHLVL")) { const wchar_t *end; // TODO: Figure out how to handle invalid numbers better. Shouldn't we issue a diagnostic? - long shlvl_i = fish_wcstol(shlvl_var->as_string().c_str(), &end); + long shlvl_i = fish_wcstol(str2wcstring(shlvl_var).c_str(), &end); if (!errno && shlvl_i >= 0) { nshlvl_str = to_string(shlvl_i + 1); } } env_set_one(L"SHLVL", ENV_GLOBAL | ENV_EXPORT, nshlvl_str); - env_read_only.emplace(L"SHLVL"); // Set up the HOME variable. // Unlike $USER, it doesn't seem that `su`s pass this along @@ -1286,9 +1296,8 @@ void env_var_t::to_list(wcstring_list_t &out) const { env_var_t::env_var_flags_t env_var_t::flags_for(const wchar_t *name) { env_var_flags_t result = 0; - wcstring tmp(name); // todo: eliminate - if (is_read_only(tmp)) result |= flag_read_only; - if (variable_is_colon_delimited_var(tmp)) result |= flag_colon_delimit; + if (is_read_only(name)) result |= flag_read_only; + if (variable_is_colon_delimited_var(name)) result |= flag_colon_delimit; return result; } From 3d1975c6a6deb221058b931b0df68e3b1092576e Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Tue, 30 Jan 2018 13:31:36 -0800 Subject: [PATCH 14/58] Convert variable_is_colon_delimited_var to a const array --- src/env.cpp | 31 ++++++++++++++++++------------- 1 file changed, 18 insertions(+), 13 deletions(-) diff --git a/src/env.cpp b/src/env.cpp index 61f57f0e4..a65c1161d 100644 --- a/src/env.cpp +++ b/src/env.cpp @@ -103,11 +103,6 @@ static const wcstring_list_t locale_variables({L"LANG", L"LANGUAGE", L"LC_ALL", /// subsystem. static const wcstring_list_t curses_variables({L"TERM", L"TERMINFO", L"TERMINFO_DIRS"}); -/// List of "path" like variable names that need special handling. This includes automatic -/// splitting and joining on import/export. As well as replacing empty elements, which implicitly -/// refer to the CWD, with an explicit '.' in the case of PATH and CDPATH. -static const wcstring_list_t colon_delimited_variable({L"PATH", L"MANPATH", L"CDPATH"}); - // Some forward declarations to make it easy to logically group the code. static void init_locale(); static void init_curses(); @@ -315,6 +310,7 @@ static env_universal_t *uvars() { return s_universal_variables; } // Helper class for storing constant strings, without needing to wrap them in a wcstring. // Comparer for const string set. +// Note our sets are small so we don't bother to sort them. typedef std::unordered_set const_string_set_t; // A typedef for a set of constant strings. Note our sets are typically on the order of 6 elements, @@ -337,6 +333,22 @@ static bool is_read_only(const wchar_t *val) { static bool is_read_only(const wcstring &val) { return is_read_only(val.c_str()); } +// Here is the whitelist of variables that we colon-delimit, both incoming from the environment and +// outgoing back to it. This is deliberately very short - we don't want to add language-specific +// values like CLASSPATH. +static const string_set_t colon_delimited_variable = {L"PATH", L"CDPATH", L"MANPATH"}; +static bool variable_is_colon_delimited_var(const wchar_t *str) { + /// List of "path" like variable names that need special handling. This includes automatic + /// splitting and joining on import/export. As well as replacing empty elements, which + /// implicitly refer to the CWD, with an explicit '.' in the case of PATH and CDPATH. Note this + /// is sorted + return string_set_contains(colon_delimited_variable, str); +} + +static bool variable_is_colon_delimited_var(const wcstring &str) { + return variable_is_colon_delimited_var(str.c_str()); +} + /// Table of variables whose value is dynamically calculated, such as umask, status, etc. static const_string_set_t env_electric; @@ -552,7 +564,7 @@ static bool initialize_curses_using_fallback(const char *term) { /// Ensure the content of the magic path env vars is reasonable. Specifically, that empty path /// elements are converted to explicit "." to make the vars easier to use in fish scripts. static void init_path_vars() { - for (const auto &var_name : colon_delimited_variable) { + for (const wchar_t *var_name : colon_delimited_variable) { fix_colon_delimited_var(var_name); } } @@ -598,13 +610,6 @@ static void init_curses() { curses_initialized = true; } -// Here is the whitelist of variables that we colon-delimit, both incoming from the environment and -// outgoing back to it. This is deliberately very short - we don't want to add language-specific -// values like CLASSPATH. -static bool variable_is_colon_delimited_var(const wcstring &str) { - return contains(colon_delimited_variable, str); -} - /// React to modifying the given variable. static void react_to_variable_change(const wchar_t *op, const wcstring &key) { // Don't do any of this until `env_init()` has run. We only want to do this in response to From 816d35de4368f4d08c29cb720c70b676f6d603e3 Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Tue, 30 Jan 2018 17:39:25 -0800 Subject: [PATCH 15/58] Clean up expand_variables Partially rewrite this function to be shorter and easier to follow. --- src/expand.cpp | 313 +++++++++++++++++++++---------------------------- 1 file changed, 133 insertions(+), 180 deletions(-) diff --git a/src/expand.cpp b/src/expand.cpp index a37015c73..60da42671 100644 --- a/src/expand.cpp +++ b/src/expand.cpp @@ -709,213 +709,166 @@ static size_t parse_slice(const wchar_t *in, wchar_t **end_ptr, std::vector *out, long last_idx, - parse_error_list_t *errors) { +static bool expand_variables(const wcstring &instr, std::vector *out, size_t last_idx, + parse_error_list_t *errors) { const size_t insize = instr.size(); // last_idx may be 1 past the end of the string, but no further. - assert(last_idx >= 0 && (size_t)last_idx <= insize); - + assert(last_idx <= insize && "Invalid last_idx"); if (last_idx == 0) { append_completion(out, instr); return true; } - bool is_ok = true; - bool empty = false; - - wcstring var_tmp; - - // List of indexes. - std::vector var_idx_list; - - // Parallel array of source positions of each index in the variable list. - std::vector var_pos_list; - - // CHECK( out, 0 ); - - for (long i = last_idx - 1; (i >= 0) && is_ok && !empty; i--) { - const wchar_t c = instr.at(i); - if (c != VARIABLE_EXPAND && c != VARIABLE_EXPAND_SINGLE) { - continue; - } - - long var_len; - int is_single = (c == VARIABLE_EXPAND_SINGLE); - size_t start_pos = i + 1; - size_t stop_pos = start_pos; - - while (stop_pos < insize) { - const wchar_t nc = instr.at(stop_pos); - if (nc == VARIABLE_EXPAND_EMPTY) { - stop_pos++; - break; - } - if (!valid_var_name_char(nc)) break; - - stop_pos++; - } - - // fwprintf(stdout, L"Stop for '%c'\n", in[stop_pos]); - var_len = stop_pos - start_pos; - - if (var_len == 0) { - if (errors) { - parse_util_expand_variable_error(instr, 0 /* global_token_pos */, i, errors); - } - - is_ok = false; + // Locate the last VARIABLE_EXPAND or VARIABLE_EXPAND_SINGLE + bool is_single = false; + size_t varexp_char_idx = last_idx; + while (varexp_char_idx--) { + const wchar_t c = instr.at(varexp_char_idx); + if (c == VARIABLE_EXPAND || c == VARIABLE_EXPAND_SINGLE) { + is_single = (c == VARIABLE_EXPAND_SINGLE); break; } + } + if (varexp_char_idx >= instr.size()) { + // No variable expand char, we're done. + append_completion(out, instr); + return true; + } - var_tmp.append(instr, start_pos, var_len); - maybe_t var; - if (var_len == 1 && var_tmp[0] == VARIABLE_EXPAND_EMPTY) { - var = none(); - } else { - var = env_get(var_tmp); + // Get the variable name. + const size_t var_name_start = varexp_char_idx + 1; + size_t var_name_stop = var_name_start; + while (var_name_stop < insize) { + const wchar_t nc = instr.at(var_name_stop); + if (nc == VARIABLE_EXPAND_EMPTY) { + var_name_stop++; + break; } + if (!valid_var_name_char(nc)) break; + var_name_stop++; + } + assert(var_name_stop >= var_name_start && "Bogus variable name indexes"); + const size_t var_name_len = var_name_stop - var_name_start; - if (var) { - int all_vars = 1; - wcstring_list_t var_item_list; - - if (is_ok) { - var->to_list(var_item_list); - - const size_t slice_start = stop_pos; - if (slice_start < insize && instr.at(slice_start) == L'[') { - wchar_t *slice_end; - size_t bad_pos; - all_vars = 0; - const wchar_t *in = instr.c_str(); - bad_pos = parse_slice(in + slice_start, &slice_end, var_idx_list, var_pos_list, - var_item_list.size()); - if (bad_pos != 0) { - append_syntax_error(errors, stop_pos + bad_pos, L"Invalid index value"); - is_ok = false; - break; - } - stop_pos = (slice_end - in); - } - - if (!all_vars) { - wcstring_list_t string_values(var_idx_list.size()); - size_t k = 0; - for (size_t j = 0; j < var_idx_list.size(); j++) { - long tmp = var_idx_list.at(j); - // Check that we are within array bounds. If not, skip the element. Note: - // Negative indices (`echo $foo[-1]`) are already converted to positive ones - // here, So tmp < 1 means it's definitely not in. - if ((size_t)tmp > var_item_list.size() || tmp < 1) { - continue; - } - // Replace each index in var_idx_list inplace with the string value - // at the specified index. - string_values.at(k++) = var_item_list.at(tmp - 1); - } - - // string_values is the new var_item_list. Resize to remove invalid elements. - string_values.resize(k); - var_item_list = std::move(string_values); - } - } - - if (!is_ok) { - return is_ok; - } - - if (is_single) { - wcstring res(instr, 0, i); - if (i > 0) { - if (instr.at(i - 1) != VARIABLE_EXPAND_SINGLE) { - res.push_back(INTERNAL_SEPARATOR); - } else if (var_item_list.empty() || var_item_list.front().empty()) { - // First expansion is empty, but we need to recursively expand. - res.push_back(VARIABLE_EXPAND_EMPTY); - } - } - - for (size_t j = 0; j < var_item_list.size(); j++) { - const wcstring &next = var_item_list.at(j); - if (is_ok) { - if (j != 0) res.append(L" "); - res.append(next); - } - } - assert(stop_pos <= insize); - res.append(instr, stop_pos, insize - stop_pos); - is_ok &= expand_variables(res, out, i, errors); - } else { - for (size_t j = 0; j < var_item_list.size(); j++) { - const wcstring &next = var_item_list.at(j); - if (is_ok && i == 0 && stop_pos == insize) { - append_completion(out, next); - } else { - if (is_ok) { - wcstring new_in; - new_in.append(instr, 0, i); - - if (i > 0) { - if (instr.at(i - 1) != VARIABLE_EXPAND) { - new_in.push_back(INTERNAL_SEPARATOR); - } else if (next.empty()) { - new_in.push_back(VARIABLE_EXPAND_EMPTY); - } - } - assert(stop_pos <= insize); - new_in.append(next); - new_in.append(instr, stop_pos, insize - stop_pos); - is_ok &= expand_variables(new_in, out, i, errors); - } - } - } - } - - return is_ok; + // It's an error if the name is empty. + if (var_name_len == 0) { + if (errors) { + parse_util_expand_variable_error(instr, 0 /* global_token_pos */, varexp_char_idx, + errors); } + return false; + } - // Even with no value, we still need to parse out slice syntax. Behave as though we - // had 1 value, so $foo[1] always works. - const size_t slice_start = stop_pos; - if (slice_start < insize && instr.at(slice_start) == L'[') { - const wchar_t *in = instr.c_str(); - wchar_t *slice_end; - size_t bad_pos; + // Get the variable name as a string, then try to get the variable from env. + const wcstring var_name(instr, var_name_start, var_name_len); + const maybe_t var = + (var_name == wcstring{VARIABLE_EXPAND_EMPTY} ? none() : env_get(var_name)); - bad_pos = parse_slice(in + slice_start, &slice_end, var_idx_list, var_pos_list, 1); - if (bad_pos != 0) { - append_syntax_error(errors, stop_pos + bad_pos, L"Invalid index value"); - is_ok = 0; - return is_ok; - } - stop_pos = (slice_end - in); + // Parse out any following slice. + // Record the end of the variable name and any following slice. + size_t var_name_and_slice_stop = var_name_stop; + bool all_vars = true; + const size_t slice_start = var_name_stop; + // List of indexes, and parallel array of source positions of each index in the variable list. + std::vector var_idx_list; + std::vector var_pos_list; + if (slice_start < insize && instr.at(slice_start) == L'[') { + all_vars = false; + const wchar_t *in = instr.c_str(); + wchar_t *slice_end; + // If a variable is missing, behave as though we have one value, so that $var[1] always + // works. + size_t effective_val_count = var ? var->as_list().size() : 1; + size_t bad_pos = parse_slice(in + slice_start, &slice_end, var_idx_list, var_pos_list, + effective_val_count); + if (bad_pos != 0) { + append_syntax_error(errors, slice_start + bad_pos, L"Invalid index value"); + return false; } + var_name_and_slice_stop = (slice_end - in); + } - // Expand a non-existing variable. - if (c == VARIABLE_EXPAND) { - // Regular expansion, i.e. expand this argument to nothing. - empty = true; + if (!var) { + // Expanding a non-existent variable. + if (!is_single) { + // Normal expansions of missing variables successfully expand to nothing. + return true; } else { // Expansion to single argument. - wcstring res; - res.append(instr, 0, i); - if (i > 0 && instr.at(i - 1) == VARIABLE_EXPAND_SINGLE) { + // Replace the variable name and slice with VARIABLE_EXPAND_EMPTY. + wcstring res(instr, 0, varexp_char_idx); + if (!res.empty() && res.back() == VARIABLE_EXPAND_SINGLE) { res.push_back(VARIABLE_EXPAND_EMPTY); } - assert(stop_pos <= insize); - res.append(instr, stop_pos, insize - stop_pos); - - is_ok &= expand_variables(res, out, i, errors); - return is_ok; + res.append(instr, var_name_and_slice_stop, wcstring::npos); + return expand_variables(res, out, varexp_char_idx, errors); } } - if (!empty) { - append_completion(out, instr); + // Ok, we have a variable. Let's expand it. + // Start by respecting the sliced elements. + assert(var && "Should have variable here"); + wcstring_list_t var_item_list = var->as_list(); + if (!all_vars) { + wcstring_list_t sliced_items; + for (long item_index : var_idx_list) { + // Check that we are within array bounds. If not, skip the element. Note: + // Negative indices (`echo $foo[-1]`) are already converted to positive ones + // here, So tmp < 1 means it's definitely not in. + // Note we are 1-based. + if (item_index >= 1 && size_t(item_index) <= var_item_list.size()) { + sliced_items.push_back(var_item_list.at(item_index - 1)); + } + } + var_item_list = std::move(sliced_items); } - return is_ok; + if (is_single) { + wcstring res(instr, 0, varexp_char_idx); + if (!res.empty()) { + if (res.back() != VARIABLE_EXPAND_SINGLE) { + res.push_back(INTERNAL_SEPARATOR); + } else if (var_item_list.empty() || var_item_list.front().empty()) { + // First expansion is empty, but we need to recursively expand. + res.push_back(VARIABLE_EXPAND_EMPTY); + } + } + + // Append all entries in var_item_list, separated by spaces. + // Remove the last space. + if (!var_item_list.empty()) { + for (const wcstring &item : var_item_list) { + res.append(item); + res.push_back(L' '); + } + res.pop_back(); + } + res.append(instr, var_name_and_slice_stop, wcstring::npos); + return expand_variables(res, out, varexp_char_idx, errors); + } else { + // Normal cartesian-product expansion. + for (const wcstring &item : var_item_list) { + if (varexp_char_idx == 0 && var_name_and_slice_stop == insize) { + append_completion(out, item); + } else { + wcstring new_in(instr, 0, varexp_char_idx); + if (!new_in.empty()) { + if (new_in.back() != VARIABLE_EXPAND) { + new_in.push_back(INTERNAL_SEPARATOR); + } else if (item.empty()) { + new_in.push_back(VARIABLE_EXPAND_EMPTY); + } + } + new_in.append(item); + new_in.append(instr, var_name_and_slice_stop, wcstring::npos); + if (!expand_variables(new_in, out, varexp_char_idx, errors)) { + return false; + } + } + } + } + return true; } /// Perform bracket expansion. From 54cefeb5b14bfed76451679e9fea356f8789f2b4 Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Tue, 30 Jan 2018 18:30:30 -0800 Subject: [PATCH 16/58] Make sliced history (e.g. $history[1]) much faster This special cases expansion of $history variables, so that slicing history no longer needs to construct the entire history array. Speedup is around 100x in my test. Fixes #4650 --- CHANGELOG.md | 1 + src/expand.cpp | 74 ++++++++++++++++++++++++++++++++++++------------- src/history.cpp | 29 +++++++++++++++++-- src/history.h | 11 +++++++- 4 files changed, 93 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 06cd51b3a..aed177a1e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -35,6 +35,7 @@ This section is for changes merged to the `major` branch that are not also merge - Pager navigation has been improved. Most notably, moving down now wraps around, moving up from the commandline now jumps to the last element and moving right and left now reverse each other even when wrapping around (#4680). - Typing normal characters while the completion pager is active no longer shows the search field. Instead it enters them into the command line, and ends paging (#2249). - A new input binding `pager-toggle-search` toggles the search field in the completions pager on and off. By default this is bound to control-s. +- Slicing $history (in particular, `$history[1]` for the last executed command) is much faster. ## Other significant changes - Command substitution output is now limited to 10 MB by default (#3822). diff --git a/src/expand.cpp b/src/expand.cpp index 60da42671..52baa961e 100644 --- a/src/expand.cpp +++ b/src/expand.cpp @@ -39,11 +39,13 @@ #include "exec.h" #include "expand.h" #include "fallback.h" // IWYU pragma: keep +#include "history.h" #include "iothread.h" #include "parse_constants.h" #include "parse_util.h" #include "path.h" #include "proc.h" +#include "reader.h" #include "wildcard.h" #include "wutil.h" // IWYU pragma: keep #ifdef KERN_PROCARGS2 @@ -762,24 +764,40 @@ static bool expand_variables(const wcstring &instr, std::vector *o // Get the variable name as a string, then try to get the variable from env. const wcstring var_name(instr, var_name_start, var_name_len); - const maybe_t var = - (var_name == wcstring{VARIABLE_EXPAND_EMPTY} ? none() : env_get(var_name)); + // Do a dirty hack to make sliced history fast (#4650). We expand from either a variable, or a + // history_t. Note that "history" is read only in env.cpp so it's safe to special-case it in + // this way (it cannot be shadowed, etc). + history_t *history = nullptr; + maybe_t var{}; + if (var_name == L"history") { + // We do this only on the main thread, matching env.cpp. + if (is_main_thread()) { + history = reader_get_history(); + } + } else if (var_name != wcstring{VARIABLE_EXPAND_EMPTY}) { + var = env_get(var_name); + } // Parse out any following slice. // Record the end of the variable name and any following slice. size_t var_name_and_slice_stop = var_name_stop; - bool all_vars = true; + bool all_values = true; const size_t slice_start = var_name_stop; // List of indexes, and parallel array of source positions of each index in the variable list. std::vector var_idx_list; std::vector var_pos_list; if (slice_start < insize && instr.at(slice_start) == L'[') { - all_vars = false; + all_values = false; const wchar_t *in = instr.c_str(); wchar_t *slice_end; // If a variable is missing, behave as though we have one value, so that $var[1] always // works. - size_t effective_val_count = var ? var->as_list().size() : 1; + size_t effective_val_count = 1; + if (var) { + effective_val_count = var->as_list().size(); + } else if (history) { + effective_val_count = history->size(); + } size_t bad_pos = parse_slice(in + slice_start, &slice_end, var_idx_list, var_pos_list, effective_val_count); if (bad_pos != 0) { @@ -789,7 +807,7 @@ static bool expand_variables(const wcstring &instr, std::vector *o var_name_and_slice_stop = (slice_end - in); } - if (!var) { + if (!var && !history) { // Expanding a non-existent variable. if (!is_single) { // Normal expansions of missing variables successfully expand to nothing. @@ -806,22 +824,40 @@ static bool expand_variables(const wcstring &instr, std::vector *o } } - // Ok, we have a variable. Let's expand it. + // Ok, we have a variable or a history. Let's expand it. // Start by respecting the sliced elements. - assert(var && "Should have variable here"); - wcstring_list_t var_item_list = var->as_list(); - if (!all_vars) { - wcstring_list_t sliced_items; - for (long item_index : var_idx_list) { - // Check that we are within array bounds. If not, skip the element. Note: - // Negative indices (`echo $foo[-1]`) are already converted to positive ones - // here, So tmp < 1 means it's definitely not in. - // Note we are 1-based. - if (item_index >= 1 && size_t(item_index) <= var_item_list.size()) { - sliced_items.push_back(var_item_list.at(item_index - 1)); + assert((var || history) && "Should have variable or history here"); + wcstring_list_t var_item_list; + if (all_values) { + if (history) { + history->get_history(var_item_list); + } else { + var->to_list(var_item_list); + } + } else { + // We have to respect the slice. + if (history) { + // Ask history to map indexes to item strings. + // Note this may have missing entries for out-of-bounds. + auto item_map = history->items_at_indexes(var_idx_list); + for (long item_index : var_idx_list) { + auto iter = item_map.find(item_index); + if (iter != item_map.end()) { + var_item_list.push_back(iter->second); + } + } + } else { + const wcstring_list_t &all_var_items = var->as_list(); + for (long item_index : var_idx_list) { + // Check that we are within array bounds. If not, skip the element. Note: + // Negative indices (`echo $foo[-1]`) are already converted to positive ones + // here, So tmp < 1 means it's definitely not in. + // Note we are 1-based. + if (item_index >= 1 && size_t(item_index) <= all_var_items.size()) { + var_item_list.push_back(all_var_items.at(item_index - 1)); + } } } - var_item_list = std::move(sliced_items); } if (is_single) { diff --git a/src/history.cpp b/src/history.cpp index 8023a496a..96ce0ebdd 100644 --- a/src/history.cpp +++ b/src/history.cpp @@ -890,8 +890,8 @@ size_t history_t::size() { return new_item_count + old_item_count; } -history_item_t history_t::item_at_index(size_t idx) { - scoped_lock locker(lock); +history_item_t history_t::item_at_index_assume_locked(size_t idx) { + ASSERT_IS_LOCKED(lock); // 0 is considered an invalid index. assert(idx > 0); @@ -923,6 +923,31 @@ history_item_t history_t::item_at_index(size_t idx) { return history_item_t(wcstring(), 0); } +history_item_t history_t::item_at_index(size_t idx) { + scoped_lock locker(lock); + return item_at_index_assume_locked(idx); +} + +std::unordered_map history_t::items_at_indexes(const std::vector &idxs) { + scoped_lock locker(lock); + std::unordered_map result; + for (long idx : idxs) { + if (idx <= 0) { + // Skip non-positive entries. + continue; + } + // Insert an empty string to see if this is the first time the index is encountered. If so, + // we have to go fetch the item. + auto iter_inserted = result.emplace(idx, wcstring{}); + if (iter_inserted.second) { + // New key. + auto item = item_at_index_assume_locked(size_t(idx)); + iter_inserted.first->second = std::move(item.contents); + } + } + return result; +} + void history_t::populate_from_mmap(void) { mmap_type = infer_file_type(mmap_start, mmap_length); size_t cursor = 0; diff --git a/src/history.h b/src/history.h index a706d24ae..42d1dcfb1 100644 --- a/src/history.h +++ b/src/history.h @@ -12,8 +12,9 @@ #include #include -#include #include +#include +#include #include #include @@ -215,6 +216,9 @@ class history_t { // Whether we're in maximum chaos mode, useful for testing. bool chaos_mode; + // Implementation of item_at_index and items_at_indexes + history_item_t item_at_index_assume_locked(size_t idx); + public: explicit history_t(const wcstring &); // constructor @@ -272,6 +276,11 @@ class history_t { // This may be long! void get_history(wcstring_list_t &result); + // Let indexes be a list of one-based indexes into the history, matching the interpretation of + // $history. That is, $history[1] is the most recently executed command. Values less than one + // are skipped. Return a mapping from index to history item text. + std::unordered_map items_at_indexes(const std::vector &idxs); + // Sets the valid file paths for the history item with the given identifier. void set_valid_file_paths(const wcstring_list_t &valid_file_paths, history_identifier_t ident); From fb53a96a1c2a62f97b99bda424419d61ed36ab7f Mon Sep 17 00:00:00 2001 From: David Adam Date: Wed, 31 Jan 2018 13:43:05 +0800 Subject: [PATCH 17/58] Add configure-time check for std::make_unique Fixes the build on Clang 6 and closes #4685. --- cmake/ConfigureChecks.cmake | 10 ++++++++++ configure.ac | 9 +++++++++ src/common.h | 2 +- 3 files changed, 20 insertions(+), 1 deletion(-) diff --git a/cmake/ConfigureChecks.cmake b/cmake/ConfigureChecks.cmake index 6e1168e73..30cdf47a1 100644 --- a/cmake/ConfigureChecks.cmake +++ b/cmake/ConfigureChecks.cmake @@ -113,4 +113,14 @@ IF(NOT TPARM_TAKES_VARARGS) SET(TPARM_SOLARIS_KLUDGE 1) ENDIF() +CHECK_CXX_SOURCE_COMPILES(" +#include + +int main () { + std::unique_ptr foo = std::make_unique(); +} +" + HAVE_STD__MAKE_UNIQUE +) + FIND_PROGRAM(SED sed) diff --git a/configure.ac b/configure.ac index 4f923266d..1640984d7 100644 --- a/configure.ac +++ b/configure.ac @@ -400,6 +400,15 @@ AC_TRY_LINK( [ #include ], [AC_MSG_RESULT(no)], ) +AC_MSG_CHECKING([for std::make_unique]) +AC_TRY_LINK( [ #include ], + [ std::unique_ptr foo = std::make_unique(); ], + [ AC_MSG_RESULT(yes) + AC_DEFINE(HAVE_STD__MAKE_UNIQUE, 1, Define to 1 if you have the `std::make_unique' function.) + ], + [AC_MSG_RESULT(no)], + ) + if test x$local_gettext != xno; then AC_CHECK_FUNCS( gettext ) diff --git a/src/common.h b/src/common.h index 85b8f170e..1809eab1c 100644 --- a/src/common.h +++ b/src/common.h @@ -606,7 +606,7 @@ wcstring vformat_string(const wchar_t *format, va_list va_orig); void append_format(wcstring &str, const wchar_t *format, ...); void append_formatv(wcstring &str, const wchar_t *format, va_list ap); -#ifdef __cpp_lib_make_unique +#ifdef HAVE_STD__MAKE_UNIQUE using std::make_unique; #else /// make_unique implementation From 64c8d4c2652e155c5633703be84bf125621c6f4d Mon Sep 17 00:00:00 2001 From: ogaclejapan Date: Tue, 30 Jan 2018 08:16:04 +0900 Subject: [PATCH 18/58] Fix typo that invalid options in argparse --- doc_src/argparse.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc_src/argparse.txt b/doc_src/argparse.txt index af0c62ba2..d210c1689 100644 --- a/doc_src/argparse.txt +++ b/doc_src/argparse.txt @@ -34,7 +34,7 @@ The following `argparse` options are available. They must appear before all OPTI Using this command involves passing two sets of arguments separated by `--`. The first set consists of one or more option specifications (`OPTION_SPEC` above) and options that modify the behavior of `argparse`. These must be listed before the `--` argument. The second set are the arguments to be parsed in accordance with the option specifications. They occur after the `--` argument and can be empty. More about this below but here is a simple example that might be used in a function named `my_function`: \fish -argparse --name=my_function 'h/help' 'n/name:' -- $argv +argparse --name=my_function 'h/help' 'n/name=' -- $argv or return \endfish From 4fac2f98c214ae1799b0dbeacdc7c75a6a772e3b Mon Sep 17 00:00:00 2001 From: Fabian Homborg Date: Fri, 26 Jan 2018 10:28:40 +0100 Subject: [PATCH 19/58] [git completions] Use status v1 output Apparently the v2 format is too new (released Nov 2016), and the v1 format has everything we need. --- share/completions/git.fish | 81 ++++++++++++++++++-------------------- 1 file changed, 39 insertions(+), 42 deletions(-) diff --git a/share/completions/git.fish b/share/completions/git.fish index 70ce6cb49..f1cca7ba0 100644 --- a/share/completions/git.fish +++ b/share/completions/git.fish @@ -84,19 +84,17 @@ function __fish_git_files # to get _all_ kinds of staged files. # git status --porcelain gives us all the info we need, in a format we don't. - # The v2 format thankfully doesn't use " M" to denote a changed unstaged file - # and "M " to denote a changed staged file, opting for a "." in place of the space. - # So the v1 format would require something other than this `while read` loop. - # - # Unfortunately, the v2 format is really 4 different subformats - # - see the explanation inline. (Or on https://git-scm.com/docs/git-status) + # The v2 format has better documentation and doesn't use " " to denote anything, + # but it's only been added in git 2.11.0, which was released November 2016. + # Instead, we use the v1 format, without explicitly specifying it (since that errors out as well). # # Also, we ignore submodules because they aren't useful as arguments (generally), # and they slow things down quite significantly. # E.g. `git reset $submodule` won't do anything (not even print an error). + # --ignore-submodules=all was added in git 1.7.2, released July 2010. set -l use_next - command git status --porcelain=2 -z --ignore-submodules=all \ - | while read -laz -d ' ' line + command git status --porcelain -z --ignore-submodules=all \ + | while read -lz -d '' line # The entire line is the "from" from a rename. if set -q use_next[1] contains -- $use_next $argv @@ -105,67 +103,66 @@ function __fish_git_files continue end + # The format is two characters for status, then a space and then + # up to a NUL for the filename. + # + # Use IFS to handle newlines in filenames. + set -l IFS + set -l stat (string sub -l 2 -- $line) + set -l file (string sub -s 4 -- $line) + set -e IFS + # The basic status format is "XY", where X is "our" state (meaning the staging area), - # and "Y" is "their" state. - # A "." means it's unmodified. - switch "$line[1..2]" - case 'u *' + # and "Y" is "their" state (meaning the work tree). + # A " " means it's unmodified. + switch "$stat" + case DD AU UD UA DU AA UU # Unmerged - # "Unmerged entries have the following format; the first character is a "u" to distinguish from ordinary changed entries." - # "u

" - # This is first to distinguish it from normal modifications et al. contains -- unmerged $argv - and printf '%s\t%s\n' "$line[11..-1]" (_ "Unmerged file") - case '? .R*' '? R.*' + and printf '%s\t%s\n' "$file" (_ "Unmerged file") + case 'R ' RM RD # Renamed/Copied - # From the docs: "Renamed or copied entries have the following format:" - # "2 " - # Since is NUL, the (meaning the old name) is in the next batch. - # TODO: Do we care about the new one? + # These have the "from" name as the next batch. + # TODO: Do we care about the new name? set use_next renamed continue - case '? .C*' '? C.*' + case 'C ' CM CD set use_next copied continue - case '? A.*' + case 'A ' AM AD # Additions are only shown here if they are staged. # Otherwise it's an untracked file. contains -- added $argv; or contains -- all-staged $argv - and printf '%s\t%s\n' "$line[9..-1]" (_ "Added file") - case '? .M*' + and printf '%s\t%s\n' "$file" (_ "Added file") + case '?M' # Modified - # From the docs: "Ordinary changed entries have the following format:" - # "1 " - # Since can contain spaces, print from element 9 onwards contains -- modified $argv - and printf '%s\t%s\n' "$line[9..-1]" (_ "Modified file") - case '? M.*' - # If the character is first ("M."), then that means it's "our" change, + and printf '%s\t%s\n' "$file" (_ "Modified file") + case 'M?' + # If the character is first ("M "), then that means it's "our" change, # which means it is staged. # This is useless for many commands - e.g. `checkout` won't do anything with this. # So it needs to be requested explicitly. contains -- modified-staged $argv; or contains -- all-staged $argv - and printf '%s\t%s\n' "$line[9..-1]" (_ "Staged modified file") - case '? .D*' + and printf '%s\t%s\n' "$file" (_ "Staged modified file") + case '?D' contains -- deleted $argv - and printf '%s\t%s\n' "$line[9..-1]" (_ "Deleted file") - case '? D.*' + and printf '%s\t%s\n' "$file" (_ "Deleted file") + case 'D?' # TODO: The docs are unclear on this. # There is both X unmodified and Y either M or D ("not updated") # and Y is D and X is unmodified or [MARC] ("deleted in work tree"). # For our purposes, we assume this is a staged deletion. contains -- deleted-staged $argv; or contains -- all-staged $argv - and printf '%s\t%s\n' "$line[9..-1]" (_ "Staged deleted file") - case '\? *' + and printf '%s\t%s\n' "$file" (_ "Staged deleted file") + case '\?\?' # Untracked - # "? " - print from element 2 on. contains -- untracked $argv - and printf '%s\t%s\n' "$line[2..-1]" (_ "Untracked file") - case '! *' + and printf '%s\t%s\n' "$file" (_ "Untracked file") + case '!!' # Ignored - # "! " - print from element 2 on. contains -- ignored $argv - and printf '%s\t%s\n' "$line[2..-1]" (_ "Ignored file") + and printf '%s\t%s\n' "$file" (_ "Ignored file") end end end From 0ab437be2601bb4100aee1138d3af3d4faade55d Mon Sep 17 00:00:00 2001 From: Fabian Homborg Date: Mon, 29 Jan 2018 19:05:42 +0100 Subject: [PATCH 20/58] [git completions] Make paths not in $PWD relative to the root again When git prints a path like "share/completions/git.fish", that's relative to the root of the repo. So we need to either remove everything from the $PWD (if the path is inside the $PWD), or prepend a ":/", which is git-speak for "relative to the root". This was removed by mistake in the recent switch to `git status`. Fixes #4688. --- share/completions/git.fish | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/share/completions/git.fish b/share/completions/git.fish index f1cca7ba0..36c83b522 100644 --- a/share/completions/git.fish +++ b/share/completions/git.fish @@ -83,6 +83,9 @@ function __fish_git_files # and as a convenience "all-staged" # to get _all_ kinds of staged files. + # Save the repo root to remove it from the path later. + set -l root (command git rev-parse --show-toplevel ^/dev/null) + # git status --porcelain gives us all the info we need, in a format we don't. # The v2 format has better documentation and doesn't use " " to denote anything, # but it's only been added in git 2.11.0, which was released November 2016. @@ -98,7 +101,7 @@ function __fish_git_files # The entire line is the "from" from a rename. if set -q use_next[1] contains -- $use_next $argv - and echo "$line" + and string replace -- "$PWD/" "" "$root/$line" | string replace "$root/" ":/" set -e use_next[1] continue end @@ -110,6 +113,9 @@ function __fish_git_files set -l IFS set -l stat (string sub -l 2 -- $line) set -l file (string sub -s 4 -- $line) + # Print files from the current $PWD as-is, prepend all others with ":/" (relative to toplevel in git-speak) + # This is a bit simplistic but finding the lowest common directory and then replacing everything else in $PWD with ".." is a bit annoying + set file (string replace -- "$PWD/" "" "$root/$file" | string replace "$root/" ":/") set -e IFS # The basic status format is "XY", where X is "our" state (meaning the staging area), From 18e21a09920b819eb4795f472d73f2c133bf4bf5 Mon Sep 17 00:00:00 2001 From: Fabian Homborg Date: Wed, 31 Jan 2018 21:47:13 +0100 Subject: [PATCH 21/58] [git completions] Complete branches faster This saves one `git branch` invocation and improves the descriptions. --- share/completions/git.fish | 53 +++++++++++++++++--------------------- 1 file changed, 24 insertions(+), 29 deletions(-) diff --git a/share/completions/git.fish b/share/completions/git.fish index 36c83b522..79f00cca8 100644 --- a/share/completions/git.fish +++ b/share/completions/git.fish @@ -18,19 +18,14 @@ function __fish_git_recent_commits | string replace -r '(.{73}).+' '$1…' end -function __fish_git_local_branches - command git branch --no-color | string trim -c ' *' -end - -function __fish_git_remote_branches - command git branch --no-color --remote | string trim -c ' *' -end - function __fish_git_branches - # In some cases, git can end up on no branch - e.g. with a detached head - # This will result in output like `* (no branch)` or a localized `* (HEAD detached at SHA)` - # The first `string match -v` filters it out because it's not useful as a branch argument - command git branch --no-color -a $argv ^/dev/null | string match -v '\* (*)' | string match -r -v ' -> ' | string trim -c "* " | string replace -r "^remotes/" "" + command git branch --no-color -a --format='%(refname)' $argv ^/dev/null \ + # Filter out anything that's not in "refs/" notation - + # this happens mostly with a detached head ("(HEAD detached at SOMESHA)", localized). + | string replace -rf '^refs/' '' \ + # We assume anything that's not remote is a local branch. + | string replace -r '^(?!remotes/)[^/]+/(.*)' '$1\tLocal Branch' \ + | string replace -r "^remotes/(.*)" '$1\tRemote Branch' end function __fish_git_unique_remote_branches @@ -184,7 +179,8 @@ function __fish_git_ranges end set -l to (set -q both[2]; and echo $both[2]) - for from_ref in (__fish_git_refs | string match "$from") + # Remove description from the from-ref, not the to-ref. + for from_ref in (__fish_git_refs | string match "$from" | string replace -r \t'.*$' '') for to_ref in (__fish_git_refs | string match "*$to*") # if $to is empty, this correctly matches everything printf "%s..%s\n" $from_ref $to_ref end @@ -383,7 +379,7 @@ complete -f -c git -n '__fish_git_using_command log show diff-tree rev-list' -l complete -f -c git -n '__fish_git_needs_command' -a fetch -d 'Download objects and refs from another repository' # Suggest "repository", then "refspec" - this also applies to e.g. push/pull complete -f -c git -n '__fish_git_using_command fetch; and not __fish_git_branch_for_remote' -a '(__fish_git_remotes)' -d 'Remote' -complete -f -c git -n '__fish_git_using_command fetch; and __fish_git_branch_for_remote' -a '(__fish_git_branch_for_remote)' -d 'Branch' +complete -f -c git -n '__fish_git_using_command fetch; and __fish_git_branch_for_remote' -a '(__fish_git_branch_for_remote)' complete -f -c git -n '__fish_git_using_command fetch' -s q -l quiet -d 'Be quiet' complete -f -c git -n '__fish_git_using_command fetch' -s v -l verbose -d 'Be verbose' complete -f -c git -n '__fish_git_using_command fetch' -s a -l append -d 'Append ref names and object names' @@ -433,7 +429,7 @@ complete -f -c git -n "__fish_git_using_command remote; and __fish_seen_subcomma ### show complete -f -c git -n '__fish_git_needs_command' -a show -d 'Shows the last commit of a branch' -complete -f -c git -n '__fish_git_using_command show' -a '(__fish_git_branches)' -d 'Branch' +complete -f -c git -n '__fish_git_using_command show' -a '(__fish_git_branches)' complete -f -c git -n '__fish_git_using_command show' -a '(__fish_git_unique_remote_branches)' -d 'Remote branch' complete -f -c git -n '__fish_git_using_command show' -a '(__fish_git_tags)' -d 'Tag' complete -f -c git -n '__fish_git_using_command show' -a '(__fish_git_commits)' @@ -466,8 +462,7 @@ complete -f -c git -n '__fish_git_using_command add' -a '(__fish_git_files modif ### checkout complete -f -c git -n '__fish_git_needs_command' -a checkout -d 'Checkout and switch to a branch' -complete -k -f -c git -n '__fish_git_using_command checkout' -a '(__fish_git_local_branches)' -d 'Local Branch' -complete -k -f -c git -n '__fish_git_using_command checkout' -a '(__fish_git_remote_branches)' -d 'Remote Branch' +complete -k -f -c git -n '__fish_git_using_command checkout' -a '(__fish_git_branches)' complete -k -f -c git -n '__fish_git_using_command checkout' -a '(__fish_git_heads)' -d 'Head' complete -k -f -c git -n '__fish_git_using_command checkout' -a '(__fish_git_unique_remote_branches)' -d 'Remote branch' complete -k -f -c git -n '__fish_git_using_command checkout' -a '(__fish_git_tags)' -d 'Tag' @@ -492,7 +487,7 @@ complete -f -c git -n '__fish_git_needs_command' -a bisect -d 'Find the change t ### branch complete -f -c git -n '__fish_git_needs_command' -a branch -d 'List, create, or delete branches' -complete -f -c git -n '__fish_git_using_command branch' -a '(__fish_git_branches)' -d 'Branch' +complete -f -c git -n '__fish_git_using_command branch' -a '(__fish_git_branches)' complete -f -c git -n '__fish_git_using_command branch' -s d -d 'Delete branch' complete -f -c git -n '__fish_git_using_command branch' -s D -d 'Force deletion of branch' complete -f -c git -n '__fish_git_using_command branch' -s m -d 'Rename branch' @@ -506,7 +501,7 @@ complete -f -c git -n '__fish_git_using_command branch' -l no-merged -d 'List br ### cherry-pick complete -f -c git -n '__fish_git_needs_command' -a cherry-pick -d 'Apply the change introduced by an existing commit' -complete -f -c git -n '__fish_git_using_command cherry-pick' -a '(__fish_git_branches --no-merged)' -d 'Branch' +complete -f -c git -n '__fish_git_using_command cherry-pick' -a '(__fish_git_branches --no-merged)' complete -f -c git -n '__fish_git_using_command cherry-pick' -a '(__fish_git_unique_remote_branches --no-merged)' -d 'Remote branch' # TODO: Filter further complete -f -c git -n '__fish_git_using_command cherry-pick; and __fish_git_possible_commithash' -a '(__fish_git_commits)' @@ -539,7 +534,7 @@ complete -f -c git -n '__fish_git_using_command commit; and __fish_contains_opt ### diff complete -c git -n '__fish_git_needs_command' -a diff -d 'Show changes between commits, commit and working tree, etc' -complete -c git -n '__fish_git_using_command diff' -a '(__fish_git_ranges)' -d 'Branch' +complete -c git -n '__fish_git_using_command diff' -a '(__fish_git_ranges)' complete -c git -n '__fish_git_using_command diff' -l cached -d 'Show diff of changes in the index' complete -c git -n '__fish_git_using_command diff' -l no-index -d 'Compare two paths on the filesystem' complete -f -c git -n '__fish_git_using_command diff' -a '(__fish_git_files modified deleted)' @@ -547,7 +542,7 @@ complete -f -c git -n '__fish_git_using_command diff' -a '(__fish_git_files modi ### difftool complete -c git -n '__fish_git_needs_command' -a difftool -d 'Open diffs in a visual tool' -complete -c git -n '__fish_git_using_command difftool' -a '(__fish_git_ranges)' -d 'Branch' +complete -c git -n '__fish_git_using_command difftool' -a '(__fish_git_ranges)' complete -c git -n '__fish_git_using_command difftool' -l cached -d 'Visually show diff of changes in the index' complete -f -c git -n '__fish_git_using_command difftool' -a '(__fish_git_files modified deleted)' # TODO options @@ -564,7 +559,7 @@ complete -f -c git -n '__fish_git_needs_command' -a init -d 'Create an empty git ### log complete -c git -n '__fish_git_needs_command' -a shortlog -d 'Show commit shortlog' complete -c git -n '__fish_git_needs_command' -a log -d 'Show commit logs' -complete -c git -n '__fish_git_using_command log' -a '(__fish_git_refs) (__fish_git_ranges)' -d 'Branch' +complete -c git -n '__fish_git_using_command log' -a '(__fish_git_refs) (__fish_git_ranges)' complete -c git -n '__fish_git_using_command log' -l follow -d 'Continue listing file history beyond renames' complete -c git -n '__fish_git_using_command log' -l no-decorate -d 'Don\'t print ref names' @@ -765,7 +760,7 @@ complete -c git -n '__fish_git_using_command log' -l ita-invisible-in-index ### merge complete -f -c git -n '__fish_git_needs_command' -a merge -d 'Join two or more development histories together' -complete -f -c git -n '__fish_git_using_command merge' -a '(__fish_git_branches)' -d 'Branch' +complete -f -c git -n '__fish_git_using_command merge' -a '(__fish_git_branches)' complete -f -c git -n '__fish_git_using_command merge' -a '(__fish_git_unique_remote_branches)' -d 'Remote branch' complete -f -c git -n '__fish_git_using_command merge' -l commit -d "Autocommit the merge" complete -f -c git -n '__fish_git_using_command merge' -l no-commit -d "Don't autocommit the merge" @@ -826,13 +821,13 @@ complete -f -c git -n '__fish_git_using_command pull' -l no-tags -d 'Disable aut # TODO --upload-pack complete -f -c git -n '__fish_git_using_command pull' -l progress -d 'Force progress status' complete -f -c git -n '__fish_git_using_command pull; and not __fish_git_branch_for_remote' -a '(__fish_git_remotes)' -d 'Remote alias' -complete -f -c git -n '__fish_git_using_command pull; and __fish_git_branch_for_remote' -a '(__fish_git_branch_for_remote)' -d 'Branch' +complete -f -c git -n '__fish_git_using_command pull; and __fish_git_branch_for_remote' -a '(__fish_git_branch_for_remote)' # TODO other options ### push complete -f -c git -n '__fish_git_needs_command' -a push -d 'Update remote refs along with associated objects' complete -f -c git -n '__fish_git_using_command push; and not __fish_git_branch_for_remote' -a '(__fish_git_remotes)' -d 'Remote alias' -complete -f -c git -n '__fish_git_using_command push; and __fish_git_branch_for_remote' -a '(__fish_git_branches)' -d 'Branch' +complete -f -c git -n '__fish_git_using_command push; and __fish_git_branch_for_remote' -a '(__fish_git_branches)' # The "refspec" here is an optional "+" to signify a force-push complete -f -c git -n '__fish_git_using_command push; and __fish_git_branch_for_remote; and string match -q "+*" -- (commandline -ct)' -a '+(__fish_git_branches)' -d 'Force-push branch' # git push REMOTE :BRANCH deletes BRANCH on remote REMOTE @@ -859,7 +854,7 @@ complete -f -c git -n '__fish_git_using_command push' -l progress -d 'Force prog ### rebase complete -f -c git -n '__fish_git_needs_command' -a rebase -d 'Forward-port local commits to the updated upstream head' complete -f -c git -n '__fish_git_using_command rebase' -a '(__fish_git_remotes)' -d 'Remote alias' -complete -f -c git -n '__fish_git_using_command rebase' -a '(__fish_git_branches)' -d 'Branch' +complete -f -c git -n '__fish_git_using_command rebase' -a '(__fish_git_branches)' complete -f -c git -n '__fish_git_using_command rebase' -a '(__fish_git_heads)' -d 'Head' complete -f -c git -n '__fish_git_using_command rebase' -a '(__fish_git_tags)' -d 'Tag' complete -f -c git -n '__fish_git_using_command rebase' -l continue -d 'Restart the rebasing process' @@ -884,7 +879,7 @@ complete -f -c git -n '__fish_git_using_command rebase' -l no-ff -d 'No fast-for ### reset complete -c git -n '__fish_git_needs_command' -a reset -d 'Reset current HEAD to the specified state' complete -f -c git -n '__fish_git_using_command reset' -l hard -d 'Reset files in working directory' -complete -c git -n '__fish_git_using_command reset' -a '(__fish_git_branches)' -d 'Branch' +complete -c git -n '__fish_git_using_command reset' -a '(__fish_git_branches)' # reset can either undo changes to versioned modified files, # or remove files from the staging area. # TODO: Deleted files seem to need a "--" separator. @@ -920,7 +915,7 @@ complete -f -c git -n '__fish_git_using_command status' -l ignore-submodules -x ### tag complete -f -c git -n '__fish_git_needs_command' -a tag -d 'Create, list, delete or verify a tag object signed with GPG' -complete -f -c git -n '__fish_git_using_command tag; and __fish_not_contain_opt -s d; and __fish_not_contain_opt -s v; and test (count (commandline -opc | string match -r -v \'^-\')) -eq 3' -a '(__fish_git_branches)' -d 'Branch' +complete -f -c git -n '__fish_git_using_command tag; and __fish_not_contain_opt -s d; and __fish_not_contain_opt -s v; and test (count (commandline -opc | string match -r -v \'^-\')) -eq 3' -a '(__fish_git_branches)' complete -f -c git -n '__fish_git_using_command tag' -s a -l annotate -d 'Make an unsigned, annotated tag object' complete -f -c git -n '__fish_git_using_command tag' -s s -l sign -d 'Make a GPG-signed tag' complete -f -c git -n '__fish_git_using_command tag' -s d -l delete -d 'Remove a tag' @@ -956,7 +951,7 @@ complete -f -c git -n '__fish_git_needs_command' -a config -d 'Set and read git ### format-patch complete -f -c git -n '__fish_git_needs_command' -a format-patch -d 'Generate patch series to send upstream' -complete -f -c git -n '__fish_git_using_command format-patch' -a '(__fish_git_branches)' -d 'Branch' +complete -f -c git -n '__fish_git_using_command format-patch' -a '(__fish_git_branches)' complete -f -c git -n '__fish_git_using_command format-patch' -s p -l no-stat -d "Generate plain patches without diffstat" complete -f -c git -n '__fish_git_using_command format-patch' -s s -l no-patch -d "Suppress diff output" complete -f -c git -n '__fish_git_using_command format-patch' -l minimal -d "Spend more time to create smaller diffs" From 7d60d1db6d060bc82e0e3c92df0f0519f0e88756 Mon Sep 17 00:00:00 2001 From: Fabian Homborg Date: Wed, 31 Jan 2018 21:48:03 +0100 Subject: [PATCH 22/58] [git completions] Remove unique-remote-branches This never worked properly (since a branch that only exists locally would also be offered) and is dog-slow. When we come up with a better way to do it we can readd it. --- share/completions/git.fish | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/share/completions/git.fish b/share/completions/git.fish index 79f00cca8..9427538f1 100644 --- a/share/completions/git.fish +++ b/share/completions/git.fish @@ -28,12 +28,6 @@ function __fish_git_branches | string replace -r "^remotes/(.*)" '$1\tRemote Branch' end -function __fish_git_unique_remote_branches - # Allow all remote branches with one remote without the remote part - # This is useful for `git checkout` to automatically create a remote-tracking branch - command git branch --no-color -a $argv ^/dev/null | string match -v '\* (*)' | string match -r -v ' -> ' | string trim -c "* " | string replace -r "^remotes/[^/]*/" "" | sort | uniq -u -end - function __fish_git_tags command git tag ^/dev/null end @@ -430,7 +424,6 @@ complete -f -c git -n "__fish_git_using_command remote; and __fish_seen_subcomma ### show complete -f -c git -n '__fish_git_needs_command' -a show -d 'Shows the last commit of a branch' complete -f -c git -n '__fish_git_using_command show' -a '(__fish_git_branches)' -complete -f -c git -n '__fish_git_using_command show' -a '(__fish_git_unique_remote_branches)' -d 'Remote branch' complete -f -c git -n '__fish_git_using_command show' -a '(__fish_git_tags)' -d 'Tag' complete -f -c git -n '__fish_git_using_command show' -a '(__fish_git_commits)' complete -f -c git -n '__fish_git_using_command show' -l stat -d 'Generate a diffstat, showing the number of changed lines of each file' @@ -464,7 +457,6 @@ complete -f -c git -n '__fish_git_using_command add' -a '(__fish_git_files modif complete -f -c git -n '__fish_git_needs_command' -a checkout -d 'Checkout and switch to a branch' complete -k -f -c git -n '__fish_git_using_command checkout' -a '(__fish_git_branches)' complete -k -f -c git -n '__fish_git_using_command checkout' -a '(__fish_git_heads)' -d 'Head' -complete -k -f -c git -n '__fish_git_using_command checkout' -a '(__fish_git_unique_remote_branches)' -d 'Remote branch' complete -k -f -c git -n '__fish_git_using_command checkout' -a '(__fish_git_tags)' -d 'Tag' complete -k -f -c git -n '__fish_git_using_command checkout' -a '(__fish_git_files modified deleted)' complete -f -c git -n '__fish_git_using_command checkout' -s b -d 'Create a new branch' @@ -502,7 +494,6 @@ complete -f -c git -n '__fish_git_using_command branch' -l no-merged -d 'List br ### cherry-pick complete -f -c git -n '__fish_git_needs_command' -a cherry-pick -d 'Apply the change introduced by an existing commit' complete -f -c git -n '__fish_git_using_command cherry-pick' -a '(__fish_git_branches --no-merged)' -complete -f -c git -n '__fish_git_using_command cherry-pick' -a '(__fish_git_unique_remote_branches --no-merged)' -d 'Remote branch' # TODO: Filter further complete -f -c git -n '__fish_git_using_command cherry-pick; and __fish_git_possible_commithash' -a '(__fish_git_commits)' complete -f -c git -n '__fish_git_using_command cherry-pick' -s e -l edit -d 'Edit the commit message prior to committing' @@ -761,7 +752,6 @@ complete -c git -n '__fish_git_using_command log' -l ita-invisible-in-index ### merge complete -f -c git -n '__fish_git_needs_command' -a merge -d 'Join two or more development histories together' complete -f -c git -n '__fish_git_using_command merge' -a '(__fish_git_branches)' -complete -f -c git -n '__fish_git_using_command merge' -a '(__fish_git_unique_remote_branches)' -d 'Remote branch' complete -f -c git -n '__fish_git_using_command merge' -l commit -d "Autocommit the merge" complete -f -c git -n '__fish_git_using_command merge' -l no-commit -d "Don't autocommit the merge" complete -f -c git -n '__fish_git_using_command merge' -l edit -d 'Edit auto-generated merge message' From d28493dba613755b5f67e357e8102159f9a1953b Mon Sep 17 00:00:00 2001 From: David Adam Date: Tue, 23 Jan 2018 17:35:38 +0800 Subject: [PATCH 23/58] [cmake] add variable if building in-tree --- CMakeLists.txt | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/CMakeLists.txt b/CMakeLists.txt index 6eaaf7e4e..dbd646211 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -28,6 +28,12 @@ SET_PROPERTY(GLOBAL PROPERTY USE_FOLDERS ON) list(APPEND CMAKE_MODULE_PATH "${CMAKE_CURRENT_LIST_DIR}/cmake") +IF(CMAKE_CURRENT_SOURCE_DIR STREQUAL CMAKE_CURRENT_BINARY_DIR) + SET(FISH_IN_TREE_BUILD TRUE) +ELSE() + SET(FISH_IN_TREE_BUILD FALSE) +ENDIF() + # All objects that the system needs to build fish, except fish.cpp SET(FISH_SRCS src/autoload.cpp src/builtin.cpp src/builtin_bg.cpp src/builtin_bind.cpp From e1bc48492f545d1756bb40ccdd9ac9bded5f347d Mon Sep 17 00:00:00 2001 From: David Adam Date: Tue, 23 Jan 2018 17:35:43 +0800 Subject: [PATCH 24/58] [cmake] only copy tests for out-of-tree builds Makes in-tree builds work again. --- cmake/Tests.cmake | 32 +++++++++++++++++++------------- 1 file changed, 19 insertions(+), 13 deletions(-) diff --git a/cmake/Tests.cmake b/cmake/Tests.cmake index e37b74718..aadcecf2c 100644 --- a/cmake/Tests.cmake +++ b/cmake/Tests.cmake @@ -16,13 +16,15 @@ SET(TEST_ROOT_DIR ${TEST_DIR}/root) FILE(GLOB TESTS_FILES tests/*) ADD_CUSTOM_TARGET(tests_dir DEPENDS tests) -ADD_CUSTOM_COMMAND(TARGET tests_dir - COMMAND ${CMAKE_COMMAND} -E copy_directory - ${CMAKE_SOURCE_DIR}/tests/ ${CMAKE_BINARY_DIR}/tests/ - COMMENT "Copying test files to binary dir" - VERBATIM) +IF(NOT FISH_IN_TREE_BUILD) + ADD_CUSTOM_COMMAND(TARGET tests_dir + COMMAND ${CMAKE_COMMAND} -E copy_directory + ${CMAKE_SOURCE_DIR}/tests/ ${CMAKE_BINARY_DIR}/tests/ + COMMENT "Copying test files to binary dir" + VERBATIM) -ADD_DEPENDENCIES(fish_tests tests_dir) + ADD_DEPENDENCIES(fish_tests tests_dir) +ENDIF() # Create the 'test' target. # Set a policy so CMake stops complaining about the name 'test'. @@ -51,13 +53,17 @@ ADD_CUSTOM_TARGET(tests_buildroot_target ${TEST_ROOT_DIR} DEPENDS fish) -# We need to symlink share/functions for the tests. -# This should be simplified. -ADD_CUSTOM_TARGET(symlink_functions - COMMAND ${CMAKE_COMMAND} -E create_symlink - ${CMAKE_CURRENT_SOURCE_DIR}/share/functions - ${CMAKE_CURRENT_BINARY_DIR}/share/functions) -ADD_DEPENDENCIES(tests_buildroot_target symlink_functions) +IF(NOT FISH_IN_TREE_BUILD) + # We need to symlink share/functions for the tests. + # This should be simplified. + ADD_CUSTOM_TARGET(symlink_functions + COMMAND ${CMAKE_COMMAND} -E create_symlink + ${CMAKE_CURRENT_SOURCE_DIR}/share/functions + ${CMAKE_CURRENT_BINARY_DIR}/share/functions) + ADD_DEPENDENCIES(tests_buildroot_target symlink_functions) +ELSE() + ADD_CUSTOM_TARGET(symlink_functions) +ENDIF() # # Prep the environment for running the unit tests. From 56be0453248a95bff72531124d94d637f9d63672 Mon Sep 17 00:00:00 2001 From: David Adam Date: Thu, 1 Feb 2018 22:46:15 +0800 Subject: [PATCH 25/58] [cmake] copy shipped documentation files if they exist Enables documentation in tarballs to be installed, even if Doxygen isn't present. --- cmake/Docs.cmake | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/cmake/Docs.cmake b/cmake/Docs.cmake index 7989881fe..45b0348da 100644 --- a/cmake/Docs.cmake +++ b/cmake/Docs.cmake @@ -5,10 +5,27 @@ INCLUDE(FeatureSummary) IF(DOXYGEN_FOUND) OPTION(BUILD_DOCS "build documentation (requires Doxygen)" ON) ELSE(DOXYGEN_FOUND) - SET(BUILD_DOCS OFF) + OPTION(BUILD_DOCS "build documentation (requires Doxygen)" OFF) ENDIF(DOXYGEN_FOUND) -ADD_FEATURE_INFO(Documentation BUILD_DOCS "user manual and documentation") +IF(BUILD_DOCS AND NOT DOXYGEN_FOUND) + MESSAGE(FATAL_ERROR "build documentation selected, but Doxygen could not be found") +ENDIF() + +IF(IS_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/user_doc/html + AND IS_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/share/man/man1) + SET(HAVE_PREBUILT_DOCS TRUE) +ELSE() + SET(HAVE_PREBUILT_DOCS FALSE) +ENDIF() + +IF(BUILD_DOCS OR HAVE_PREBUILT_DOCS) + SET(INSTALL_DOCS ON) +ELSE() + SET(INSTALL_DOCS OFF) +ENDIF() + +ADD_FEATURE_INFO(Documentation INSTALL_DOCS "user manual and documentation") IF(BUILD_DOCS) # Files in ./share/completions/ From 3be026129435facf1f31e21cafa33211b06f1601 Mon Sep 17 00:00:00 2001 From: David Adam Date: Thu, 1 Feb 2018 22:48:12 +0800 Subject: [PATCH 26/58] travis: move to in-tree builds --- .travis.yml | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/.travis.yml b/.travis.yml index 19aa25a70..80127ddfb 100644 --- a/.travis.yml +++ b/.travis.yml @@ -38,8 +38,7 @@ matrix: env: - USE_CMAKE="1" # Dummy value, shows up in the Travis UI only script: - - mkdir build && cd build && - cmake -DCMAKE_INSTALL_PREFIX=$HOME/prefix .. || cat CMakeFiles/CMakeError.log && + - cmake -DCMAKE_INSTALL_PREFIX=$HOME/prefix . || cat CMakeFiles/CMakeError.log && make -j2 && make install && make test SHOW_INTERACTIVE_LOG=1 From 52627199952b35b65d0b6135361e365fb9f2dfc1 Mon Sep 17 00:00:00 2001 From: Fabian Homborg Date: Fri, 2 Feb 2018 23:31:53 +0100 Subject: [PATCH 27/58] Don't fire exit events for jobs with pgid == -2 This fixes a hang common on WSL, when fish has PID 2. Fixes #4582. --- src/proc.cpp | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/proc.cpp b/src/proc.cpp index 27b08f600..42ec0730a 100644 --- a/src/proc.cpp +++ b/src/proc.cpp @@ -617,7 +617,13 @@ static int process_clean_after_marking(bool allow_interactive) { format_job_info(j, JOB_ENDED); found = 1; } - proc_fire_event(L"JOB_EXIT", EVENT_EXIT, -j->pgid, 0); + // Don't fire the exit-event for jobs with pgid -2. + // That's our "sentinel" pgid, for jobs that don't (yet) have a pgid, + // or jobs that consist entirely of builtins (and hence don't have a process). + // This causes issues if fish is PID 2, which is quite common on WSL. See #4582. + if (j->pgid != -2) { + proc_fire_event(L"JOB_EXIT", EVENT_EXIT, -j->pgid, 0); + } proc_fire_event(L"JOB_EXIT", EVENT_JOB_ID, j->job_id, 0); job_remove(j); From 89709c3a890b4f33bffc698081bd52f257a309ac Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sat, 3 Feb 2018 14:10:47 -0800 Subject: [PATCH 28/58] Clean up some history search interfaces --- src/reader.cpp | 55 ++++++++++++++++++++++---------------------------- 1 file changed, 24 insertions(+), 31 deletions(-) diff --git a/src/reader.cpp b/src/reader.cpp index 7ac94394c..91dcc7061 100644 --- a/src/reader.cpp +++ b/src/reader.cpp @@ -114,20 +114,13 @@ /// current contents of the kill buffer. #define KILL_PREPEND 1 -/// History search mode. This value means that no search is currently performed. -#define NO_SEARCH 0 +enum class history_search_mode_t { + none, // no search + line, // searching by line + token // searching by token +}; -/// History search mode. This value means that we are performing a line history search. -#define LINE_SEARCH 1 - -/// History search mode. This value means that we are performing a token history search. -#define TOKEN_SEARCH 2 - -/// History search mode. This value means we are searching backwards. -#define SEARCH_BACKWARD 0 - -/// History search mode. This value means we are searching forwards. -#define SEARCH_FORWARD 1 +enum class history_search_direction_t { forward, backward }; /// Any time the contents of a buffer changes, we update the generation count. This allows for our /// background threads to notice it and skip doing work that they would otherwise have to do. @@ -230,7 +223,7 @@ class reader_data_t { /// Pointer to previous reader_data. reader_data_t *next; /// This variable keeps state on if we are in search mode, and if yes, what mode. - int search_mode; + history_search_mode_t search_mode = history_search_mode_t::none; /// Keep track of whether any internal code has done something which is known to require a /// repaint. bool repaint_needed; @@ -279,7 +272,6 @@ class reader_data_t { end_loop(false), prev_end_loop(false), next(0), - search_mode(0), repaint_needed(false), screen_reset_needed(false), exit_on_interrupt(false) {} @@ -1699,10 +1691,11 @@ static void reset_token_history() { /// Handles a token search command. /// -/// \param forward if the search should be forward or reverse +/// \param dir if the search should be forward or reverse /// \param reset whether the current token should be made the new search token -static void handle_token_history(int forward, int reset) { +static void handle_token_history(history_search_direction_t dir, bool reset = false) { if (!data) return; + const bool forward = (dir == history_search_direction_t::forward); wcstring str; size_t current_pos; @@ -1773,7 +1766,7 @@ static void handle_token_history(int forward, int reset) { data->search_prev.push_back(str); } else if (!reader_interrupted()) { data->token_history_pos = -1; - handle_token_history(0, 0); + handle_token_history(history_search_direction_t::forward); } } } @@ -1851,7 +1844,7 @@ static void reader_set_buffer_maintaining_pager(const wcstring &b, size_t pos) { update_buff_pos(&data->command_line, pos); // Clear history search and pager contents. - data->search_mode = NO_SEARCH; + data->search_mode = history_search_mode_t::none; data->search_buff.clear(); data->history_search.go_to_end(); @@ -2352,7 +2345,7 @@ const wchar_t *reader_readline(int nchars) { data->cycle_cursor_pos = 0; data->search_buff.clear(); - data->search_mode = NO_SEARCH; + data->search_mode = history_search_mode_t::none; exec_prompt(); @@ -2687,8 +2680,8 @@ const wchar_t *reader_readline(int nchars) { } // Escape was pressed. case L'\e': { - if (data->search_mode) { - data->search_mode = NO_SEARCH; + if (data->search_mode != history_search_mode_t::none) { + data->search_mode = history_search_mode_t::none; if (data->token_history_pos == (size_t)-1) { // history_reset(); @@ -2809,12 +2802,12 @@ const wchar_t *reader_readline(int nchars) { case R_HISTORY_SEARCH_FORWARD: case R_HISTORY_TOKEN_SEARCH_FORWARD: { int reset = 0; - if (data->search_mode == NO_SEARCH) { + if (data->search_mode == history_search_mode_t::none) { reset = 1; if ((c == R_HISTORY_SEARCH_BACKWARD) || (c == R_HISTORY_SEARCH_FORWARD)) { - data->search_mode = LINE_SEARCH; + data->search_mode = history_search_mode_t::line; } else { - data->search_mode = TOKEN_SEARCH; + data->search_mode = history_search_mode_t::token; } const editable_line_t *el = &data->command_line; @@ -2833,7 +2826,7 @@ const wchar_t *reader_readline(int nchars) { data->history_search.skip_matches(skip_list); } - if (data->search_mode == LINE_SEARCH) { + if (data->search_mode == history_search_mode_t::line) { if ((c == R_HISTORY_SEARCH_BACKWARD) || (c == R_HISTORY_TOKEN_SEARCH_BACKWARD)) { data->history_search.go_backwards(); @@ -2851,12 +2844,12 @@ const wchar_t *reader_readline(int nchars) { new_text = data->history_search.current_string(); } set_command_line_and_position(&data->command_line, new_text, new_text.size()); - } else if (data->search_mode == TOKEN_SEARCH) { + } else if (data->search_mode == history_search_mode_t::token) { if ((c == R_HISTORY_SEARCH_BACKWARD) || (c == R_HISTORY_TOKEN_SEARCH_BACKWARD)) { - handle_token_history(SEARCH_BACKWARD, reset); + handle_token_history(history_search_direction_t::backward, reset); } else { - handle_token_history(SEARCH_FORWARD, reset); + handle_token_history(history_search_direction_t::forward, reset); } } break; @@ -3217,7 +3210,7 @@ const wchar_t *reader_readline(int nchars) { if ((c != R_HISTORY_SEARCH_BACKWARD) && (c != R_HISTORY_SEARCH_FORWARD) && (c != R_HISTORY_TOKEN_SEARCH_BACKWARD) && (c != R_HISTORY_TOKEN_SEARCH_FORWARD) && (c != R_NULL) && (c != R_REPAINT) && (c != R_FORCE_REPAINT)) { - data->search_mode = NO_SEARCH; + data->search_mode = history_search_mode_t::none; data->search_buff.clear(); data->history_search.go_to_end(); data->token_history_pos = -1; @@ -3253,7 +3246,7 @@ int reader_search_mode() { if (!data) { return -1; } - return data->search_mode == NO_SEARCH ? 0 : 1; + return data->search_mode == history_search_mode_t::none ? 0 : 1; } int reader_has_pager_contents() { From 85fba3a316ad847409bae84826889144ef4b87de Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sat, 3 Feb 2018 14:34:28 -0800 Subject: [PATCH 29/58] Remove HISTORY_SEARCH_TYPE_*_PCRE These were unused and unimplemented. --- src/history.cpp | 6 ------ src/history.h | 6 +----- 2 files changed, 1 insertion(+), 11 deletions(-) diff --git a/src/history.cpp b/src/history.cpp index 96ce0ebdd..190208eaa 100644 --- a/src/history.cpp +++ b/src/history.cpp @@ -482,12 +482,6 @@ bool history_item_t::matches_search(const wcstring &term, enum history_search_ty if (wcpattern2.back() != ANY_STRING) wcpattern2.push_back(ANY_STRING); return wildcard_match(content_to_match, wcpattern2); } - case HISTORY_SEARCH_TYPE_CONTAINS_PCRE: { - abort(); - } - case HISTORY_SEARCH_TYPE_PREFIX_PCRE: { - abort(); - } } DIE("unexpected history_search_type_t value"); } diff --git a/src/history.h b/src/history.h index 42d1dcfb1..da4ae2e92 100644 --- a/src/history.h +++ b/src/history.h @@ -53,11 +53,7 @@ enum history_search_type_t { // Search for commands containing the given glob pattern. HISTORY_SEARCH_TYPE_CONTAINS_GLOB, // Search for commands starting with the given glob pattern. - HISTORY_SEARCH_TYPE_PREFIX_GLOB, - // Search for commands containing the given PCRE pattern. - HISTORY_SEARCH_TYPE_CONTAINS_PCRE, - // Search for commands starting with the given PCRE pattern. - HISTORY_SEARCH_TYPE_PREFIX_PCRE + HISTORY_SEARCH_TYPE_PREFIX_GLOB }; typedef uint32_t history_identifier_t; From fea1597a27b9d73cdceaa500b7bc34b9fb7c6edf Mon Sep 17 00:00:00 2001 From: Mahmoud Al-Qudsi Date: Sun, 4 Feb 2018 02:56:29 -0600 Subject: [PATCH 30/58] [cmake] Correct test for term.h (include curses.h first) The non-ncurses version of term.h requires that curses.h be first included. Only very recent versions of CMake include a LANGUAGE option to CHECK_INCLUDE_FILES, so we aren't using it and specifying CXX here.. --- cmake/ConfigureChecks.cmake | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/cmake/ConfigureChecks.cmake b/cmake/ConfigureChecks.cmake index 30cdf47a1..8919749fd 100644 --- a/cmake/ConfigureChecks.cmake +++ b/cmake/ConfigureChecks.cmake @@ -41,8 +41,10 @@ CHECK_CXX_SYMBOL_EXISTS(killpg "sys/types.h;signal.h" HAVE_KILLPG) CHECK_CXX_SYMBOL_EXISTS(lrand48_r stdlib.h HAVE_LRAND48_R) # mkostemp is in stdlib in glibc and FreeBSD, but unistd on macOS CHECK_CXX_SYMBOL_EXISTS(mkostemp "stdlib.h;unistd.h" HAVE_MKOSTEMP) +SET(HAVE_CURSES_H ${CURSES_HAVE_CURSES_H}) SET(HAVE_NCURSES_CURSES_H ${CURSES_HAVE_NCURSES_CURSES_H}) SET(HAVE_NCURSES_H ${CURSES_HAVE_NCURSES_H}) +CHECK_INCLUDE_FILES("curses.h;term.h" HAVE_TERM_H) CHECK_INCLUDE_FILE_CXX("ncurses/term.h" HAVE_NCURSES_TERM_H) CHECK_INCLUDE_FILE_CXX(siginfo.h HAVE_SIGINFO_H) CHECK_INCLUDE_FILE_CXX(spawn.h HAVE_SPAWN_H) @@ -60,7 +62,6 @@ CHECK_INCLUDE_FILE_CXX(sys/ioctl.h HAVE_SYS_IOCTL_H) CHECK_INCLUDE_FILE_CXX(sys/select.h HAVE_SYS_SELECT_H) CHECK_INCLUDE_FILES("sys/types.h;sys/sysctl.h" HAVE_SYS_SYSCTL_H) CHECK_INCLUDE_FILE_CXX(termios.h HAVE_TERMIOS_H) # Needed for TIOCGWINSZ -CHECK_INCLUDE_FILE_CXX(term.h HAVE_TERM_H) CHECK_CXX_SYMBOL_EXISTS(wcscasecmp wchar.h HAVE_WCSCASECMP) CHECK_CXX_SYMBOL_EXISTS(wcsdup wchar.h HAVE_WCSDUP) CHECK_CXX_SYMBOL_EXISTS(wcslcpy wchar.h HAVE_WCSLCPY) From a95a83b1407b0358c7a645094d3edfddda53a5f6 Mon Sep 17 00:00:00 2001 From: Mahmoud Al-Qudsi Date: Sun, 4 Feb 2018 02:59:03 -0600 Subject: [PATCH 31/58] [cmake] Add missing HAVE_CURSES_H #cmakedefine --- config_cmake.h.in | 3 +++ 1 file changed, 3 insertions(+) diff --git a/config_cmake.h.in b/config_cmake.h.in index c38807eb4..f5e186c43 100644 --- a/config_cmake.h.in +++ b/config_cmake.h.in @@ -40,6 +40,9 @@ /* Define to 1 if you have the `mkostemp' function. */ #cmakedefine HAVE_MKOSTEMP 1 +/* Define to 1 if you have the header file. */ +#cmakedefine HAVE_CURSES_H 1 + /* Define to 1 if you have the header file. */ #cmakedefine HAVE_NCURSES_CURSES_H 1 From 63c8a197e5dc1c946cc9dcb789f3c3645d99fec0 Mon Sep 17 00:00:00 2001 From: Mahmoud Al-Qudsi Date: Sun, 4 Feb 2018 02:59:37 -0600 Subject: [PATCH 32/58] [cmake] Clean up curses vs ncurses includes There were several issues with the way that the include tests for curses.h were being done that were ultimately causing fish to use the headers from ncurses but link against curses on platforms that provide an actual libcurses.so that isn't just a symlink to libncurses.so In particular, the old code was first testing for curses's cureses.h and then falling back to libncurses's implementation of the same - but that logic was reversed when it came to including term.h, in which case it was testing for the ncurses term.h and falling back to the curses.h header. Long story short, while cmake will link against libcurses.so if both libcurses.so and libncurses.so are present (unless CURSES_NEED_NCURSES evaluates to TRUE, but that makes ncurses a hard requirement), but we were brining in some of the defines from the ncurses headers, causing SIGSEGV panics when fish ultimately tried to access variables that weren't exported or were mapped to undefined areas of memory in the other library. Additionally it is an error to include termios.h prior to including the plain Jane curses.h (not ncurses/curses.h), causing errors about unimplemented types SGTTY/chtype. So far as I can tell, both curses.h and ncurses/curses.h pull in termios.h themselves so it shouldn't even be necessary to manually include it, but I have just moved its #include below that of curses.h --- src/builtin_set_color.cpp | 6 +++--- src/env.cpp | 6 +++--- src/fallback.cpp | 6 +++--- src/fallback.h | 3 --- src/input.cpp | 3 ++- src/output.cpp | 6 +++--- src/proc.cpp | 3 ++- src/screen.cpp | 6 +++--- 8 files changed, 19 insertions(+), 20 deletions(-) diff --git a/src/builtin_set_color.cpp b/src/builtin_set_color.cpp index b6d9e12bd..1ce2920ac 100644 --- a/src/builtin_set_color.cpp +++ b/src/builtin_set_color.cpp @@ -4,12 +4,12 @@ #include #include -#if HAVE_NCURSES_H +#if HAVE_CURSES_H +#include +#elif HAVE_NCURSES_H #include #elif HAVE_NCURSES_CURSES_H #include -#else -#include #endif #if HAVE_TERM_H #include diff --git a/src/env.cpp b/src/env.cpp index a65c1161d..f40bf5da7 100644 --- a/src/env.cpp +++ b/src/env.cpp @@ -16,12 +16,12 @@ #include #include -#if HAVE_NCURSES_H +#if HAVE_CURSES_H +#include +#elif HAVE_NCURSES_H #include #elif HAVE_NCURSES_CURSES_H #include -#else -#include #endif #if HAVE_TERM_H #include diff --git a/src/fallback.cpp b/src/fallback.cpp index 40d468a5f..16deb29fc 100644 --- a/src/fallback.cpp +++ b/src/fallback.cpp @@ -23,12 +23,12 @@ #if HAVE_GETTEXT #include #endif -#if HAVE_NCURSES_H +#if HAVE_CURSES_H +#include +#elif HAVE_NCURSES_H #include // IWYU pragma: keep #elif HAVE_NCURSES_CURSES_H #include -#else -#include #endif #if HAVE_TERM_H #include // IWYU pragma: keep diff --git a/src/fallback.h b/src/fallback.h index f0b3bb91f..c3a152793 100644 --- a/src/fallback.h +++ b/src/fallback.h @@ -15,9 +15,6 @@ // in . At least on OS X if we don't do this we get compilation errors do to the macro // substitution if wchar.h is included after this header. #include // IWYU pragma: keep -#if HAVE_NCURSES_H -#include // IWYU pragma: keep -#endif /// fish's internal versions of wcwidth and wcswidth, which can use an internal implementation if /// the system one is busted. diff --git a/src/input.cpp b/src/input.cpp index 9c2464509..6a5fcc87a 100644 --- a/src/input.cpp +++ b/src/input.cpp @@ -2,14 +2,15 @@ #include "config.h" #include -#include #include #include #if HAVE_TERM_H +#include #include #elif HAVE_NCURSES_TERM_H #include #endif +#include #include #include diff --git a/src/output.cpp b/src/output.cpp index 10009956f..b50d54bce 100644 --- a/src/output.cpp +++ b/src/output.cpp @@ -4,12 +4,12 @@ #include #include #include -#if HAVE_NCURSES_H +#if HAVE_CURSES_H +#include +#elif HAVE_NCURSES_H #include #elif HAVE_NCURSES_CURSES_H #include -#else -#include #endif #if HAVE_TERM_H #include diff --git a/src/proc.cpp b/src/proc.cpp index 42ec0730a..a9b313656 100644 --- a/src/proc.cpp +++ b/src/proc.cpp @@ -10,16 +10,17 @@ #include #include #include -#include #include #include #include #if HAVE_TERM_H +#include #include #elif HAVE_NCURSES_TERM_H #include #endif +#include #ifdef HAVE_SIGINFO_H #include #endif diff --git a/src/screen.cpp b/src/screen.cpp index 64fd40fa5..0e4b3e1b5 100644 --- a/src/screen.cpp +++ b/src/screen.cpp @@ -15,12 +15,12 @@ #include #include -#if HAVE_NCURSES_H +#if HAVE_CURSES_H +#include +#elif HAVE_NCURSES_H #include #elif HAVE_NCURSES_CURSES_H #include -#else -#include #endif #if HAVE_TERM_H #include From 9ba6b62791e9c912310ead9051f131bdf14890ed Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sun, 4 Feb 2018 14:03:08 -0800 Subject: [PATCH 33/58] Remove some ancient "#if 0' code and fix formatting errors --- src/reader.cpp | 10 +++--- src/screen.cpp | 86 -------------------------------------------------- 2 files changed, 5 insertions(+), 91 deletions(-) diff --git a/src/reader.cpp b/src/reader.cpp index 91dcc7061..0c5d77d38 100644 --- a/src/reader.cpp +++ b/src/reader.cpp @@ -2734,14 +2734,14 @@ const wchar_t *reader_readline(int nchars) { bool continue_on_next_line = false; if (el->position >= el->size()) { // We're at the end of the text and not in a comment (issue #1225). - continue_on_next_line = is_backslashed(el->text, el->position) && - !text_ends_in_comment(el->text); + continue_on_next_line = + is_backslashed(el->text, el->position) && !text_ends_in_comment(el->text); } else { // Allow mid line split if the following character is whitespace (issue #613). if (is_backslashed(el->text, el->position) && iswspace(el->text.at(el->position))) { continue_on_next_line = true; - // Check if the end of the line is backslashed (issue #4467). + // Check if the end of the line is backslashed (issue #4467). } else if (is_backslashed(el->text, el->size()) && !text_ends_in_comment(el->text)) { // Move the cursor to the end of the line. @@ -2974,7 +2974,8 @@ const wchar_t *reader_readline(int nchars) { select_completion_in_direction(direction); } else if (!data->pager.empty()) { // We pressed a direction with a non-empty pager, begin navigation. - select_completion_in_direction(c == R_DOWN_LINE ? direction_south : direction_north); + select_completion_in_direction(c == R_DOWN_LINE ? direction_south + : direction_north); } else { // Not navigating the pager contents. editable_line_t *el = data->active_edit_line(); @@ -3188,7 +3189,6 @@ const wchar_t *reader_readline(int nchars) { // Other, if a normal character, we add it to the command. if (!fish_reserved_codepoint(c) && (c >= L' ' || c == L'\n' || c == L'\r') && c != 0x7F) { - // Regular character. editable_line_t *el = data->active_edit_line(); bool allow_expand_abbreviations = (el == &data->command_line); diff --git a/src/screen.cpp b/src/screen.cpp index 64fd40fa5..3b5950e01 100644 --- a/src/screen.cpp +++ b/src/screen.cpp @@ -360,23 +360,6 @@ static size_t calc_prompt_lines(const wcstring &prompt) { /// Stat stdout and stderr and save result. This should be done before calling a function that may /// cause output. static void s_save_status(screen_t *s) { -// PCA Let's not do this futimes stuff, because sudo dumbly uses the tty's ctime as part of its -// tty_tickets feature. Disabling this should fix issue #122. -#if 0 - // This futimes call tries to trick the system into using st_mtime as a tampering flag. This of - // course only works on systems where futimes is defined, but it should make the status saving - // stuff failsafe. - struct timeval t[] = { - { time(0)-1, 0 }, - { time(0)-1, 0 } - }; - - // Don't check return value on these. We don't care if they fail, really. This is all just to - // make the prompt look ok, which is impossible to do 100% reliably. We try, at least. - futimes(1, t); - futimes(2, t); -#endif - fstat(1, &s->prev_buff_1); fstat(2, &s->prev_buff_2); } @@ -508,11 +491,6 @@ static void s_move(screen_t *s, data_buffer_t *b, int new_x, int new_y) { int x_steps, y_steps; char *str; - /* - debug( 0, L"move from %d %d to %d %d", - s->screen_cursor[0], s->screen_cursor[1], - new_x, new_y ); - */ scoped_buffer_t scoped_buffer(b); y_steps = new_y - s->actual.cursor.y; @@ -645,64 +623,6 @@ static bool perform_any_impending_soft_wrap(screen_t *scr, int x, int y) { /// Make sure we don't soft wrap. static void invalidate_soft_wrap(screen_t *scr) { scr->soft_wrap_location = INVALID_LOCATION; } -#if 0 -/// Various code for testing term behavior. -static bool test_stuff(screen_t *scr) -{ - data_buffer_t output; - scoped_buffer_t scoped_buffer(&output); - - s_move(scr, &output, 0, 0); - int screen_width = common_get_width(); - - const wchar_t *left = L"left"; - const wchar_t *right = L"right"; - - for (size_t idx = 0; idx < 80; idx++) - { - output.push_back('A'); - } - - if (! output.empty()) - { - write_loop(STDOUT_FILENO, &output.at(0), output.size()); - output.clear(); - } - - sleep(5); - - for (size_t i=0; i < 1; i++) - { - writembs(cursor_left); - } - - if (! output.empty()) - { - write_loop(1, &output.at(0), output.size()); - output.clear(); - } - - - - while (1) - { - int c = getchar(); - if (c != EOF) break; - } - - - while (1) - { - int c = getchar(); - if (c != EOF) break; - } - fwprintf(stdout, L"Bye\n"); - exit(0); - while (1) sleep(10000); - return true; -} -#endif - /// Update the screen to match the desired output. static void s_update(screen_t *scr, const wchar_t *left_prompt, const wchar_t *right_prompt) { // if (test_stuff(scr)) return; @@ -741,12 +661,6 @@ static void s_update(screen_t *scr, const wchar_t *left_prompt, const wchar_t *r // Determine how many lines have stuff on them; we need to clear lines with stuff that we don't // want. const size_t lines_with_stuff = maxi(actual_lines_before_reset, scr->actual.line_count()); -#if 0 - if (lines_with_stuff > scr->desired.line_count()) { - // There are lines that we output to previously that will need to be cleared. - need_clear_lines = true; - } -#endif if (wcscmp(left_prompt, scr->actual_left_prompt.c_str())) { s_move(scr, &output, 0, 0); From 27c1c06ed42a92fc5c6a57fec4f9c300291839ce Mon Sep 17 00:00:00 2001 From: slama Date: Sun, 4 Feb 2018 16:04:43 +0900 Subject: [PATCH 34/58] improve the size of completions page to show the entire prompt --- src/reader.cpp | 8 +++++--- src/screen.cpp | 2 +- src/screen.h | 3 +++ 3 files changed, 9 insertions(+), 4 deletions(-) diff --git a/src/reader.cpp b/src/reader.cpp index 0c5d77d38..90fa55733 100644 --- a/src/reader.cpp +++ b/src/reader.cpp @@ -434,9 +434,11 @@ static void reader_repaint() { std::vector indents = data->indents; indents.resize(len); - // Re-render our completions page if necessary. We set the term size to 1 less than the true - // term height. This means we will always show the (bottom) line of the prompt. - data->pager.set_term_size(maxi(1, common_get_width()), maxi(1, common_get_height() - 1)); + // Re-render our completions page if necessary. We set the term size to less than the true + // term height by the number of prompt lines. This means we will always show the entire line of + // the prompt. + data->pager.set_term_size(maxi(1, common_get_width()), + maxi(1, common_get_height() - (int)calc_prompt_lines(full_line))); data->pager.update_rendering(&data->current_page_rendering); bool focused_on_pager = data->active_edit_line() == &data->pager.search_field_line; diff --git a/src/screen.cpp b/src/screen.cpp index 3b5950e01..c26142fff 100644 --- a/src/screen.cpp +++ b/src/screen.cpp @@ -346,7 +346,7 @@ static prompt_layout_t calc_prompt_layout(const wchar_t *prompt, prompt_type_t w return prompt_layout; } -static size_t calc_prompt_lines(const wcstring &prompt) { +size_t calc_prompt_lines(const wcstring &prompt) { // Hack for the common case where there's no newline at all. I don't know if a newline can // appear in an escape sequence, so if we detect a newline we have to defer to // calc_prompt_width_and_lines. diff --git a/src/screen.h b/src/screen.h index b7897bf5f..11eb607e4 100644 --- a/src/screen.h +++ b/src/screen.h @@ -240,4 +240,7 @@ class cached_esc_sequences_t { // change by calling `cached_esc_sequences.clear()`. extern cached_esc_sequences_t cached_esc_sequences; +// Calculates the number of prompt lines. +size_t calc_prompt_lines(const wcstring &prompt); + #endif From a87970fbb5061360d9f0168bfc5ecbd9abca9fdc Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sun, 4 Feb 2018 14:14:37 -0800 Subject: [PATCH 35/58] Have the pager use a simple newline count to determine reserved lines When the pager wants to use the full screen to show many options, it reserves space at the top to see the command. Previously it pretended the command was a prompt and engaged the prompt layout mechanism to compute these lines. Instead let's juts count newlines since escape sequences within commands are very rare. --- src/reader.cpp | 9 +++++---- src/screen.cpp | 2 +- src/screen.h | 3 --- 3 files changed, 6 insertions(+), 8 deletions(-) diff --git a/src/reader.cpp b/src/reader.cpp index 90fa55733..6c86c786d 100644 --- a/src/reader.cpp +++ b/src/reader.cpp @@ -434,11 +434,12 @@ static void reader_repaint() { std::vector indents = data->indents; indents.resize(len); - // Re-render our completions page if necessary. We set the term size to less than the true - // term height by the number of prompt lines. This means we will always show the entire line of - // the prompt. + // Re-render our completions page if necessary. Limit the term size of the pager to the true + // term size, minus the number of lines consumed by our string. (Note this doesn't yet consider + // wrapping). + int full_line_count = 1 + std::count(full_line.begin(), full_line.end(), '\n'); data->pager.set_term_size(maxi(1, common_get_width()), - maxi(1, common_get_height() - (int)calc_prompt_lines(full_line))); + maxi(1, common_get_height() - full_line_count)); data->pager.update_rendering(&data->current_page_rendering); bool focused_on_pager = data->active_edit_line() == &data->pager.search_field_line; diff --git a/src/screen.cpp b/src/screen.cpp index c26142fff..3b5950e01 100644 --- a/src/screen.cpp +++ b/src/screen.cpp @@ -346,7 +346,7 @@ static prompt_layout_t calc_prompt_layout(const wchar_t *prompt, prompt_type_t w return prompt_layout; } -size_t calc_prompt_lines(const wcstring &prompt) { +static size_t calc_prompt_lines(const wcstring &prompt) { // Hack for the common case where there's no newline at all. I don't know if a newline can // appear in an escape sequence, so if we detect a newline we have to defer to // calc_prompt_width_and_lines. diff --git a/src/screen.h b/src/screen.h index 11eb607e4..b7897bf5f 100644 --- a/src/screen.h +++ b/src/screen.h @@ -240,7 +240,4 @@ class cached_esc_sequences_t { // change by calling `cached_esc_sequences.clear()`. extern cached_esc_sequences_t cached_esc_sequences; -// Calculates the number of prompt lines. -size_t calc_prompt_lines(const wcstring &prompt); - #endif From 8386a815d3cdac0bf3602f852af56ec4b2ad646a Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sun, 4 Feb 2018 14:17:27 -0800 Subject: [PATCH 36/58] Add updated pager reserved line behavior to changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index aed177a1e..cd5919c08 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -36,6 +36,7 @@ This section is for changes merged to the `major` branch that are not also merge - Typing normal characters while the completion pager is active no longer shows the search field. Instead it enters them into the command line, and ends paging (#2249). - A new input binding `pager-toggle-search` toggles the search field in the completions pager on and off. By default this is bound to control-s. - Slicing $history (in particular, `$history[1]` for the last executed command) is much faster. +- The pager will now show the full command instead of just its last line if the number of completions is large (#4702). ## Other significant changes - Command substitution output is now limited to 10 MB by default (#3822). From 634feac600705a964fd7f4117dc0e35223eeb416 Mon Sep 17 00:00:00 2001 From: Fabian Homborg Date: Sun, 4 Feb 2018 23:24:29 +0100 Subject: [PATCH 37/58] [docs] Remove mention of cached variables from `math` The variable cache was removed in 95162ef19d11cee48f952076d1e1ffa5cdb67383, so this is now wrong. --- doc_src/math.txt | 18 ++---------------- 1 file changed, 2 insertions(+), 16 deletions(-) diff --git a/doc_src/math.txt b/doc_src/math.txt index 179beaa59..29755a3f5 100644 --- a/doc_src/math.txt +++ b/doc_src/math.txt @@ -11,20 +11,6 @@ math [-sN | --scale=N] [--] EXPRESSION The `math` command is based on the MuParser library which is documented here. The stock MuParser does not support the modulo, `%`, operator but fish implements it using integer semantics. -You can, and should, use bare variable names (i.e., without the dollar-sign). If you use the dollar-sign form the fish parser will expand the variable and you will greatly reduce the effectiveness of the expression cache. In other words do this: - -\fish -math x + 1 -\endfish - -rather than this: - -\fish -math $x + 1 -\endfish - -Note that if the variable has more than one element (e.g., `set x 1 2 3`) only the first element is used in the expression when you use the bare var name. But if you instead write `math $x + 1` all three elements will be inserted yielding an invalid expression. - Keep in mind that parameter expansion takes before expressions are evaluated. This can be very useful in order to perform calculations involving shell variables or the output of command substitutions, but it also means that parenthesis and the asterisk glob character have to be escaped or quoted. The `math` command can evaluate multiple expressions separated by commas. The result of each expression is written on a separate line. This means you can evaluate multiple expressions and capture the results in a single invocation just like you can with commands like `string`. See the examples below. @@ -53,7 +39,7 @@ Capture the result of three expressions: \fish $ set x 5 -$ set results (math 'x+x, x*3, x^2') +$ set results (math "$x+$x, $x*3, $x^2") $ set --show results $results: not set in local scope $results: set in global scope, unexported, with 3 elements @@ -69,4 +55,4 @@ Fish 1.x and 2.x releases relied on the `bc` command for handling `math` express You don't need to use `--` before the expression even if it begins with a minus sign which might otherwise be interpreted as an invalid option. If you do insert `--` before the expression it will cause option scanning to stop just like for every other command and it won't be part of the expression. -Note that the modulo operator (`x % y`) is not well defined for floating point arithmetic. Fish rounds down all floating point values to the nearest integer before performing the modulo operation. So `10.5 % 6.1` is `4`. Regardless of what `--scale` value is in effect. +Note that the modulo operator (`x % y`) is not well defined for floating point arithmetic. Fish rounds down all floating point values to the nearest integer before performing the modulo operation. So `10.5 % 6.1` is `4`, regardless of what `--scale` value is in effect. From d1436486e2659ef385ec23a4c58b8fa9d9c1e41d Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sun, 4 Feb 2018 15:03:50 -0800 Subject: [PATCH 38/58] Rename cached_esc_sequences_t to layout_cache_t Preparation for migrating the prompt cache into this struct. --- src/env.cpp | 2 +- src/fish_tests.cpp | 40 ++++++++++++++++++++-------------------- src/screen.cpp | 13 +++---------- src/screen.h | 44 ++++++++++++++++++++++++++++++-------------- 4 files changed, 54 insertions(+), 45 deletions(-) diff --git a/src/env.cpp b/src/env.cpp index a65c1161d..5f94789b1 100644 --- a/src/env.cpp +++ b/src/env.cpp @@ -606,7 +606,7 @@ static void init_curses() { term_has_xn = tigetflag((char *)"xenl") == 1; // does terminal have the eat_newline_glitch update_fish_color_support(); // Invalidate the cached escape sequences since they may no longer be valid. - cached_esc_sequences.clear(); + cached_layouts.clear(); curses_initialized = true; } diff --git a/src/fish_tests.cpp b/src/fish_tests.cpp index 6d86c07d6..eacaed021 100644 --- a/src/fish_tests.cpp +++ b/src/fish_tests.cpp @@ -4333,27 +4333,27 @@ void test_maybe() { } void test_cached_esc_sequences() { - cached_esc_sequences_t seqs; - do_test(seqs.find_entry(L"abc") == 0); - seqs.add_entry(L"abc"); - seqs.add_entry(L"abc"); - do_test(seqs.size() == 1); - do_test(seqs.find_entry(L"abc") == 3); - do_test(seqs.find_entry(L"abcd") == 3); - do_test(seqs.find_entry(L"abcde") == 3); - do_test(seqs.find_entry(L"xabcde") == 0); - seqs.add_entry(L"ac"); - do_test(seqs.find_entry(L"abcd") == 3); - do_test(seqs.find_entry(L"acbd") == 2); - seqs.add_entry(L"wxyz"); - do_test(seqs.find_entry(L"abc") == 3); - do_test(seqs.find_entry(L"abcd") == 3); - do_test(seqs.find_entry(L"wxyz123") == 4); - do_test(seqs.find_entry(L"qwxyz123") == 0); - do_test(seqs.size() == 3); + layout_cache_t seqs; + do_test(seqs.find_escape_code(L"abc") == 0); + seqs.add_escape_code(L"abc"); + seqs.add_escape_code(L"abc"); + do_test(seqs.esc_cache_size() == 1); + do_test(seqs.find_escape_code(L"abc") == 3); + do_test(seqs.find_escape_code(L"abcd") == 3); + do_test(seqs.find_escape_code(L"abcde") == 3); + do_test(seqs.find_escape_code(L"xabcde") == 0); + seqs.add_escape_code(L"ac"); + do_test(seqs.find_escape_code(L"abcd") == 3); + do_test(seqs.find_escape_code(L"acbd") == 2); + seqs.add_escape_code(L"wxyz"); + do_test(seqs.find_escape_code(L"abc") == 3); + do_test(seqs.find_escape_code(L"abcd") == 3); + do_test(seqs.find_escape_code(L"wxyz123") == 4); + do_test(seqs.find_escape_code(L"qwxyz123") == 0); + do_test(seqs.esc_cache_size() == 3); seqs.clear(); - do_test(seqs.size() == 0); - do_test(seqs.find_entry(L"abcd") == 0); + do_test(seqs.esc_cache_size() == 0); + do_test(seqs.find_escape_code(L"abcd") == 0); } /// Main test. diff --git a/src/screen.cpp b/src/screen.cpp index 3b5950e01..a0ef2c51a 100644 --- a/src/screen.cpp +++ b/src/screen.cpp @@ -81,7 +81,7 @@ class scoped_buffer_t { // Singleton of the cached escape sequences seen in prompts and similar strings. // Note this is deliberately exported so that init_curses can clear it. -cached_esc_sequences_t cached_esc_sequences; +layout_cache_t cached_layouts; /// Tests if the specified narrow character sequence is present at the specified position of the /// specified wide character string. All of \c seq must match, but str may be longer than seq. @@ -269,7 +269,7 @@ size_t escape_code_length(const wchar_t *code) { assert(code != NULL); if (*code != L'\e') return 0; - size_t esc_seq_len = cached_esc_sequences.find_entry(code); + size_t esc_seq_len = cached_layouts.find_escape_code(code); if (esc_seq_len) return esc_seq_len; bool found = is_color_escape_seq(code, &esc_seq_len); @@ -279,17 +279,10 @@ size_t escape_code_length(const wchar_t *code) { if (!found) found = is_single_byte_escape_seq(code, &esc_seq_len); if (!found) found = is_csi_style_escape_seq(code, &esc_seq_len); if (!found) found = is_two_byte_escape_seq(code, &esc_seq_len); - if (found) cached_esc_sequences.add_entry(wcstring(code, esc_seq_len)); + if (found) cached_layouts.add_escape_code(wcstring(code, esc_seq_len)); return esc_seq_len; } -// Information about the layout of a prompt. -struct prompt_layout_t { - size_t line_count; // how many lines the prompt consumes - size_t max_line_width; // width of the longest line - size_t last_line_width; // width of the last line -}; - // These are used by `calc_prompt_layout()` to avoid redundant calculations. static const wchar_t *cached_left_prompt = wcsdup(L""); static const wchar_t *cached_right_prompt = wcsdup(L""); diff --git a/src/screen.h b/src/screen.h index b7897bf5f..cb3d0bd81 100644 --- a/src/screen.h +++ b/src/screen.h @@ -16,9 +16,10 @@ #include #include +#include #include -#include #include +#include #include #include "common.h" @@ -199,45 +200,60 @@ bool screen_force_clear_to_end(); /// Returns the length of an escape code. Exposed for testing purposes only. size_t escape_code_length(const wchar_t *code); +// Information about the layout of a prompt. +struct prompt_layout_t { + size_t line_count; // how many lines the prompt consumes + size_t max_line_width; // width of the longest line + size_t last_line_width; // width of the last line +}; + // Maintain a mapping of escape sequences to their length for fast lookup. -class cached_esc_sequences_t { +class layout_cache_t { private: // Cached escape sequences we've already detected in the prompt and similar strings, ordered // lexicographically. - std::vector cache_; + std::vector esc_cache_; + + // LRU-list of prompts and their layouts. + // Use a list so we can promote to the front on a cache hit. + using prompt_layout_pair_t = std::pair; + std::list prompt_cache_; public: - /// \return the size of the cache. - size_t size() const { return cache_.size(); } + /// \return the size of the escape code cache. + size_t esc_cache_size() const { return esc_cache_.size(); } /// Insert the entry \p str in its sorted position, if it is not already present in the cache. - void add_entry(wcstring str) { - auto where = std::upper_bound(cache_.begin(), cache_.end(), str); - if (where == cache_.begin() || where[-1] != str) { - cache_.emplace(where, std::move(str)); + void add_escape_code(wcstring str) { + auto where = std::upper_bound(esc_cache_.begin(), esc_cache_.end(), str); + if (where == esc_cache_.begin() || where[-1] != str) { + esc_cache_.emplace(where, std::move(str)); } } /// \return the length of a string that matches a prefix of \p entry. - size_t find_entry(const wchar_t *entry) const { + size_t find_escape_code(const wchar_t *entry) const { // Do a binary search and see if the escape code right before our entry is a prefix of our // entry. Note this assumes that escape codes are prefix-free: no escape code is a prefix of // another one. This seems like a safe assumption. - auto where = std::upper_bound(cache_.begin(), cache_.end(), entry); + auto where = std::upper_bound(esc_cache_.begin(), esc_cache_.end(), entry); // 'where' is now the first element that is greater than entry. Thus where-1 is the last // element that is less than or equal to entry. - if (where != cache_.begin()) { + if (where != esc_cache_.begin()) { const wcstring &candidate = where[-1]; if (string_prefixes_string(candidate.c_str(), entry)) return candidate.size(); } return 0; } - void clear() { cache_.clear(); } + void clear() { + esc_cache_.clear(); + prompt_cache_.clear(); + } }; // Singleton that is exposed so that the cache can be invalidated when terminal related variables // change by calling `cached_esc_sequences.clear()`. -extern cached_esc_sequences_t cached_esc_sequences; +extern layout_cache_t cached_layouts; #endif From 1bd5946d235a49dafa1b95b6006609a65a457231 Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sun, 4 Feb 2018 15:43:03 -0800 Subject: [PATCH 39/58] Add prompt layout caching to layout_cache_t --- src/fish_tests.cpp | 24 ++++++++++++++++++++++-- src/screen.cpp | 21 +++++++++++++++++++++ src/screen.h | 8 ++++++++ 3 files changed, 51 insertions(+), 2 deletions(-) diff --git a/src/fish_tests.cpp b/src/fish_tests.cpp index eacaed021..d11a33b47 100644 --- a/src/fish_tests.cpp +++ b/src/fish_tests.cpp @@ -4332,8 +4332,10 @@ void test_maybe() { do_test(m2.missing_or_empty()); } -void test_cached_esc_sequences() { +void test_layout_cache() { layout_cache_t seqs; + + // Verify escape code cache. do_test(seqs.find_escape_code(L"abc") == 0); seqs.add_escape_code(L"abc"); seqs.add_escape_code(L"abc"); @@ -4354,6 +4356,24 @@ void test_cached_esc_sequences() { seqs.clear(); do_test(seqs.esc_cache_size() == 0); do_test(seqs.find_escape_code(L"abcd") == 0); + + // Verify prompt layout cache. + for (size_t i = 0; i < layout_cache_t::prompt_cache_max_size; i++) { + wcstring input = std::to_wstring(i); + do_test(!seqs.find_prompt_layout(input)); + seqs.add_prompt_layout(input, {i}); + do_test(seqs.find_prompt_layout(input)->line_count == i); + } + + size_t expected_evictee = 3; + for (size_t i = 0; i < layout_cache_t::prompt_cache_max_size; i++) { + if (i != expected_evictee) + do_test(seqs.find_prompt_layout(std::to_wstring(i))->line_count == i); + } + + seqs.add_prompt_layout(L"whatever", {100}); + do_test(!seqs.find_prompt_layout(std::to_wstring(expected_evictee))); + do_test(seqs.find_prompt_layout(L"whatever")->line_count == 100); } /// Main test. @@ -4452,7 +4472,7 @@ int main(int argc, char **argv) { if (should_test_function("string")) test_string(); if (should_test_function("illegal_command_exit_code")) test_illegal_command_exit_code(); if (should_test_function("maybe")) test_maybe(); - if (should_test_function("cached_esc_sequences")) test_cached_esc_sequences(); + if (should_test_function("layout_cache")) test_layout_cache(); // history_tests_t::test_history_speed(); say(L"Encountered %d errors in low-level tests", err_count); diff --git a/src/screen.cpp b/src/screen.cpp index a0ef2c51a..a63c48278 100644 --- a/src/screen.cpp +++ b/src/screen.cpp @@ -283,6 +283,27 @@ size_t escape_code_length(const wchar_t *code) { return esc_seq_len; } +maybe_t layout_cache_t::find_prompt_layout(const wcstring &input) { + auto start = prompt_cache_.begin(); + auto end = prompt_cache_.end(); + for (auto iter = start; iter != end; ++iter) { + if (iter->first == input) { + // Found it. Move it to the front if not already there. + if (iter != start) prompt_cache_.splice(start, prompt_cache_, iter); + return iter->second; + } + } + return none(); +} + +void layout_cache_t::add_prompt_layout(wcstring input, prompt_layout_t layout) { + assert(!find_prompt_layout(input) && "Should not have a prompt layout for this input"); + prompt_cache_.emplace_front(std::move(input), std::move(layout)); + if (prompt_cache_.size() > prompt_cache_max_size) { + prompt_cache_.pop_back(); + } +} + // These are used by `calc_prompt_layout()` to avoid redundant calculations. static const wchar_t *cached_left_prompt = wcsdup(L""); static const wchar_t *cached_right_prompt = wcsdup(L""); diff --git a/src/screen.h b/src/screen.h index cb3d0bd81..146f60a78 100644 --- a/src/screen.h +++ b/src/screen.h @@ -220,6 +220,8 @@ class layout_cache_t { std::list prompt_cache_; public: + static constexpr size_t prompt_cache_max_size = 8; + /// \return the size of the escape code cache. size_t esc_cache_size() const { return esc_cache_.size(); } @@ -246,6 +248,12 @@ class layout_cache_t { return 0; } + // Finds the layout for a prompt, promoting it to the front. Returns none() if not found. + maybe_t find_prompt_layout(const wcstring &input); + + // Adds a prompt layout for a given string. + void add_prompt_layout(wcstring input, prompt_layout_t layout); + void clear() { esc_cache_.clear(); prompt_cache_.clear(); From 72208a9438ccf92475272c797cd65e02415259ec Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sun, 4 Feb 2018 16:17:57 -0800 Subject: [PATCH 40/58] Use the layout cache instead of static variables for caching prompts This correctly reacts to changes in TERM (which might affect prompt width due to escape code differences), and eliminates some ugly static variables. --- src/screen.cpp | 50 +++++++++++++++----------------------------------- 1 file changed, 15 insertions(+), 35 deletions(-) diff --git a/src/screen.cpp b/src/screen.cpp index a63c48278..fee6cffc2 100644 --- a/src/screen.cpp +++ b/src/screen.cpp @@ -50,8 +50,6 @@ /// A helper value for an invalid location. #define INVALID_LOCATION (screen_data_t::cursor_t(-1, -1)) -enum prompt_type_t { UNKNOWN_PROMPT, LEFT_PROMPT, RIGHT_PROMPT }; - static void invalidate_soft_wrap(screen_t *scr); /// Ugly kludge. The internal buffer used to store output of tputs. Since tputs external function @@ -304,20 +302,11 @@ void layout_cache_t::add_prompt_layout(wcstring input, prompt_layout_t layout) { } } -// These are used by `calc_prompt_layout()` to avoid redundant calculations. -static const wchar_t *cached_left_prompt = wcsdup(L""); -static const wchar_t *cached_right_prompt = wcsdup(L""); -static prompt_layout_t cached_left_prompt_layout = {1, 0, 0}; -static prompt_layout_t cached_right_prompt_layout = {1, 0, 0}; - /// Calculate layout information for the given prompt. Does some clever magic to detect common /// escape sequences that may be embeded in a prompt, such as those to set visual attributes. -static prompt_layout_t calc_prompt_layout(const wchar_t *prompt, prompt_type_t which_prompt) { - if (which_prompt == LEFT_PROMPT && wcscmp(cached_left_prompt, prompt) == 0) { - return cached_left_prompt_layout; - } - if (which_prompt == RIGHT_PROMPT && wcscmp(cached_right_prompt, prompt) == 0) { - return cached_right_prompt_layout; +static prompt_layout_t calc_prompt_layout(const wcstring &prompt, layout_cache_t &cache) { + if (auto cached_layout = cache.find_prompt_layout(prompt)) { + return *cached_layout; } prompt_layout_t prompt_layout = {1, 0, 0}; @@ -345,18 +334,8 @@ static prompt_layout_t calc_prompt_layout(const wchar_t *prompt, prompt_type_t w } } } - prompt_layout.last_line_width = current_line_width; - if (which_prompt == LEFT_PROMPT) { - free((void *)cached_left_prompt); - cached_left_prompt = wcsdup(prompt); - cached_left_prompt_layout = prompt_layout; - } - if (which_prompt == RIGHT_PROMPT) { - free((void *)cached_right_prompt); - cached_right_prompt = wcsdup(prompt); - cached_right_prompt_layout = prompt_layout; - } + cache.add_prompt_layout(prompt, prompt_layout); return prompt_layout; } @@ -366,7 +345,7 @@ static size_t calc_prompt_lines(const wcstring &prompt) { // calc_prompt_width_and_lines. size_t result = 1; if (prompt.find(L'\n') != wcstring::npos || prompt.find(L'\f') != wcstring::npos) { - result = calc_prompt_layout(prompt.c_str(), UNKNOWN_PROMPT).line_count; + result = calc_prompt_layout(prompt, cached_layouts).line_count; } return result; } @@ -638,11 +617,12 @@ static bool perform_any_impending_soft_wrap(screen_t *scr, int x, int y) { static void invalidate_soft_wrap(screen_t *scr) { scr->soft_wrap_location = INVALID_LOCATION; } /// Update the screen to match the desired output. -static void s_update(screen_t *scr, const wchar_t *left_prompt, const wchar_t *right_prompt) { +static void s_update(screen_t *scr, const wcstring &left_prompt, const wcstring &right_prompt) { // if (test_stuff(scr)) return; - const size_t left_prompt_width = calc_prompt_layout(left_prompt, LEFT_PROMPT).last_line_width; + const size_t left_prompt_width = + calc_prompt_layout(left_prompt, cached_layouts).last_line_width; const size_t right_prompt_width = - calc_prompt_layout(right_prompt, RIGHT_PROMPT).last_line_width; + calc_prompt_layout(right_prompt, cached_layouts).last_line_width; int screen_width = common_get_width(); @@ -676,9 +656,9 @@ static void s_update(screen_t *scr, const wchar_t *left_prompt, const wchar_t *r // want. const size_t lines_with_stuff = maxi(actual_lines_before_reset, scr->actual.line_count()); - if (wcscmp(left_prompt, scr->actual_left_prompt.c_str())) { + if (left_prompt != scr->actual_left_prompt) { s_move(scr, &output, 0, 0); - s_write_str(&output, left_prompt); + s_write_str(&output, left_prompt.c_str()); scr->actual_left_prompt = left_prompt; scr->actual.cursor.x = (int)left_prompt_width; } @@ -789,7 +769,7 @@ static void s_update(screen_t *scr, const wchar_t *left_prompt, const wchar_t *r if (i == 0 && right_prompt_width > 0) { //!OCLINT(Use early exit/continue) s_move(scr, &output, (int)(screen_width - right_prompt_width), (int)i); s_set_color(scr, &output, 0xffffffff); - s_write_str(&output, right_prompt); + s_write_str(&output, right_prompt.c_str()); scr->actual.cursor.x += right_prompt_width; // We output in the last column. Some terms (Linux) push the cursor further right, past @@ -875,8 +855,8 @@ static screen_layout_t compute_layout(screen_t *s, size_t screen_width, const wchar_t *right_prompt = right_prompt_str.c_str(); const wchar_t *autosuggestion = autosuggestion_str.c_str(); - prompt_layout_t left_prompt_layout = calc_prompt_layout(left_prompt, LEFT_PROMPT); - prompt_layout_t right_prompt_layout = calc_prompt_layout(right_prompt, RIGHT_PROMPT); + prompt_layout_t left_prompt_layout = calc_prompt_layout(left_prompt_str, cached_layouts); + prompt_layout_t right_prompt_layout = calc_prompt_layout(right_prompt_str, cached_layouts); size_t left_prompt_width = left_prompt_layout.last_line_width; size_t right_prompt_width = right_prompt_layout.last_line_width; @@ -1102,7 +1082,7 @@ void s_write(screen_t *s, const wcstring &left_prompt, const wcstring &right_pro // Append pager_data (none if empty). s->desired.append_lines(pager.screen_data); - s_update(s, layout.left_prompt.c_str(), layout.right_prompt.c_str()); + s_update(s, layout.left_prompt, layout.right_prompt); s_save_status(s); } void s_reset(screen_t *s, screen_reset_mode_t mode) { From 54c9d57e421950aa58b9826611553a9baccbbaa2 Mon Sep 17 00:00:00 2001 From: Benoit Hamon Date: Tue, 6 Feb 2018 17:53:23 +0100 Subject: [PATCH 41/58] Ansible completions (#4697) * :rocket: * prepare to merge into fish-shell * split into different files * remove deprecated option * captitalize descriptions * make shorter description for ansible * update ansible-playbook (and ansible for consistency) * update version on vault and galaxy --- share/completions/ansible-galaxy.fish | 4 +++ share/completions/ansible-playbook.fish | 37 +++++++++++++++++++++++++ share/completions/ansible-vault.fish | 7 +++++ share/completions/ansible.fish | 36 ++++++++++++++++++++++++ 4 files changed, 84 insertions(+) create mode 100644 share/completions/ansible-galaxy.fish create mode 100644 share/completions/ansible-playbook.fish create mode 100644 share/completions/ansible-vault.fish create mode 100644 share/completions/ansible.fish diff --git a/share/completions/ansible-galaxy.fish b/share/completions/ansible-galaxy.fish new file mode 100644 index 000000000..d21ac2971 --- /dev/null +++ b/share/completions/ansible-galaxy.fish @@ -0,0 +1,4 @@ +complete -c ansible-galaxy -a "delete import info init install list login remove search setup" +complete -c ansible-galaxy -s h -l help -d "Show this help message and exit" +complete -c ansible-galaxy -s v -l verbose -d "Verbose mode (-vvv for more, -vvvv to enable connection debugging)" +complete -c ansible-galaxy -l version -d "Display version and exit" diff --git a/share/completions/ansible-playbook.fish b/share/completions/ansible-playbook.fish new file mode 100644 index 000000000..a77ecc5d0 --- /dev/null +++ b/share/completions/ansible-playbook.fish @@ -0,0 +1,37 @@ +complete -c ansible-playbook -l ask-vault-pass -f -d "Ask for vault password" +complete -c ansible-playbook -s C -l check -f -d "Just check, don't make any changes" +complete -c ansible-playbook -s D -l diff -f -d "Show the differences in files and templates; works great with --check" +complete -c ansible-playbook -s e -l extra-vars -r -d "Set additional variables as key=value or YAML/JSON" +complete -c ansible-playbook -l flush-cache -d "Clear the fact cache" +complete -c ansible-playbook -l force-handlers -d "Run handlers even if a task fails" +complete -c ansible-playbook -s f -l forks -a "(seq 0 100)" -d "Number of parallel processes to use (default=5)" +complete -c ansible-playbook -s h -l help -d "Shows a help message" +complete -c ansible-playbook -s i -l inventory -r -d "Specify inventory host path (default=/etc/ansible/hosts) or comma separated host list." +complete -c ansible-playbook -s l -l limit -r -d "Further limit selected hosts to an additional pattern" +complete -c ansible-playbook -l list-hosts -d "List all matching hosts" +complete -c ansible-playbook -l list-tags -d "List all available tags" +complete -c ansible-playbook -l list-tasks -d "List all tasks that would be executed" +complete -c ansible-playbook -s M -l module-path -r -d "Specify path(s) to module library (default=None)" +complete -c ansible-playbook -l new-vault-password-file -f -d "New vault password file for rekey" +complete -c ansible-playbook -l output -f -d "Output file name for encrypt or decrypt; use - for stdout" +complete -c ansible-playbook -l skip-tags -r -d "Only run plays and tasks whose tags do not match these values" +complete -c ansible-playbook -l start-at-task -r -d "Start the playbook at the task matching this name" +complete -c ansible-playbook -l step -d "Confirm each task before running" +complete -c ansible-playbook -l syntax-check -f -d "Perform a syntax check on the playbook" +complete -c ansible-playbook -s t -l tags -r -f -d "Only run plays and tasks tagged with these values" +complete -c ansible-playbook -l vault-password-file -d "Vault password file" +complete -c ansible-playbook -s v -l verbose -f -d "Verbose mode (-vvv for more, -vvvv to enable connection debugging)" +complete -c ansible-playbook -l version -f -d "Display version and exit" +complete -c ansible-playbook -s k -l ask-pass -f -d "Ask for connection password" +complete -c ansible-playbook -l private-key -r -d "Use this file to authenticate the connection" +complete -c ansible-playbook -s u -l user -f -a "(__fish_complete_users)" -d "Connect as this user (default=None)" +complete -c ansible-playbook -s c -l connection -f -d "Connection type to use (default=smart)" +complete -c ansible-playbook -s T -l timeout -f -d "Set the connection timeout in seconds (default=10)" +complete -c ansible-playbook -l ssh-common-args -f -d "Specify common arguments to pass to sftp/scp/ssh" +complete -c ansible-playbook -l sftp-extra-args -f -d "Specify extra arguments to pass to sftp only" +complete -c ansible-playbook -l scp-extra-args -f -d "Specify extra arguments to pass to scp only" +complete -c ansible-playbook -l ssh-extra-args -f -d "Specify extra arguments to pass to ssh only" +complete -c ansible-playbook -s b -l become -f -d "Run operations with become (does not imply password prompting)" +complete -c ansible-playbook -l become-method -r -f -a "sudo su pbrun pfexec runas doas dzdo" -d "Privilege escalation method to use (default=sudo)" +complete -c ansible-playbook -l become-user -r -f -a "(__fish_complete_users)" -d "Run operations as this user (default=root)" +complete -c ansible-playbook -s K -l ask-become-pass -f -d "Ask for privilege escalation password" diff --git a/share/completions/ansible-vault.fish b/share/completions/ansible-vault.fish new file mode 100644 index 000000000..7437ef5b9 --- /dev/null +++ b/share/completions/ansible-vault.fish @@ -0,0 +1,7 @@ +complete -c ansible-vault -l ask-vault-pass -f -d "Ask for vault password" +complete -c ansible-vault -s h -l help -f -d "Show this help message and exit" +complete -c ansible-vault -l new-vault-password-file -r -d "New vault password file for rekey" +complete -c ansible-vault -l output -r -d "Output file name for encrypt or decrypt; use - for stdout" +complete -c ansible-vault -l vault-password-file -r -d "Vault password file" +complete -c ansible-vault -s v -l verbose -d "Verbose mode (-vvv for more, -vvvv to enable connection debugging)" +complete -c ansible-vault -l version -d "Display version and exit" diff --git a/share/completions/ansible.fish b/share/completions/ansible.fish new file mode 100644 index 000000000..9be082136 --- /dev/null +++ b/share/completions/ansible.fish @@ -0,0 +1,36 @@ +complete -c ansible -s a -l args -r -f -d "Module arguments" +complete -c ansible -l ask-vault-pass -f -d "Ask for vault password" +complete -c ansible -s B -l background -r -f -d "Run asynchronously, failing after X seconds" +complete -c ansible -s C -l check -f -d "Just check, don't make any changes" +complete -c ansible -s D -l diff -f -d "Show the differences in files and templates; works great with --check" +complete -c ansible -s e -l extra-vars -r -d "Set additional variables as key=value or YAML/JSON" +complete -c ansible -s f -l forks -a "(seq 0 100)" -d "Number of parallel processes to use (default=5)" +complete -c ansible -s h -l help -d "Shows a help message" +complete -c ansible -s i -l inventory -r -d "Specify inventory host path (default=/etc/ansible/hosts) or comma separated host list." +complete -c ansible -s l -l limit -r -d "Further limit selected hosts to an additional pattern" +complete -c ansible -l limit-hosts -r -d "List all matching hosts" +complete -c ansible -s m -l module-name -r -d "Module name to execute (default=command)" +complete -c ansible -s M -l module-path -r -d "Specify path(s) to module library (default=None)" +complete -c ansible -l new-vault-password-file -f -d "New vault password file for rekey" +complete -c ansible -s o -l one-line -d "Condense output" +complete -c ansible -l output -f -d "Output file name for encrypt or decrypt; use - for stdout" +complete -c ansible -s P --l poll -r -d "Set the poll interval if using -B (default=15)" +complete -c ansible -l syntax-check -f -d "Perform a syntax check on the playbook" +complete -c ansible -s t -l tree -f -a "(__fish_complete_directories)" -d "Log output to this directory" +complete -c ansible -l vault-password-file -d "Vault password file" +complete -c ansible -s v -l verbose -f -d "Verbose mode (-vvv for more, -vvvv to enable connection debugging)" +complete -c ansible -l version -f -d "Display version and exit" +complete -c ansible -s k -l ask-pass -f -d "Ask for connection password" +complete -c ansible -l private-key -r -d "Use this file to authenticate the connection" +complete -c ansible -l key-file -r -d "Use this file to authenticate the connection" +complete -c ansible -s u -l user -f -a "(__fish_complete_users)" -d "Connect as this user (default=None)" +complete -c ansible -s c -l connection -f -d "Connection type to use (default=smart)" +complete -c ansible -s T -l timeout -f -d "Set the connection timeout in seconds (default=10)" +complete -c ansible -l ssh-common-args -f -d "Specify common arguments to pass to sftp/scp/ssh" +complete -c ansible -l sftp-extra-args -f -d "Specify extra arguments to pass to sftp only" +complete -c ansible -l scp-extra-args -f -d "Specify extra arguments to pass to scp only" +complete -c ansible -l ssh-extra-args -f -d "Specify extra arguments to pass to ssh only" +complete -c ansible -s b -l become -f -d "Run operations with become (does not imply password prompting)" +complete -c ansible -l become-method -r -f -a "sudo su pbrun pfexec runas doas dzdo" -d "Privilege escalation method to use (default=sudo)" +complete -c ansible -l become-user -r -f -a "(__fish_complete_users)" -d "Run operations as this user (default=root)" +complete -c ansible -s K -l ask-become-pass -f -d "Ask for privilege escalation password" From aa31fb92f209b89446aaecff10e07e66f89cd8a0 Mon Sep 17 00:00:00 2001 From: Fabian Homborg Date: Tue, 6 Feb 2018 18:36:47 +0100 Subject: [PATCH 42/58] Skip interactive tests on macOS on Travis These just add noise since Travis' macOS machines are apparently hopelessly overloaded. Fixes my sanity. --- tests/interactive.fish | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tests/interactive.fish b/tests/interactive.fish index 87c8717e1..41552f615 100644 --- a/tests/interactive.fish +++ b/tests/interactive.fish @@ -4,6 +4,11 @@ # should be running it via `make test` to ensure the environment is properly # setup. +if test "$TRAVIS_OS_NAME" = osx + echo "Skipping interactive tests on macOS on Travis" + exit 0 +end + # This is a list of flakey tests that often succeed when rerun. set TESTS_TO_RETRY bind.expect From 50bec2c32ea28b74edee585f4382e6ee7bd93013 Mon Sep 17 00:00:00 2001 From: Fabian Homborg Date: Wed, 7 Feb 2018 01:01:18 +0100 Subject: [PATCH 43/58] [docs] Add test example to tutorial As stated in https://stackoverflow.com/questions/48653771/fishshell-checking-equvalancy-like-does-in-most-languages, we didn't have one of those, and `test` is a relatively important part of plenty of `if`s. --- doc_src/tutorial.hdr | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/doc_src/tutorial.hdr b/doc_src/tutorial.hdr index c21277813..84e72cdba 100644 --- a/doc_src/tutorial.hdr +++ b/doc_src/tutorial.hdr @@ -450,6 +450,22 @@ else end \endfish +To check string, number or file properties, use test, like + +\fish{cli-dark} +if test "$fish" = "flounder" + echo FLOUNDER +end + +# or + +if test "$number" -gt 5 + echo $number is greater than five +else + echo $number is five or less +end +\endfish + Combiners can also be used to make more complex conditions, like \fish From 59a90fc543cba1862a695747b50fd06780a7c02a Mon Sep 17 00:00:00 2001 From: Fabian Homborg Date: Wed, 7 Feb 2018 01:12:01 +0100 Subject: [PATCH 44/58] [docs] Reword test example That was a bit too stuffy. [ci skip] --- doc_src/tutorial.hdr | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc_src/tutorial.hdr b/doc_src/tutorial.hdr index 84e72cdba..fe39039c9 100644 --- a/doc_src/tutorial.hdr +++ b/doc_src/tutorial.hdr @@ -450,7 +450,7 @@ else end \endfish -To check string, number or file properties, use test, like +To compare strings or number or check file properties (whether a file exists or is writeable and such), use test, like \fish{cli-dark} if test "$fish" = "flounder" From a04a6d116e6c8000a699fca577358c63fea538ee Mon Sep 17 00:00:00 2001 From: Fabian Homborg Date: Wed, 7 Feb 2018 01:12:30 +0100 Subject: [PATCH 45/58] [docs] PLURALIZE My last commit for the day. --- doc_src/tutorial.hdr | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc_src/tutorial.hdr b/doc_src/tutorial.hdr index fe39039c9..061792af7 100644 --- a/doc_src/tutorial.hdr +++ b/doc_src/tutorial.hdr @@ -450,7 +450,7 @@ else end \endfish -To compare strings or number or check file properties (whether a file exists or is writeable and such), use test, like +To compare strings or numbers or check file properties (whether a file exists or is writeable and such), use test, like \fish{cli-dark} if test "$fish" = "flounder" From cef39cdcc077653354a037ef9e9b0cc832081142 Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sun, 24 Dec 2017 16:29:12 -0800 Subject: [PATCH 46/58] Add is_windows_subsystem_for_linux to detect WSL --- src/common.cpp | 19 +++++++++++++++++++ src/common.h | 3 +++ 2 files changed, 22 insertions(+) diff --git a/src/common.cpp b/src/common.cpp index 4896c2d7a..17da93408 100644 --- a/src/common.cpp +++ b/src/common.cpp @@ -1997,6 +1997,25 @@ void assert_is_locked(void *vmutex, const char *who, const char *caller) { } } +/// Detect if we are Windows Subsystem for Linux by inspecting /proc/sys/kernel/osrelease +/// and checking if "Microsoft" is in the first line. +/// See https://github.com/Microsoft/WSL/issues/423 +bool is_windows_subsystem_for_linux() { + ASSERT_IS_NOT_FORKED_CHILD(); + static bool s_is_wsl = false; + static std::once_flag oflag; + std::call_once(oflag, []() { + // 'e' sets CLOEXEC if possible. + FILE *fp = fopen("/proc/sys/kernel/osrelease", "re"); + if (fp) { + char buff[256]; + if (fgets(buff, sizeof buff, fp)) s_is_wsl = (strstr(buff, "Microsoft") != NULL); + fclose(fp); + } + }); + return s_is_wsl; +} + template static CharType_t **make_null_terminated_array_helper( const std::vector > &argv) { diff --git a/src/common.h b/src/common.h index 1809eab1c..04a53ce76 100644 --- a/src/common.h +++ b/src/common.h @@ -728,6 +728,9 @@ void assert_is_not_forked_child(const char *who); #define ASSERT_IS_NOT_FORKED_CHILD_TRAMPOLINE(x) assert_is_not_forked_child(x) #define ASSERT_IS_NOT_FORKED_CHILD() ASSERT_IS_NOT_FORKED_CHILD_TRAMPOLINE(__FUNCTION__) +/// Return whether we are running in Windows Subsystem for Linux. +bool is_windows_subsystem_for_linux(); + extern "C" { __attribute__((noinline)) void debug_thread_error(void); } From 1b1fd5ab9b7c7ac1b4152f636c63fc425dc3cf5f Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sun, 24 Dec 2017 16:48:00 -0800 Subject: [PATCH 47/58] Mark needs_keepalive more often for WSL Have WSL use a keepalive whenever the first process is external. This works around the fact that WSL prohibits setting an exited process as the group leader. --- src/exec.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/exec.cpp b/src/exec.cpp index 73bab322b..15d8f2660 100644 --- a/src/exec.cpp +++ b/src/exec.cpp @@ -576,6 +576,12 @@ void exec_job(parser_t &parser, job_t *j) { } } + // When running under WSL, create a keepalive process unconditionally if our first process is external. + // This is because WSL does not permit joining the pgrp of an exited process. + // (see https://github.com/Microsoft/WSL/issues/2786), also fish PR #4676 + if (j->processes.front()->type == EXTERNAL && is_windows_subsystem_for_linux()) + needs_keepalive = true; + if (needs_keepalive) { // Call fork. No need to wait for threads since our use is confined and simple. keepalive.pid = execute_fork(false); From e9f676a7f4be1d06c5750ae47ff9b794b783647d Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sun, 24 Dec 2017 12:33:02 -0800 Subject: [PATCH 48/58] Provide a way to stop blocking children via s_block_children This is to investigate alternatives to the existing kill(SIGSTOP) WSL compatibility thing. --- src/exec.cpp | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/exec.cpp b/src/exec.cpp index 15d8f2660..a83e3bdf5 100644 --- a/src/exec.cpp +++ b/src/exec.cpp @@ -53,6 +53,8 @@ /// Base open mode to pass to calls to open. #define OPEN_MASK 0666 +static bool s_block_children = false; + /// Called in a forked child. static void exec_write_and_exit(int fd, const char *buff, size_t count, int status) { if (write_loop(fd, buff, count) == -1) { @@ -750,7 +752,7 @@ void exec_job(parser_t &parser, job_t *j) { // SIGCONT before they've actually stopped, and they'll remain suspended indefinitely. if (blocked_pid != -1) { debug(2, L"Unblocking process %d.\n", blocked_pid); - kill(blocked_pid, SIGCONT); + if (s_block_children) kill(blocked_pid, SIGCONT); blocked_pid = -1; } }; @@ -772,7 +774,7 @@ void exec_job(parser_t &parser, job_t *j) { // down-chain commands in the job a chance to join their process group and read // their pipes. The process will be resumed when the next command in the chain is // started. - if (block_child) { + if (block_child && s_block_children) { kill(p->pid, SIGSTOP); } // The parent will wake us up when we're ready to execute. @@ -1118,7 +1120,7 @@ void exec_job(parser_t &parser, job_t *j) { } bool child_blocked = block_child && child_forked; - if (child_blocked) { + if (child_blocked && s_block_children) { // We have to wait to ensure the child has set their progress group and is in SIGSTOP // state otherwise, we can later call SIGCONT before they call SIGSTOP and they'll be // blocked indefinitely. The child is SIGSTOP'd and is guaranteed to have called From 14880ce7d1ab5ba94811beb12aa2681a25366538 Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Tue, 26 Dec 2017 01:41:04 -0800 Subject: [PATCH 49/58] Resume setting group ID in both parent and child --- src/postfork.cpp | 21 ++++++++------------- 1 file changed, 8 insertions(+), 13 deletions(-) diff --git a/src/postfork.cpp b/src/postfork.cpp index f6d597ded..cdb040af0 100644 --- a/src/postfork.cpp +++ b/src/postfork.cpp @@ -72,24 +72,12 @@ static void debug_safe_int(int level, const char *format, int val) { /// Returns true on sucess, false on failiure. bool child_set_group(job_t *j, process_t *p) { bool retval = true; - if (j->get_flag(JOB_CONTROL)) { // New jobs have the pgid set to -2 if (j->pgid == -2) { j->pgid = p->pid; } - // Retry on EPERM because there's no way that a child cannot join an existing progress group - // because we are SIGSTOPing the previous job in the chain. Sometimes we have to try a few - // times to get the kernel to see the new group. (Linux 4.4.0) - int failure = setpgid(p->pid, j->pgid); - while (failure == -1 && (errno == EPERM || errno == EINTR)) { - debug_safe(4, "Retrying setpgid in child process"); - failure = setpgid(p->pid, j->pgid); - } - // TODO: Figure out why we're testing whether the pgid is correct after attempting to - // set it failed. This was added in commit 4e912ef8 from 2012-02-27. - failure = failure && getpgid(p->pid) != j->pgid; - if (failure) { //!OCLINT(collapsible if statements) + if (setpgid(p->pid, j->pgid) < 0) { char pid_buff[128]; char job_id_buff[128]; char getpgid_buff[128]; @@ -136,6 +124,13 @@ bool set_child_group(job_t *j, pid_t child_pid) { if (j->pgid == -2) { j->pgid = child_pid; } + // The parent sets the child's group. This incurs the well-known unavoidable race with the + // child exiting, so ignore ESRCH and EPERM (in case the pid was recycled). + if (setpgid(child_pid, j->pgid) < 0) { + if (errno != ESRCH && errno != EPERM) { + safe_perror("setpgid"); + } + } } else { j->pgid = getpgrp(); } From 080521071fee874f2b9945b696d0051e8e873d30 Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sat, 20 Jan 2018 23:43:48 -0800 Subject: [PATCH 50/58] Teach keepalives to exit when their parent dies keepalive processes are typically killed by the main shell process. However if the main shell exits the keepalive may linger. In WSL keepalives are used more often, and the lingering keepalives are both leaks and prevent the tests from finishing. Have keepalives poll for their parent process ID and exit when it changes, so they can clean themselves up. The polling frequency can be low. --- src/exec.cpp | 3 ++- src/postfork.cpp | 12 ++++++++++++ src/postfork.h | 3 +++ 3 files changed, 17 insertions(+), 1 deletion(-) diff --git a/src/exec.cpp b/src/exec.cpp index a83e3bdf5..da3a31ee1 100644 --- a/src/exec.cpp +++ b/src/exec.cpp @@ -586,12 +586,13 @@ void exec_job(parser_t &parser, job_t *j) { if (needs_keepalive) { // Call fork. No need to wait for threads since our use is confined and simple. + pid_t parent_pid = getpid(); keepalive.pid = execute_fork(false); if (keepalive.pid == 0) { // Child keepalive.pid = getpid(); child_set_group(j, &keepalive); - pause(); + run_as_keepalive(parent_pid); exit_without_destructors(0); } else { // Parent diff --git a/src/postfork.cpp b/src/postfork.cpp index cdb040af0..c8b4d1028 100644 --- a/src/postfork.cpp +++ b/src/postfork.cpp @@ -546,3 +546,15 @@ bool do_builtin_io(const char *out, size_t outlen, const char *err, size_t errle errno = saved_errno; return success; } + +void run_as_keepalive(pid_t parent_pid) { + // Run this process as a keepalive. In typical usage the parent process will kill us. However + // this may not happen if the parent process exits abruptly, either via kill or exec. What we do + // is poll our ppid() and exit when it differs from parent_pid. We can afford to do this with + // low frequency because in the great majority of cases, fish will kill(9) us. + for (;;) { + // Note sleep is async-safe. + if (sleep(1)) break; + if (getppid() != parent_pid) break; + } +} diff --git a/src/postfork.h b/src/postfork.h index c397e342e..a4119418b 100644 --- a/src/postfork.h +++ b/src/postfork.h @@ -47,6 +47,9 @@ bool do_builtin_io(const char *out, size_t outlen, const char *err, size_t errle void safe_report_exec_error(int err, const char *actual_cmd, const char *const *argv, const char *const *envv); +/// Runs the process as a keepalive, until the parent process given by parent_pid exits. +void run_as_keepalive(pid_t parent_pid); + #if FISH_USE_POSIX_SPAWN /// Initializes and fills in a posix_spawnattr_t; on success, the caller should destroy it via /// posix_spawnattr_destroy. From 0c18f68cc2f595b2fe6f001a77c81a668fb7ba79 Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sun, 21 Jan 2018 00:20:01 -0800 Subject: [PATCH 51/58] Remove support for blocking children This removes support for blocking children via signals, which was used to orchestrate processes on WSL. Now we use the keepalive mechanism instead. --- src/exec.cpp | 104 ++++------------------------------------------- src/postfork.cpp | 12 +----- 2 files changed, 11 insertions(+), 105 deletions(-) diff --git a/src/exec.cpp b/src/exec.cpp index da3a31ee1..de9c24d9b 100644 --- a/src/exec.cpp +++ b/src/exec.cpp @@ -53,8 +53,6 @@ /// Base open mode to pass to calls to open. #define OPEN_MASK 0666 -static bool s_block_children = false; - /// Called in a forked child. static void exec_write_and_exit(int fd, const char *buff, size_t count, int status) { if (write_loop(fd, buff, count) == -1) { @@ -399,13 +397,11 @@ void internal_exec(job_t *j, const io_chain_t &&all_ios) { /// Execute an internal builtin. Given a parser, a job within that parser, and a process within that /// job corresponding to a builtin, execute the builtin with the given streams. If pipe_read is set, -/// assign stdin to it; otherwise infer stdin from the IO chain. unblock_previous is a hack used to -/// prevent jobs from finishing; see commit cdb72b7024. +/// assign stdin to it; otherwise infer stdin from the IO chain. /// return true on success, false if there is an exec error. static bool exec_internal_builtin_proc(parser_t &parser, job_t *j, process_t *p, const io_pipe_t *pipe_read, const io_chain_t &proc_io_chain, - io_streams_t &streams, - const std::function &unblock_previous) { + io_streams_t &streams) { assert(p->type == INTERNAL_BUILTIN && "Process must be a builtin"); int local_builtin_stdin = STDIN_FILENO; bool close_stdin = false; @@ -493,10 +489,6 @@ static bool exec_internal_builtin_proc(parser_t &parser, job_t *j, process_t *p, // make exec handle things. const int fg = j->get_flag(JOB_FOREGROUND); j->set_flag(JOB_FOREGROUND, false); - - // Main loop may need to perform a blocking read from previous command's output. - // Make sure read source is not blocked. - unblock_previous(); p->status = builtin_run(parser, p->get_argv(), streams); // Restore the fg flag, which is temporarily set to false during builtin @@ -616,9 +608,6 @@ void exec_job(parser_t &parser, job_t *j) { // We are careful to set these to -1 when closed, so if we exit the loop abruptly, we can still // close them. bool pgrp_set = false; - // This is static since processes can block on input/output across jobs the main exec_job loop - // is only ever run in a single thread, so this is OK. - static pid_t blocked_pid = -1; int pipe_current_read = -1, pipe_current_write = -1, pipe_next_read = -1; for (std::unique_ptr &unique_p : j->processes) { if (exec_error) { @@ -639,7 +628,6 @@ void exec_job(parser_t &parser, job_t *j) { // Set to true if we end up forking for this process. bool child_forked = false; bool child_spawned = false; - bool block_child = true; // The pipes the current process write to and read from. Unfortunately these can't be just // allocated on the stack, since j->io wants shared_ptr. @@ -745,23 +733,10 @@ void exec_job(parser_t &parser, job_t *j) { // a pipeline. shared_ptr block_output_io_buffer; - // Used to SIGCONT the previously SIGSTOP'd process so the main loop or next command in job - // can read from its output. - auto unblock_previous = [&j]() { - // We've already called waitpid after forking the child, so we've guaranteed that - // they're already SIGSTOP'd. Otherwise we'd be risking a deadlock because we can call - // SIGCONT before they've actually stopped, and they'll remain suspended indefinitely. - if (blocked_pid != -1) { - debug(2, L"Unblocking process %d.\n", blocked_pid); - if (s_block_children) kill(blocked_pid, SIGCONT); - blocked_pid = -1; - } - }; - // This is the io_streams we pass to internal builtins. std::unique_ptr builtin_io_streams(new io_streams_t(stdout_read_limit)); - auto do_fork = [&j, &p, &pid, &exec_error, &process_net_io_chain, &block_child, + auto do_fork = [&j, &p, &pid, &exec_error, &process_net_io_chain, &child_forked](bool drain_threads, const char *fork_type, std::function child_action) -> bool { pid = execute_fork(drain_threads); @@ -769,16 +744,7 @@ void exec_job(parser_t &parser, job_t *j) { // This is the child process. Setup redirections, print correct output to // stdout and stderr, and then exit. p->pid = getpid(); - blocked_pid = -1; child_set_group(j, p); - // Make child processes pause after executing setup_child_process() to give - // down-chain commands in the job a chance to join their process group and read - // their pipes. The process will be resumed when the next command in the chain is - // started. - if (block_child && s_block_children) { - kill(p->pid, SIGSTOP); - } - // The parent will wake us up when we're ready to execute. setup_child_process(p, process_net_io_chain); child_action(); DIE("Child process returned control to do_fork lambda!"); @@ -794,10 +760,8 @@ void exec_job(parser_t &parser, job_t *j) { debug(2, L"Fork #%d, pid %d: %s for '%ls'", g_fork_count, pid, fork_type, p->argv0()); child_forked = true; - if (block_child) { - debug(2, L"Blocking process %d waiting for next command in chain.\n", pid); - } p->pid = pid; + set_child_group(j, p->pid); } return true; @@ -867,7 +831,7 @@ void exec_job(parser_t &parser, job_t *j) { case INTERNAL_BUILTIN: { if (!exec_internal_builtin_proc(parser, j, p, pipe_read.get(), process_net_io_chain, - *builtin_io_streams, unblock_previous)) { + *builtin_io_streams)) { exec_error = true; } break; @@ -953,9 +917,8 @@ void exec_job(parser_t &parser, job_t *j) { bool must_fork = redirection_is_to_real_file(stdout_io.get()) || redirection_is_to_real_file(stderr_io.get()); if (!must_fork && p->is_last_in_job) { - // We are handling reads directly in the main loop. Make sure source is - // unblocked. Note that we may still end up forking. - unblock_previous(); + // We are handling reads directly in the main loop. Note that we may still end + // up forking. const bool stdout_is_to_buffer = stdout_io && stdout_io->io_mode == IO_BUFFER; const bool no_stdout_output = stdout_buffer.empty(); const bool no_stderr_output = stderr_buffer.empty(); @@ -1108,8 +1071,9 @@ void exec_job(parser_t &parser, job_t *j) { } // This is the parent process. Store away information on the child, and possibly - // fice it control over the terminal. + // give it control over the terminal. p->pid = pid; + set_child_group(j, p->pid); break; } @@ -1120,51 +1084,6 @@ void exec_job(parser_t &parser, job_t *j) { } } - bool child_blocked = block_child && child_forked; - if (child_blocked && s_block_children) { - // We have to wait to ensure the child has set their progress group and is in SIGSTOP - // state otherwise, we can later call SIGCONT before they call SIGSTOP and they'll be - // blocked indefinitely. The child is SIGSTOP'd and is guaranteed to have called - // child_set_group() at this point. but we only need to call set_child_group for the - // first process in the group. If needs_keepalive is set, this has already been called - // for the keepalive process. - int result; - int pid_status; - while ((result = waitpid(p->pid, &pid_status, WUNTRACED)) == -1 && errno == EINTR) { - // This could be a superfluous interrupt or Ctrl+C at the terminal In all cases, it - // is OK to retry since the forking code above is specifically designed to never, - // ever hang/block in a child process before the SIGSTOP call is reached. - ; // do nothing - } - if (result == -1) { - exec_error = true; - debug(1, L"waitpid(%d) call in unblock_pid failed:!\n", p->pid); - wperror(L"waitpid"); - break; - } - // We are not unblocking the child via SIGCONT just yet to give the next process a - // chance to open the pipes and join the process group. We only unblock the last process - // in the job chain because no one awaits it. - } - // Regardless of whether the child blocked or not: only once per job, and only once we've - // actually executed an external command. - if ((child_spawned || child_forked) && !pgrp_set) { - // This should be called after waitpid if child_forked and pipes_to_next_command it can - // be called at any time if child_spawned. - set_child_group(j, p->pid); - // we can't rely on p->is_first_in_job because a builtin may have been the first. - pgrp_set = true; - } - // If the command we ran _before_ this one was SIGSTOP'd to let this one catch up, unblock - // it now. this must be after the wait_pid on the process we just started, if any. - unblock_previous(); - - if (child_blocked) { - // Store the newly-blocked command's PID so that it can be SIGCONT'd once the next - // process in the chain is started. That may be in this job or in another job. - blocked_pid = p->pid; - } - // Close the pipe the current process uses to read from the previous process_t. if (pipe_current_read >= 0) { exec_close(pipe_current_read); @@ -1176,11 +1095,6 @@ void exec_job(parser_t &parser, job_t *j) { exec_close(pipe_current_write); pipe_current_write = -1; } - - // Unblock the last process because there's no need for it to stay SIGSTOP'd for anything. - if (p->is_last_in_job) { - unblock_previous(); - } } // Clean up any file descriptors we left open. diff --git a/src/postfork.cpp b/src/postfork.cpp index c8b4d1028..91becfa17 100644 --- a/src/postfork.cpp +++ b/src/postfork.cpp @@ -60,15 +60,7 @@ static void debug_safe_int(int level, const char *format, int val) { } /// Called only by the child to set its own process group (possibly creating a new group in the -/// process if it is the first in a JOB_CONTROL job. The parent will wait for this to finish. -/// A process that isn't already in control of the terminal can't give itself control of the -/// terminal without hanging, but it's not right for the child to try and give itself control -/// from the very beginning because the parent may not have gotten around to doing so yet. Let -/// the parent figure it out; if the child doesn't have terminal control and it later tries to -/// read from the terminal, the kernel will send it SIGTTIN and it'll hang anyway. -/// The key here is that the parent should transfer control of the terminal (if appropriate) -/// prior to sending the child SIGCONT to wake it up to exec. -/// +/// process if it is the first in a JOB_CONTROL job. /// Returns true on sucess, false on failiure. bool child_set_group(job_t *j, process_t *p) { bool retval = true; @@ -100,7 +92,7 @@ bool child_set_group(job_t *j, process_t *p) { retval = false; } } else { - // This is probably stays unused in the child. + // The child does not actualyl use this field. j->pgid = getpgrp(); } From 8de266afb45d6a35a82c912e64d0b8cfcb6782cd Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Fri, 2 Feb 2018 14:31:10 -0800 Subject: [PATCH 52/58] Improve commenting regarding process groups and builtins. --- src/exec.cpp | 2 ++ src/postfork.cpp | 6 +----- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/src/exec.cpp b/src/exec.cpp index de9c24d9b..4e2f3ed95 100644 --- a/src/exec.cpp +++ b/src/exec.cpp @@ -489,6 +489,8 @@ static bool exec_internal_builtin_proc(parser_t &parser, job_t *j, process_t *p, // make exec handle things. const int fg = j->get_flag(JOB_FOREGROUND); j->set_flag(JOB_FOREGROUND, false); + + // Note this call may block for a long time, while the builtin performs I/O. p->status = builtin_run(parser, p->get_argv(), streams); // Restore the fg flag, which is temporarily set to false during builtin diff --git a/src/postfork.cpp b/src/postfork.cpp index 91becfa17..766681ff5 100644 --- a/src/postfork.cpp +++ b/src/postfork.cpp @@ -103,11 +103,7 @@ bool child_set_group(job_t *j, process_t *p) { /// guaranteeing the job control process group has been created and that the child belongs to the /// correct process group. Here we can update our job_t structure to reflect the correct process /// group in the case of JOB_CONTROL, and we can give the new process group control of the terminal -/// if it's to run in the foreground. Note that we can guarantee the child won't try to read from -/// the terminal before we've had a chance to run this code, because we haven't woken them up with a -/// SIGCONT yet. This musn't be called as a part of setup_child_process because that can hang -/// indefinitely until data is available to read/write in the case of IO_FILE, which means we'll -/// never reach our SIGSTOP and everything hangs. +/// if it's to run in the foreground. bool set_child_group(job_t *j, pid_t child_pid) { bool retval = true; From cb03be9fe634aed8247f10bea4b64fde7a3d5c22 Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Wed, 7 Feb 2018 12:54:26 -0800 Subject: [PATCH 53/58] Remove unused 'pgrp_set' variable --- src/exec.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/src/exec.cpp b/src/exec.cpp index 4e2f3ed95..91cf99cd6 100644 --- a/src/exec.cpp +++ b/src/exec.cpp @@ -609,7 +609,6 @@ void exec_job(parser_t &parser, job_t *j) { // // We are careful to set these to -1 when closed, so if we exit the loop abruptly, we can still // close them. - bool pgrp_set = false; int pipe_current_read = -1, pipe_current_write = -1, pipe_next_read = -1; for (std::unique_ptr &unique_p : j->processes) { if (exec_error) { From 82b7e6de691151f9fbd997a0da44b99bb2c4841e Mon Sep 17 00:00:00 2001 From: Mahmoud Al-Qudsi Date: Thu, 8 Feb 2018 16:54:27 -0600 Subject: [PATCH 54/58] Fix unused code (coverity defect #7520283) Due to the logic above, isz cannot be zero if we take the else branch. --- src/common.cpp | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/common.cpp b/src/common.cpp index 17da93408..55a753399 100644 --- a/src/common.cpp +++ b/src/common.cpp @@ -1848,11 +1848,7 @@ void format_size_safe(char buff[128], unsigned long long sz) { if (isz > 9) { append_ull(buff, isz, &idx, max_len); } else { - if (isz == 0) { - append_str(buff, "0", &idx, max_len); - } else { - append_ull(buff, isz, &idx, max_len); - } + append_ull(buff, isz, &idx, max_len); // Maybe append a single fraction digit. unsigned long long remainder = sz % 1024; From 3083e0ea80ca60c500455bf59c848dbcd8a9b489 Mon Sep 17 00:00:00 2001 From: Mahmoud Al-Qudsi Date: Thu, 8 Feb 2018 16:59:38 -0600 Subject: [PATCH 55/58] Work around false positive RESOURCE_LEAK in coverity scan Fixes defect number 7520322 --- src/common.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/common.cpp b/src/common.cpp index 55a753399..4c88ca4fc 100644 --- a/src/common.cpp +++ b/src/common.cpp @@ -313,7 +313,10 @@ char *wcs2str(const wchar_t *in) { // Here we probably allocate a buffer probably much larger than necessary. char *out = (char *)malloc(MAX_UTF8_BYTES * wcslen(in) + 1); assert(out); - return wcs2str_internal(in, out); + //Instead of returning the return value of wcs2str_internal, return `out` directly. + //This eliminates false warnings in coverity about resource leaks. + wcs2str_internal(in, out); + return out; } char *wcs2str(const wcstring &in) { return wcs2str(in.c_str()); } From fbc6c68f3d3955791791bd46d423839113a62b57 Mon Sep 17 00:00:00 2001 From: Mahmoud Al-Qudsi Date: Thu, 8 Feb 2018 17:05:13 -0600 Subject: [PATCH 56/58] Handle error opening /dev/null in `redirect_tty_output` This fixes coverity scan defect number 7520299 --- src/common.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/common.cpp b/src/common.cpp index 4c88ca4fc..98fc57238 100644 --- a/src/common.cpp +++ b/src/common.cpp @@ -2091,6 +2091,9 @@ bool fish_reserved_codepoint(wchar_t c) { void redirect_tty_output() { struct termios t; int fd = open("/dev/null", O_WRONLY); + if (fd == -1) { + __fish_assert("Could not open /dev/null!", __FILE__, __LINE__, errno); + } if (tcgetattr(STDIN_FILENO, &t) == -1 && errno == EIO) dup2(fd, STDIN_FILENO); if (tcgetattr(STDOUT_FILENO, &t) == -1 && errno == EIO) dup2(fd, STDOUT_FILENO); if (tcgetattr(STDERR_FILENO, &t) == -1 && errno == EIO) dup2(fd, STDERR_FILENO); From 6108b1d8139f909292c48e08a0d6249810db619c Mon Sep 17 00:00:00 2001 From: Mahmoud Al-Qudsi Date: Thu, 8 Feb 2018 17:10:51 -0600 Subject: [PATCH 57/58] Assert value of `tok_begin` after call to `parse_util_token_extent` We later assign the value of `tok_begin` to a `wcstring` which would cause a null dereference if `tok_begin` were still null. --- src/complete.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/complete.cpp b/src/complete.cpp index fbb4e4799..00f9e6cd1 100644 --- a/src/complete.cpp +++ b/src/complete.cpp @@ -1264,8 +1264,9 @@ void complete(const wcstring &cmd_with_subcmds, std::vector *out_c // debug( 1, L"Complete '%ls'", cmd.c_str() ); const wchar_t *cmd_cstr = cmd.c_str(); - const wchar_t *tok_begin = NULL, *prev_begin = NULL, *prev_end = NULL; + const wchar_t *tok_begin = nullptr, *prev_begin = nullptr, *prev_end = nullptr; parse_util_token_extent(cmd_cstr, cmd.size(), &tok_begin, NULL, &prev_begin, &prev_end); + assert(tok_begin != nullptr); // If we are completing a variable name or a tilde expansion user name, we do that and return. // No need for any other completions. From 22a67885e1f1a763d21ae124a70b61cb1f9ffb19 Mon Sep 17 00:00:00 2001 From: Mahmoud Al-Qudsi Date: Thu, 8 Feb 2018 17:16:56 -0600 Subject: [PATCH 58/58] Fix memory leak in `term_env` Use wcstring/string instead of a character array. The variable `term_env` was not being freed before the function exited. Fixes defect 7520324 in coverity scan. --- src/env.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/env.cpp b/src/env.cpp index 7fbac6d9a..13f694c0e 100644 --- a/src/env.cpp +++ b/src/env.cpp @@ -548,8 +548,8 @@ static bool initialize_curses_using_fallback(const char *term) { auto term_var = env_get(L"TERM"); if (term_var.missing_or_empty()) return false; - const char *term_env = wcs2str(term_var->as_string()); - if (!strcmp(term_env, DEFAULT_TERM1) || !strcmp(term_env, DEFAULT_TERM2)) return false; + auto term_env = wcs2string(term_var->as_string()); + if (term_env != DEFAULT_TERM1 || term_env != DEFAULT_TERM2) return false; if (is_interactive_session) debug(1, _(L"Using fallback terminal type '%s'."), term);