Re: [Emacs-diffs] trunk r113958: * minibuffer.el (completion--sifn-requote): Bind `non-essential'.

Re: [Emacs-diffs] trunk r113958: * minibuffer.el (completion--sifn-requote): Bind `non-essential'.

[Stefan Monnier]

>   * minibuffer.el (completion--sifn-requote): Bind `non-essential'.
  
This is wrong.  completion--sifn-requote is used in many contexts, some
of which are "essential" and some not.  IOW if you need to bind
non-essential, it shouldn't be here, but somewhere higher up the
call-chain.


        Stefan

Re: [Emacs-diffs] trunk r113958: * minibuffer.el (completion--sifn-requote): Bind `non-essential'.

[Michael Albinus]

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

>>   * minibuffer.el (completion--sifn-requote): Bind `non-essential'.
>   
> This is wrong.  completion--sifn-requote is used in many contexts, some
> of which are "essential" and some not.  IOW if you need to bind
> non-essential, it shouldn't be here, but somewhere higher up the
> call-chain.

Honestly, I don't know too much about the completion code in
minibuffer.el. The OP has reported the following backtrace (shortened by
me):

      substitute-in-file-name("/ssh:vagrant@192.168.10.")
      completion--sifn-requote(27 "/ssh:vagrant@192.168.10.2:/")
      completion--twq-try("/ssh:vagrant@192.168.10.2:/" "/ssh:vagrant@192.168.10.2:/" "/ssh:vagrant@192.168.10.2:/" 0 substitute-in-file-name completion--sifn-requote)
      completion--file-name-table("/ssh:vagrant@192.168.10.2:/" file-exists-p nil)
      complete-with-action(nil completion--file-name-table "/ssh:vagrant@192.168.10.2:/" file-exists-p)
      #[257 "\303\302\300\301$\207" ["/ssh:vagrant@192.168.10.2:/" file-exists-p nil complete-with-action] 6 "\n\n(fn TABLE)"](completion--file-name-table)
      funcall(#[257 "\303\302\300\301$\207" ["/ssh:vagrant@192.168.10.2:/" file-exists-p nil complete-with-action] 6 "\n\n(fn TABLE)"] completion--file-name-table)
      (setq res (funcall fun (car (prog1 xs (setq xs (cdr xs))))))
      (condition-case err (setq res (funcall fun (car (prog1 xs (setq xs (cdr xs)))))) (error (if firsterror nil (setq firsterror err)) nil))
      (while (and (not res) xs) (condition-case err (setq res (funcall fun (car (prog1 xs (setq xs (cdr xs)))))) (error (if firsterror nil (setq firsterror err)) nil)))
      (let ((firsterror nil) res) (while (and (not res) xs) (condition-case err (setq res (funcall fun (car (prog1 xs (setq xs ...))))) (error (if firsterror nil (setq firsterror err)) nil))) (or res (if firsterror (signal (car firsterror) (cdr firsterror)))))
      completion--some(#[257 "\303\302\300\301$\207" ["/ssh:vagrant@192.168.10.2:/" file-exists-p nil complete-with-action] 6 "\n\n(fn TABLE)"] (completion--embedded-envvar-table completion--file-name-table))
      read-file-name-internal("/ssh:vagrant@192.168.10.2:/" file-exists-p nil)
      try-completion("/ssh:vagrant@192.168.10.2:/" read-file-name-internal file-exists-p)

Sadly, I cannot reproduce this scenario locally. And I have no idea, why

      completion--sifn-requote(27 "/ssh:vagrant@192.168.10.2:/")

calls

      substitute-in-file-name("/ssh:vagrant@192.168.10.")

>         Stefan

Best regards, Michael.

Re: [Emacs-diffs] trunk r113958: * minibuffer.el (completion--sifn-requote): Bind `non-essential'.

[Stefan Monnier]

