Skip to content

Commit 329163b

Browse files
committed
Improve SSHKit::Host comparison
* #equal? should test for object identity, not whether the contents are equal, as per the Ruby documentation. * #eql? and #== should check the contents directly instead of comparing the hash. This prevents false positives.
1 parent 08a213b commit 329163b

File tree

2 files changed

+27
-3
lines changed

2 files changed

+27
-3
lines changed

lib/sshkit/host.rb

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ module SSHKit
55
UnparsableHostStringError = Class.new(SSHKit::StandardError)
66

77
class Host
8+
DEFAULT_SSH_PORT = 22
89

910
attr_accessor :password, :hostname, :port, :user, :ssh_options
1011

@@ -60,10 +61,11 @@ def username
6061
end
6162

6263
def eql?(other_host)
63-
other_host.hash == hash
64+
user == other_host.user &&
65+
hostname == other_host.hostname &&
66+
port_with_default == other_host.send(:port_with_default)
6467
end
6568
alias :== :eql?
66-
alias :equal? :eql?
6769

6870
def to_s
6971
hostname
@@ -84,6 +86,12 @@ def properties
8486
@properties ||= OpenStruct.new
8587
end
8688

89+
private
90+
91+
def port_with_default
92+
port || DEFAULT_SSH_PORT
93+
end
94+
8795
end
8896

8997
# @private

test/unit/test_host.rb

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,23 @@ def test_assert_hosts_hash_equally
5858
def test_assert_hosts_compare_equal
5959
assert Host.new('example.com') == Host.new('example.com')
6060
assert Host.new('example.com').eql? Host.new('example.com')
61-
assert Host.new('example.com').equal? Host.new('example.com')
61+
62+
assert Host.new('example.com:22') == Host.new('example.com')
63+
assert Host.new('example.com:22').eql? Host.new('example.com')
64+
65+
assert Host.new('example.com:22') != Host.new('example.com:23')
66+
assert !Host.new('example.com:22').eql?(Host.new('example.com:23'))
67+
68+
assert Host.new('foo@example.com') == Host.new('foo@example.com')
69+
assert Host.new('foo@example.com').eql? Host.new('foo@example.com')
70+
71+
assert Host.new('foo@example.com') != Host.new('bar@example.com')
72+
assert !Host.new('foo@example.com').eql?(Host.new('bar@example.com'))
73+
74+
a = Host.new('example.com')
75+
b = Host.new('example.com')
76+
assert a.equal? a
77+
assert !a.equal?(b)
6278
end
6379

6480
def test_arbitrary_host_properties

0 commit comments

Comments
 (0)