Fix history ranges starting past the end

CHANGELOG.md

@@ -1,3 +1,34 @@
+## 4.4.0 (TBD)
+
+- Breaking Changes
+    - `History.get()` and `History.span()` now raise `HistoryIndexError`, an `IndexError` subclass,
+      for invalid indices and bounded ranges selecting no history items. Unbounded ranges (`:` and
+      `..`) can still return an empty dictionary. Starts before the beginning of history are
+      rejected rather than clamped. Malformed ranges, including zero endpoints, raise
+      `HistoryFormatError`, a `ValueError` subclass. The `history` command reports these selection
+      errors without a traceback hint, and index zero includes the reminder that the first command
+      is command 1.
+    - End-only ranges now start at command 1 in the full history, including persisted commands, just
+      like explicit-start ranges and single indices. Bare `history` and unbounded ranges still
+      select only the current session unless `--all` is supplied.
+    - Replaying or editing a selection containing persisted commands now requires `--all`. This
+      includes single indices and bounded ranges. Displaying or saving these selections does not
+      require `--all`, and command numbers are unchanged.
+    - Running, editing, or saving an empty history selection now fails before replay, opening an
+      editor, or creating or overwriting a file. This includes `history -o FILE` and `history -e`
+      with no selector in an empty session, and actions on searches with no matches. Empty unbounded
+      selections report `No history items in range`, while search misses report
+      `No matching history items`; displaying them still succeeds without output. The no-argument
+      `history --run` safety refusal is unchanged.
+- Bug Fixes
+    - Fixed history ranges starting past the end selecting the last command. Invalid indices and
+      bounded ranges selecting no items report `No history items in range` before running, editing,
+      or saving commands.
+    - Negative single-index history selections now display and return the positive command number.
+    - History regular expressions containing range separators are now handled as regex searches.
+      Other selectors are treated as numbers or ranges only when they match the supported syntax;
+      strings such as URLs, `a:b`, `+1`, and `1_0` remain literal searches.
+
 ## 4.3.1 (TBD)
 
 - Bug Fixes

cmd2/cmd2.py

@@ -153,7 +153,11 @@
     SkipPostcommandHooks,
 )
 from .history import (
+    NO_HISTORY_ITEMS,
+    NO_MATCHING_HISTORY_ITEMS,
     History,
+    HistoryFormatError,
+    HistoryIndexError,
     HistoryItem,
 )
 from .parsing import (
@@ -5446,13 +5450,14 @@ def _build_history_parser(cls) -> Cmd2ArgumentParser:
             "-a",
             "--all",
             action="store_true",
-            help="display all commands, including ones persisted from previous sessions",
+            help="include persisted commands; required to run or edit commands from previous sessions",
         )
 
         history_arg_help = (
-            "empty               all history items\n"
-            "a                   one history item by number\n"
-            "a..b, a:b, a:, ..b  items by indices (inclusive)\n"
+            "empty               current session (--all includes persisted commands)\n"
+            "a                   one item by number in the full history\n"
+            "a..b, a:b, a:, ..b  inclusive ranges in the full history\n"
+            ":, ..               current session (--all includes persisted commands)\n"
             "string              items containing string\n"
             "/regex/             items matching regular expression"
         )
@@ -5498,18 +5503,33 @@ def do_history(self, args: argparse.Namespace) -> bool | None:
 
             return None
 
+        if args.run and not args.arg:
+            self.perror("Cowardly refusing to run all previously entered commands.")
+            self.perror("If this is what you want to do, specify '1:' as the range of history.")
+            return None
+
         # If an argument was supplied, then retrieve partial contents of the history, otherwise retrieve it all
-        history = self._get_history(args)
+        try:
+            history = self._get_history(args)
+        except (HistoryIndexError, HistoryFormatError) as ex:
+            self.perror(str(ex))
+            return None
+
+        # Selection is independent of the requested action. Empty displays are harmless.
+        if not history and (args.run or args.edit or args.output_file):
+            is_search = args.arg and not self.history.spanpattern.fullmatch(args.arg)
+            self.perror(NO_MATCHING_HISTORY_ITEMS if is_search else NO_HISTORY_ITEMS)
+            return None
+
+        if (args.run or args.edit) and not args.all and any(index <= self.history.session_start_index for index in history):
+            self.perror("Use --all to run or edit commands from previous sessions.")
+            return None
 
         if args.run:
-            if not args.arg:
-                self.perror("Cowardly refusing to run all previously entered commands.")
-                self.perror("If this is what you want to do, specify '1:' as the range of history.")
-            else:
-                stop = self.runcmds_plus_hooks(list(history.values()))
-                self.last_result = True
-                return stop
-        elif args.edit:
+            stop = self.runcmds_plus_hooks(list(history.values()))
+            self.last_result = True
+            return stop
+        if args.edit:
             fd, fname = tempfile.mkstemp(suffix=".txt", text=True)
             fobj: TextIO
             with os.fdopen(fd, "w") as fobj:
@@ -5550,26 +5570,28 @@ def do_history(self, args: argparse.Namespace) -> bool | None:
     def _get_history(self, args: argparse.Namespace) -> dict[int, HistoryItem]:
         """If an argument was supplied, then retrieve partial contents of the history; otherwise retrieve entire history.
 
-        This function returns a dictionary with history items keyed by their 1-based index in ascending order.
+        This function returns a dictionary with history items keyed by their positive 1-based index in ascending order.
+        Index and range errors propagate to the caller. Empty selections return an empty dictionary.
         """
         if args.arg:
-            try:
-                int_arg = int(args.arg)
-                return {int_arg: self.history.get(int_arg)}
-            except ValueError:
-                pass
-
-            if ".." in args.arg or ":" in args.arg:
-                # Get a slice of history
-                history = self.history.span(args.arg, args.all)
-            elif args.arg.startswith(r"/") and args.arg.endswith(r"/"):
-                history = self.history.regex_search(args.arg, args.all)
-            else:
-                history = self.history.str_search(args.arg, args.all)
-        else:
-            # Get a copy of the history so it doesn't get mutated while we are using it
-            history = self.history.span(":", args.all)
-        return history
+            if args.arg.startswith(r"/") and args.arg.endswith(r"/"):
+                return self.history.regex_search(args.arg, args.all)
+            if re.fullmatch(r"-?[0-9]+", args.arg):
+                try:
+                    index = int(args.arg)
+                except ValueError:
+                    # Python may reject integer strings exceeding its conversion limit.
+                    raise HistoryIndexError(NO_HISTORY_ITEMS) from None
+                if index == 0:
+                    # Preserve the dedicated index-zero error and command-one reminder.
+                    self.history.get(index)
+                return self.history.span(f"{index}:{index}")
+            if self.history.spanpattern.fullmatch(args.arg):
+                return self.history.span(args.arg, args.all)
+            return self.history.str_search(args.arg, args.all)
+
+        # Get a copy of the history so it doesn't get mutated while we are using it.
+        return self.history.span(":", args.all)
 
     def _initialize_history(self, hist_file: str) -> None:
         """Initialize history using history related attributes.

cmd2/history.py

@@ -18,6 +18,11 @@
     shlex_split,
 )
 
+NO_HISTORY_ITEMS = "No history items in range"
+NO_MATCHING_HISTORY_ITEMS = "No matching history items"
+FIRST_HISTORY_INDEX = "The first command in history is command 1."
+INVALID_HISTORY_RANGE = "History indices must be positive or negative integers, and may not be zero."
+
 
 def single_line_format(statement: Statement) -> str:
     """Format a command line to display on a single line.
@@ -131,6 +136,14 @@ def from_dict(source_dict: dict[str, Any]) -> "HistoryItem":
         return HistoryItem(Statement.from_dict(statement_dict))
 
 
+class HistoryIndexError(IndexError):
+    """Raised when a history index is invalid or a bounded range selects no items."""
+
+
+class HistoryFormatError(ValueError):
+    """Raised when a history range has invalid syntax or a zero endpoint."""
+
+
 class History(list[HistoryItem]):
     """A list of [HistoryItem][cmd2.history.HistoryItem] objects with additional methods for searching and managing the list.
 
@@ -190,46 +203,30 @@ def get(self, index: int) -> HistoryItem:
 
         :param index: optional item to get
         :return: a single [cmd2.history.HistoryItem][]
+        :raises HistoryIndexError: if index is zero or outside the history
         """
         if index == 0:
-            raise IndexError("The first command in history is command 1.")
+            raise HistoryIndexError(f"{NO_HISTORY_ITEMS}. {FIRST_HISTORY_INDEX}")
+        if abs(index) > len(self):
+            raise HistoryIndexError(NO_HISTORY_ITEMS)
         if index < 0:
             return self[index]
         return self[index - 1]
 
