Re: bug#59976: ERC 5.4.1: erc-networks--id gets clobbered in erc server buffer on /query name conflict

[J.P.]

Mike Kazantsev <mk.fraggod@gmail.com> writes:

>> > - What I'm trying to do:
>> >
>> >   - (erc-tls ...)  ;; cleates "slack" server buffer and some channels.
>> >   - Open "slack" buffer  
>> 
>> You mean opened manually, like by issuing a /query slack, right? So this
>> is in addition to the observation above where "it sends you a message
>> from user named 'slack'"?
>
> Sorry, no, I meant as in (switch-to-buffer "slack"), to type command into.
>
> No manual /query command is implied anywhere, but maybe it can also be
> used to reproduce the issue, given how it should do roughly same thing.

OK, gotcha. Thanks for clarifying.

>> >   - Send "/ping myuser" IRC-command there
>> 
>> You also mention "case-sensitive channel names." It'd be nice to know
>> what those look like when they first arrive off the wire. (BTW, if
>> you're expecting ERC to interoperate with a case-sensitive network, like
>> one for which "#FOO" and "#foo" are distinct channels, then that may
>> need a separate bug report.)
>
> It's not related to this issue at all, and I probably shouldn't have
> mentioned it, to avoid adding extra confusion, sorry.
> I.e. "slack" buffer in case of this issue is all-lowercase everywhere.

I see, so "slack" (the nick) is already lowercase when it arrives from
the server.

> But as a separate unrelated thing, here is specific problem that I was
> thinking to try solving with updated ERC, as it seem to have some new
> code added, related to casemapping:
>
>   (to stress this - it's a separate issue from this one, only here as a
>   curiosity, should be ignored entirely in context of this bug report)
>
>   Detailed description:
>   https://github.com/progval/matrix2051/issues/41
>
>   I thought at first that issue might be CASEMAPPING=rfc3454 used
>   there, which I was not familiar with, and assumed that it doesn't
>   behave like ascii/rfc1459 casemapping.
>
>   So I was thinking to try adding it to new erc-downcase func locally,
>   and fix the issue with case-sensitive channels that way.
>
>   But after looking at RFC 3454 I've found that it includes casemapping
>   for ASCII letters and pretty much just extends casemapping to larger
>   unicode character set, and should be compatible with ERC, at least
>   for the case of simple ASCII letters.
>
>   So it seem to be a bug in progval/matrix2051 ad-hoc ircd
>   implementation, which I've reported there at the URL above, with a
>   workaround I've used in ERC mentioned in the first comment there,
>   just in case someone might find it via google when bumping into same
>   issue.
>
> So, again, it's not really related to this issue, and not an ERC issue
> at all, and I probably shouldn't have mentioned it here.
>

I've only run across 3454 in passing but always thought of it more as a
standardizing framework through which abiding protocols could arrive at
identical notions of a character from disparate input representations.
However, in chapter 3, they do mention that their provided mapping
tables "MUST be used by implementations of this specification," so I'm
just confused, basically. But thanks for the lowdown regardless. I'll
try diving into it properly at some point.

>> > - Workaround: changing announced name of the server to something else
>> >   helps - "slack" query-buffer gets created and is separate from server 
>> > buffer.  
>> 
>> You mean by changing `erc-server-announced-name'?
>
> Yes... sort of.
>
> As I understand it, this name ends up being converted via
> erc-networks-alist from erc-server-announced-name to a network symbol
> (e.g. 'OFTC or 'slack) in erc-network value.

Right, exactly. It would seem that since most IRC networks specify the
NETWORK ISUPPORT parameter these days, `erc-networks-alist' now serves
more as a means of specifying overrides.

> So guess my description above is incorrect or somewhat incomplete -
> what you'd actually want is have symbol in that alist not conflict with
> query-channel name, not just whatever erc-server-announced-name ends up
> being.
>
> Only slightly related to description above, and should likely be
> ignored in context of this issue:
>
>   To set erc-network to 'slack value I ended up replacing that alist
>   lookup with a much simplier local solution:
>   https://github.com/mk-fg/emacs-setup/blob/4dd532b/core/fg_erc.el#L72-L95
>   (current version uses '🖧-slack symbol with that unicode prefix
>   exactly to avoid any name conflicts)

That's a very clean looking ERC setup! Let's hope you won't experience
too much churn with these upcoming "fixes."

>   It's partly needed because ad-hoc protocol-bridge ircd's don't
>   announce network names, and partly because just seemed simplier
>   to derive them this way, but I didn't think about :id solution that
>   you've mentioned below, which might work even better here.
>
>   But pretty sure if one did use the alist-to-symbol lookup, they'd 
>   end up with same result, e.g. 'slack in erc-network, just like it
>   works with 'OFTC in there.

Right, same result. It seems the bridge doesn't send a NETWORK ISUPPORT
parameter at all. So, at present, I believe you have no choice but to
add an entry to that alist or advise/shadow as you've done in your
config. However, the `:id' stuff was always meant to work for these
situations. That it doesn't is definitely a bug, which I've tried to
partially remedy in the latest iteration of the growing patch set for
this thread.

>> > Was able to understand what seem to be the issue here by enabling
>> > logging for erc-networks--id changes to *Messages* like this:
>> >
>> >   (defun debug-watch-log (sym newval op where)
>> >     (message "Variable update: %s = %S -> %S [%S %S]"
>> >       sym (symbol-value sym) newval op where)
>> >     (backtrace))
>> >   (add-variable-watcher 'erc-networks--id #'debug-watch-log)
>> >
>> > (run M-x cancel-debug-on-variable-change afterwards to disable this)  
>> 
>> Thanks for the detective work. In this case, I'm pretty sure the root
>> cause is not directly related to that variable, despite all appearances.

Actually, I now see how `erc-networks--id' fails to be set (along with
`erc-server-connected'). So, it's all really closely related. The real
problem is that the connection is allowed to proceed in a degraded state
when it should just be destroyed immediately when network detection
fails.

>> BTW, the most helpful insights are usually to be found in the contents
>> of the *erc-protocol* buffer, which can be generated by calling
>> `erc-toggle-debug-irc-protocol' before connecting for the first time in
>> an Emacs session. If you're not comfortable sharing such info on the
>> tracker, please consider doing so out of band.
>
> I think issue should be easily reproducible without needing the
> protocol logs, by setting erc-network to your own nickname and then
> sending /msg to yourself, or maybe even e.g. "/q OFTC".
>
> Will try it out a bit later and report on whether such trivial ways to
> do it work.

No worries, I ended up just creating a minimal setup locally. I can see
that, in addition to not supplying a NETWORK, the bridge also doesn't
furnish a 004, which ERC uses to populate `erc-server-announced-name'.
However, at least on my instance, it sets the :source (prefix) to
"server.", and ERC just ends up using that as a fallback.

>> That said, at the moment, we only support unambiguous "network IDs" via
>> the ":id" keyword parameter of `erc' and `erc-tls'. IOW, calling these
>> with args like
>> 
>>   :server "127.0.0.1"
>>   :port 2051
>>   ...
>>   :id 'Slack
>> 
>> is supposed to always work. But, I can definitely see how that doesn't
>> in some cases (e.g., a lowercase `:id'). As you say, you may have no
>> control over who sends you a query, which is still pertinent in the case
>> of an explicit `:id' so long as it consists entirely of valid nick
>> characters.
>
> That looks like it might be a good solution to my other issue with
> deriving names for these ad-hoc protocol-bridge networks mentioned
> above, thanks for mentioning it.

Well, it's supposed to be a good solution, but it's bugged in a few
places, which you've probably noticed by now (all my bad). I've
attempted to address the most glaring aspects in the patches below.

>> Unfortunately, the best we can do for ERC 5.5 (Emacs 29) is to mention
>> somewhere, like in (info "(erc) Network Identifier"), that users really
>> worried about this issue should choose an `:id' containing characters
>> disallowed in nicks by their network (or just something improbable and
>> unlikely to be guessed). But, hopefully, we can address this in a more
>> DWIM-like fashion in an upcoming ELPA release, such as ERC 5.6.
>
> Hopefully won't be an issue for most people, although it might be not
> that uncommon for this kind of service/bot/proxy ircd's to also use
> their name for messaging user with any kind of service-related info/issues.

Hm, right. That's good to know. I think the key for now is to make
people aware that they should assign an ID or modify
`erc-networks-alist' if they find themselves in a similar boat. Most
folks just connecting to a public network should be safe because those
NETWORKs are mostly titlecase and/or contain a dot. But, in the long
run, I'd definitely like the default behavior to account for this
possibility.

>> > I'm running ERC 5.4.1 from current 0e5d059 git master on Emacs 28.2
>> > (replacing "erc" dir in /usr/share/emacs), so it is also possible that
>> > maybe some change in Emacs 29+ prevents this behavior, but it seems 
>> > unlikely.  
>> 
>> Unlikely, yes. But I sometimes worry about ERC's loaddefs when people
>> manually shadow/replace instead of building alongside Emacs or
>> installing from devel:
>> 
>>   (push '("devel" . "https://elpa.gnu.org/devel/";) package-archives)
>
> I didn't consider that newer ERC can be installed this way, given that
> it seem to be part of the emacs tree, but good to know, thanks, will
> probably use this instead.
>
> (although tbf it seems more complicated than replacing a dir, but if
> it's cleaner then yeah, probably best to do it via package-archives)

(One thing to note that I've learned recently is that the package
snapshot is only updated when a commit touches the "main" file,
as in lisp/erc/erc.el.)

>> (Not saying you need to do either, of course.) Thanks again for the very
>> nice bug report.
>
> Thank you for looking into this and work on ERC in general.
>
> As someone who spends most of the day looking at emacs, it's by far the
> most convenient and configurable client for me, that I end up using for
> pretty much all modern IM protocols because of that.

Thanks, that's really encouraging to hear. Hopefully, with the help of
bug reports like this, we can make the experience more consistent and
flexible for adventurous folks, like yourself, rather than simply cater
to the lowest common denominator.

0000-v1-v2.diff
Description: Text Data

0001-Be-nicer-when-updating-browse-url-var-in-erc-compat.patch
Description: Text Data

0002-Actually-accept-non-symbols-as-IDs-in-erc-open.patch
Description: Text Data

0003-Limit-casemapping-to-appropriate-ranges-in-ERC.patch
Description: Text Data

0004-Set-erc-network-to-a-given-ID-instead-of-failing.patch
Description: Text Data

0005-Add-escape-hatch-for-picky-servers-in-erc-sasl.patch
Description: Text Data