From 38a6d17ee5ce6e1096c3dfd6d11f6f35d9a71ca6 Mon Sep 17 00:00:00 2001 From: Shivaram Lingamneni Date: Thu, 1 Jun 2023 04:43:13 -0400 Subject: [PATCH 1/3] clean up nested batch logic --- irc/caps/constants.go | 2 ++ irc/channel.go | 2 +- irc/client.go | 2 +- irc/handlers.go | 6 ++---- irc/responsebuffer.go | 16 +++------------- 5 files changed, 9 insertions(+), 19 deletions(-) diff --git a/irc/caps/constants.go b/irc/caps/constants.go index da37c1ed..57f088e3 100644 --- a/irc/caps/constants.go +++ b/irc/caps/constants.go @@ -62,6 +62,8 @@ const ( RelaymsgTagName = "draft/relaymsg" // BOT mode: https://ircv3.net/specs/extensions/bot-mode BotTagName = "bot" + // https://ircv3.net/specs/extensions/chathistory + ChathistoryTargetsBatchType = "draft/chathistory-targets" ) func init() { diff --git a/irc/channel.go b/irc/channel.go index d8c5606f..61b5a159 100644 --- a/irc/channel.go +++ b/irc/channel.go @@ -1059,7 +1059,7 @@ func (channel *Channel) replayHistoryItems(rb *ResponseBuffer, items []history.I } } - batchID := rb.StartNestedHistoryBatch(chname) + batchID := rb.StartNestedBatch(chname, "chathistory") defer rb.EndNestedBatch(batchID) for _, item := range items { diff --git a/irc/client.go b/irc/client.go index f26985aa..feca599b 100644 --- a/irc/client.go +++ b/irc/client.go @@ -850,7 +850,7 @@ func (client *Client) replayPrivmsgHistory(rb *ResponseBuffer, items []history.I if target == "" { target = nick } - batchID = rb.StartNestedHistoryBatch(target) + batchID = rb.StartNestedBatch(target, "chathistory") isSelfMessage := func(item *history.Item) bool { // XXX: Params[0] is the message target. if the source of this message is an in-memory diff --git a/irc/handlers.go b/irc/handlers.go index 0c54a02c..71c724ef 100644 --- a/irc/handlers.go +++ b/irc/handlers.go @@ -655,10 +655,8 @@ func chathistoryHandler(server *Server, client *Client, msg ircmsg.Message, rb * } else { // successful responses are sent as a chathistory or history batch if listTargets { - if rb.session.capabilities.Has(caps.Batch) { // #2066 - batchID := rb.StartNestedBatch("draft/chathistory-targets") - defer rb.EndNestedBatch(batchID) - } + batchID := rb.StartNestedBatch(caps.ChathistoryTargetsBatchType) + defer rb.EndNestedBatch(batchID) for _, target := range targets { name := server.UnfoldName(target.CfName) rb.Add(nil, server.name, "CHATHISTORY", "TARGETS", name, diff --git a/irc/responsebuffer.go b/irc/responsebuffer.go index bacfbc11..6dfeade8 100644 --- a/irc/responsebuffer.go +++ b/irc/responsebuffer.go @@ -193,6 +193,9 @@ func (rb *ResponseBuffer) sendBatchEnd(blocking bool) { // Starts a nested batch (see the ResponseBuffer struct definition for a description of // how this works) func (rb *ResponseBuffer) StartNestedBatch(batchType string, params ...string) (batchID string) { + if !rb.session.capabilities.Has(caps.Batch) { + return + } batchID = rb.session.generateBatchID() msgParams := make([]string, len(params)+2) msgParams[0] = "+" + batchID @@ -219,19 +222,6 @@ func (rb *ResponseBuffer) EndNestedBatch(batchID string) { rb.AddMessage(ircmsg.MakeMessage(nil, rb.target.server.name, "BATCH", "-"+batchID)) } -// Convenience to start a nested batch for history lines, at the highest level -// supported by the client (`history`, `chathistory`, or no batch, in descending order). -func (rb *ResponseBuffer) StartNestedHistoryBatch(params ...string) (batchID string) { - var batchType string - if rb.session.capabilities.Has(caps.Batch) { - batchType = "chathistory" - } - if batchType != "" { - batchID = rb.StartNestedBatch(batchType, params...) - } - return -} - // Send sends all messages in the buffer to the client. // Afterwards, the buffer is in an undefined state and MUST NOT be used further. // If `blocking` is true you MUST be sending to the client from its own goroutine. From 60af8ee491d1f713b8a2d347dcad0c0dfb458d15 Mon Sep 17 00:00:00 2001 From: Shivaram Lingamneni Date: Thu, 1 Jun 2023 06:28:02 -0400 Subject: [PATCH 2/3] clean up force-trailing logic --- irc/client.go | 30 +++++++++++++++--------------- irc/message_cache.go | 6 ++---- irc/utils/types.go | 8 ++++++++ 3 files changed, 25 insertions(+), 19 deletions(-) diff --git a/irc/client.go b/irc/client.go index feca599b..1f74dee9 100644 --- a/irc/client.go +++ b/irc/client.go @@ -1425,27 +1425,27 @@ func composeMultilineBatch(batchID, fromNickMask, fromAccount string, isBot bool } var ( - // these are all the output commands that MUST have their last param be a trailing. - // this is needed because dumb clients like to treat trailing params separately from the - // other params in messages. - commandsThatMustUseTrailing = map[string]bool{ - "PRIVMSG": true, - "NOTICE": true, - - RPL_WHOISCHANNELS: true, - RPL_USERHOST: true, - + // in practice, many clients require that the final parameter be a trailing + // (prefixed with `:`) even when this is not syntactically necessary. + // by default, force the following commands to use a trailing: + commandsThatMustUseTrailing = utils.SetLiteral( + "PRIVMSG", + "NOTICE", + RPL_WHOISCHANNELS, + RPL_USERHOST, // mirc's handling of RPL_NAMREPLY is broken: // https://forums.mirc.com/ubbthreads.php/topics/266939/re-nick-list - RPL_NAMREPLY: true, - } + RPL_NAMREPLY, + ) ) +func forceTrailing(config *Config, command string) bool { + return config.Server.Compatibility.forceTrailing && commandsThatMustUseTrailing.Has(command) +} + // SendRawMessage sends a raw message to the client. func (session *Session) SendRawMessage(message ircmsg.Message, blocking bool) error { - // use dumb hack to force the last param to be a trailing param if required - config := session.client.server.Config() - if config.Server.Compatibility.forceTrailing && commandsThatMustUseTrailing[message.Command] { + if forceTrailing(session.client.server.Config(), message.Command) { message.ForceTrailing() } diff --git a/irc/message_cache.go b/irc/message_cache.go index cde8b68d..2d731823 100644 --- a/irc/message_cache.go +++ b/irc/message_cache.go @@ -79,8 +79,7 @@ func (m *MessageCache) Initialize(server *Server, serverTime time.Time, msgid st m.params = params var msg ircmsg.Message - config := server.Config() - if config.Server.Compatibility.forceTrailing && commandsThatMustUseTrailing[command] { + if forceTrailing(server.Config(), command) { msg.ForceTrailing() } msg.Source = nickmask @@ -111,8 +110,7 @@ func (m *MessageCache) InitializeSplitMessage(server *Server, nickmask, accountN m.target = target m.splitMessage = message - config := server.Config() - forceTrailing := config.Server.Compatibility.forceTrailing && commandsThatMustUseTrailing[command] + forceTrailing := forceTrailing(server.Config(), command) if message.Is512() { isTagmsg := command == "TAGMSG" diff --git a/irc/utils/types.go b/irc/utils/types.go index ca1e4f7a..8c2ca4eb 100644 --- a/irc/utils/types.go +++ b/irc/utils/types.go @@ -20,6 +20,14 @@ func (s HashSet[T]) Remove(elem T) { delete(s, elem) } +func SetLiteral[T comparable](elems ...T) HashSet[T] { + result := make(HashSet[T], len(elems)) + for _, elem := range elems { + result.Add(elem) + } + return result +} + func CopyMap[K comparable, V any](input map[K]V) (result map[K]V) { result = make(map[K]V, len(input)) for key, value := range input { From 3d4d8228aa3f7a254ab5f8657096a87bf24692de Mon Sep 17 00:00:00 2001 From: Shivaram Lingamneni Date: Fri, 2 Jun 2023 02:58:32 -0400 Subject: [PATCH 3/3] bump irctest --- irctest | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/irctest b/irctest index 22c6743b..5a5dbdb5 160000 --- a/irctest +++ b/irctest @@ -1 +1 @@ -Subproject commit 22c6743b24f2a85bf79a92fb0c7fab325c047a6c +Subproject commit 5a5dbdb50dda3dc9e5ed3d542ab75b1b87fccac9