Relay nick changes feature

[llmII]

This relies on #69

It allows Nick changes in IRC to be shown in Discord. This can be a source of annoyance but some people do want to see that level of info in Discord. This level of info also would be helpful in the future when I add a way for Discord to ignore IRC nicks (can see join/quit/part/nick change and build up a proper hostmask for addition to the config).

Right now this piggybacks on the show_joinquit feature and config setting. I think we may want to either rename that config variable to something along the lines of show_metainfo or make the relayed stuff fully configurable like

irc_relay_cmds:
  - PRIVMSG
  - JOIN
  - QUIT
  - PART
  - NICK
  - CTCP_ACTION
  - NOTICE

That said if the latter is preferred I'd think using the show_metainfo idea as a stop-gap until the latter is implemented a good idea. I'm not sure if it is worth the trouble to implement it that granularly.

[llmII]

Worked through it and made some fixes, now works properly.

This also includes a fix to puppet detection support. Didn't realize it before but we were clearing out the builtin callbacks for NICK which destroyed nick tracking! This would effect anything depending on channel.Users.

[llmII]

fatal error: concurrent map read and map write

goroutine 15238 [running]:
runtime.throw(0x89163f, 0x21)
        /usr/local/go/src/runtime/panic.go:1116 +0x72 fp=0xc000269c58 sp=0xc000269c28 pc=0x4368b2
runtime.mapaccess2_faststr(0x7fe8e0, 0xc000183020, 0xc000269d10, 0xc, 0xb, 0xc000269d10)
        /usr/local/go/src/runtime/map_faststr.go:116 +0x4a5 fp=0xc000269cc8 sp=0xc000269c58 pc=0x415685
github.com/qaisjp/go-discord-irc/bridge.userOnChannelFix(0xc00039c0ae, 0xb, 0x0, 0x0, 0x0, 0x0, 0xc000183020, 0x0)
        /home/user/src/go-discord-irc/bridge/irc_listener.go:67 +0x118 fp=0xc000269d40 sp=0xc000269cc8 pc=0x7298b8
github.com/qaisjp/go-discord-irc/bridge.(*ircListener).OnNickRelayToDiscord(0xc00000ee40, 0xc0001fa3c0)
        /home/user/src/go-discord-irc/bridge/irc_listener.go:95 +0x289 fp=0xc000269e98 sp=0xc000269d40 pc=0x729bc9
github.com/qaisjp/go-discord-irc/bridge.(*ircListener).OnNickRelayToDiscord-fm(0xc0001fa3c0)
        /home/user/src/go-discord-irc/bridge/irc_listener.go:75 +0x34 fp=0xc000269eb8 sp=0xc000269e98 pc=0x731434
github.com/qaisjp/go-ircevent.(*Connection).RunCallbacks.func1(0xc0000f81c0, 0x2, 0xc000444060, 0xc000126760, 0xc0001fa3c0)
        /home/user/go/pkg/mod/github.com/qaisjp/go-ircevent@v0.0.0-20180911155239-e71f5fec2a8d/irc_callback.go:164 +0x6d fp=0xc000269fb8 sp=0xc000269eb8 pc=0x70f7cd
runtime.goexit()
        /usr/local/go/src/runtime/asm_amd64.s:1374 +0x1 fp=0xc000269fc0 sp=0xc000269fb8 pc=0x46ac01
created by github.com/qaisjp/go-ircevent.(*Connection).RunCallbacks
        /home/user/go/pkg/mod/github.com/qaisjp/go-ircevent@v0.0.0-20180911155239-e71f5fec2a8d/irc_callback.go:162 +0x45e

Would this be indicative of a need for a mutex around channel.Users? Cannot really just do this here, it'd need to be done in the nick tracking code external to this project as well.

[llmII]

Since ignores were merged, I'm going to need to rebase this to master (try to preserve most commits that were not specifically mine only), and merge in PR #85 since that would make this a complete feature based on the current codebase, instead of the historical one wherein I wasn't certain which things might be merged. Hold tight on this PR for a bit, I'll pop a note and hit a "ready for review" button after I've updated this appropriately.

I cannot fix the possible race condition however until qaisjp/go-ircevent #2 is resolved.

There will remain a kludge to support extra channel status modes until qaisjp/go-ircevent #1 is resolved as well.

After merging #85 into this PR I'd suggest that this would be ready for merging and that later on the kludge for channel status modes could be removed at a later date. I would also say merging this despite the race condition is fine so long as we note that there is a possible race condition in code external to this repository and that we'll resolve the issue when it is resolved in the external code.

[llmII]

With the prior commit this is now ready for review, as per the prior comment.

[qaisjp]

Right now this piggybacks on the show_joinquit feature and config setting. I think we may want to either rename that config variable to something along the lines of show_metainfo or make the relayed stuff fully configurable like

Yep, piggybacking is cool.

Instead of show_metainfo couldn't it use the same system as https://github.com/qaisjp/go-discord-irc/pull/76? (Probably in that PR, once this is merged?)

[llmII]

I don't use this feature, so I'm trusting that you've tested it well & that it works! Is this ready to merge?

Unless the merge of wim's discord would effect this it should work, if wanted can hold off for me to recreate my dev branch and merge in all the other branches and re-test (so many options got renamed, and have held off for a moment trying not to merge stuff together and have to do it again too soon lol).

Do also note the above crash note which I believe is caused by go-ircevent (and go-discord-irc). I think that channel.Users needs a synchronization primitive tied to it but go-discord-irc is the wrong place to have the primitive. I believe go-ircevent needs the synchronization primitive or to expose iteration over that in another manner and go-discord-irc would need to either lock and unlock appropriately or use the new iteration interface.

Also note there is a kludgy fix here for detecting extra modes qaohv because the builtin nick-tracking from go-ircevent doesn't handle those characters correctly.

Instead of show_metainfo couldn't it use the same system as #76? (Probably in that PR, once this is merged?)

When #76 is merged, I believe "show_joinquit" should get removed in a separate PR (have to change the conditions for enabling/disabling those callbacks).

[qaisjp]

Unless the merge of wim's discord would effect this it should work, if wanted can hold off for me to recreate my dev branch and merge in all the other branches and re-test (so many options got renamed, and have held off for a moment trying not to merge stuff together and have to do it again too soon lol).

I've merged master, so you should be able to pull and test :D

Do also note the above crash note which I believe is caused by go-ircevent (and go-discord-irc). I think that channel.Users needs a synchronization primitive tied to it but go-discord-irc is the wrong place to have the primitive. I believe go-ircevent needs the synchronization primitive or to expose iteration over that in another manner and go-discord-irc would need to either lock and unlock appropriately or use the new iteration interface.

Yeah, I think adding a mutex will work. Feel free to submit a PR to the fork!

Also note there is a kludgy fix here for detecting extra modes qaohv because the builtin nick-tracking from go-ircevent doesn't handle those characters correctly.

And we can fix that in the fork too?

Instead of show_metainfo couldn't it use the same system as #76? (Probably in that PR, once this is merged?)

When #76 is merged, I believe "show_joinquit" should get removed in a separate PR (have to change the conditions for enabling/disabling those callbacks).

Coolio 👍

[llmII]

Yeah, I think adding a mutex will work. Feel free to submit a PR to the fork!

Also note there is a kludgy fix here for detecting extra modes qaohv because the builtin nick-tracking from go-ircevent doesn't handle those characters correctly.

And we can fix that in the fork too?

I'll look into it. For the Mutex situation I think it'd be better to make Users private and expose a few functions like HasUser, GetUser, and UserIter (this one would take a function that accepts a user as an argument).

Did test tonight, and for some reason seems my kludge isn't working for +qa users (and there are other modes for users that InspIRCd has that would need to work as well that the kludge was covering for). It worked with +o user, but failed with a +q one. I'm going to investigate it more tomorrow and fix my kludge to work properly hopefully. If it's deeper in tracking than just the odd user in Users having their key be prefixed by their mode on the channel then it'll end up being a "can't fix and this works as good as it will till the go-ircevent fork is updated" ordeal.

[llmII]

Ok, this works, kludge won't work as intended.

