Merge pull request #1809 from slingamn/issue1676_again.3

fix #1676, take 2

irc/accounts.go

@@ -2294,7 +2294,6 @@ type ReplayJoinsSetting uint
 const (
 	ReplayJoinsCommandsOnly = iota // replay in HISTORY or CHATHISTORY output
 	ReplayJoinsAlways              // replay in HISTORY, CHATHISTORY, or autoreplay
-	ReplayJoinsNever               // never replay
 )
 
 func replayJoinsSettingFromString(str string) (result ReplayJoinsSetting, err error) {
@@ -2303,8 +2302,6 @@ func replayJoinsSettingFromString(str string) (result ReplayJoinsSetting, err er
 		result = ReplayJoinsCommandsOnly
 	case "always":
 		result = ReplayJoinsAlways
-	case "never":
-		result = ReplayJoinsNever
 	default:
 		err = errInvalidParams
 	}

irc/channel.go

@@ -917,7 +917,7 @@ func (channel *Channel) autoReplayHistory(client *Client, rb *ResponseBuffer, sk
 	}
 
 	if hasAutoreplayTimestamps {
-		_, seq, _ := channel.server.GetHistorySequence(channel, client, "", 0)
+		_, seq, _ := channel.server.GetHistorySequence(channel, client, "")
 		if seq != nil {
 			zncMax := channel.server.Config().History.ZNCMax
 			items, _ = seq.Between(history.Selector{Time: start}, history.Selector{Time: end}, zncMax)
@@ -935,7 +935,7 @@ func (channel *Channel) autoReplayHistory(client *Client, rb *ResponseBuffer, sk
 			replayLimit = channel.server.Config().History.AutoreplayOnJoin
 		}
 		if 0 < replayLimit {
-			_, seq, _ := channel.server.GetHistorySequence(channel, client, "", 0)
+			_, seq, _ := channel.server.GetHistorySequence(channel, client, "")
 			if seq != nil {
 				items, _ = seq.Between(history.Selector{}, history.Selector{}, replayLimit)
 			}
@@ -952,7 +952,7 @@ func (channel *Channel) autoReplayHistory(client *Client, rb *ResponseBuffer, sk
 		}
 	}
 	if 0 < numItems {
-		channel.replayHistoryItems(rb, items, true)
+		channel.replayHistoryItems(rb, items, false)
 		rb.Flush(true)
 	}
 }
@@ -1035,7 +1035,7 @@ func (channel *Channel) Part(client *Client, message string, rb *ResponseBuffer)
 	client.server.logger.Debug("channels", fmt.Sprintf("%s left channel %s", details.nick, chname))
 }
 
-func (channel *Channel) replayHistoryItems(rb *ResponseBuffer, items []history.Item, autoreplay bool) {
+func (channel *Channel) replayHistoryItems(rb *ResponseBuffer, items []history.Item, chathistoryCommand bool) {
 	// send an empty batch if necessary, as per the CHATHISTORY spec
 	chname := channel.Name()
 	client := rb.target
@@ -1043,13 +1043,15 @@ func (channel *Channel) replayHistoryItems(rb *ResponseBuffer, items []history.I
 	extendedJoin := rb.session.capabilities.Has(caps.ExtendedJoin)
 	var playJoinsAsPrivmsg bool
 	if !eventPlayback {
-		switch client.AccountSettings().ReplayJoins {
+		if chathistoryCommand {
-		case ReplayJoinsCommandsOnly:
-			playJoinsAsPrivmsg = !autoreplay
-		case ReplayJoinsAlways:
 			playJoinsAsPrivmsg = true
-		case ReplayJoinsNever:
+		} else {
-			playJoinsAsPrivmsg = false
+			switch client.AccountSettings().ReplayJoins {
+			case ReplayJoinsCommandsOnly:
+				playJoinsAsPrivmsg = false
+			case ReplayJoinsAlways:
+				playJoinsAsPrivmsg = true
+			}
 		}
 	}
 
@@ -1066,6 +1068,9 @@ func (channel *Channel) replayHistoryItems(rb *ResponseBuffer, items []history.I
 		case history.Tagmsg:
 			if eventPlayback {
 				rb.AddSplitMessageFromClient(item.Nick, item.AccountName, item.IsBot, item.Tags, "TAGMSG", chname, item.Message)
+			} else if chathistoryCommand {
+				// #1676, we have to send something here or else it breaks pagination
+				rb.AddFromClient(item.Message.Time, history.HistservMungeMsgid(item.Message.Msgid), histservService.prefix, "*", false, nil, "PRIVMSG", chname, fmt.Sprintf(client.t("%s sent a TAGMSG"), nick))
 			}
 		case history.Join:
 			if eventPlayback {

irc/client.go

@@ -853,7 +853,7 @@ func (session *Session) Ping() {
 	session.Send(nil, "", "PING", session.client.Nick())
 }
 
-func (client *Client) replayPrivmsgHistory(rb *ResponseBuffer, items []history.Item, target string) {
+func (client *Client) replayPrivmsgHistory(rb *ResponseBuffer, items []history.Item, target string, chathistoryCommand bool) {
 	var batchID string
 	details := client.Details()
 	nick := details.nick
@@ -893,10 +893,15 @@ func (client *Client) replayPrivmsgHistory(rb *ResponseBuffer, items []history.I
 		case history.Tagmsg:
 			if hasEventPlayback && hasTags {
 				command = "TAGMSG"
+			} else if chathistoryCommand {
+				// #1676: send something for TAGMSG; we can't discard it entirely
+				// because it'll break pagination
+				rb.AddFromClient(item.Message.Time, history.HistservMungeMsgid(item.Message.Msgid), histservService.prefix, "*", false, nil, "PRIVMSG", fmt.Sprintf(client.t("%[1]s sent you a TAGMSG"), NUHToNick(item.Nick)))
 			} else {
 				continue
 			}
 		default:
+			// see #1676, this shouldn't happen
 			continue
 		}
 		var tags map[string]string
@@ -1713,7 +1718,7 @@ func (client *Client) listTargets(start, end history.Selector, limit int) (resul
 	var base, extras []history.TargetListing
 	var chcfnames []string
 	for _, channel := range client.Channels() {
-		_, seq, err := client.server.GetHistorySequence(channel, client, "", 0)
+		_, seq, err := client.server.GetHistorySequence(channel, client, "")
 		if seq == nil || err != nil {
 			continue
 		}
@@ -1734,7 +1739,7 @@ func (client *Client) listTargets(start, end history.Selector, limit int) (resul
 		extras = append(extras, persistentExtras...)
 	}
 
-	_, cSeq, err := client.server.GetHistorySequence(nil, client, "", 0)
+	_, cSeq, err := client.server.GetHistorySequence(nil, client, "")
 	if err == nil && cSeq != nil {
 		correspondents, err := cSeq.ListCorrespondents(start, end, limit)
 		if err == nil {
@@ -1758,7 +1763,7 @@ func (client *Client) privmsgsBetween(startTime, endTime time.Time, targetLimit,
 		if strings.HasPrefix(target.CfName, "#") {
 			continue
 		}
-		_, seq, err := client.server.GetHistorySequence(nil, client, target.CfName, 0)
+		_, seq, err := client.server.GetHistorySequence(nil, client, target.CfName)
 		if err == nil && seq != nil {
 			items, err := seq.Between(start, end, messageLimit)
 			if err == nil {

irc/database.go

@@ -24,7 +24,7 @@ const (
 	// 'version' of the database schema
 	keySchemaVersion = "db.version"
 	// latest schema of the db
-	latestDbSchema = 21
+	latestDbSchema = 22
 
 	keyCloakSecret = "crypto.cloak_secret"
 )
@@ -1059,6 +1059,56 @@ func schemaChangeV20To21(config *Config, tx *buntdb.Tx) error {
 	return nil
 }
 
+// #1676: we used to have ReplayJoinsNever, now it's desupported
+func schemaChangeV21To22(config *Config, tx *buntdb.Tx) error {
+	type accountSettingsv22 struct {
+		AutoreplayLines  *int
+		NickEnforcement  NickEnforcementMethod
+		AllowBouncer     MulticlientAllowedSetting
+		ReplayJoins      ReplayJoinsSetting
+		AlwaysOn         PersistentStatus
+		AutoreplayMissed bool
+		DMHistory        HistoryStatus
+		AutoAway         PersistentStatus
+		Email            string
+	}
+
+	var accounts []string
+	var serializedSettings []string
+	settingsPrefix := "account.settings "
+	tx.AscendGreaterOrEqual("", settingsPrefix, func(key, value string) bool {
+		if !strings.HasPrefix(key, settingsPrefix) {
+			return false
+		}
+		account := strings.TrimPrefix(key, settingsPrefix)
+		if _, err := tx.Get("account.verified " + account); err != nil {
+			return true
+		}
+		var settings accountSettingsv22
+		err := json.Unmarshal([]byte(value), &settings)
+		if err != nil {
+			log.Printf("error (v21-22) processing settings for %s: %v\n", account, err)
+			return true
+		}
+		// if necessary, change ReplayJoinsNever (2) to ReplayJoinsCommandsOnly (0)
+		if settings.ReplayJoins == ReplayJoinsSetting(2) {
+			settings.ReplayJoins = ReplayJoinsSetting(0)
+			if b, err := json.Marshal(settings); err == nil {
+				accounts = append(accounts, account)
+				serializedSettings = append(serializedSettings, string(b))
+			} else {
+				log.Printf("error (v21-22) processing settings for %s: %v\n", account, err)
+			}
+		}
+		return true
+	})
+
+	for i, account := range accounts {
+		tx.Set(settingsPrefix+account, serializedSettings[i], nil)
+	}
+	return nil
+}
+
 func getSchemaChange(initialVersion int) (result SchemaChange, ok bool) {
 	for _, change := range allChanges {
 		if initialVersion == change.InitialVersion {
@@ -1169,4 +1219,9 @@ var allChanges = []SchemaChange{
 		TargetVersion:  21,
 		Changer:        schemaChangeV20To21,
 	},
+	{
+		InitialVersion: 21,
+		TargetVersion:  22,
+		Changer:        schemaChangeV21To22,
+	},
 }

irc/handlers.go

@@ -611,9 +611,9 @@ func chathistoryHandler(server *Server, client *Client, msg ircmsg.Message, rb *
 						target.Time.Format(IRCv3TimestampFormat))
 				}
 			} else if channel != nil {
-				channel.replayHistoryItems(rb, items, false)
+				channel.replayHistoryItems(rb, items, true)
 			} else {
-				client.replayPrivmsgHistory(rb, items, target)
+				client.replayPrivmsgHistory(rb, items, target, true)
 			}
 		}
 	}()
@@ -725,17 +725,7 @@ func chathistoryHandler(server *Server, client *Client, msg ircmsg.Message, rb *
 	if listTargets {
 		targets, err = client.listTargets(start, end, limit)
 	} else {
-		// see #1676; for CHATHISTORY we need to make the paging window as exact as possible,
+		channel, sequence, err = server.GetHistorySequence(nil, client, target)
-		// hence filtering out undisplayable messages on the backend, in order to send a full
-		// paging window if possible
-		var flags history.ExcludeFlags
-		if !rb.session.capabilities.Has(caps.EventPlayback) {
-			flags |= history.ExcludeTagmsg
-		}
-		if client.AccountSettings().ReplayJoins == ReplayJoinsNever {
-			flags |= history.ExcludeJoins
-		}
-		channel, sequence, err = server.GetHistorySequence(nil, client, target, flags)
 		if err != nil || sequence == nil {
 			return
 		}
@@ -1132,9 +1122,9 @@ func historyHandler(server *Server, client *Client, msg ircmsg.Message, rb *Resp
 
 	if len(items) != 0 {
 		if channel != nil {
-			channel.replayHistoryItems(rb, items, false)
+			channel.replayHistoryItems(rb, items, true)
 		} else {
-			client.replayPrivmsgHistory(rb, items, "")
+			client.replayPrivmsgHistory(rb, items, "", true)
 		}
 	}
 	return false

irc/history/history.go

@@ -53,17 +53,6 @@ func (item *Item) HasMsgid(msgid string) bool {
 	return item.Message.Msgid == msgid
 }
 
-func (item *Item) IsExcluded(excludeFlags ExcludeFlags) bool {
-	switch item.Type {
-	case Tagmsg:
-		return excludeFlags&ExcludeTagmsg != 0
-	case Join, Part, Quit:
-		return excludeFlags&ExcludeJoins != 0
-	default:
-		return false
-	}
-}
-
 type Predicate func(item *Item) (matches bool)
 
 func Reverse(results []Item) {
@@ -166,7 +155,7 @@ func (list *Buffer) lookup(msgid string) (result Item, found bool) {
 // with an indication of whether the results are complete or are missing items
 // because some of that period was discarded. A zero value of `before` is considered
 // higher than all other times.
-func (list *Buffer) betweenHelper(start, end Selector, cutoff time.Time, pred Predicate, limit int, excludeFlags ExcludeFlags) (results []Item, complete bool, err error) {
+func (list *Buffer) betweenHelper(start, end Selector, cutoff time.Time, pred Predicate, limit int) (results []Item, complete bool, err error) {
 	var ascending bool
 
 	defer func() {
@@ -206,8 +195,7 @@ func (list *Buffer) betweenHelper(start, end Selector, cutoff time.Time, pred Pr
 	satisfies := func(item *Item) bool {
 		return (after.IsZero() || item.Message.Time.After(after)) &&
 			(before.IsZero() || item.Message.Time.Before(before)) &&
-			(pred == nil || pred(item)) &&
+			(pred == nil || pred(item))
-			!item.IsExcluded(excludeFlags)
 	}
 
 	return list.matchInternal(satisfies, ascending, limit), complete, nil
@@ -291,10 +279,9 @@ type bufferSequence struct {
 	list   *Buffer
 	pred   Predicate
 	cutoff time.Time
-	flags  ExcludeFlags
 }
 
-func (list *Buffer) MakeSequence(correspondent string, cutoff time.Time, flags ExcludeFlags) Sequence {
+func (list *Buffer) MakeSequence(correspondent string, cutoff time.Time) Sequence {
 	var pred Predicate
 	if correspondent != "" {
 		pred = func(item *Item) bool {
@@ -305,12 +292,11 @@ func (list *Buffer) MakeSequence(correspondent string, cutoff time.Time, flags E
 		list:   list,
 		pred:   pred,
 		cutoff: cutoff,
-		flags:  flags,
 	}
 }
 
 func (seq *bufferSequence) Between(start, end Selector, limit int) (results []Item, err error) {
-	results, _, err = seq.list.betweenHelper(start, end, seq.cutoff, seq.pred, limit, seq.flags)
+	results, _, err = seq.list.betweenHelper(start, end, seq.cutoff, seq.pred, limit)
 	return
 }
 
@@ -391,7 +377,7 @@ func (list *Buffer) Delete(predicate Predicate) (count int) {
 // latest returns the items most recently added, up to `limit`. If `limit` is 0,
 // it returns all items.
 func (list *Buffer) latest(limit int) (results []Item) {
-	results, _, _ = list.betweenHelper(Selector{}, Selector{}, time.Time{}, nil, limit, 0)
+	results, _, _ = list.betweenHelper(Selector{}, Selector{}, time.Time{}, nil, limit)
 	return
 }
 

irc/history/history_test.go

@@ -15,7 +15,7 @@ const (
 )
 
 func betweenTimestamps(buf *Buffer, start, end time.Time, limit int) (result []Item, complete bool) {
-	result, complete, _ = buf.betweenHelper(Selector{Time: start}, Selector{Time: end}, time.Time{}, nil, limit, 0)
+	result, complete, _ = buf.betweenHelper(Selector{Time: start}, Selector{Time: end}, time.Time{}, nil, limit)
 	return
 }
 
@@ -45,7 +45,7 @@ func TestEmptyBuffer(t *testing.T) {
 	})
 	since, complete = betweenTimestamps(buf, pastTime, time.Now(), 0)
 	if len(since) != 1 {
-		t.Errorf("should be able to store items in a nonempty buffer: expected %d, got %d", 1, len(since))
+		t.Error("should be able to store items in a nonempty buffer")
 	}
 	if !complete {
 		t.Error("results should be complete")

irc/history/queries.go

@@ -8,13 +8,6 @@ import (
 	"time"
 )
 
-type ExcludeFlags uint
-
-const (
-	ExcludeTagmsg ExcludeFlags = 1 << iota
-	ExcludeJoins
-)
-
 // Selector represents a parameter to a CHATHISTORY command
 type Selector struct {
 	Msgid string

irc/histserv.go

@@ -199,7 +199,7 @@ func histservPlayHandler(service *ircService, server *Server, client *Client, co
 
 // handles parameter parsing and history queries for /HISTORY and /HISTSERV PLAY
 func easySelectHistory(server *Server, client *Client, params []string) (items []history.Item, channel *Channel, err error) {
-	channel, sequence, err := server.GetHistorySequence(nil, client, params[0], 0)
+	channel, sequence, err := server.GetHistorySequence(nil, client, params[0])
 
 	if sequence == nil || err != nil {
 		return nil, nil, errNoSuchChannel

irc/mysql/history.go

@@ -40,10 +40,6 @@ const (
 	keySchemaMinorVersion = "db.minorversion"
 	cleanupRowLimit       = 50
 	cleanupPauseTime      = 10 * time.Minute
-
-	// if we don't fill the pagination window due to exclusions,
-	// retry with an expanded window at most this many times
-	maxPaginationRetries = 3
 )
 
 type e struct{}
@@ -1037,18 +1033,9 @@ type mySQLHistorySequence struct {
 	target        string
 	correspondent string
 	cutoff        time.Time
-	excludeFlags  history.ExcludeFlags
 }
 
 func (s *mySQLHistorySequence) Between(start, end history.Selector, limit int) (results []history.Item, err error) {
-	if s.excludeFlags == 0 {
-		return s.baseBetween(start, end, limit)
-	} else {
-		return s.betweenWithRetries(start, end, limit)
-	}
-}
-
-func (s *mySQLHistorySequence) baseBetween(start, end history.Selector, limit int) (results []history.Item, err error) {
 	ctx, cancel := context.WithTimeout(context.Background(), s.mysql.getTimeout())
 	defer cancel()
 
@@ -1071,45 +1058,7 @@ func (s *mySQLHistorySequence) baseBetween(start, end history.Selector, limit in
 	return results, err
 }
 
-func (s *mySQLHistorySequence) betweenWithRetries(start, end history.Selector, limit int) (results []history.Item, err error) {
-	applyExclusions := func(currentResults []history.Item, excludeFlags history.ExcludeFlags, trueLimit int) (filteredResults []history.Item) {
-		filteredResults = make([]history.Item, 0, len(currentResults))
-		for _, item := range currentResults {
-			if !item.IsExcluded(excludeFlags) {
-				filteredResults = append(filteredResults, item)
-			}
-			if len(filteredResults) == trueLimit {
-				break
-			}
-		}
-		return
-	}
-
-	i := 1
-	for {
-		currentLimit := limit * i
-		currentResults, err := s.baseBetween(start, end, currentLimit)
-		if err != nil {
-			return nil, err
-		}
-		results = applyExclusions(currentResults, s.excludeFlags, limit)
-		// we're done in any of these three cases:
-		// (1) we filled the window (2) we ran out of results on the backend (3) we can't retry anymore
-		if len(results) == limit || len(currentResults) < currentLimit || i == maxPaginationRetries {
-			return results, nil
-		}
-		i++
-	}
-}
-
 func (s *mySQLHistorySequence) Around(start history.Selector, limit int) (results []history.Item, err error) {
-	// temporarily clear the exclude flags when running GenericAround, since we don't care about
-	// the exactness of the paging window at all
-	oldExcludeFlags := s.excludeFlags
-	s.excludeFlags = 0
-	defer func() {
-		s.excludeFlags = oldExcludeFlags
-	}()
 	return history.GenericAround(s, start, limit)
 }
 
@@ -1134,12 +1083,11 @@ func (seq *mySQLHistorySequence) Ephemeral() bool {
 	return false
 }
 
-func (mysql *MySQL) MakeSequence(target, correspondent string, cutoff time.Time, excludeFlags history.ExcludeFlags) history.Sequence {
+func (mysql *MySQL) MakeSequence(target, correspondent string, cutoff time.Time) history.Sequence {
 	return &mySQLHistorySequence{
 		target:        target,
 		correspondent: correspondent,
 		mysql:         mysql,
 		cutoff:        cutoff,
-		excludeFlags:  excludeFlags,
 	}
 }

irc/nickserv.go

@@ -284,8 +284,8 @@ default.`,
 				`$bREPLAY-JOINS$b
 'replay-joins' controls whether replayed channel history will include
 lines for join and part. This provides more information about the context of
-messages, but may be spammy. Your options are 'always', 'never', and the default
+messages, but may be spammy. Your options are 'always' and the default of
-of 'commands-only' (the messages will be replayed in /HISTORY output, but not
+'commands-only' (the messages will be replayed in CHATHISTORY output, but not
 during autoreplay).`,
 				`$bALWAYS-ON$b
 'always-on' controls whether your nickname/identity will remain active
@@ -440,8 +440,6 @@ func displaySetting(service *ircService, settingName string, settings AccountSet
 			service.Notice(rb, client.t("You will see JOINs and PARTs in /HISTORY output, but not in autoreplay"))
 		case ReplayJoinsAlways:
 			service.Notice(rb, client.t("You will see JOINs and PARTs in /HISTORY output and in autoreplay"))
-		case ReplayJoinsNever:
-			service.Notice(rb, client.t("You will not see JOINs and PARTs in /HISTORY output or in autoreplay"))
 		}
 	case "multiclient":
 		if !config.Accounts.Multiclient.Enabled {

irc/server.go

@@ -868,7 +868,7 @@ func (server *Server) setupListeners(config *Config) (err error) {
 // suitable for ListCorrespondents (i.e., this function is still used to
 // decide whether the ringbuf or mysql is authoritative about the client's
 // message history).
-func (server *Server) GetHistorySequence(providedChannel *Channel, client *Client, query string, excludeFlags history.ExcludeFlags) (channel *Channel, sequence history.Sequence, err error) {
+func (server *Server) GetHistorySequence(providedChannel *Channel, client *Client, query string) (channel *Channel, sequence history.Sequence, err error) {
 	config := server.Config()
 	// 4 cases: {persistent, ephemeral} x {normal, conversation}
 	// with ephemeral history, target is implicit in the choice of `hist`,
@@ -946,9 +946,9 @@ func (server *Server) GetHistorySequence(providedChannel *Channel, client *Clien
 	}
 
 	if hist != nil {
-		sequence = hist.MakeSequence(correspondent, cutoff, excludeFlags)
+		sequence = hist.MakeSequence(correspondent, cutoff)
 	} else if target != "" {
-		sequence = server.historyDB.MakeSequence(target, correspondent, cutoff, excludeFlags)
+		sequence = server.historyDB.MakeSequence(target, correspondent, cutoff)
 	}
 	return
 }

irc/znc.go

@@ -189,14 +189,14 @@ func zncPlaybackPlayHandler(client *Client, command string, params []string, rb
 }
 
 func zncPlayPrivmsgsFrom(client *Client, rb *ResponseBuffer, target string, start, end time.Time) {
-	_, sequence, err := client.server.GetHistorySequence(nil, client, target, 0)
+	_, sequence, err := client.server.GetHistorySequence(nil, client, target)
 	if sequence == nil || err != nil {
 		return
 	}
 	zncMax := client.server.Config().History.ZNCMax
 	items, err := sequence.Between(history.Selector{Time: start}, history.Selector{Time: end}, zncMax)
 	if err == nil && len(items) != 0 {
-		client.replayPrivmsgHistory(rb, items, target)
+		client.replayPrivmsgHistory(rb, items, target, false)
 	}
 }
 
@@ -204,7 +204,7 @@ func zncPlayPrivmsgsFromAll(client *Client, rb *ResponseBuffer, start, end time.
 	zncMax := client.server.Config().History.ZNCMax
 	items, err := client.privmsgsBetween(start, end, maxDMTargetsForAutoplay, zncMax)
 	if err == nil && len(items) != 0 {
-		client.replayPrivmsgHistory(rb, items, "")
+		client.replayPrivmsgHistory(rb, items, "", false)
 	}
 }
 

irctest

@@ -1 +1 @@
-Subproject commit 33f0702c260ea716a4ad0f24821a50d44c91fca1
+Subproject commit 5e4ae7c99965801cd91d974637ad344a47b5414f

traditional.yaml

@@ -939,7 +939,7 @@ history:
         # if `default` is false, store TAGMSG containing any of these tags:
         whitelist:
             - "+draft/react"
-            - "react"
+            - "+react"
 
         # if `default` is true, don't store TAGMSG containing any of these tags:
         #blacklist: