From 45b85d28bb484f31e316c7eaf7c996f9e1b15dc6 Mon Sep 17 00:00:00 2001
From: ridiculousfish <rf@fishshell.com>
Date: Sat, 26 Sep 2020 13:39:47 -0700
Subject: [PATCH] Completion wrap chain visited set to store only wrapped
 command

The "wrap chain" refers to a sequence of commands which wrap other
commands, for completion purposes. One possibility is that a wrap chain
will produce a combinatorial explosion or even an infinite loop, so there
needs to be logic to prevent that. Part of that logic is encapsulated in a
visited set (wrap_chain_visited_set_t) to prevent exploring the same item
twice.

Prior to this change, we stored pairs (command, wrapped_command). But we
only really need to store the wrapped command. Switch to that.

One consequence is that if a command wraps another command in more than
one way, we won't explore both ways. This seems unlikely in practice.
---
 src/complete.cpp        | 27 +++++++++++++--------------
 tests/checks/wraps.fish |  6 ++++++
 2 files changed, 19 insertions(+), 14 deletions(-)

diff --git a/src/complete.cpp b/src/complete.cpp
index 311563ef0..7de4638ae 100644
--- a/src/complete.cpp
+++ b/src/complete.cpp
@@ -377,8 +377,8 @@ class completer_t {
         return result;
     }
 
-    // A set tracking which (command, wrap) pairs we have seen.
-    using wrap_chain_visited_set_t = std::set<std::pair<wcstring, wcstring>>;
+    // A set tracking the wrapped commands which we have seen.
+    using wrap_chain_visited_set_t = std::set<wcstring>;
 
     void complete_custom(const wcstring &cmd, const wcstring &cmdline,
                          const wcstring &previous_argument, const wcstring &current_argument,
@@ -1418,18 +1418,17 @@ void completer_t::walk_wrap_chain(const wcstring &command_line, source_range_t c
         // imagine the first token being 'builtin' or similar. Nevertheless that is simpler than
         // re-parsing everything.
         wcstring wrapped_command = tok_first(wt);
-        if (!wrapped_command.empty()) {
-            size_t where = faux_commandline.find(wrapped_command, command_range.start);
-            if (where != wcstring::npos) {
-                // Do not recurse if we have already seen this.
-                if (visited->insert({command, wrapped_command}).second) {
-                    // Recurse with our new command and command line.
-                    source_range_t faux_source_range{uint32_t(where),
-                                                     uint32_t(wrapped_command.size())};
-                    walk_wrap_chain(faux_commandline, faux_source_range, previous_argument,
-                                    current_argument, had_ddash, visited, depth + 1, do_file);
-                }
-            }
+        if (wrapped_command.empty()) continue;
+
+        size_t where = faux_commandline.find(wrapped_command, command_range.start);
+        assert(where != wcstring::npos &&
+               "Should have found the wrapped command since we inserted it");
+
+        // Recurse with our new command and command line, if we have not seen this wrap before.
+        if (visited->insert(wrapped_command).second) {
+            source_range_t faux_source_range{uint32_t(where), uint32_t(wrapped_command.size())};
+            walk_wrap_chain(faux_commandline, faux_source_range, previous_argument,
+                            current_argument, had_ddash, visited, depth + 1, do_file);
         }
     }
 }
diff --git a/tests/checks/wraps.fish b/tests/checks/wraps.fish
index 98c014ba5..09f2cbae4 100644
--- a/tests/checks/wraps.fish
+++ b/tests/checks/wraps.fish
@@ -14,6 +14,12 @@ complete -C'testcommand '
 # We get the same completion twice. TODO: fix this.
 # CHECK: normal
 
+# Test double wraps.
+complete -c testcommand0 -x -a crosswalk
+complete -c testcommand1 -x --wraps testcommand0
+complete -c testcommand2 -x --wraps testcommand1
+complete -C 'testcommand 0'
+# CHECK: crosswalk
 
 # This tests that a call to complete from within a completion doesn't trigger
 # wrap chain explosion - #5638 again.