>>> * minibuffer.el (completion--sifn-requote): Bind `non-essential'.
>> This is wrong.  completion--sifn-requote is used in many contexts, some
>> of which are "essential" and some not.  IOW if you need to bind
>> non-essential, it shouldn't be here, but somewhere higher up the
>> call-chain.
> Honestly, I don't know too much about the completion code in
> minibuffer.el. The OP has reported the following backtrace (shortened by
> me):

My point is simply that the backtrace you show does not seem to perform
non-essential work, so binding non-essential is not right.  It is
computing the completion of /ssh:vagrant@192.168.10.2:/ (not quite sure
why, since the backtrace doesn't go further up).

> Sadly, I cannot reproduce this scenario locally. And I have no idea, why
>       completion--sifn-requote(27 "/ssh:vagrant@192.168.10.2:/")
> calls
>       substitute-in-file-name("/ssh:vagrant@192.168.10.")

Can't say offhand either why not.  Maybe we should simply demote errors
from substitute-in-file-name inside completion--sifn-requote.

At the same time I wonder why (substitute-in-file-name
"/ssh:vagrant@192.168.10.") should signal an error: it looks like an
incomplete filename, so we don't know yet whether it'll really be an
error or not.


        Stefan

Re: [Emacs-diffs] trunk r113958: * minibuffer.el (completion--sifn-requote): Bind `non-essential'.

[Michael Albinus]

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

Hi Stefan,

>> Honestly, I don't know too much about the completion code in
>> minibuffer.el. The OP has reported the following backtrace (shortened by
>> me):
>
> My point is simply that the backtrace you show does not seem to perform
> non-essential work, so binding non-essential is not right.  It is
> computing the completion of /ssh:vagrant@192.168.10.2:/ (not quite sure
> why, since the backtrace doesn't go further up).

The whole backtrace is in
<http://lists.gnu.org/archive/html/emacs-devel/2013-08/msg00533.html>
(the second backtrace in this message).

>> Sadly, I cannot reproduce this scenario locally. And I have no idea, why
>>       completion--sifn-requote(27 "/ssh:vagrant@192.168.10.2:/")
>> calls
>>       substitute-in-file-name("/ssh:vagrant@192.168.10.")
>
> Can't say offhand either why not.  Maybe we should simply demote errors
> from substitute-in-file-name inside completion--sifn-requote.
>
> At the same time I wonder why (substitute-in-file-name
> "/ssh:vagrant@192.168.10.") should signal an error: it looks like an
> incomplete filename, so we don't know yet whether it'll really be an
> error or not.

For Tramp, it is a complete filename wrt syntax. "ssh" is recognised as
host name, which is regarded as an error since we had decided this some
weeks ago. See <http://debbugs.gnu.org/cgi/bugreport.cgi?bug=13900#24>.

Setting `non-essential' to t is a mean to tell Tramp not to check this.

>         Stefan

Best regards, Michael.

Re: [Emacs-diffs] trunk r113958: * minibuffer.el (completion--sifn-requote): Bind `non-essential'.

[Stefan Monnier]

>> My point is simply that the backtrace you show does not seem to perform
>> non-essential work, so binding non-essential is not right.  It is
>> computing the completion of /ssh:vagrant@192.168.10.2:/ (not quite sure
>> why, since the backtrace doesn't go further up).
> The whole backtrace is in
> <http://lists.gnu.org/archive/html/emacs-devel/2013-08/msg00533.html>
> (the second backtrace in this message).

So we're in the middle of minibuffer--complete-and-exit which calls
try-completion.  That's very definitely not "non-essential".

>> At the same time I wonder why (substitute-in-file-name
>> "/ssh:vagrant@192.168.10.") should signal an error: it looks like an
>> incomplete filename, so we don't know yet whether it'll really be an
>> error or not.
> For Tramp, it is a complete filename wrt syntax. "ssh" is recognised as
> host name, which is regarded as an error since we had decided this some
> weeks ago. See <http://debbugs.gnu.org/cgi/bugreport.cgi?bug=13900#24>.

Yes, it could be a "complete but ambiguous/erroneous" file name, but it
could also be a "not yet complete file name".  So maybe, if there's no
":" nor "/" after "/ssh:", we should refrain from signaling an error.

> Setting `non-essential' to t is a mean to tell Tramp not to check this.

I do know that, but that's just an ugly hack here, since the code is not
non-essential.


        Stefan

Re: [Emacs-diffs] trunk r113958: * minibuffer.el (completion--sifn-requote): Bind `non-essential'.

[Michael Albinus]

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

>>> My point is simply that the backtrace you show does not seem to perform
>>> non-essential work, so binding non-essential is not right.  It is
>>> computing the completion of /ssh:vagrant@192.168.10.2:/ (not quite sure
>>> why, since the backtrace doesn't go further up).
>> The whole backtrace is in
>> <http://lists.gnu.org/archive/html/emacs-devel/2013-08/msg00533.html>
>> (the second backtrace in this message).
>
> So we're in the middle of minibuffer--complete-and-exit which calls
> try-completion.  That's very definitely not "non-essential".

Not `minibuffer--complete-and-exit'. But the completion functions I
would call non-essential.

As said, I don't know too much about completion code in minibuffer.el
(and the functions' docstrings don't help much). Maybe
`completion--sifn-requote' is not suited to bind `non-essential'. But
there are other `completion--*' functions in the backtrace, up to
`minibuffer--complete-and-exit'.

>>> At the same time I wonder why (substitute-in-file-name
>>> "/ssh:vagrant@192.168.10.") should signal an error: it looks like an
>>> incomplete filename, so we don't know yet whether it'll really be an
>>> error or not.
>> For Tramp, it is a complete filename wrt syntax. "ssh" is recognised as
>> host name, which is regarded as an error since we had decided this some
>> weeks ago. See <http://debbugs.gnu.org/cgi/bugreport.cgi?bug=13900#24>.
>
> Yes, it could be a "complete but ambiguous/erroneous" file name, but it
> could also be a "not yet complete file name".  So maybe, if there's no
> ":" nor "/" after "/ssh:", we should refrain from signaling an error.

Hmm. Why not signalling a pilot error, when a user applies "C-x f /sudo: RET"?
It's not so different from the case the user has described in Bug#13900,
which was the trigger to introduce the rule "host name != any method name".

>> Setting `non-essential' to t is a mean to tell Tramp not to check this.
>
> I do know that, but that's just an ugly hack here, since the code is not
> non-essential.

I don't know which further plans you have with `non-essential'. But its
setting to t is acknowledged so far only in ange-ftp and Tramp.

And it is bound to t only in icomplete.el, ido.el and rfn-eshadow.el, as
yet. For exactly that reason: "please relax your tests and connection
actions when performing file name completion or minibuffer decoration".
That doesn't sound ugly to me.

>         Stefan

Best regards, Michael.

Re: [Emacs-diffs] trunk r113958: * minibuffer.el (completion--sifn-requote): Bind `non-essential'.

[Stefan Monnier]

>> So we're in the middle of minibuffer--complete-and-exit which calls
>> try-completion.  That's very definitely not "non-essential".
> Not `minibuffer--complete-and-exit'.  But the completion functions I
> would call non-essential.

Huh?  You did notice that the command's name has "-complete-" in it, right?

> As said, I don't know too much about completion code in minibuffer.el
> (and the functions' docstrings don't help much).  Maybe
> `completion--sifn-requote' is not suited to bind `non-essential'. But
> there are other `completion--*' functions in the backtrace, up to
> `minibuffer--complete-and-exit'.

They're all just computing the completion.

>> Yes, it could be a "complete but ambiguous/erroneous" file name, but it
>> could also be a "not yet complete file name".  So maybe, if there's no
>> ":" nor "/" after "/ssh:", we should refrain from signaling an error.
> Hmm. Why not signalling a pilot error, when a user applies "C-x
> f /sudo: RET"?  It's not so different from the case the user has
> described in Bug#13900, which was the trigger to introduce the rule
> "host name != any method name".

Then how about a different take on the problem: Tramp shouldn't signal
an "ambiguous file name" error when its behavior would be the same
either way.  AFAIK substitute-in-file-name would return the same result
regardless of whether "/ssh:vagrant@192.168.10." is taken to be the file
"vagrant@192.168.10." in host "ssh" or access via ssh to host
"192.168.10.".

> I don't know which further plans you have with `non-essential'.  But its
> setting to t is acknowledged so far only in ange-ftp and Tramp.

So far.

> And it is bound to t only in icomplete.el, ido.el and rfn-eshadow.el, as
> yet. For exactly that reason: "please relax your tests and connection
> actions when performing file name completion or minibuffer decoration".

The key word is "decoration" and not "completion".  The present case is
about completion and not about decoration.

> That doesn't sound ugly to me.

It's called `non-essential' and not `tramp-inhibit' specifically because
I don't want icomplete.el and friends to have Tramp-specific hacks
in them.  If you start abusing it on the premise that "that's how I use
it in Tramp", then it defeats the whole purpose.


        Stefan

Re: [Emacs-diffs] trunk r113958: * minibuffer.el (completion--sifn-requote): Bind `non-essential'.

[Michael Albinus]

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

>> Hmm. Why not signalling a pilot error, when a user applies "C-x
>> f /sudo: RET"?  It's not so different from the case the user has
>> described in Bug#13900, which was the trigger to introduce the rule
>> "host name != any method name".
>
> Then how about a different take on the problem: Tramp shouldn't signal
> an "ambiguous file name" error when its behavior would be the same
> either way.  AFAIK substitute-in-file-name would return the same result
> regardless of whether "/ssh:vagrant@192.168.10." is taken to be the file
> "vagrant@192.168.10." in host "ssh" or access via ssh to host
> "192.168.10.".

Well, I could try to implement it in Tramp's `substitute-in-file-name'
handler. However, I don't know which other functions with file name
handlers are called when applying `minibuffer--complete-and-exit'. Let's
see. (But it would be a just a hack)

>> And it is bound to t only in icomplete.el, ido.el and rfn-eshadow.el, as
>> yet. For exactly that reason: "please relax your tests and connection
>> actions when performing file name completion or minibuffer decoration".
>
> The key word is "decoration" and not "completion".  The present case is
> about completion and not about decoration.
>
>> That doesn't sound ugly to me.
>
> It's called `non-essential' and not `tramp-inhibit' specifically because
> I don't want icomplete.el and friends to have Tramp-specific hacks
> in them.  If you start abusing it on the premise that "that's how I use
> it in Tramp", then it defeats the whole purpose.

I don't want to spread trampish functionality somewhere else. But Tramp
is broken by design: its syntax is ambigous, and sometimes Tramp cannot
decide merely on the file name syntax whether this is the intended file
name to be used on the wire.

That's why it needs advice from the caller: "don't take this file name
serious, I'm not interested in a working connection yet". That does not
mean that Tramp wouldn't use an established connection if it exists
already, it simply means that Tramp doesn't open a new connection if
there isn't one.

Minibuffer decoration is such a case where Tramp needs such an advice,
and file name completion is another case. An incomplete file name does
not need a working connection. Consequently, some predicates used in
file name completion (like `file-exists-p') will give proper results
only when the connection is established finally. But that might be
acceptable.

`non-essential' bound to t looks to me perfect for such an advice. If
you believe it is an abuse of that variable, please give me a hint how
Tramp could get such an advice otherwise.

It is not "that's how I use it in Tramp". Since I hack for Tramp (11
years these days) this problem eats a serious amount of my maintenance
time. I would be more than happy if it could be solved finally,
somehow. Due to the misdesigned Tramp file name syntax, I don't see how
to solve it inside Tramp.

>         Stefan

Best regards, Michael.

Re: [Emacs-diffs] trunk r113958: * minibuffer.el (completion--sifn-requote): Bind `non-essential'.

[Stefan Monnier]

> Well, I could try to implement it in Tramp's `substitute-in-file-name'
> handler. However, I don't know which other functions with file name
> handlers are called when applying `minibuffer--complete-and-exit'.
> Let's see. (But it would be a just a hack)

If you only do it in substitute-in-file-name it will be a hack, indeed.
But if you do it by moving the check to a place where the ambiguity has
consequences, I think it would not be a hack.

> Minibuffer decoration is such a case where Tramp needs such an advice,
> and file name completion is another case.

I don't see why you put completion in the same camp as decorations.


        Stefan

Re: [Emacs-diffs] trunk r113958: * minibuffer.el (completion--sifn-requote): Bind `non-essential'.

[Michael Albinus]

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

> If you only do it in substitute-in-file-name it will be a hack, indeed.
> But if you do it by moving the check to a place where the ambiguity has
> consequences, I think it would not be a hack.

Finally, I have reverted the fix in minibuffer.el. In Tramp, the check
for a proper host name is delayed until the connection is opened.

I'm still not convinced, that Tramp doesn't need more indications for
`non-essential'. But let's wait until it is reported by users.

>         Stefan

Best regards, Michael.