diff --git a/client/connection.go b/client/connection.go index 12d0e59..20a7fa3 100644 --- a/client/connection.go +++ b/client/connection.go @@ -125,7 +125,7 @@ func NewConfig(nick string, args ...string) *Config { cfg := &Config{ Me: &state.Nick{Nick: nick}, PingFreq: 3 * time.Minute, - NewNick: func(s string) string { return s + "_" }, + NewNick: DefaultNewNick, Recover: (*Conn).LogPanic, // in dispatch.go SplitLen: defaultSplit, Timeout: 60 * time.Second, @@ -143,6 +143,28 @@ func NewConfig(nick string, args ...string) *Config { return cfg } +// Because networks limit nick lengths, the easy approach of appending +// an '_' to a nick that is already in use can cause problems. When the +// length limit is reached, the clients idea of what its nick is +// ends up being different from the server. Hilarity ensues. +// Thanks to github.com/purpleidea for the bug report! +// Thanks to 'man ascii' for +func DefaultNewNick(old string) string { + if len(old) == 0 { + return "_" + } + c := old[len(old)-1] + switch { + case c >= '0' && c <= '9': + c = '0' + (((c - '0') + 1) % 10) + case c >= 'A' && c <= '}': + c = 'A' + (((c - 'A') + 1) % 61) + default: + c = '_' + } + return old[:len(old)-1] + string(c) +} + // SimpleClient creates a new Conn, passing its arguments to NewConfig. // If you don't need to change any client options and just want to get // started quickly, this is a convenient shortcut. diff --git a/client/connection_test.go b/client/connection_test.go index acf4713..3b7e5b7 100644 --- a/client/connection_test.go +++ b/client/connection_test.go @@ -583,3 +583,24 @@ func TestRateLimit(t *testing.T) { t.Errorf("l=%d, badness=%d", l, c.badness) } } + +func TestDefaultNewNick(t *testing.T) { + tests := []struct{ in, want string }{ + {"", "_"}, + {"0", "1"}, + {"9", "0"}, + {"A", "B"}, + {"Z", "["}, + {"_", "`"}, + {"`", "a"}, + {"}", "A"}, + {"-", "_"}, + {"fluffle", "flufflf"}, + } + + for _, test := range tests { + if got := DefaultNewNick(test.in); got != test.want { + t.Errorf("DefaultNewNick(%q) = %q, want %q", test.in, got, test.want) + } + } +} diff --git a/client/handlers_test.go b/client/handlers_test.go index 7808022..d706ae6 100644 --- a/client/handlers_test.go +++ b/client/handlers_test.go @@ -1,10 +1,11 @@ package client import ( - "github.com/fluffle/goirc/state" - "github.com/golang/mock/gomock" "testing" "time" + + "github.com/fluffle/goirc/state" + "github.com/golang/mock/gomock" ) // This test performs a simple end-to-end verification of correct line parsing @@ -80,25 +81,26 @@ func Test433(t *testing.T) { // Call handler with a 433 line, not triggering c.cfg.Me.Renick() s.st.EXPECT().Me().Return(c.cfg.Me) c.h_433(ParseLine(":irc.server.org 433 test new :Nickname is already in use.")) - s.nc.Expect("NICK new_") + s.nc.Expect("NICK " + DefaultNewNick("new")) // Send a line that will trigger a renick. This happens when our wanted // nick is unavailable during initial negotiation, so we must choose a // different one before the connection can proceed. No NICK line will be // sent by the server to confirm nick change in this case. + want := DefaultNewNick(c.cfg.Me.Nick) gomock.InOrder( s.st.EXPECT().Me().Return(c.cfg.Me), - s.st.EXPECT().ReNick("test", "test_").Return(c.cfg.Me), + s.st.EXPECT().ReNick("test", want).Return(c.cfg.Me), ) c.h_433(ParseLine(":irc.server.org 433 test test :Nickname is already in use.")) - s.nc.Expect("NICK test_") + s.nc.Expect("NICK " + want) // Test the code path that *doesn't* involve state tracking. c.st = nil c.h_433(ParseLine(":irc.server.org 433 test test :Nickname is already in use.")) - s.nc.Expect("NICK test_") + s.nc.Expect("NICK " + want) - if c.cfg.Me.Nick != "test_" { + if c.cfg.Me.Nick != want { t.Errorf("My nick not updated from '%s'.", c.cfg.Me.Nick) } c.st = s.st