Merge pull request #265 from slingamn/issue262.1

fix #262

.travis.yml

@@ -7,4 +7,4 @@ script:
 - tar -xzf goreleaser_Linux_x86_64.tar.gz -C $GOPATH/bin
 - make
 - make test
-- bash ./.travis.gofmt.sh
+- ./.check-gofmt.sh

Makefile

@@ -16,3 +16,4 @@ test:
 	cd irc/isupport && go test . && go vet .
 	cd irc/modes && go test . && go vet .
 	cd irc/utils && go test . && go vet .
+	./.check-gofmt.sh

irc/client.go

@@ -690,7 +690,7 @@ func (client *Client) destroy(beingResumed bool) {
 	client.Quit("Connection closed")
 
 	if !beingResumed {
-		client.server.whoWas.Append(client)
+		client.server.whoWas.Append(client.WhoWas())
 	}
 
 	// remove from connection limits

irc/getters.go

@@ -209,6 +209,19 @@ func (client *Client) Channels() (result []*Channel) {
 	return
 }
 
+func (client *Client) WhoWas() (result WhoWas) {
+	client.stateMutex.RLock()
+	defer client.stateMutex.RUnlock()
+
+	result.nicknameCasefolded = client.nickCasefolded
+	result.nickname = client.nick
+	result.username = client.username
+	result.hostname = client.hostname
+	result.realname = client.realname
+
+	return
+}
+
 func (channel *Channel) Name() string {
 	channel.stateMutex.RLock()
 	defer channel.stateMutex.RUnlock()

irc/handlers.go

@@ -2507,27 +2507,29 @@ func whoisHandler(server *Server, client *Client, msg ircmsg.IrcMessage, rb *Res
 func whowasHandler(server *Server, client *Client, msg ircmsg.IrcMessage, rb *ResponseBuffer) bool {
 	nicknames := strings.Split(msg.Params[0], ",")
 
-	var count int64
+	// 0 means "all the entries", as does a negative number
+	var count uint64
 	if len(msg.Params) > 1 {
-		count, _ = strconv.ParseInt(msg.Params[1], 10, 64)
+		count, _ = strconv.ParseUint(msg.Params[1], 10, 64)
 	}
 	//var target string
 	//if len(msg.Params) > 2 {
 	//	target = msg.Params[2]
 	//}
+	cnick := client.Nick()
 	for _, nickname := range nicknames {
-		results := server.whoWas.Find(nickname, count)
+		results := server.whoWas.Find(nickname, int(count))
 		if len(results) == 0 {
 			if len(nickname) > 0 {
-				rb.Add(nil, server.name, ERR_WASNOSUCHNICK, client.nick, nickname, client.t("There was no such nickname"))
+				rb.Add(nil, server.name, ERR_WASNOSUCHNICK, cnick, nickname, client.t("There was no such nickname"))
 			}
 		} else {
 			for _, whoWas := range results {
-				rb.Add(nil, server.name, RPL_WHOWASUSER, client.nick, whoWas.nickname, whoWas.username, whoWas.hostname, "*", whoWas.realname)
+				rb.Add(nil, server.name, RPL_WHOWASUSER, cnick, whoWas.nickname, whoWas.username, whoWas.hostname, "*", whoWas.realname)
 			}
 		}
 		if len(nickname) > 0 {
-			rb.Add(nil, server.name, RPL_ENDOFWHOWAS, client.nick, nickname, client.t("End of WHOWAS"))
+			rb.Add(nil, server.name, RPL_ENDOFWHOWAS, cnick, nickname, client.t("End of WHOWAS"))
 		}
 	}
 	return false

irc/nickname.go

@@ -43,8 +43,8 @@ func performNickChange(server *Server, client *Client, target *Client, newnick s
 	}
 
 	hadNick := target.HasNick()
-	origNick := target.Nick()
 	origNickMask := target.NickMaskString()
+	whowas := client.WhoWas()
 	err = client.server.clients.SetNick(target, nickname)
 	if err == errNicknameInUse {
 		rb.Add(nil, server.name, ERR_NICKNAMEINUSE, client.nick, nickname, client.t("Nickname is already in use"))
@@ -61,8 +61,8 @@ func performNickChange(server *Server, client *Client, target *Client, newnick s
 
 	client.server.logger.Debug("nick", fmt.Sprintf("%s changed nickname to %s [%s]", origNickMask, nickname, cfnick))
 	if hadNick {
-		target.server.snomasks.Send(sno.LocalNicks, fmt.Sprintf(ircfmt.Unescape("$%s$r changed nickname to %s"), origNick, nickname))
-		target.server.whoWas.Append(client)
+		target.server.snomasks.Send(sno.LocalNicks, fmt.Sprintf(ircfmt.Unescape("$%s$r changed nickname to %s"), whowas.nickname, nickname))
+		target.server.whoWas.Append(whowas)
 		for friend := range target.Friends() {
 			friend.Send(nil, origNickMask, "NICK", nickname)
 		}

irc/whowas.go

@@ -10,11 +10,16 @@ import (
 
 // WhoWasList holds our list of prior clients (for use with the WHOWAS command).
 type WhoWasList struct {
-	buffer []*WhoWas
+	buffer []WhoWas
+	// three possible states:
+	// empty: start == end == -1
+	// partially full: start != end
+	// full: start == end > 0
+	// if entries exist, they go from `start` to `(end - 1) % length`
 	start int
 	end   int
 
-	accessMutex sync.RWMutex // tier 2
+	accessMutex sync.RWMutex // tier 1
 }
 
 // WhoWas is an entry in the WhoWasList.
@@ -29,77 +34,71 @@ type WhoWas struct {
 // NewWhoWasList returns a new WhoWasList
 func NewWhoWasList(size uint) *WhoWasList {
 	return &WhoWasList{
-		buffer: make([]*WhoWas, size+1),
+		buffer: make([]WhoWas, size),
+		start:  -1,
+		end:    -1,
 	}
 }
 
 // Append adds an entry to the WhoWasList.
-func (list *WhoWasList) Append(client *Client) {
+func (list *WhoWasList) Append(whowas WhoWas) {
 	list.accessMutex.Lock()
 	defer list.accessMutex.Unlock()
 
-	list.buffer[list.end] = &WhoWas{
-		nicknameCasefolded: client.nickCasefolded,
-		nickname:           client.nick,
-		username:           client.username,
-		hostname:           client.hostname,
-		realname:           client.realname,
+	if len(list.buffer) == 0 {
+		return
 	}
+
+	var pos int
+	if list.start == -1 { // empty
+		pos = 0
+		list.start = 0
+		list.end = 1
+	} else if list.start != list.end { // partially full
+		pos = list.end
 		list.end = (list.end + 1) % len(list.buffer)
-	if list.end == list.start {
-		list.start = (list.end + 1) % len(list.buffer)
+	} else if list.start == list.end { // full
+		pos = list.end
+		list.end = (list.end + 1) % len(list.buffer)
+		list.start = list.end // advance start as well, overwriting first entry
 	}
+
+	list.buffer[pos] = whowas
 }
 
 // Find tries to find an entry in our WhoWasList with the given details.
-func (list *WhoWasList) Find(nickname string, limit int64) []*WhoWas {
+func (list *WhoWasList) Find(nickname string, limit int) (results []WhoWas) {
+	casefoldedNickname, err := CasefoldName(nickname)
+	if err != nil {
+		return
+	}
+
 	list.accessMutex.RLock()
 	defer list.accessMutex.RUnlock()
 
-	results := make([]*WhoWas, 0)
-
-	casefoldedNickname, err := CasefoldName(nickname)
-	if err != nil {
-		return results
+	if list.start == -1 {
+		return
 	}
-
-	for whoWas := range list.Each() {
-		if casefoldedNickname != whoWas.nicknameCasefolded {
-			continue
+	// iterate backwards through the ring buffer
+	pos := list.prev(list.end)
+	for limit == 0 || len(results) < limit {
+		if casefoldedNickname == list.buffer[pos].nicknameCasefolded {
+			results = append(results, list.buffer[pos])
 		}
-		results = append(results, whoWas)
-		if int64(len(results)) >= limit {
+		if pos == list.start {
 			break
 		}
+		pos = list.prev(pos)
 	}
-	return results
+
+	return
 }
 
 func (list *WhoWasList) prev(index int) int {
-	list.accessMutex.RLock()
-	defer list.accessMutex.RUnlock()
-
-	index--
-	if index < 0 {
-		index += len(list.buffer)
+	switch index {
+	case 0:
+		return len(list.buffer) - 1
+	default:
+		return index - 1
 	}
-	return index
-}
-
-// Each iterates the WhoWasList in reverse.
-func (list *WhoWasList) Each() <-chan *WhoWas {
-	ch := make(chan *WhoWas)
-	go func() {
-		defer close(ch)
-		if list.start == list.end {
-			return
-		}
-		start := list.prev(list.end)
-		end := list.prev(list.start)
-		for start != end {
-			ch <- list.buffer[start]
-			start = list.prev(start)
-		}
-	}()
-	return ch
 }

irc/whowas_test.go

@@ -0,0 +1,101 @@
+// Copyright (c) 2018 Shivaram Lingamneni <slingamn@cs.stanford.edu>
+// released under the MIT license
+
+package irc
+
+import (
+	"testing"
+)
+
+func makeTestWhowas(nick string) WhoWas {
+	cfnick, err := CasefoldName(nick)
+	if err != nil {
+		panic(err)
+	}
+	return WhoWas{
+		nicknameCasefolded: cfnick,
+		nickname:           nick,
+		username:           "user",
+		hostname:           "oragono.io",
+		realname:           "Real Name",
+	}
+}
+
+func TestWhoWas(t *testing.T) {
+	var results []WhoWas
+	wwl := NewWhoWasList(3)
+	// test Find on empty list
+	results = wwl.Find("nobody", 10)
+	if len(results) != 0 {
+		t.Fatalf("incorrect whowas results: %v", results)
+	}
+
+	wwl.Append(makeTestWhowas("dan-"))
+	results = wwl.Find("nobody", 10)
+	if len(results) != 0 {
+		t.Fatalf("incorrect whowas results: %v", results)
+	}
+	results = wwl.Find("dan-", 10)
+	if len(results) != 1 || results[0].nickname != "dan-" {
+		t.Fatalf("incorrect whowas results: %v", results)
+	}
+
+	wwl.Append(makeTestWhowas("slingamn"))
+	results = wwl.Find("slingamN", 10)
+	if len(results) != 1 || results[0].nickname != "slingamn" {
+		t.Fatalf("incorrect whowas results: %v", results)
+	}
+
+	wwl.Append(makeTestWhowas("Dan-"))
+	results = wwl.Find("dan-", 10)
+	// reverse chronological order
+	if len(results) != 2 || results[0].nickname != "Dan-" || results[1].nickname != "dan-" {
+		t.Fatalf("incorrect whowas results: %v", results)
+	}
+	// 0 means no limit
+	results = wwl.Find("dan-", 0)
+	if len(results) != 2 || results[0].nickname != "Dan-" || results[1].nickname != "dan-" {
+		t.Fatalf("incorrect whowas results: %v", results)
+	}
+	// a limit of 1 should return the most recent entry only
+	results = wwl.Find("dan-", 1)
+	if len(results) != 1 || results[0].nickname != "Dan-" {
+		t.Fatalf("incorrect whowas results: %v", results)
+	}
+
+	wwl.Append(makeTestWhowas("moocow"))
+	results = wwl.Find("moocow", 10)
+	if len(results) != 1 || results[0].nickname != "moocow" {
+		t.Fatalf("incorrect whowas results: %v", results)
+	}
+	results = wwl.Find("dan-", 10)
+	// should have overwritten the original entry, leaving the second
+	if len(results) != 1 || results[0].nickname != "Dan-" {
+		t.Fatalf("incorrect whowas results: %v", results)
+	}
+
+	// overwrite the second entry
+	wwl.Append(makeTestWhowas("enckse"))
+	results = wwl.Find("enckse", 10)
+	if len(results) != 1 || results[0].nickname != "enckse" {
+		t.Fatalf("incorrect whowas results: %v", results)
+	}
+	results = wwl.Find("slingamn", 10)
+	if len(results) != 0 {
+		t.Fatalf("incorrect whowas results: %v", results)
+	}
+}
+
+func TestEmptyWhoWas(t *testing.T) {
+	// stupid edge case; setting an empty whowas buffer should not panic
+	wwl := NewWhoWasList(0)
+	results := wwl.Find("slingamn", 10)
+	if len(results) != 0 {
+		t.Fatalf("incorrect whowas results: %v", results)
+	}
+	wwl.Append(makeTestWhowas("slingamn"))
+	results = wwl.Find("slingamn", 10)
+	if len(results) != 0 {
+		t.Fatalf("incorrect whowas results: %v", results)
+	}
+}