~emersion/soju-dev

This thread contains a patchset. You're looking at the original emails, but you may wish to use the patch review UI. Review patch
3 2

[PATCH v2] Forward RPL_TOPICWHOTIME to downstreams

Details
Message ID
<20200820081313.166620-1-hubert@hirtzfr.eu>
DKIM signature
pass
Download raw message
Patch: +38 -5
---
 bridge.go   | 16 +++++++++++++---
 upstream.go | 27 +++++++++++++++++++++++++--
 2 files changed, 38 insertions(+), 5 deletions(-)

diff --git a/bridge.go b/bridge.go
index e434c49..c4df602 100644
--- a/bridge.go
@@ -1,8 +1,10 @@
package soju

import (
	"gopkg.in/irc.v3"
	"strconv"
	"strings"

	"gopkg.in/irc.v3"
)

func forwardChannel(dc *downstreamConn, ch *upstreamChannel) {
@@ -11,8 +13,6 @@ func forwardChannel(dc *downstreamConn, ch *upstreamChannel) {
	}

	sendTopic(dc, ch)

	// TODO: rpl_topicwhotime
	sendNames(dc, ch)
}

@@ -25,6 +25,16 @@ func sendTopic(dc *downstreamConn, ch *upstreamChannel) {
			Command: irc.RPL_TOPIC,
			Params:  []string{dc.nick, downstreamName, ch.Topic},
		})
		if ch.TopicWho != nil {
			topicWho := ch.TopicWho.Copy()
			topicWho.Name = dc.marshalEntity(ch.conn.network, topicWho.Name)
			topicTime := strconv.FormatInt(ch.TopicTime.Unix(), 10)
			dc.SendMessage(&irc.Message{
				Prefix:  dc.srv.prefix(),
				Command: rpl_topicwhotime,
				Params:  []string{dc.nick, downstreamName, topicWho.String(), topicTime},
			})
		}
	} else {
		dc.SendMessage(&irc.Message{
			Prefix:  dc.srv.prefix(),
diff --git a/upstream.go b/upstream.go
index 07d6266..f95e011 100644
--- a/upstream.go
+++ b/upstream.go
@@ -36,7 +36,7 @@ type upstreamChannel struct {
	Name         string
	conn         *upstreamConn
	Topic        string
	TopicWho     string
	TopicWho     *irc.Prefix
	TopicTime    time.Time
	Status       channelStatus
	modes        channelModes
@@ -830,6 +830,10 @@ func (uc *upstreamConn) handleMessage(msg *irc.Message) error {
			ch.Topic = ""
		}
	case "TOPIC":
		if msg.Prefix == nil {
			return fmt.Errorf("expected a prefix")
		}

		var name string
		if err := parseMessageParams(msg, &name); err != nil {
			return err
@@ -840,6 +844,8 @@ func (uc *upstreamConn) handleMessage(msg *irc.Message) error {
		}
		if len(msg.Params) > 1 {
			ch.Topic = msg.Params[1]
			ch.TopicWho = msg.Prefix.Copy()
			ch.TopicTime = time.Now() // TODO use msg.Tags["time"]
		} else {
			ch.Topic = ""
		}
@@ -971,12 +977,29 @@ func (uc *upstreamConn) handleMessage(msg *irc.Message) error {
		if err != nil {
			return err
		}
		ch.TopicWho = who
		firstTopicWhoTime := ch.TopicWho == nil
		ch.TopicWho = irc.ParsePrefix(who)
		sec, err := strconv.ParseInt(timeStr, 10, 64)
		if err != nil {
			return fmt.Errorf("failed to parse topic time: %v", err)
		}
		ch.TopicTime = time.Unix(sec, 0)
		if firstTopicWhoTime {
			uc.forEachDownstream(func(dc *downstreamConn) {
				topicWho := ch.TopicWho.Copy()
				topicWho.Name = dc.marshalEntity(uc.network, topicWho.Name)
				dc.SendMessage(&irc.Message{
					Prefix:  dc.srv.prefix(),
					Command: rpl_topicwhotime,
					Params: []string{
						dc.nick,
						dc.marshalEntity(uc.network, ch.Name),
						topicWho.String(),
						timeStr,
					},
				})
			})
		}
	case irc.RPL_LIST:
		var channel, clients, topic string
		if err := parseMessageParams(msg, nil, &channel, &clients, &topic); err != nil {
-- 
2.28.0
Details
Message ID
<Jsb9WKm9mZd9ZpmZ3oSQgC2YAZqinImX8LDUFuSuKOsSDSW4pyEZmLUNIeZDrQC_pXtri3t6Bwgx-Mfow5_9mLPsQrv0Gd8g09k6Ou5WwCM=@emersion.fr>
In-Reply-To
<20200820081313.166620-1-hubert@hirtzfr.eu> (view parent)
DKIM signature
fail
Download raw message
DKIM signature: fail
Two minor comments, LGTM otherwise!

> ---
>  bridge.go   | 16 +++++++++++++---
>  upstream.go | 27 +++++++++++++++++++++++++--
>  2 files changed, 38 insertions(+), 5 deletions(-)
>
> diff --git a/bridge.go b/bridge.go
> index e434c49..c4df602 100644
> --- a/bridge.go
> +++ b/bridge.go
> @@ -1,8 +1,10 @@
>  package soju
>
>  import (
> -	"gopkg.in/irc.v3"
> +	"strconv"
>  	"strings"
> +
> +	"gopkg.in/irc.v3"
>  )
>
>  func forwardChannel(dc *downstreamConn, ch *upstreamChannel) {
> @@ -11,8 +13,6 @@ func forwardChannel(dc *downstreamConn, ch *upstreamChannel) {
>  	}
>
>  	sendTopic(dc, ch)
> -
> -	// TODO: rpl_topicwhotime
>  	sendNames(dc, ch)
>  }
>
> @@ -25,6 +25,16 @@ func sendTopic(dc *downstreamConn, ch *upstreamChannel) {
>  			Command: irc.RPL_TOPIC,
>  			Params:  []string{dc.nick, downstreamName, ch.Topic},
>  		})
> +		if ch.TopicWho != nil {
> +			topicWho := ch.TopicWho.Copy()
> +			topicWho.Name = dc.marshalEntity(ch.conn.network, topicWho.Name)

We have marshalUserPrefix for this purpose.

> +			topicTime := strconv.FormatInt(ch.TopicTime.Unix(), 10)
> +			dc.SendMessage(&irc.Message{
> +				Prefix:  dc.srv.prefix(),
> +				Command: rpl_topicwhotime,
> +				Params:  []string{dc.nick, downstreamName, topicWho.String(), topicTime},
> +			})
> +		}
>  	} else {
>  		dc.SendMessage(&irc.Message{
>  			Prefix:  dc.srv.prefix(),
> diff --git a/upstream.go b/upstream.go
> index 07d6266..f95e011 100644
> --- a/upstream.go
> +++ b/upstream.go
> @@ -36,7 +36,7 @@ type upstreamChannel struct {
>  	Name         string
>  	conn         *upstreamConn
>  	Topic        string
> -	TopicWho     string
> +	TopicWho     *irc.Prefix
>  	TopicTime    time.Time
>  	Status       channelStatus
>  	modes        channelModes
> @@ -830,6 +830,10 @@ func (uc *upstreamConn) handleMessage(msg *irc.Message) error {
>  			ch.Topic = ""
>  		}
>  	case "TOPIC":
> +		if msg.Prefix == nil {
> +			return fmt.Errorf("expected a prefix")
> +		}
> +
>  		var name string
>  		if err := parseMessageParams(msg, &name); err != nil {
>  			return err
> @@ -840,6 +844,8 @@ func (uc *upstreamConn) handleMessage(msg *irc.Message) error {
>  		}
>  		if len(msg.Params) > 1 {
>  			ch.Topic = msg.Params[1]
> +			ch.TopicWho = msg.Prefix.Copy()

Is it necessary to perform a copy here?

> +			ch.TopicTime = time.Now() // TODO use msg.Tags["time"]
>  		} else {
>  			ch.Topic = ""
>  		}
> @@ -971,12 +977,29 @@ func (uc *upstreamConn) handleMessage(msg *irc.Message) error {
>  		if err != nil {
>  			return err
>  		}
> -		ch.TopicWho = who
> +		firstTopicWhoTime := ch.TopicWho == nil
> +		ch.TopicWho = irc.ParsePrefix(who)
>  		sec, err := strconv.ParseInt(timeStr, 10, 64)
>  		if err != nil {
>  			return fmt.Errorf("failed to parse topic time: %v", err)
>  		}
>  		ch.TopicTime = time.Unix(sec, 0)
> +		if firstTopicWhoTime {
> +			uc.forEachDownstream(func(dc *downstreamConn) {
> +				topicWho := ch.TopicWho.Copy()
> +				topicWho.Name = dc.marshalEntity(uc.network, topicWho.Name)

Ditto

> +				dc.SendMessage(&irc.Message{
> +					Prefix:  dc.srv.prefix(),
> +					Command: rpl_topicwhotime,
> +					Params: []string{
> +						dc.nick,
> +						dc.marshalEntity(uc.network, ch.Name),
> +						topicWho.String(),
> +						timeStr,
> +					},
> +				})
> +			})
> +		}
>  	case irc.RPL_LIST:
>  		var channel, clients, topic string
>  		if err := parseMessageParams(msg, nil, &channel, &clients, &topic); err != nil {
> --
> 2.28.0
Details
Message ID
<5fb5eb97-4e27-ff8c-0656-754bfade6b7b@hirtzfr.eu>
In-Reply-To
<Jsb9WKm9mZd9ZpmZ3oSQgC2YAZqinImX8LDUFuSuKOsSDSW4pyEZmLUNIeZDrQC_pXtri3t6Bwgx-Mfow5_9mLPsQrv0Gd8g09k6Ou5WwCM=@emersion.fr> (view parent)
DKIM signature
pass
Download raw message
> Is it necessary to perform a copy here?

> Ditto


In the first case, not right now.  However, the prefix is behind a
pointer.  IIUC should there be no copy, modifying the prefix later in
the function would also modify "TopicWho".

In the second case, we're actually modifying the value of
"TopicWho.Name" in each iteration, so the "Copy()" is necessary (again,
I may be wrong, go is a very subtle language :).

FTR, "irc.Prefix.Copy()" doesn't copy the strings, only the string pointers.
Details
Message ID
<l_xm2xcFtHgdYzxDnLjt4VcIdT3LWqwkwZ-fzzm5NQ-HzDbAYSjDnljaEP7XAKoHicTvs7HnJO6qHjKEhq-Z16uo_wha0T6T6y6lGtwBroI=@emersion.fr>
In-Reply-To
<5fb5eb97-4e27-ff8c-0656-754bfade6b7b@hirtzfr.eu> (view parent)
DKIM signature
fail
Download raw message
DKIM signature: fail
On Thursday, August 20, 2020 10:27 AM, Hubert Hirtz <hubert@hirtzfr.eu> wrote:

> > Is it necessary to perform a copy here?
>
> > Ditto
>
> In the first case, not right now. However, the prefix is behind a
> pointer. IIUC should there be no copy, modifying the prefix later in
> the function would also modify "TopicWho".

I guess an extra copy is better than a missing copy, so I'm not feeling
strongly about this one.

> In the second case, we're actually modifying the value of
> "TopicWho.Name" in each iteration, so the "Copy()" is necessary (again,
> I may be wrong, go is a very subtle language :).

I should have been more explicit: this "Ditto" was about using
marshalUserPrefix, not about the copy.
Review patch Export thread (mbox)