From 3847d2e9d1b96293407d331dea986c4757e39f13 Mon Sep 17 00:00:00 2001
From: Fabian Homborg <FHomborg@gmail.com>
Date: Fri, 18 Jan 2019 19:24:49 +0100
Subject: [PATCH] Also set the read-only flag for non-electric vars

For some reason, we have two places where a variable can be read-only:

- By key in env.cpp:is_read_only(), which is checked via set*

- By flag on the actual env_var_t, which is checked e.g. in
  parse_execution

The latter didn't happen for non-electric variables like hostname,
because they used the default constructor, because they were
constructed via operator[] (or some such C++-iness).

This caused for-loops to crash on an assert if they used a
non-electric read-only var like $hostname or $SHLVL.

Instead, we explicitly set the flag.

We might want to remove one of the two read-only checks, or something?

Fixes #5548.
---
 src/env.cpp     | 1 +
 src/env.h       | 8 ++++++++
 tests/test1.err | 6 ++++++
 tests/test1.in  | 5 +++++
 tests/test1.out | 3 +++
 5 files changed, 23 insertions(+)

diff --git a/src/env.cpp b/src/env.cpp
index b41f61d6c..6582f928d 100644
--- a/src/env.cpp
+++ b/src/env.cpp
@@ -1221,6 +1221,7 @@ int env_stack_t::set_internal(const wcstring &key, env_mode_flags_t input_var_mo
 
             var.set_vals(std::move(val));
             var.set_pathvar(var_mode & ENV_PATHVAR);
+            var.set_read_only(is_read_only(key));
 
             if (var_mode & ENV_EXPORT) {
                 // The new variable is exported.
diff --git a/src/env.h b/src/env.h
index 970d05cad..5d2334efc 100644
--- a/src/env.h
+++ b/src/env.h
@@ -126,6 +126,14 @@ class env_var_t {
         }
     }
 
+    void set_read_only(bool read_only) {
+        if (read_only) {
+            flags |= flag_read_only;
+        } else {
+            flags &= ~flag_read_only;
+        }
+    }
+
     static env_var_flags_t flags_for(const wchar_t *name);
 
     env_var_t &operator=(const env_var_t &var) = default;
diff --git a/tests/test1.err b/tests/test1.err
index 5e9cda705..e5a978376 100644
--- a/tests/test1.err
+++ b/tests/test1.err
@@ -38,6 +38,12 @@ fish: You cannot use read-only variable 'status' in a for loop
 for status in a b c
     ^
 
+####################
+# That goes for non-electric ones as well (#5548)
+fish: You cannot use read-only variable 'hostname' in a for loop
+for hostname in a b c
+    ^
+
 ####################
 # For loop control vars available outside the for block
 
diff --git a/tests/test1.in b/tests/test1.in
index d2910b6f4..f006e774a 100644
--- a/tests/test1.in
+++ b/tests/test1.in
@@ -163,6 +163,11 @@ for status in a b c
   echo $status
 end
 
+logmsg "That goes for non-electric ones as well (#5548)"
+for hostname in a b c
+    echo $hostname
+end
+
 logmsg For loop control vars available outside the for block
 begin
   set -l loop_var initial-value
diff --git a/tests/test1.out b/tests/test1.out
index 51ab3d525..6c141412d 100644
--- a/tests/test1.out
+++ b/tests/test1.out
@@ -96,6 +96,9 @@ Checking for infinite loops in no-execute
 ####################
 # For loops with read-only vars is an error (#4342)
 
+####################
+# That goes for non-electric ones as well (#5548)
+
 ####################
 # For loop control vars available outside the for block
 $loop_var: set in local scope, unexported, with 1 elements