Add support for "global" section at beginning of file

This commit does the following:

* Fixes a bug in ExternalIterator
* Adds support for a "global" section before the first named
  section at the beginning of the INI file
* Improves test coverage

5 files changed, 249 insertions, 46 deletions

diff --git a/lib/puppet/util/external_iterator.rb b/lib/puppet/util/external_iterator.rb
index 67b3375..45c0fa4 100644
--- a/lib/puppet/util/external_iterator.rb
+++ b/lib/puppet/util/external_iterator.rb
@@ -3,7 +3,7 @@ module Util
   class ExternalIterator
     def initialize(coll)
       @coll = coll
-      @cur_index = 0
+      @cur_index = -1
     end
 
     def next
@@ -17,7 +17,11 @@ module Util
 
     private
     def item_at(index)
-      [@coll[index], index]
+      if @coll.length > index
+        [@coll[index], index]
+      else
+        [nil, nil]
+      end
     end
   end
 end
diff --git a/lib/puppet/util/ini_file.rb b/lib/puppet/util/ini_file.rb
index 9fd08ad..8629db0 100644
--- a/lib/puppet/util/ini_file.rb
+++ b/lib/puppet/util/ini_file.rb
@@ -43,18 +43,13 @@ module Util
 
     def save
       File.open(@path, 'w') do |fh|
-        first_section = @sections_hash[@section_names[0]]
-        first_section.start_line == nil ? start_line = 0 : start_line = first_section.start_line
-        (0..start_line - 1).each do |line_num|
-          fh.puts(lines[line_num])
-        end
 
         @section_names.each do |name|
           section = @sections_hash[name]
 
-          if (section.start_line.nil?)
+          if section.start_line.nil?
             fh.puts("\n[#{section.name}]")
-          else
+          elsif ! section.end_line.nil?
             (section.start_line..section.end_line).each do |line_num|
               fh.puts(lines[line_num])
             end
@@ -76,7 +71,13 @@ module Util
 
     def parse_file
       line_iter = create_line_iter
+
+      # We always create a "global" section at the beginning of the file, for
+      # anything that appears before the first named section.
+      section = read_section('', 0, line_iter)
+      add_section(section)
       line, line_num = line_iter.next
+
       while line
         if (match = SECTION_REGEX.match(line))
           section = read_section(match[1], line_num, line_iter)
@@ -88,13 +89,15 @@ module Util
 
     def read_section(name, start_line, line_iter)
       settings = {}
+      end_line_num = nil
       while true
         line, line_num = line_iter.peek
-        if (line.nil? or match = SECTION_REGEX.match(line))
-          return Section.new(name, start_line, line_num - 1, settings)
+        if (line_num.nil? or match = SECTION_REGEX.match(line))
+          return Section.new(name, start_line, end_line_num, settings)
         elsif (match = SETTING_REGEX.match(line))
           settings[match[1]] = match[2]
         end
+        end_line_num = line_num
         line_iter.next
       end
     end
diff --git a/spec/unit/puppet/provider/ini_setting/ruby_spec.rb b/spec/unit/puppet/provider/ini_setting/ruby_spec.rb
index 72a5da0..d26c78d 100644
--- a/spec/unit/puppet/provider/ini_setting/ruby_spec.rb
+++ b/spec/unit/puppet/provider/ini_setting/ruby_spec.rb
@@ -7,24 +7,12 @@ describe provider_class do
 
   let(:tmpfile) { tmpfilename("ini_setting_test") }
   let(:emptyfile) { tmpfilename("ini_setting_test_empty") }
-  let(:orig_content) {
-    <<-EOS
-# This is a comment
-[section1]
-; This is also a comment
-foo=foovalue
 
-bar = barvalue
-master = true
-[section2]
-
-foo= foovalue2
-baz=bazvalue
-url = http://192.168.1.1:8080
-    #another comment
- ; yet another comment
-    EOS
-}
+  let(:common_params) { {
+      :title    => 'ini_setting_ensure_present_test',
+      :path     => tmpfile,
+      :section  => 'section2',
+  } }
 
   def validate_file(expected_content,tmpfile = tmpfile)
     File.read(tmpfile).should == expected_content
@@ -41,11 +29,24 @@ url = http://192.168.1.1:8080
   end
 
   context "when ensuring that a setting is present" do
