Eli Zaretskii <eliz@gnu.org> writes:

> 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.

Well, I myself am just about the furthest thing from (an expert), which
certainly doesn't comport well with dropping rash changes at the
eleventh hour. (That was rather disrespectful on my part, so shame on
me.) As such, if it's easier to revisit this once things settle down,
just ignore this email and I'll re-ping you sometime down the road.

> 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?

Just good old fashioned stupidity, I'm afraid. (And also recklessness in
overly trusting the me from eight months ago, surely.) I guess I somehow
assumed that if the caller didn't set :coding explicitly, they would do
so once handed back the process, which is certifiably dumb.

>> @@ -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?

This sets the fallback message to "Unknown error" (made up) rather than
"General SOCKS server failure" (an official error code). At first, I
figured the distinction more faithfully conveyed the nature of the
error, but now I see that it just adds clutter because the fallback path
can only be triggered by a protocol mishap, and that's unlikely, given
that the conversation must progress to its third back-and-forth by the
time this runs.

(BTW, the words "error handling" in the patch's title refer to the added
"(0 0)" `pcase' condition in `socks-filter' and not the snippet above.)

>> -(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?

Just my being callous. I now see that obsoleting that variable is
problematic, not least because we continue to honor `socks-noproxy'. But
the two complement each other and are closely coupled, usage-wise.
Getting rid of the one and pretending the other still works as intended
was doubly irresponsible.

>> +(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.

The old signature was

  (name buffer host service) -> process

and the new &rest arguments would be optional. And since the lone
in-tree call site sticks to the four required positionals, I didn't
think a move from (4 . 4) to (4 . many), in `func-arity' terms, stood to
break any advice in the wild. Still, there are side effects in the new
version that could use more thorough exploring, and further attention
could be paid to its treatment of `socks-override-functions' in terms of
preserving old behavior.

> 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.

You're very gracious, but I think I've learned my lesson and will
refrain from pursuing any of these changes for Emacs 29. Apologies for
abusing your time and maintainerly patience (yet again).