bug#53941: Last-minute socks.el improvements for Emacs 29?

[Eli Zaretskii]

> From: "J.P." <jp@neverwas.me>
> Cc: Jacobo <gnuhacker@member.fsf.org>, Lars Ingebrigtsen <larsi@gnus.org>,
>  Eli Zaretskii <eliz@gnu.org>
> Date: Mon, 28 Nov 2022 07:30:16 -0800
> 
> I've lifted some fixes and minor enhancements from my POC stuff posted
> to this thread earlier this year. Nothing is directly Tor related, so I
> can create a new bug report, if necessary.
> 
> The second patch fixes a problem involving SOCKS 5 error handling. It
> also adds support for SOCKS 4a, which allows tools that don't speak
> SOCKS 5, like socat, to resolve host names. The third addresses a couple
> FIXMEs but no bugs, strictly speaking. The fourth is just a demo [1].
> Happy to explain whatever in detail.

I'm really uncomfortable with installing these changes before the release
branch is cut.  The changes are hardly trivial, some controversial even to
my eyes, even though I'm no expert on network connections.  For example:

> +(defun socks-open-connection (server-info &rest stream-params)
> +  "Create and initialize a SOCKS process.
> +Perform authentication if needed.  Expect SERVER-INFO to take the
> +form of `socks-server' and STREAM-PARAMS to be keyword params
> +accepted by `open-network-stream'."
>    (interactive)
> +  (unless (plist-member stream-params :coding)
> +    (setf (plist-get stream-params :coding) '(binary . binary)))

AFAIU, this constitutes an incompatible change in behavior: the default for
:coding is was never 'binary' before, it was determined from the locale's
preferences.  Why are we making this change here?

> @@ -446,7 +474,11 @@ socks-send-command
>       nil                             ; Sweet sweet success!
>        (delete-process proc)
>        (error "SOCKS: %s"
> -             (nth (or (process-get proc 'socks-reply) 1) socks-errors)))
> +             (let ((no (or (process-get proc 'socks-reply) 99)))
> +               (or (if (eq version 5) ; 99 - 90 >= length(errors)
> +                       (nth no socks-errors)
> +                     (nth (- no 90) socks--errors-4))
> +                   "Unknown error"))))

I don't really understand the semantics here (so maybe comments need to be
upgraded), but the old and the new versions don't look to me like equivalent
code -- why the change?

> -(when socks-override-functions
> -  (advice-add 'open-network-stream :around #'socks--open-network-stream))
> +;; Libraries typically offer a "stream opener" option, such as ERC's
> +;; `erc-server-connect-function'.  These provide a level of
> +;; flexibility tantamount to what this variable formerly offered.
> +(make-obsolete-variable
> + 'socks-override-functions
> + "see `socks-open-network-stream' and `socks-connect-function'." "29.1")

Why this last-minute obsolescence?

> +(defun socks-open-network-stream (name buffer host service &rest params)
> +  "Open and return a connection, possibly proxied over SOCKS.

The changes in this public function are so significant that I don't
understand how they can be suggested so close to the branching.

If it is possible to add support for SOCKS 4a without affecting any
previously supported versions, I'm fine.  Adding tests is also fine.  But
for the rest, I think you should wait until after the release branch is cut
and install this on the master branch.  Sorry, it really is too late for
such changes.