NewNick handler that doesn't vary nick length. Fixes #108. Sort of.

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.

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)
+		}
+	}
+}

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