repoconfig: fix unexpected overwrites

We were overwriting the config if `overwrite_from` was set, even if that file didn't exist. Oops. Signed-off-by: Phil Dibowitz <[email protected]>
jaymzh · Dec 20, 2023 · bb1e4fd · bb1e4fd
1 parent d3ff4c1
commit bb1e4fd
Show file tree

Hide file tree

Showing 2 changed files with 51 additions and 17 deletions.
diff --git a/lib/sugarjar/repoconfig.rb b/lib/sugarjar/repoconfig.rb
@@ -16,15 +16,13 @@ def self.repo_config_path(config)
     end
 
     def self.hash_from_file(config_file)
-      if File.exist?(config_file)
-        SugarJar::Log.debug("Loading repo config: #{config_file}")
-        YAML.safe_load(File.read(config_file))
-      else
-        SugarJar::Log.debug(
-          "Skipping repo config (#{config_file}), returning empty hash",
-        )
-        {}
-      end
+      SugarJar::Log.debug("Loading repo config: #{config_file}")
+      YAML.safe_load(File.read(config_file))
+    end
+
+    # wrapper for File.exist to make unittests easier
+    def self.config_file?(config_file)
+      File.exist?(config_file)
     end
 
     def self.config(config = CONFIG_NAME)
@@ -34,14 +32,14 @@ def self.config(config = CONFIG_NAME)
         return data
       end
       config_file = repo_config_path(config)
-      data = hash_from_file(config_file)
-      if data['overwrite_from']
+      data = hash_from_file(config_file) if config_file?(config_file)
+      if data['overwrite_from'] && config_file?(data['overwrite_from'])
         SugarJar::Log.debug(
           "Attempting overwrite_from #{data['overwrite_from']}",
         )
         data = config(data['overwrite_from'])
         data.delete('overwrite_from')
-      elsif data['include_from']
+      elsif data['include_from'] && config_file?(data['include_from'])
         SugarJar::Log.debug("Attempting include_from #{data['include_from']}")
         data.deep_merge!(config(data['include_from']))
         data.delete('include_from')

diff --git a/spec/repoconfig_spec.rb b/spec/repoconfig_spec.rb
@@ -16,6 +16,8 @@
           'lint',
         ],
       }
+      allow(SugarJar::RepoConfig).to receive(:config_file?).
+        and_return(true)
       allow(SugarJar::RepoConfig).to receive(:hash_from_file).
         and_return(expected)
       data = SugarJar::RepoConfig.config('whatever')
@@ -55,6 +57,8 @@
         with('base').and_return('base')
       allow(SugarJar::RepoConfig).to receive(:repo_config_path).
         with('additional').and_return('additional')
+      allow(SugarJar::RepoConfig).to receive(:config_file?).
+        and_return(true)
       allow(SugarJar::RepoConfig).to receive(:hash_from_file).
         with('base').and_return(base)
       allow(SugarJar::RepoConfig).to receive(:hash_from_file).
@@ -75,10 +79,12 @@
       additional = {
         'new' => ['thing'],
       }
-      allow(SugarJar::RepoConfig).to receive(:repo_config_path).
-        with('base').and_return('base')
-      allow(SugarJar::RepoConfig).to receive(:repo_config_path).
-        with('additional').and_return('additional')
+      %w{base additional}.each do |word|
+        allow(SugarJar::RepoConfig).to receive(:repo_config_path).
+          with(word).and_return(word)
+      end
+      allow(SugarJar::RepoConfig).to receive(:config_file?).
+        and_return(true)
       allow(SugarJar::RepoConfig).to receive(:hash_from_file).
         with('base').and_return(base)
       allow(SugarJar::RepoConfig).to receive(:hash_from_file).
@@ -88,7 +94,7 @@
       expect(data).to eq(additional)
     end
 
-    it 'handles recurseive includes' do
+    it 'handles recursive includes' do
       base = {
         'include_from' => 'additional',
         'top1' => ['entryA'],
@@ -128,6 +134,8 @@
         allow(SugarJar::RepoConfig).to receive(:repo_config_path).
           with(word).and_return(word)
       end
+      allow(SugarJar::RepoConfig).to receive(:config_file?).
+        and_return(true)
       allow(SugarJar::RepoConfig).to receive(:hash_from_file).
         with('base').and_return(base)
       allow(SugarJar::RepoConfig).to receive(:hash_from_file).
@@ -137,5 +145,33 @@
       data = SugarJar::RepoConfig.config('base')
       expect(data).to eq(expected)
     end
+
+    it "doesn't overwrite from non-existent files" do
+      base = {
+        'include_from' => 'additional',
+        'top1' => ['entryA'],
+        'top2' => {
+          'top2key1' => 'a',
+          'top2key2' => 'b',
+        },
+      }
+      additional = {
+        'something' => 'else',
+      }
+      %w{base additional}.each do |word|
+        allow(SugarJar::RepoConfig).to receive(:repo_config_path).
+          with(word).and_return(word)
+      end
+      allow(SugarJar::RepoConfig).to receive(:config_file?).
+        with('base').and_return(true)
+      allow(SugarJar::RepoConfig).to receive(:config_file?).
+        with('additional').and_return(true)
+      allow(SugarJar::RepoConfig).to receive(:hash_from_file).
+        with('base').and_return(base)
+      allow(SugarJar::RepoConfig).to receive(:hash_from_file).
+        with('additional').and_return(additional)
+      data = SugarJar::RepoConfig.config('base')
+      expect(data).to eq(data)
+    end
   end
 end