From 663919228b1009a0475defc1d8fd86af93788945 Mon Sep 17 00:00:00 2001
From: Mahmoud Al-Qudsi <mqudsi@neosmart.net>
Date: Tue, 20 Sep 2022 21:49:30 -0500
Subject: [PATCH] Fix stomping of last_option_requires_param

This flag determines whether or not more shortopt switches will be offered up as
potential completions (vs only the payload for the last-parsed shortopt switch).

Previously, it was being stomped before it was determined whether or not two
`complete` rules with different `result_mode.requires_param` values were
actually resolved against the current command line or not, and the last
evaluated completion rule would win out.

There are two changes here:
* `last_option_requires_param` is only assigned if all associated conditions for
  a potential completion are also met, and
* If already assigned by a conflicting rule (which can only be user/developer
  error), `last_option_requires_param` is allowed to change from true to false
  but not the other way around (i.e. in case of a conflict, generate both
  payloads and other shortopt completions)

The first change is immediately noticeable and affects many of our own
completions, see the discussion in #9221 for an example regarding `git` where
`-c` has any of about a million different possible meanings depending on which
completion preconditions have been met. The second change should only happen if
a dev/user mistakenly enters a `complete -c ...` rule for the same shortopt more
than once, both with conditions matching, sometimes requiring an argument and
not sometimes not. It should be a rare occurence.
---
 src/complete.cpp | 35 +++++++++++++++++++++++++++--------
 1 file changed, 27 insertions(+), 8 deletions(-)

diff --git a/src/complete.cpp b/src/complete.cpp
index 85d3cba2f..f6d40b78e 100644
--- a/src/complete.cpp
+++ b/src/complete.cpp
@@ -856,7 +856,10 @@ bool completer_t::complete_param_for_command(const wcstring &cmd_orig, const wcs
     // the lock because callouts (like the condition) may add or remove completions. See issue 2.
     for (const option_list_t &options : all_options) {
         size_t short_opt_pos = short_option_pos(str, options);
-        bool last_option_requires_param = false, use_common = true;
+        // We want last_option_requires_param to default to false but distinguish between when
+        // a previous completion has set it to false and when it has its default value.
+        maybe_t<bool> last_option_requires_param = none();
+        bool use_common = true;
         if (use_switches) {
             if (str[0] == L'-') {
                 // Check if we are entering a combined option and argument (like --color=auto or
@@ -866,16 +869,27 @@ bool completer_t::complete_param_for_command(const wcstring &cmd_orig, const wcs
                     if (o.type == option_type_short) {
                         if (short_opt_pos == wcstring::npos) continue;
                         if (o.option.at(0) != str.at(short_opt_pos)) continue;
-                        last_option_requires_param = o.result_mode.requires_param;
                         arg = str.c_str() + short_opt_pos + 1;
                     } else {
                         arg = param_match2(&o, str.c_str());
                     }
-                    if (arg != nullptr && this->condition_test(o.condition)) {
-                        if (o.result_mode.requires_param) use_common = false;
-                        if (o.result_mode.no_files) use_files = false;
-                        if (o.result_mode.force_files) has_force = true;
-                        complete_from_args(arg, o.comp, o.localized_desc(), o.flags);
+
+                    if (this->condition_test(o.condition)) {
+                        if (o.type == option_type_short) {
+                            // Only override a true last_option_requires_param value with a false one
+                            if (last_option_requires_param.has_value()) {
+                                last_option_requires_param =
+                                    last_option_requires_param && o.result_mode.requires_param;
+                            } else {
+                                last_option_requires_param = o.result_mode.requires_param;
+                            }
+                        }
+                        if (arg != nullptr) {
+                            if (o.result_mode.requires_param) use_common = false;
+                            if (o.result_mode.no_files) use_files = false;
+                            if (o.result_mode.force_files) has_force = true;
+                            complete_from_args(arg, o.comp, o.localized_desc(), o.flags);
+                        }
                     }
                 }
             } else if (popt[0] == L'-') {
@@ -932,6 +946,11 @@ bool completer_t::complete_param_for_command(const wcstring &cmd_orig, const wcs
             continue;
         }
 
+        // Set a default value for last_option_requires_param only if one hasn't been set
+        if (!last_option_requires_param.has_value()) {
+            last_option_requires_param = false;
+        }
+
         // Now we try to complete an option itself
         for (const complete_entry_opt_t &o : options) {
             // If this entry is for the base command, check if any of the arguments match.
@@ -958,7 +977,7 @@ bool completer_t::complete_param_for_command(const wcstring &cmd_orig, const wcs
                     // Only complete when the last short option has no parameter yet..
                     if (short_opt_pos + 1 != str.size()) continue;
                     // .. and it does not require one ..
-                    if (last_option_requires_param) continue;
+                    if (*last_option_requires_param) continue;
                     // .. and the option is not already there.
                     if (str.find(optchar) != wcstring::npos) continue;
                 }