Merge pull request #1846 from dbenamy/fast-add-role

Make add_role O(hosts) & fix port handling

CHANGELOG.md

@@ -9,6 +9,8 @@ https://github.com/capistrano/capistrano/compare/v3.7.2...HEAD
 * [#1835](https://github.com/capistrano/capistrano/pull/1835): Stopped printing parenthesis in ask prompt if no default or nil was passed as argument [(@chamini2)](https://github.com/chamini2)
 * [#1840](https://github.com/capistrano/capistrano/pull/1840): Git plugin: shellescape git_wrapper_path [(@olleolleolle)](https://github.com/olleolleolle)
 * [#1843](https://github.com/capistrano/capistrano/pull/1843): Properly shell escape git:wrapper steps - [@mattbrictson](https://github.com/mattbrictson)
+* [#1846](https://github.com/capistrano/capistrano/pull/1846): Defining a role is now O(hosts) instead of O(hosts^2) [(@dbenamy)](https://github.com/dbenamy)
+* [#1846](https://github.com/capistrano/capistrano/pull/1846): add_host will add a new host in a case where it used to incorrectly update an existing one (potentially breaking) [(@dbenamy)](https://github.com/dbenamy)
 * Your contribution here!
 
 ## `3.7.2` (2017-01-27)

lib/capistrano/configuration/server.rb

@@ -64,6 +64,7 @@ def roles_array
       end
 
       def matches?(other)
+        # This matching logic must stay in sync with `Servers#add_host`.
         hostname == other.hostname && port == other.port
       end
 

lib/capistrano/configuration/servers.rb

@@ -9,11 +9,15 @@ class Servers
 
       def add_host(host, properties={})
         new_host = Server[host]
-        if (server = servers.find { |s| s.matches? new_host })
-          server.user = new_host.user if new_host.user
-          server.with(properties)
+        new_host.port = properties[:port] if properties.key?(:port)
+        # This matching logic must stay in sync with `Server#matches?`.
+        key = ServerKey.new(new_host.hostname, new_host.port)
+        existing = servers_by_key[key]
+        if existing
+          existing.user = new_host.user if new_host.user
+          existing.with(properties)
         else
-          servers << new_host.with(properties)
+          servers_by_key[key] = new_host.with(properties)
         end
       end
 
@@ -26,7 +30,7 @@ def add_role(role, hosts, options={})
 
       def roles_for(names)
         options = extract_options(names)
-        s = Filter.new(:role, names).filter(servers)
+        s = Filter.new(:role, names).filter(servers_by_key.values)
         s.select { |server| server.select?(options) }
       end
 
@@ -53,13 +57,15 @@ def fetch_primary(role)
       end
 
       def each
-        servers.each { |server| yield server }
+        servers_by_key.values.each { |server| yield server }
       end
 
       private
 
-      def servers
-        @servers ||= []
+      ServerKey = Struct.new(:hostname, :port)
+
+      def servers_by_key
+        @servers_by_key ||= {}
       end
 
       def extract_options(array)

spec/lib/capistrano/configuration/servers_spec.rb

@@ -130,13 +130,14 @@ class Configuration
 
         it "can accept multiple servers with the same hostname but different ports or users" do
           servers.add_host("1", roles: [:app, "web"], test: :value, port: 12)
+          expect(servers.count).to eq(2)
           servers.add_host("1", roles: [:app, "web"], test: :value, port: 34)
           servers.add_host("1", roles: [:app, "web"], test: :value, user: "root")
           servers.add_host("1", roles: [:app, "web"], test: :value, user: "deployer")
           servers.add_host("1", roles: [:app, "web"], test: :value, user: "root", port: 34)
           servers.add_host("1", roles: [:app, "web"], test: :value, user: "deployer", port: 34)
           servers.add_host("1", roles: [:app, "web"], test: :value, user: "deployer", port: 56)
-          expect(servers.count).to eq(5)
+          expect(servers.count).to eq(4)
         end
 
         describe "with a :user property" do