From 717df09be020e3a3ac613a0f5e5625263c7f08b1 Mon Sep 17 00:00:00 2001
From: Hans-Christoph Steiner <hans@eds.org>
Date: Thu, 2 May 2024 14:05:56 +0200
Subject: [PATCH] clarify that config/options can be global or module-level
 variable

---
 fdroidserver/common.py | 22 ++++++++++++++++-
 tests/common.TestCase  | 54 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 75 insertions(+), 1 deletion(-)

diff --git a/fdroidserver/common.py b/fdroidserver/common.py
index 1f51fd21..4074d2aa 100644
--- a/fdroidserver/common.py
+++ b/fdroidserver/common.py
@@ -374,7 +374,27 @@ def fill_config_defaults(thisconfig):
 
 
 def get_config(opts=None):
-    """Get config instace. This function takes care of initializing config data before returning it."""
+    """Get the initalized, singleton config instance.
+
+    config and options are intertwined in read_config(), so they have
+    to be here too.  In the current ugly state of things, there are
+    multiple potential instances of config and options in use:
+
+    * global
+    * module-level in the subcommand module (e.g. fdroidserver/build.py)
+    * module-level in fdroidserver.common
+
+    There are some insane parts of the code that are probably
+    referring to multiple instances of these at different points.
+    This can be super confusing and maddening.
+
+    The current intermediate refactoring step is to move all
+    subcommands to always get/set config and options via this function
+    so that there is no longer a distinction between the global and
+    module-level instances.  Then there can be only one module-level
+    instance in fdroidserver.common.
+
+    """
     global config, options
 
     if config is not None:
diff --git a/tests/common.TestCase b/tests/common.TestCase
index 11ec5d7b..a162826e 100755
--- a/tests/common.TestCase
+++ b/tests/common.TestCase
@@ -3237,6 +3237,60 @@ class SignerExtractionTest(unittest.TestCase):
                 )
 
 
+class ConfigOptionsScopeTest(unittest.TestCase):
+    """Test assumptions about variable scope for "config" and "options".
+
+    The ancient architecture of config and options in fdroidserver has
+    weird issues around unexpected scope, like there are cases where
+    the global config is not the same as the module-level config, and
+    more.
+
+    This is about describing what is happening, it is not about
+    documenting behaviors that are good design. The config and options
+    handling should really be refactored into a well-known, workable
+    Pythonic pattern.
+
+    """
+
+    def setUp(self):
+        # these are declared as None at the top of the module file
+        fdroidserver.common.config = None
+        fdroidserver.common.options = None
+
+    def tearDown(self):
+        fdroidserver.common.config = None
+        fdroidserver.common.options = None
+        if 'config' in globals():
+            global config
+            del config
+        if 'options' in globals():
+            global options
+            del options
+
+    def test_get_config(self):
+        """Show how the module-level variables are initialized."""
+        self.assertTrue('config' not in vars() and 'config' not in globals())
+        self.assertIsNone(fdroidserver.common.config)
+        config = fdroidserver.common.get_config()
+        self.assertIsNotNone(fdroidserver.common.config)
+        self.assertEqual(dict, type(config))
+        self.assertEqual(config, fdroidserver.common.config)
+
+    def test_get_config_global(self):
+        """Test assumptions about variable scope using global keyword."""
+        global config
+        self.assertTrue('config' not in vars() and 'config' not in globals())
+        self.assertIsNone(fdroidserver.common.config)
+        c = fdroidserver.common.get_config()
+        self.assertIsNotNone(fdroidserver.common.config)
+        self.assertEqual(dict, type(c))
+        self.assertEqual(c, fdroidserver.common.config)
+        self.assertTrue(
+            'config' not in vars() and 'config' not in globals(),
+            "The config should not be set in the global context, only module-level.",
+        )
+
+
 if __name__ == "__main__":
     os.chdir(os.path.dirname(__file__))