The kludge will be ok to be left in even when go-ircevent is fixed (though it should definitely be removed after).

In irc_nicktrack.go this is where the issue that makes it impossible for the kludge to even work properly is. It does not see the ~nick change to something else because the key is "~nick" which != event.Nick and != event.Message() || event.Arguments[0]. Question could be asked if this breaks puppet tracking but it doesn't. Relaying nick changes depends on determination of if the user is in a channel or not so we check the channel's users map. Puppeting doesn't interact with irc_nictrack.go much at all.

I'd say add this and make a note that it works but the messages for nick changes drop when the user is +Yyqa. I'll look into fixing this in the go-ircevent fork but I don't know if I'm going to actually try to parse out ISUPPORT or put in a stop-gap that just tests against more possible prefix modes.

In the external nick tracking code this is completely an issue with handling NAMES_REPL. If a nick /join's then gets mode +q it's not using NAMES to track the user and thus it manages to track it correctly by using the JOIN related callback.

All this to say - This is ready to be merged and should work correctly when go-ircevent is fixed and already works correctly for IRC Networks without special prefix mode.

[llmII]

If qaisjp/go-ircevent #3 gets merged before this this PR will need to be updated.

[llmII]

This is ready for merge. From a logical standpoint go-discord-irc is sound. The issue is the underlying IRC library and I don't think it makes sense to hold up waiting on that to be fixed.

The IRC library does not provide for a way to have a set of callbacks that must complete execution before other callbacks. I need to think on this some more but there are a number of ways to solve this in my opinion.

1. Store a list of channels on each user (or map) -- hackish IMO.
2. Introduce rewrites of each event nick tracking handles for a way of insuring that tracking callbacks get handled first, and tracking introduces a new event after tracking is completed.
3. Move all tracking code into the same functions that execute event callbacks (sort of, this is very loosely worded)

I plan to go for option 2 but this shouldn't be held back by underlying library issues. Everything needing to basically wait on state tracking to complete can be changed later to listen for new events that signify exactly the same as current, except that tracking has fully completed and it is safe to assume the same as currently assumed. I'd probably even extend certain events to have a list of channels instead of just figuring it out from the callbacks that respond to the event.

[llmII]

Tested, NICK/PART/KICK/JOIN works, QUIT doesn't until the silly logic error I made is fixed in go-ircevent (PR already submitted).

[llmII]

@qaisjp Is varys finished? Is it able to call back into the main program (for example, on quit, it auto-reconnects IIRC and it will need to get it's irc_listener_prejoin_commands settings) so either it has them or it has to call back to the server and ask for them? Should I start looking at redoing this of sorts to work with varys?

Edit: Nevermind, I think I see how it works, I'll try to get this PR updated to be based upon the recent introduction of varys to the main branch.

Edit 2: If varys's puppet gets disconnected whilst vary's driver (the actual bridge) is offline will this prevent the disconnected puppet which auto-reconnects from popping out it's irc_puppet_prejoin_commands when OnWelcome is triggered?

[qaisjp]

Is varys finished?

Not really. The default implementation is in-process, and seems to work. The networked implementation is unimplemented.

If varys's puppet gets disconnected whilst vary's driver (the actual bridge) is offline will this prevent the disconnected puppet which auto-reconnects from popping out it's irc_puppet_prejoin_commands when OnWelcome is triggered?

Good point. That will be an issue we need to solve! (Right now this situation is impossible, as everything is in-process.)

[llmII]

NOTE: relaying quits needs to check puppetNicks before it's removed from the list of puppet nicks.

[llmII]

I'm going to rework this PR. There's some things that could be split out (fixes to use ST* events, etc) and then this will need a major rework and checkup for the Varys changes. Would you like to keep this PR open and the rework of this (minus the split out fixes or features) be here? or would you rather close this one and have a different PR come about when I've got the feature back into a working state?

[llmII]

This has been reworked, and is untested, depends on #95.

EDIT: Tested, functions as before.

[qaisjp]

Merge conflict :)

[llmII]

Merge conflict :)

Fixed. I'll have to do this again for the formatting related one after this merges so holding off on fixing that over there till after this one merges.