Shell: Improve the parsing of history event designators

3 files changed, 98 insertions, 72 deletions

diff --git a/Base/usr/share/man/man5/Shell.md b/Base/usr/share/man/man5/Shell.md
index b70cd87096..29d2d041c4 100644
--- a/Base/usr/share/man/man5/Shell.md
+++ b/Base/usr/share/man/man5/Shell.md
@@ -362,20 +362,25 @@ match "$(make_some_value)" {
 History expansion may be utilized to reuse previously typed words or commands.
 Such expressions are of the general form `!<event_designator>(:<word_designator>)`, where `event_designator` would select an entry in the shell history, and `word_designator` would select a word (or a range of words) from that entry.
 
-| Event designator | effect                      |
+| Event designator | Effect                      |
 | :-               | :-----                      |
-| `!`              | Select the immediately preceding command |
-| _n_              | Select the _n_'th entry in the history   |
-| -_n_             | Select the last _n_'th entry in the history |
-| _str_            | Select the most recent entry starting with _str_ |
-| ?_str_           | Select the most recent entry containing _str_ |
+| `!`              | The immediately preceding command |
+| _n_              | The _n_'th entry in the history, starting with 1 as the first entry |
+| -_n_             | The last _n_'th entry in the history, starting with -1 as the previous entry |
+| _str_            | The most recent entry starting with _str_ |
+| `?`_str_         | The most recent entry containing _str_ |
 
-| Word designator  | effect                      |
+| Word designator  | Effect                      |
 | :--              | :-----                      |
-| _n_              | The _n_'th word, starting with 0 as the command |
-| `^`              | The first word (index 0) |
-| `$`              | The last word |
-| _x_-_y_          | The range of words starting at _x_ and ending at _y_ (inclusive) |
+| _n_              | The word at index _n_, starting with 0 as the first word (usually the command) |
+| `^`              | The first argument (index 1) |
+| `$`              | The last argument |
+| _x_-_y_          | The range of words starting at _x_ and ending at _y_ (inclusive). _x_ defaults to 0 if omitted |
+| `*`              | All the arguments. Equivalent to `^`-`$` |
+| _x_`*`           | The range of words starting at _x_ and ending at the last word (`$`) (inclusive) |
+| _x_-             | The range of words starting at _x_ and ending at the second to last word (inclusive). _x_ defaults to 0 if omitted |
+
+Note: The event designator and the word designator should usually be separated by a colon (`:`). This colon can be omitted only if the word designator starts with `^`, `$` or `*` (such as `!1^` for the first argument of the first entry in the history).
 
 ## Formal Grammar
 
diff --git a/Userland/Shell/AST.h b/Userland/Shell/AST.h
index 67adbe367d..d11f591a95 100644
--- a/Userland/Shell/AST.h
+++ b/Userland/Shell/AST.h
@@ -917,7 +917,7 @@ struct HistorySelector {
             if (kind == Index)
                 return selector;
             if (kind == Last)
-                return size - 1;
+                return size - selector - 1;
             VERIFY_NOT_REACHED();
         }
     };
diff --git a/Userland/Shell/Parser.cpp b/Userland/Shell/Parser.cpp
index 843ec639cd..e5cf0f496a 100644
--- a/Userland/Shell/Parser.cpp
+++ b/Userland/Shell/Parser.cpp
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2020, the SerenityOS developers.
+ * Copyright (c) 2020-2021, the SerenityOS developers.
  *
  * SPDX-License-Identifier: BSD-2-Clause
  */
@@ -1589,7 +1589,17 @@ RefPtr<AST::Node> Parser::parse_history_designator()
             nullptr }
     };
 
+    bool is_word_selector = false;
+
     switch (peek()) {
+    case ':':
+        consume();
+        [[fallthrough]];
+    case '^':
+    case '$':
+    case '*':
+        is_word_selector = true;
+        break;
     case '!':
         consume();
         selector.event.kind = AST::HistorySelector::EventKind::IndexFromEnd;
@@ -1601,7 +1611,7 @@ RefPtr<AST::Node> Parser::parse_history_designator()
         selector.event.kind = AST::HistorySelector::EventKind::ContainingStringLookup;
         [[fallthrough]];
     default: {
-        TemporaryChange chars_change { m_extra_chars_not_allowed_in_barewords, { ':' } };
+        TemporaryChange chars_change { m_extra_chars_not_allowed_in_barewords, { ':', '^', '$', '*' } };
 
         auto bareword = parse_bareword();
         if (!bareword || !bareword->is_bareword()) {
@@ -1622,91 +1632,102 @@ RefPtr<AST::Node> Parser::parse_history_designator()
                 selector.event.kind = AST::HistorySelector::EventKind::IndexFromEnd;
             else
                 selector.event.kind = AST::HistorySelector::EventKind::IndexFromStart;
-            auto number = selector.event.text.to_int();
-            if (number.has_value())
-                selector.event.index = abs(number.value());
+            auto number = abs(selector.event.text.to_int().value_or(0));
+            if (number != 0)
+                selector.event.index = number - 1;
             else
                 syntax_error = create<AST::SyntaxError>("History entry index value invalid or out of range");
         }
-        break;
+        if (":^$*"sv.contains(peek())) {
+            is_word_selector = true;
+            if (peek() == ':')
+                consume();
+        }
     }
     }
 
