Re: bug#70840: 29.3; ERC 5.5.0.29.1: erc-kill-buffer-on-part kills server buffer

[J.P.]

Hi Thibaut,

Thibaut Meyer writes:

> Hello,
> reminiscent of
> https://lists.gnu.org/archive/html/emacs-devel/2009-06/msg00064.html,
> when `erc-kill-buffer-on-part` is set to t, parting a channel
> by killing its buffer, makes erc want to kill the server's buffer. When
> parting by issuing the "/part" command, everything is fine.
>
> I reckon that in `define-erc-response-handler` in the erc-backend.el
> file, since we kill the buffer ourselves, (erc-get-buffer chnl proc)
> returns the server buffer instead of the channel buffer that has already
> been killed. So then it tries to kill that instead, which is not wanted.

I believe calling `erc-get-buffer' with a nonexistent target returns
nil, which has a similar effect to what you describe. For example, it
causes `erc-display-message' to fall back on routing its output to the
server buffer. Likewise, in terms of user bookkeeping, the calls to
`erc-remove-channel-users' and `erc-remove-channel-member' are
effectively suppressed by `buffer' being nil. This is fine because the
parted buffer has already been killed, so its `erc-channel-users' table
no longer exists, and the buffer has already been removed from the
`buffers' slot of the user's `erc-server-users' entry via

  kill-buffer-hook
  -> erc-kill-buffer-function
   -> erc-remove-channel-users
    -> erc-remove-channel-user
     -> erc-remove-server-user

> To reproduce:
> - set erc-kill-buffer-on-part to t
> - kill a channel buffer
> - notice emacs trying to kill the corresponding server buffer (it should
> not happen directly thanks to the "buffer has running process. Kill it?"
> confirmation though)

Yes, I can confirm this to be the case. Thanks.

So, without thinking about this too seriously, I'm guessing the main
challenge to overcome when fixing this will likely be `erc-part-hook'
and whether to gate it behind the parted buffer being non-nil. Since the
corner case of it being called with a nil buffer has been around
forever, some users may already account for the possibility. Others who
don't may have code that simply doesn't care and yet still needs to run
whenever an incoming (self-issued) PART response arrives. However, since
this is a clear bug, we're justified in changing the behavior. The
question is whether improving it for new users will cause more harm
overall.

If we do elect to only run the hook in the response handler when the
parted buffer still exists, we may want to compensate by running it
elsewhere in the code path triggered by the killing of a channel buffer.
One way to do this would be via a new function running just before
`erc-kill-channel' on `erc-kill-channel-hook'. We'd run `erc-part-hook'
there, in the server buffer, passing it the still-live target buffer
that's slated to be killed.

Such an addition would require us checking how `erc-kill-channel-hook'
is used in erc-log.el and erc-status-sidebar.el, etc., and confirming
that running `erc-part-hook' in this fashion (within another hook) won't
cause any problems. It's probably also worth doing the same with popular
third-party packages and configs. We'd of course also have to announce
the change in etc/ERC-NEWS.

One approach to cutting down on any associated churn would be to add an
escape hatch, such as an "internal" compatibility switch in the form of
a global variable, like an `erc--run-part-hook-with-nil-buffer-p' or
similar. Basically, we'd condition both hook sites on its value. For
example, in `erc-server-PART', we'd have something like

  (when (or buffer erc--run-part-hook-with-nil-buffer-p)
    (run-hook-with-args 'erc-part-hook buffer)) 

And in the new `erc-kill-channel-hook' member, we'd have

  (unless erc--run-part-hook-with-nil-buffer-p
    (let ((buffer (current-buffer)))
      (erc-with-server-buffer
        (run-hook-with-args 'erc-part-hook buffer))))

This would grant a temporary reprieve to users transitioning to the
latest version. They could just set the variable to put off dealing with
it immediately. BTW, if going this route, the doc string for the hook
should probably mention the second call site and possibly the existence
of an escape hatch.

There may well be other issues here I've not yet considered, so if you
can think of any, please share. Also, if you're up for taking this on
(not necessarily in the way I've described), please feel free to send a
patch. The same goes for anyone else out there. I can definitely help
with the leg work and test coverage if that makes things easier.
Otherwise, I may not be able to get to this in time for the next release
(not that there's any rush).

Thanks,
J.P.