-    let(:common_params) { {
-        :title    => 'ini_setting_ensure_present_test',
-        :path     => tmpfile,
-        :section  => 'section2',
-    } }
+    let(:orig_content) {
+      <<-EOS
+# This is a comment
+[section1]
+; This is also a comment
+foo=foovalue
+
+bar = barvalue
+master = true
+[section2]
+
+foo= foovalue2
+baz=bazvalue
+url = http://192.168.1.1:8080
+    #another comment
+ ; yet another comment
+      EOS
+    }
 
     it "should add a missing setting to the correct section" do
       resource = Puppet::Type::Ini_setting.new(common_params.merge(
@@ -75,7 +76,7 @@ yahoo = yippee
 
     it "should modify an existing setting with a different value" do
       resource = Puppet::Type::Ini_setting.new(common_params.merge(
-                                                   :setting => 'baz', :value => 'bazvalue2'))
+           :setting => 'baz', :value => 'bazvalue2'))
       provider = described_class.new(resource)
       provider.exists?.should == false
       provider.create
@@ -100,7 +101,7 @@ url = http://192.168.1.1:8080
 
     it "should be able to handle settings with non alphanumbering settings " do
       resource = Puppet::Type::Ini_setting.new(common_params.merge(
-                                                   :setting => 'url', :value => 'http://192.168.0.1:8080'))
+           :setting => 'url', :value => 'http://192.168.0.1:8080'))
       provider = described_class.new(resource)
       provider.exists?.should == false
       provider.create
@@ -126,7 +127,7 @@ url = http://192.168.0.1:8080
 
     it "should recognize an existing setting with the specified value" do
       resource = Puppet::Type::Ini_setting.new(common_params.merge(
-                                                   :setting => 'baz', :value => 'bazvalue'))
+           :setting => 'baz', :value => 'bazvalue'))
       provider = described_class.new(resource)
       provider.exists?.should == true
     end
@@ -179,6 +180,109 @@ setting1 = hellowworld
       provider.create
     end
 
+  end
 
+  context "when dealing with a global section" do
+    let(:orig_content) {
+      <<-EOS
+# This is a comment
+foo=blah
+[section2]
+foo = http://192.168.1.1:8080
+ ; yet another comment
+      EOS
+    }
+
+
+    it "should add a missing setting if it doesn't exist" do
+      resource = Puppet::Type::Ini_setting.new(common_params.merge(
+          :section => '', :setting => 'bar', :value => 'yippee'))
+      provider = described_class.new(resource)
+      provider.exists?.should == false
+      provider.create
+      validate_file(<<-EOS
+# This is a comment
+foo=blah
+bar = yippee
+[section2]
+foo = http://192.168.1.1:8080
+ ; yet another comment
+      EOS
+      )
+    end
+
+    it "should modify an existing setting with a different value" do
+      resource = Puppet::Type::Ini_setting.new(common_params.merge(
+           :section => '', :setting => 'foo', :value => 'yippee'))
+      provider = described_class.new(resource)
+      provider.exists?.should == false
+      provider.create
+      validate_file(<<-EOS
+# This is a comment
+foo = yippee
+[section2]
+foo = http://192.168.1.1:8080
+ ; yet another comment
+      EOS
+      )
+    end
+
+    it "should recognize an existing setting with the specified value" do
+      resource = Puppet::Type::Ini_setting.new(common_params.merge(
+           :section => '', :setting => 'foo', :value => 'blah'))
+      provider = described_class.new(resource)
+      provider.exists?.should == true
+    end
+  end
+
+  context "when the first line of the file is a section" do
+    let(:orig_content) {
+      <<-EOS
+[section2]
+foo = http://192.168.1.1:8080
+      EOS
+    }
+
+    it "should be able to add a global setting" do
+      resource = Puppet::Type::Ini_setting.new(common_params.merge(
+           :section => '', :setting => 'foo', :value => 'yippee'))
+      provider = described_class.new(resource)
+      provider.exists?.should == false
+      provider.create
+      validate_file(<<-EOS
+foo = yippee
+[section2]
+foo = http://192.168.1.1:8080
+      EOS
+      )
+    end
+
+    it "should modify an existing setting" do
+      resource = Puppet::Type::Ini_setting.new(common_params.merge(
+          :section => 'section2', :setting => 'foo', :value => 'yippee'))
+      provider = described_class.new(resource)
+      provider.exists?.should == false
+      provider.create
+      validate_file(<<-EOS
+[section2]
+foo = yippee
+      EOS
+      )
+    end
+
+    it "should add a new setting" do
+      resource = Puppet::Type::Ini_setting.new(common_params.merge(
+          :section => 'section2', :setting => 'bar', :value => 'baz'))
+      provider = described_class.new(resource)
+      provider.exists?.should == false
+      provider.create
+      validate_file(<<-EOS
+[section2]
+foo = http://192.168.1.1:8080
+bar = baz
+      EOS
+      )
+    end
   end
+
 end