-    if (peek() != ':') {
+    if (!is_word_selector) {
         auto node = create<AST::HistoryEvent>(move(selector));
         if (syntax_error)
             node->set_is_syntax_error(*syntax_error);
         return node;
     }
 
-    consume();
-
     // Word selectors
     auto parse_word_selector = [&]() -> Optional<AST::HistorySelector::WordSelector> {
-        auto rule_start = push_start();
         auto c = peek();
+        AST::HistorySelector::WordSelectorKind word_selector_kind;
+        ssize_t offset = -1;
         if (isdigit(c)) {
             auto num = consume_while(is_digit);
             auto value = num.to_uint();
-            if (!value.has_value()) {
-                return AST::HistorySelector::WordSelector {
-                    AST::HistorySelector::WordSelectorKind::Index,
-                    0,
-                    { m_rule_start_offsets.last(), m_offset, m_rule_start_lines.last(), line() },
-                    syntax_error ? NonnullRefPtr(*syntax_error) : create<AST::SyntaxError>("Word selector value invalid or out of range")
-                };
-            }
-            return AST::HistorySelector::WordSelector {
-                AST::HistorySelector::WordSelectorKind::Index,
-                value.value(),
-                { m_rule_start_offsets.last(), m_offset, m_rule_start_lines.last(), line() },
-                syntax_error
-            };
-        }
-        if (c == '^') {
+            if (!value.has_value())
+                return {};
+            word_selector_kind = AST::HistorySelector::WordSelectorKind::Index;
+            offset = value.value();
+        } else if (c == '^') {
             consume();
-            return AST::HistorySelector::WordSelector {
-                AST::HistorySelector::WordSelectorKind::Index,
-                0,
-                { m_rule_start_offsets.last(), m_offset, m_rule_start_lines.last(), line() },
-                syntax_error
-            };
-        }
-        if (c == '$') {
+            word_selector_kind = AST::HistorySelector::WordSelectorKind::Index;
+            offset = 1;
+        } else if (c == '$') {
             consume();
-            return AST::HistorySelector::WordSelector {
-                AST::HistorySelector::WordSelectorKind::Last,
-                0,
-                { m_rule_start_offsets.last(), m_offset, m_rule_start_lines.last(), line() },
-                syntax_error
-            };
+            word_selector_kind = AST::HistorySelector::WordSelectorKind::Last;
+            offset = 0;
         }
-        return {};
+        if (offset == -1)
+            return {};
+        return AST::HistorySelector::WordSelector {
+            word_selector_kind,
+            static_cast<size_t>(offset),
+            { m_rule_start_offsets.last(), m_offset, m_rule_start_lines.last(), line() },
+            syntax_error
+        };
     };
 
-    auto start = parse_word_selector();
-    if (!start.has_value()) {
+    auto make_word_selector = [&](AST::HistorySelector::WordSelectorKind word_selector_kind, size_t offset) {
+        return AST::HistorySelector::WordSelector {
+            word_selector_kind,
+            offset,
+            { m_rule_start_offsets.last(), m_offset, m_rule_start_lines.last(), line() },
+            syntax_error
+        };
+    };
+
+    auto first_char = peek();
+    if (!(is_digit(first_char) || "^$-*"sv.contains(first_char))) {
         if (!syntax_error)
             syntax_error = create<AST::SyntaxError>("Expected a word selector after ':' in a history event designator", true);
-        auto node = create<AST::HistoryEvent>(move(selector));
-        node->set_is_syntax_error(*syntax_error);
-        return node;
-    }
-    selector.word_selector_range.start = start.release_value();
-
-    if (peek() == '-') {
+    } else if (first_char == '*') {
         consume();
-        auto end = parse_word_selector();
-        if (!end.has_value()) {
-            if (!syntax_error)
-                syntax_error = create<AST::SyntaxError>("Expected a word selector after '-' in a history event designator word selector", true);
-            auto node = create<AST::HistoryEvent>(move(selector));
-            node->set_is_syntax_error(*syntax_error);
-            return node;
-        }
-        selector.word_selector_range.end = move(end);
+        selector.word_selector_range.start = make_word_selector(AST::HistorySelector::WordSelectorKind::Index, 1);
+        selector.word_selector_range.end = make_word_selector(AST::HistorySelector::WordSelectorKind::Last, 0);
+    } else if (first_char == '-') {
+        consume();
+        selector.word_selector_range.start = make_word_selector(AST::HistorySelector::WordSelectorKind::Index, 0);
+        auto last_selector = parse_word_selector();
+        if (!last_selector.has_value())
+            selector.word_selector_range.end = make_word_selector(AST::HistorySelector::WordSelectorKind::Last, 1);
+        else
+            selector.word_selector_range.end = last_selector.release_value();
     } else {
-        selector.word_selector_range.end.clear();
+        auto first_selector = parse_word_selector();
+        // peek() should be a digit, ^, or $ here, so this should always have value.
+        VERIFY(first_selector.has_value());
+        selector.word_selector_range.start = first_selector.release_value();
+        if (peek() == '-') {
+            consume();
+            auto last_selector = parse_word_selector();
+            if (last_selector.has_value()) {
+                selector.word_selector_range.end = last_selector.release_value();
+            } else {
+                selector.word_selector_range.end = make_word_selector(AST::HistorySelector::WordSelectorKind::Last, 1);
+            }
+        } else if (peek() == '*') {
+            consume();
+            selector.word_selector_range.end = make_word_selector(AST::HistorySelector::WordSelectorKind::Last, 0);
+        } else {
+            selector.word_selector_range.end.clear();
+        }
     }
 
     auto node = create<AST::HistoryEvent>(move(selector));