-    # This regular expression parses input for the span() method. There are five parts:
-    #
-    #    ^\s*                          matches any whitespace at the beginning of the
-    #                                  input. This is here so you don't have to trim the input
-    #
-    #    (?P<start>-?[1-9]{1}\d*)?     create a capture group named 'start' which matches an
-    #                                  optional minus sign, followed by exactly one non-zero
-    #                                  digit, and as many other digits as you want. This group
-    #                                  is optional so that we can match an input string like '..2'.
-    #                                  This regex will match 1, -1, 10, -10, but not 0 or -0.
-    #
-    #    (?P<separator>:|(\.{2,}))?    create a capture group named 'separator' which matches either
-    #                                  a colon or two periods.
-    #
-    #    (?P<end>-?[1-9]{1}\d*)?       create a capture group named 'end' which matches an
-    #                                  optional minus sign, followed by exactly one non-zero
-    #                                  digit, and as many other digits as you want. This group is
-    #                                  optional so that we can match an input string like ':'
-    #                                  or '5:'. This regex will match 1, -1, 10, -10, but not
-    #                                  0 or -0.
-    #
-    #    \s*$                          match any whitespace at the end of the input. This is here so
-    #                                  you don't have to trim the input
-    #
-    spanpattern = re.compile(r"^\s*(?P<start>-?[1-9]\d*)?(?P<separator>:|(\.{2,}))(?P<end>-?[1-9]\d*)?\s*$")
+    # Recognize ranges with optional signed ASCII integer endpoints. Zero is parsed so that
+    # span() can report its specific error; it is never a valid history index.
+    spanpattern = re.compile(r"\s*(?P<start>-?[0-9]+)?(?P<separator>:|\.{2,})(?P<end>-?[0-9]+)?\s*")
 
     def span(self, span: str, include_persisted: bool = False) -> dict[int, "HistoryItem"]:
         """Return a slice of the History list.
 
         :param span: string containing an index or a slice
-        :param include_persisted: if True, then retrieve full results including from persisted history
+        :param include_persisted: if True, include persisted history for unbounded ranges.
+                                  Ranges with either endpoint always use the full history.
         :return: a dictionary of history items keyed by their 1-based index in ascending order,
-                 or an empty dictionary if no results were found
+                 or an empty dictionary if an unbounded range (``:`` or ``..``) has no results
+        :raises HistoryFormatError: if the range format is invalid or an index is zero
+        :raises HistoryIndexError: if the start is out of bounds or a bounded range selects no history items
 
         This method can accommodate input in any of these forms:
 
@@ -249,27 +246,34 @@ def span(self, span: str, include_persisted: bool = False) -> dict[int, "History
         - off by one errors
 
         """
-        results = self.spanpattern.search(span)
+        results = self.spanpattern.fullmatch(span)
         if not results:
             # our regex doesn't match the input, bail out
-            raise ValueError("History indices must be positive or negative integers, and may not be zero.")
+            raise HistoryFormatError(INVALID_HISTORY_RANGE)
 
         start_token = results.group("start")
+        end_token = results.group("end")
+        if any(token is not None and int(token) == 0 for token in (start_token, end_token)):
+            raise HistoryFormatError(f"{INVALID_HISTORY_RANGE} {FIRST_HISTORY_INDEX}")
         if start_token:
-            start = min(self._zero_based_index(start_token), len(self) - 1)
+            start = self._zero_based_index(start_token)
             if start < 0:
-                start = max(0, len(self) + start)
+                start += len(self)
+                if start < 0:
+                    raise HistoryIndexError(NO_HISTORY_ITEMS)
         else:
-            start = 0 if include_persisted else self.session_start_index
+            start = 0 if include_persisted or end_token else self.session_start_index
 
-        end_token = results.group("end")
         if end_token:
             end = min(int(end_token), len(self))
             if end < 0:
                 end = max(0, len(self) + end + 1)
         else:
             end = len(self)
 
+        if (start_token or end_token) and (start >= len(self) or end <= start):
+            raise HistoryIndexError(NO_HISTORY_ITEMS)
+
         return self._build_result_dictionary(start, end)
 
     def str_search(self, search: str, include_persisted: bool = False) -> dict[int, "HistoryItem"]:

docs/features/history.md

@@ -59,7 +59,8 @@ that saved input. The examples to follow all assume that you have entered the fo
     Alias 'four' created
 
 In its simplest form, the `history` command displays previously entered commands. With no additional
-arguments, it displays all previously entered commands:
+arguments, it displays commands entered in the current session. Use `history --all` to also display
+commands loaded from a persistent history file:
 
     (Cmd) history
         1  alias create one !echo one
@@ -87,8 +88,10 @@ separated by `..` or `:`, and you will see all commands between, and including,
         2  alias create two !echo two
         3  alias create three !echo three
 
-If you omit the first number, it will start at the beginning. If you omit the last number, it will
-continue to the end:
+Command numbers refer to the full history, including commands loaded from a persistent history file.
+A single number or a range with either endpoint can select those commands without `--all`. If you
+omit the first number, the range starts at command 1. If you omit the last number, it continues to
+the end of the full history:
 
     (Cmd) history :2
         1  alias create one !echo one
@@ -98,6 +101,11 @@ continue to the end:
         3  alias create three !echo three
         4  alias create four !echo four
 
+With both endpoints omitted (`:` or `..`), only the current session is selected, as with plain
+`history`. Use `--all` to include persisted commands in these unbounded selections or in searches.
+For example, after loading five persisted commands into a fresh session, `history :5` and
+`history 1:5` both display commands 1 through 5, while `history :` displays nothing.
+
 If you want to display the last three commands entered:
 
     (Cmd) history -- -3:
@@ -116,7 +124,9 @@ an argument:
 
 There is no zeroth command, so don't ask for it. If you are a Python programmer, you've probably
 noticed this looks a lot like the slice syntax for lists and arrays. It is, with the exception that
-the first history command is 1, where the first element in a Python array is 0.
+the first history command is 1, where the first element in a Python array is 0. A start before the
+first command or after the last command is an error. An end after the last command stops at the end
+of history.
 
 Besides selecting previous commands by number, you can also search for them. You can use a simple
 string search:
@@ -132,6 +142,10 @@ Or a regular expression search by enclosing your regex in slashes:
 If your regular expression contains any characters that `argparse` finds interesting, like dash or
 plus, you also need to enclose your regular expression in quotation marks.
 
+Regular expressions are recognized before ranges, so `/a..b/` and `/one:/` are regex searches.
+Numbers use ASCII digits with an optional leading minus sign. Other arguments, such as `http://x`,
+`a:b`, `+1`, and `1_0`, are literal searches rather than command numbers or ranges.
+
 This all sounds great, but doesn't it seem like a bit of overkill to have all these ways to select
 commands if all we can do is display them? Turns out, displaying history commands is just the
 beginning. The history command can perform many other actions:
@@ -160,13 +174,29 @@ file are executed. To edit and then re-run commands 2-4 you would:
 
     (Cmd) history --edit 2:4
 
+Running or editing a selection that includes commands from previous sessions requires `--all`, even
+when selecting a single command or a bounded range. For example, use `history --all --run 1` to
+replay a persisted command 1. Without `--all`, the entire action is refused if any selected command
+is persisted; commands are not silently filtered out or renumbered. Current-session-only selections
+do not require `--all`.
+
 If you want to save the commands to a text file, but not edit and re-run them, use the `-o` or
 `--output-file` option. This is a great way to create [Scripts](./scripting.md), which can be
-executed using the `run_script` command. To save the first 5 commands entered in this session to a
-text file:
+executed using the `run_script` command. To save commands 1 through 5 from the full history,
+including persisted commands, to a text file:
 
     (Cmd) history :5 -o history.txt
 
+To save only commands entered in the current session, use `history -o history.txt`. Running,
+editing, or saving an empty selection reports an error before executing commands, opening an editor,
+or creating or overwriting a file. Empty searches report `No matching history items` for these
+actions, while empty selections with no selector or an unbounded range report
+`No history items in range`. Displaying these empty selections succeeds without output. Invalid
+indices and bounded ranges selecting no commands also report `No history items in range`. Index zero
+and zero range endpoints include a reminder that the first command is command 1. The
+`History.span()` API reports malformed ranges as format errors; the command treats arguments that do
+not match range syntax as literal searches.
+
 The last action the history command can perform is to clear the command history using `-c` or
 `--clear`: