Re: [Emacs-diffs] master adfb6f1: Continue to fix bug#25607

Re: [Emacs-diffs] master adfb6f1: Continue to fix bug#25607

[Stefan Monnier]

>     * lisp/ido.el (ido-complete): Let-bind `non-essential' to nil.

This looks wrong.  This in an interactive command executing an explicit
request from the user, so it's definitely not non-essential.

The compact on-the-fly display of the possible completions performed by
IDO does qualify as `non-essential', but not ide-complete.


        Stefan

Re: [Emacs-diffs] master adfb6f1: Continue to fix bug#25607

[Dmitry Gutov]

On 15.02.2017 22:35, Stefan Monnier wrote:
>>      * lisp/ido.el (ido-complete): Let-bind `non-essential' to nil.
> 
> This looks wrong.  This in an interactive command executing an explicit
> request from the user, so it's definitely not non-essential.

Hence "to nil"?

The removal of the other non-essential binding looks more dubious to me.

Re: [Emacs-diffs] master adfb6f1: Continue to fix bug#25607

[Michael Albinus]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

>>     * lisp/ido.el (ido-complete): Let-bind `non-essential' to nil.
>
> This looks wrong.  This in an interactive command executing an explicit
> request from the user, so it's definitely not non-essential.
>
> The compact on-the-fly display of the possible completions performed by
> IDO does qualify as `non-essential', but not ide-complete.

`non-essential' is bound exactly two times in ido.el now. In
`ido-read-internal' it is bound to t, preventing non-desired new
connections while the user is typing a file name in the minibuffer. And
in `ido-complete', called inside `ido-read-internal', it is bound to
nil, allowing new remote connections, since the user has triggered this
(by typing <TAB>.

I cannot see what's wrong with this.

>         Stefan

Best regards, Michael.

Re: [Emacs-diffs] master adfb6f1: Continue to fix bug#25607

[Michael Albinus]

Dmitry Gutov <dgutov@yandex.ru> writes:

> The removal of the other non-essential binding looks more dubious to me.

This was inside `ido-file-name-all-completions-1'. According to the
traces I've collected, this function is always called inside
`ido-read-internal', which has already bound `non-essential' to t.

And my (limited, I'm not an ido user) tests haven't shown any regression.

Best regards, Michael.

Re: [Emacs-diffs] master adfb6f1: Continue to fix bug#25607

[Stefan Monnier]

>>> * lisp/ido.el (ido-complete): Let-bind `non-essential' to nil.
>> This looks wrong.  This in an interactive command executing an explicit
>> request from the user, so it's definitely not non-essential.
> Hence "to nil"?

Oh, right, sorry, duh!  Indeed, it's perfectly safe then to bind it to
nil there.

Hmm... then I wonder why this binding would be needed at all: why wouldn't
the variable already be nil?

[...time passes...]

Oh, I see: there is an incorrect binding in ido-read-internal which
means that all commands within IDO are treated as non-essential.

The way I see it, The Right Way would be to do the binding in
ido-exhibit, since that's the code run from post-command-hook, i.e. the
code that's not explicitly requested by the user.


        Stefan


diff --git a/lisp/ido.el b/lisp/ido.el
index e18464d1d6b..bb4c67c7c01 100644
--- a/lisp/ido.el
+++ b/lisp/ido.el
@@ -1882,7 +1882,6 @@ ido-read-internal
        ido-selected
        ido-final-text
        (done nil)
-       (non-essential t) ;; prevent eager Tramp connection
        (icomplete-mode nil) ;; prevent icomplete starting up
        ;; Exported dynamic variables:
        ido-cur-list
@@ -3556,7 +3555,6 @@ ido-file-name-all-completions-1
     ;; Strip method:user@host: part of tramp completions.
     ;; Tramp completions do not include leading slash.
     (let* ((len (1- (length dir)))
-	   (non-essential t)
 	   (compl
 	    (or ;; We do not want to be disturbed by "File does not
                 ;; exist" errors.
@@ -4413,6 +4411,7 @@ ido-exhibit
 
   (when (ido-active)
     (let ((contents (buffer-substring-no-properties (minibuffer-prompt-end) (point-max)))
+          (non-essential t)
 	  (buffer-undo-list t)
 	  try-single-dir-match
 	  refresh)

Re: [Emacs-diffs] master adfb6f1: Continue to fix bug#25607

[Michael Albinus]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

> Oh, I see: there is an incorrect binding in ido-read-internal which
> means that all commands within IDO are treated as non-essential.
>
> The way I see it, The Right Way would be to do the binding in
> ido-exhibit, since that's the code run from post-command-hook, i.e. the
> code that's not explicitly requested by the user.

Your patch doesn't work at all. I've applied it, and I tried:

1. # src/emacs -Q -f ido-mode

2. C-x C-f / s u

--> This doesn't give you completion for /sudo:

3. d o :

--> Error: Host name must not match method "sudo"

I don't understand what's your problem with the patch I have
applied. Pls show me an example where it behaves wrong.

>         Stefan

Best regards, Michael.

Re: [Emacs-diffs] master adfb6f1: Continue to fix bug#25607

[Stefan Monnier]

> I don't understand what's your problem with the patch I have
> applied. Pls show me an example where it behaves wrong.

The whole point of `non-essential` is that its meaning is defined
independently from its users (i.e. independently from Tramp).
So if Tramp doesn't complete the way you want it, it's Tramp's fault.

All the patch does is set non-essential when running code whose proper
behavior is not essential (an information which can be used to decide
whether it's worth prompting the user for a password, for example).

> 1. # src/emacs -Q -f ido-mode
>
> 2. C-x C-f / s u
>
> --> This doesn't give you completion for /sudo:

Apparently Tramp's maintainer decided that it's not essential to list
"/sudo:" as a possible completion of "/su" in this case.

> 3. d o :
>
> --> Error: Host name must not match method "sudo"

And apparently Tramp's maintainer decided that it's not essential to
avoid signaling an error either (and he also decided that "/sudo:"
should be taken to refer to some place on the "sudo" host rather than
to the local host, using the sudo access method).


        Stefan

Re: [Emacs-diffs] master adfb6f1: Continue to fix bug#25607

[Michael Albinus]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

>>     * lisp/ido.el (ido-complete): Let-bind `non-essential' to nil.
>
> This looks wrong.  This in an interactive command executing an explicit
> request from the user, so it's definitely not non-essential.
>
> The compact on-the-fly display of the possible completions performed by
> IDO does qualify as `non-essential', but not ide-complete.

According to recent Tramp changes, binding `non-essential' in ido-mode
is not needed anymore. I've removed the bindings.

According to my (short) test, ido still behaves correctly. But I'm not
an ido user; other people might check this also.

>         Stefan

Best regards, Michael.

Re: [Emacs-diffs] master adfb6f1: Continue to fix bug#25607

[Stefan Monnier]

> According to recent Tramp changes, binding `non-essential' in ido-mode
> is not needed anymore. I've removed the bindings.

Fundamentally, I think that ido should bind non-essential somewhere so
as not to "accidentally" open up a new connection and ask for
a password, but given the way ido.el is structured it's not completely
clear how/where this should be done.  So I think not binding
non-essential is good enough for now.

Thanks.


        Stefan

Re: [Emacs-diffs] master adfb6f1: Continue to fix bug#25607

[Michael Albinus]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

Hi Stefan,

>> According to recent Tramp changes, binding `non-essential' in ido-mode
>> is not needed anymore. I've removed the bindings.
>
> Fundamentally, I think that ido should bind non-essential somewhere so
> as not to "accidentally" open up a new connection and ask for
> a password, but given the way ido.el is structured it's not completely
> clear how/where this should be done.  So I think not binding
> non-essential is good enough for now.

I had the same feeling when removing the current bindings. But I'm not
fluent with ido's code like many other people around here ... maybe we
must wait for bug reports from ido users.

> Thanks.
>
>         Stefan

Best regards, Michael.

Re: [Emacs-diffs] master adfb6f1: Continue to fix bug#25607

[Stefan Monnier]

> I had the same feeling when removing the current bindings.  But I'm not
> fluent with ido's code like many other people around here ...  maybe we
> must wait for bug reports from ido users.

That's my opinion, yes.


        Stefan