diff --git a/spec/unit/puppet/util/external_iterator_spec.rb b/spec/unit/puppet/util/external_iterator_spec.rb
new file mode 100644
index 0000000..92b17af
--- /dev/null
+++ b/spec/unit/puppet/util/external_iterator_spec.rb
@@ -0,0 +1,35 @@
+require 'spec_helper'
+require 'puppet/util/external_iterator'
+
+describe Puppet::Util::ExternalIterator do
+  let(:subject) { Puppet::Util::ExternalIterator.new(["a", "b", "c"]) }
+
+  context "#next" do
+    it "should iterate over the items" do
+      subject.next.should == ["a", 0]
+      subject.next.should == ["b", 1]
+      subject.next.should == ["c", 2]      
+    end
+  end
+
+  context "#peek" do
+    it "should return the 0th item repeatedly" do
+      subject.peek.should == ["a", 0]
+      subject.peek.should == ["a", 0]
+    end
+    
+    it "should not advance the iterator, but should reflect calls to #next" do
+      subject.peek.should == ["a", 0]
+      subject.peek.should == ["a", 0]
+      subject.next.should == ["a", 0]
+      subject.peek.should == ["b", 1]
+      subject.next.should == ["b", 1]
+      subject.peek.should == ["c", 2]
+      subject.next.should == ["c", 2]
+      subject.peek.should == [nil, nil]
+      subject.next.should == [nil, nil]
+    end
+  end
+
+
+end
diff --git a/spec/unit/puppet/util/ini_file_spec.rb b/spec/unit/puppet/util/ini_file_spec.rb
index f2c6e20..f30c8cd 100644
--- a/spec/unit/puppet/util/ini_file_spec.rb
+++ b/spec/unit/puppet/util/ini_file_spec.rb
@@ -2,8 +2,16 @@ require 'spec_helper'
 require 'puppet/util/ini_file'
 
 describe Puppet::Util::IniFile do
+  let(:subject) { Puppet::Util::IniFile.new("/my/ini/file/path") }
+
+  before :each do
+    File.should_receive(:file?).with("/my/ini/file/path") { true }
+    described_class.should_receive(:readlines).once.with("/my/ini/file/path") do
+      sample_content
+    end
+  end
+
   context "when parsing a file" do
-    let(:subject) { Puppet::Util::IniFile.new("/my/ini/file/path") }
     let(:sample_content) {
       template = <<-EOS
 # This is a comment
@@ -22,19 +30,14 @@ baz=bazvalue
       template.split("\n")
     }
 
-    before :each do
-      File.should_receive(:file?).with("/my/ini/file/path") { true }
-      described_class.should_receive(:readlines).once.with("/my/ini/file/path") do
-        sample_content
-      end
-    end
-
     it "should parse the correct number of sections" do
-      subject.section_names.length.should == 2
+      # there is always a "global" section, so our count should be 3.
+      subject.section_names.length.should == 3
     end
 
     it "should parse the correct section_names" do
-      subject.section_names.should == ["section1", "section2"]
+      # there should always be a "global" section named "" at the beginning of the list
+      subject.section_names.should == ["", "section1", "section2"]
     end
 
     it "should expose settings for sections" do
@@ -45,4 +48,58 @@ baz=bazvalue
     end
 
   end
+
+  context "when parsing a file whose first line is a section" do
+    let(:sample_content) {
+      template = <<-EOS
+[section1]
+; This is a comment
+foo=foovalue
+      EOS
+      template.split("\n")
+    }
+
+    it "should parse the correct number of sections" do
+      # there is always a "global" section, so our count should be 2.
+      subject.section_names.length.should == 2
+    end
+
+    it "should parse the correct section_names" do
+      # there should always be a "global" section named "" at the beginning of the list
+      subject.section_names.should == ["", "section1"]
+    end
+
+    it "should expose settings for sections" do
+      subject.get_value("section1", "foo").should == "foovalue"
+    end
+
+  end
+
+  context "when parsing a file with a 'global' section" do
+    let(:sample_content) {
+      template = <<-EOS
+foo = bar
+[section1]
+; This is a comment
+foo=foovalue
+      EOS
+      template.split("\n")
+    }
+
+    it "should parse the correct number of sections" do
+      # there is always a "global" section, so our count should be 2.
+      subject.section_names.length.should == 2
+    end
+
+    it "should parse the correct section_names" do
+      # there should always be a "global" section named "" at the beginning of the list
+      subject.section_names.should == ["", "section1"]
+    end
+
+    it "should expose settings for sections" do
+      subject.get_value("", "foo").should == "bar"
+      subject.get_value("section1", "foo").should == "foovalue"
+    end
+
+  end
 end
\ No newline at end of file