From 01452da5bf17224106b3b0117eb7bd978d75863d Mon Sep 17 00:00:00 2001
From: Mahmoud Al-Qudsi <mqudsi@neosmart.net>
Date: Sat, 31 Mar 2018 22:12:52 -0500
Subject: [PATCH] Add and use new exit code for env_remove() when var doesn't
 exist

The previous commit caused the tests to fail since env_remove() was
returning a blanket `!0` when a variable couldn't be unset because it
didn't exist in the first place. This caused the wrong message to be
emitted since the code clashed with a return code for `env_set()`.

Added `ENV_NOT_FOUND` to signify that the variable requested unset
didn't exist in the first place, but _not_ printing the error message
currently so as not to break existing behavior before checking if this
is something we want.
---
 src/builtin_set.cpp | 13 +++++++++++--
 src/env.cpp         |  2 +-
 src/env.h           |  2 +-
 3 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/src/builtin_set.cpp b/src/builtin_set.cpp
index 4386742c5..aff84eec4 100644
--- a/src/builtin_set.cpp
+++ b/src/builtin_set.cpp
@@ -305,6 +305,12 @@ static void handle_env_return(int retval, const wchar_t *cmd, const wchar_t *key
             retval = STATUS_CMD_ERROR;
             break;
         }
+        case ENV_NOT_FOUND: {
+            streams.err.append_format(
+                _(L"%ls: The variable '%ls' does not exist\n"), cmd, key);
+            retval = STATUS_CMD_ERROR;
+            break;
+        }
         default: {
             DIE("unexpected env_set() ret val");
             break;
@@ -622,6 +628,11 @@ static int builtin_set_erase(const wchar_t *cmd, set_cmd_opts_t &opts, int argc,
 
     if (idx_count == 0) {  // unset the var
         retval = env_remove(dest, scope);
+        // Temporarily swallowing ENV_NOT_FOUND errors to prevent
+        // breaking all tests that unset variables that aren't set.
+        if (retval != ENV_NOT_FOUND) {
+            handle_env_return(retval, cmd, dest, streams);
+        }
     } else {  // remove just the specified indexes of the var
         const auto dest_var = env_get(dest, scope);
         if (!dest_var) return STATUS_CMD_ERROR;
@@ -631,8 +642,6 @@ static int builtin_set_erase(const wchar_t *cmd, set_cmd_opts_t &opts, int argc,
         retval = my_env_set(cmd, dest, scope, result, streams);
     }
 
-    handle_env_return(retval, cmd, dest, streams);
-
     if (retval != STATUS_CMD_OK) return retval;
     return check_global_scope_exists(cmd, opts, dest, streams);
 }
diff --git a/src/env.cpp b/src/env.cpp
index 089875ce2..b156da24a 100644
--- a/src/env.cpp
+++ b/src/env.cpp
@@ -1286,7 +1286,7 @@ int env_remove(const wcstring &key, int var_mode) {
 
     react_to_variable_change(L"ERASE", key);
 
-    return !erased;
+    return erased ? ENV_OK : ENV_NOT_FOUND;
 }
 
 const wcstring_list_t &env_var_t::as_list() const { return vals; }
diff --git a/src/env.h b/src/env.h
index 7ebafb51a..aac7b8f7a 100644
--- a/src/env.h
+++ b/src/env.h
@@ -44,7 +44,7 @@ enum {
 typedef uint32_t env_mode_flags_t;
 
 /// Return values for `env_set()`.
-enum { ENV_OK, ENV_PERM, ENV_SCOPE, ENV_INVALID };
+enum { ENV_OK, ENV_PERM, ENV_SCOPE, ENV_INVALID, ENV_NOT_FOUND };
 
 /// A struct of configuration directories, determined in main() that fish will optionally pass to
 /// env_init.