bug#50387: Possible bug in Tramp or in completions

bug#50387: Possible bug in Tramp or in completions

[Gregory Heytings]

With a ~/.ssh/config file containing two entries:

Host foo
   ...
Host bar
   ...

emacs -Q
M-: (setq tramp-default-method "ssh")
M-: (add-to-list 'completion-styles 'substring)
C-x C-x /ssh: TAB

displays "Sole completion".  If, instead of having completion-styles set 
to:

(substring basic partial-completion emacs22)

it is set to:

(basic substring partial-completion emacs22)

then two completion candidates are shown (as expected): "ssh:foo:" and 
"ssh:bar:".

This happens because the "substring" completion mechanism is tried first 
by completion--some in completion--nth-completion, and returns (t . 
substring), because

(completion-substring-try-completion "/ssh:" #'completion-file-name-table 
#'file-exists-p 5)

returns t, because

(completion-substring--all-completions "/ssh:" 
#'completion-file-name-table #'file-exists-p 5) returns (("ssh:")

returns (prefix "ssh:") "/" "" 1), because

(completion-boundaries "/ssh:" #'completion-file-name-table 
#'file-exists-p "")

returns (1 . 0).

When the "basic" completion mechanism is tried first,

(completion-basic-try-completion "/ssh:" #'completion-file-name-table 
#'file-exists-p 5)

returns ("/ssh:" . 5).

It is not clear to me whether this is a bug (I always thought that the 
order of completion mechanisms in completion-styles did not matter, and in 
this case it does indeed make no difference with (setq 
tramp-default-method "scp")), and if so if it is a bug in Tramp, or in the 
completion functions.

bug#50387: Possible bug in Tramp or in completions

[Gregory Heytings]

I analyzed this bug a bit further, and the root problem is that

(tramp-completion-handle-file-name-all-completions "" "/")

returns ("sshfs:" "ssh:" "sshx:" ...) when tramp-default-method is "ssh", 
and ("scp:" "scpx:" ...) when tramp-default-method is "scp".

This happens because "possible methods" are added to the result in 
tramp-completion-handle-file-name-all-completions.

Therefore completion-substring-try-completion finds that "ssh:" is already 
a complete match.

I'm not sure how this bug should be fixed.  Removing these two lines from 
that function fixes the bug, but that's not really optimal because in that 
case /s TAB does not show "ssh:", "sshfs:" and "sshx:" anymore.  Not 
displaying these completions is perhaps better than not displaying 
hostnames, however.

bug#50387: Possible bug in Tramp or in completions

[Michael Albinus]

Gregory Heytings <gregory@heytings.org> writes:

Hi Gregory,

> I analyzed this bug a bit further, and the root problem is that
>
> (tramp-completion-handle-file-name-all-completions "" "/")
>
> returns ("sshfs:" "ssh:" "sshx:" ...) when tramp-default-method is
> "ssh", and ("scp:" "scpx:" ...) when tramp-default-method is "scp".
>
> This happens because "possible methods" are added to the result in
> tramp-completion-handle-file-name-all-completions.
>
> Therefore completion-substring-try-completion finds that "ssh:" is
> already a complete match.
>
> I'm not sure how this bug should be fixed.  Removing these two lines
> from that function fixes the bug, but that's not really optimal
> because in that case /s TAB does not show "ssh:", "sshfs:" and "sshx:"
> anymore.  Not displaying these completions is perhaps better than not
> displaying hostnames, however.

Tramp doesn't know anything about completion-styles, it just implements
file-name-all-completions and file-name-completion. Cutting valid
results from those functions seems to be wrong, IMO.

Best regards, Michael.

bug#50387: Possible bug in Tramp or in completions

[Gregory Heytings]

Hi Michael,

Thanks for your answer!

>> I analyzed this bug a bit further, and the root problem is that
>>
>> (tramp-completion-handle-file-name-all-completions "" "/")
>>
>> returns ("sshfs:" "ssh:" "sshx:" ...) when tramp-default-method is 
>> "ssh", and ("scp:" "scpx:" ...) when tramp-default-method is "scp".
>>
>> This happens because "possible methods" are added to the result in 
>> tramp-completion-handle-file-name-all-completions.
>>
>> Therefore completion-substring-try-completion finds that "ssh:" is 
>> already a complete match.
>>
>> I'm not sure how this bug should be fixed.  Removing these two lines 
>> from that function fixes the bug, but that's not really optimal because 
>> in that case /s TAB does not show "ssh:", "sshfs:" and "sshx:" anymore. 
>> Not displaying these completions is perhaps better than not displaying 
>> hostnames, however.
>
> Tramp doesn't know anything about completion-styles, it just implements 
> file-name-all-completions and file-name-completion. Cutting valid 
> results from those functions seems to be wrong, IMO.
>

The problem is not that results are cut in this case, the problem is that 
because the return value of 
tramp-completion-handle-file-name-all-completions includes the methods 
("sshfs:" "ssh:" "sshx:"), some completion mechanisms do not look further 
when "ssh:" is entered and the user hits TAB to list host candidates:

- substring considers that there is nothing more to complete because 
"ssh:" is one completion candidate

- partial-completion and flex consider that "ssh:" is not complete, but 
the completion candidates they propose are "ssh:", "sshx:" and "sshfs:".

For the case of the substring completion mechanisms (i.e. after 
(add-to-list 'completion-styles 'substring),

(completion-substring-try-completion "/ssh:" #'completion-file-name-table 
#'file-exists-p 5)

returns t with tramp-default-method set to "ssh" (and therefore the 
completion system does not try further), and nil with tramp-default-method 
set to "scp" (and therefore the completion system does try further, and 
eventually finds the hosts).

Without

;; Possible methods.
(setq result (append result (tramp-get-completion-methods m)))

in tramp-completion-handle-file-name-all-completions, all completion 
mechanisms do what a user would expect, namely list host candidates. 
(But the tramp methods are not shown anymore in the completion candidates 
after e.g. /s TAB.)

It seems to me that adding methods (which are not file names), 
conditionally to tramp-default-method, to file-name-all-completions, is 
not TRT, hence this bug.

bug#50387: Possible bug in Tramp or in completions

[Michael Albinus]

Gregory Heytings <gregory@heytings.org> writes:

> Hi Michael,

Hi Grepory,

> It seems to me that adding methods (which are not file names),

I disagree. Methods are part of a remote file name in Emacs.

> conditionally to tramp-default-method, to file-name-all-completions,
> is not TRT, hence this bug.

Best regards, Michael.

bug#50387: Possible bug in Tramp or in completions

[Gregory Heytings]

Hi Michael,

>> It seems to me that adding methods (which are not file names), 
>> conditionally to tramp-default-method, to file-name-all-completions, is 
>> not TRT, hence this bug.
>
> I disagree. Methods are part of a remote file name in Emacs.
>

Okay.  So what would you suggest to fix that bug?

bug#50387: Possible bug in Tramp or in completions

[Michael Albinus]

Gregory Heytings <gregory@heytings.org> writes:

> Hi Michael,

Hi Gregory,

>>> It seems to me that adding methods (which are not file names),
>>> conditionally to tramp-default-method, to
>>> file-name-all-completions, is not TRT, hence this bug.
>>
>> I disagree. Methods are part of a remote file name in Emacs.
>
> Okay.  So what would you suggest to fix that bug?

I don't know completion styles, so I have no suggestions. I have
analysed what Tramp does, and this seems to be right.

I hope somebody else chimes in.

Best regards, Michael.

bug#50387: Possible bug in Tramp or in completions

[Gregory Heytings]

Hi Michael,

>>>> It seems to me that adding methods (which are not file names), 
>>>> conditionally to tramp-default-method, to file-name-all-completions, 
>>>> is not TRT, hence this bug.
>>>
>>> I disagree. Methods are part of a remote file name in Emacs.
>>
>> Okay.  So what would you suggest to fix that bug?
>
> I don't know completion styles, so I have no suggestions. I have 
> analysed what Tramp does, and this seems to be right.
>

I'm not really an expert of completion mechanisms either, but what I do 
know is that what Tramp does with methods in completions is not consistent 
with the way completion mechanisms usually work.

With emacs -Q and (require 'tramp), if you C-x C-f / TAB TAB, you'll see 
only two methods, namely "scp:" and "scpx:".  If you C-x C-f /a TAB TAB, 
you'll see two other methods "adb:" and "afp:".

If you now (add-to-list 'completion-styles 'substring), you still see 
"scp:" and "scpx:" after C-x C-f / TAB TAB, but you do not see any methods 
after C-x C-f /a TAB TAB.

>
> I hope somebody else chimes in.
>

Adding Stefan M in Cc.

bug#50387: Possible bug in Tramp or in completions

[Michael Albinus]

Gregory Heytings <gregory@heytings.org> writes:

> Hi Michael,

Hi Gregory,

> I'm not really an expert of completion mechanisms either, but what I
> do know is that what Tramp does with methods in completions is not
> consistent with the way completion mechanisms usually work.
>
> With emacs -Q and (require 'tramp), if you C-x C-f / TAB TAB, you'll
> see only two methods, namely "scp:" and "scpx:".  If you C-x C-f /a
> TAB TAB, you'll see two other methods "adb:" and "afp:".

Correct. We see

--8<---------------cut here---------------start------------->8---
$ emacs -Q -batch -l tramp --eval '(message "%S" (file-name-all-completions "" "/"))'

("scp:" "scpx:" "sbin/" "proc/" "bin/" "snap/" "opt/" "var/" "dev/" "lib64/" "./" "nonexistent/" "lost+found/" "media/" "root/" "srv/" "etc/" "mnt/" "../" "tmp/" "net/" "sys/" "lib/" "boot/" "home/" "run/" "usr/")

$ emacs -Q -batch -l tramp --eval '(message "%S" (file-name-all-completions "s" "/"))'

("sudoedit:" "sshfs:" "smb:" "scp:" "scpx:" "ssh:" "sshx:" "su:" "sg:" "sudo:" "sftp:" "sbin/" "snap/" "srv/" "sys/")
--8<---------------cut here---------------end--------------->8---

That is, if there's nothing which could be interpreted as method name
(and "" is nothing in this sense),
tramp-completion-handle-file-name-all-completions uses the default
method, "scp", instead, and it completes this to "scp: and "scpx:".

OTOH, if there is at least one character which could belong to a method
("s" in this case), tramp-completion-handle-file-name-all-completions
returns all possible completions of this like "sudoedit:", "sshfs:",
"smb:", "scp:", "scpx:", "ssh:", "sshx:", "su:", "sg:", "sudo:" and
"sftp:".

I believe this is consistent, and it is implemented like this for at
least 15 years I believe.

> If you now (add-to-list 'completion-styles 'substring), you still see
> "scp:" and "scpx:" after C-x C-f / TAB TAB, but you do not see any
> methods after C-x C-f /a TAB TAB.

What completion styles do with these results, is out of my
knowledge. The Tramp manual says

--8<---------------cut here---------------start------------->8---
User name and host name completion is activated only, if file name
completion has one of the styles ‘basic’, ‘emacs21’, or ‘emacs22’.
--8<---------------cut here---------------end--------------->8---

Does this help? Although I have added this sentence myself, I have done
this on recommendation by somebody, w/o really understanding why this is so.

>> I hope somebody else chimes in.
>
> Adding Stefan M in Cc.

Yes, hopefully he can tell us what happens then.

Best regards, Michael.

bug#50387: Possible bug in Tramp or in completions

[Gregory Heytings]

[-- Attachment #1: Type: text/plain, Size: 35 bytes --]


I squashed the bug.  Fix attached.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-diff; name=Improve-handling-of-non-default-completion-styles-wi.patch, Size: 1165 bytes --]

From efd129044567e2016d19b2a6a2bdf2dc06db9458 Mon Sep 17 00:00:00 2001
From: Gregory Heytings <gregory@heytings.org>
Date: Mon, 6 Sep 2021 19:52:52 +0000
Subject: [PATCH] Improve handling of non-default completion styles with Tramp
 methods

* lisp/net/tramp.el (tramp-completion-handle-file-name-all-completions):
Do not return methods when the filename already contains a complete method.
Fixes bug#50387.
---
 lisp/net/tramp.el | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/lisp/net/tramp.el b/lisp/net/tramp.el
index 4fd7a322d4..23c1287b1e 100644
--- a/lisp/net/tramp.el
+++ b/lisp/net/tramp.el
@@ -2839,8 +2839,11 @@ tramp-completion-handle-file-name-all-completions
 			       (delq nil all-user-hosts)))))
 
 	    ;; Possible methods.
-	    (setq result
-		  (append result (tramp-get-completion-methods m)))))))
+	    (unless (or (string-empty-p method)
+		        (string-empty-p user)
+		        (string-empty-p host))
+	      (setq result
+		    (append result (tramp-get-completion-methods m))))))))
 
     ;; Unify list, add hop, remove nil elements.
     (dolist (elt result)
-- 
2.33.0

bug#50387: Possible bug in Tramp or in completions

[Michael Albinus]

Gregory Heytings <gregory@heytings.org> writes:

Hi Gregory,

> * lisp/net/tramp.el (tramp-completion-handle-file-name-all-completions):
> Do not return methods when the filename already contains a complete method.
> Fixes bug#50387.

I don't see this implemented. You rather don't return methods when the
filename does not contain a possible method string at all.

>  	    ;; Possible methods.
> -	    (setq result
> -		  (append result (tramp-get-completion-methods m)))))))
> +	    (unless (or (string-empty-p method)
> +		        (string-empty-p user)
> +		        (string-empty-p host))

The check for (string-empty-p user) and (string-empty-p host) wouldn't
be needed, you are in the else branch of (or user host). IOW, user and
host are already nil, nothing left to test for them.

But I'm not convinced this is the proper solution. I still don't
understand why the default method must be suppressed.

Best regards, Michael.

bug#50387: Possible bug in Tramp or in completions

[Gregory Heytings]

[-- Attachment #1: Type: text/plain, Size: 1813 bytes --]


Hi Michael,

>> * lisp/net/tramp.el (tramp-completion-handle-file-name-all-completions):
>> Do not return methods when the filename already contains a complete method.
>> Fixes bug#50387.
>
> I don't see this implemented. You rather don't return methods when the 
> filename does not contain a possible method string at all.
>

Yet this is what is happening.  You can try it yourself: emacs -Q, M-: 
(require 'tramp), M-: (add-to-list 'completion-styles 'substring), C-x C-f 
/scp: TAB.  This will display "Sole completion", and it displays "Sole 
completion" because "scp:" and "scpx:" are added in the completion 
candiates in tramp-completion-handle-file-name-all-completions.  You can 
see this with

(message "method '%s' result '%s'" method (tramp-get-completion-methods m))

around "Possible methods".  With C-x C-f /scp TAB, you will see

method ’scp’ result ’(nil nil nil nil nil nil nil nil nil nil nil nil nil 
nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil /scpx: /scp: 
nil nil nil nil nil)’

which is correct, but with C-x C-f /scp: TAB, you will see

method ’’ result ’(nil nil nil nil nil nil nil nil nil nil nil nil nil nil 
nil nil nil nil nil nil nil nil nil nil nil nil nil nil /scpx: /scp: nil 
nil nil nil nil)’

which is wrong.

>>  	    ;; Possible methods.
>> -	    (setq result
>> -		  (append result (tramp-get-completion-methods m)))))))
>> +	    (unless (or (string-empty-p method)
>> +		        (string-empty-p user)
>> +		        (string-empty-p host))
>
> The check for (string-empty-p user) and (string-empty-p host) wouldn't 
> be needed, you are in the else branch of (or user host). IOW, user and 
> host are already nil, nothing left to test for them.
>

Whoops, yes, indeed.  Updated patch attached.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-diff; name=Improve-handling-of-non-default-completion-styles-wi.patch, Size: 1089 bytes --]

From 5f4b821bbf5123422d43895bee4c847cfadf31b0 Mon Sep 17 00:00:00 2001
From: Gregory Heytings <gregory@heytings.org>
Date: Mon, 6 Sep 2021 20:52:21 +0000
Subject: [PATCH] Improve handling of non-default completion styles with Tramp
 methods

* lisp/net/tramp.el (tramp-completion-handle-file-name-all-completions):
Do not return methods when the filename already contains a complete method.
Fixes bug#50387.
---
 lisp/net/tramp.el | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/lisp/net/tramp.el b/lisp/net/tramp.el
index 4fd7a322d4..259fc1fcfc 100644
--- a/lisp/net/tramp.el
+++ b/lisp/net/tramp.el
@@ -2839,8 +2839,9 @@ tramp-completion-handle-file-name-all-completions
 			       (delq nil all-user-hosts)))))
 
 	    ;; Possible methods.
-	    (setq result
-		  (append result (tramp-get-completion-methods m)))))))
+	    (unless (string-empty-p method)
+	      (setq result
+		    (append result (tramp-get-completion-methods m))))))))
 
     ;; Unify list, add hop, remove nil elements.
     (dolist (elt result)
-- 
2.33.0

bug#50387: Possible bug in Tramp or in completions

[Michael Albinus]

Gregory Heytings <gregory@heytings.org> writes:

> Hi Michael,

Hi Gregory,

>>> * lisp/net/tramp.el (tramp-completion-handle-file-name-all-completions):
>>> Do not return methods when the filename already contains a complete method.
>>> Fixes bug#50387.
>>
>> I don't see this implemented. You rather don't return methods when
>> the filename does not contain a possible method string at all.
>
> Yet this is what is happening.  You can try it yourself: emacs -Q, M-:
> (require 'tramp), M-: (add-to-list 'completion-styles 'substring), C-x
> C-f /scp: TAB.  This will display "Sole completion", and it displays
> "Sole completion" because "scp:" and "scpx:" are added in the
> completion candiates in
> tramp-completion-handle-file-name-all-completions.  You can see this
> with
>
> (message "method '%s' result '%s'" method (tramp-get-completion-methods m))
>
> around "Possible methods".  With C-x C-f /scp TAB, you will see
>
> method ’scp’ result ’(nil nil nil nil nil nil nil nil nil nil nil nil
> nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil /scpx:
> /scp: nil nil nil nil nil)’
>
> which is correct, but with C-x C-f /scp: TAB, you will see
>
> method ’’ result ’(nil nil nil nil nil nil nil nil nil nil nil nil nil
> nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil /scpx:
> /scp: nil nil nil nil nil)’
>
> which is wrong.

All of this does not fit to your commit message "Do not return methods
when the filename already contains a complete method.", I believe. But
so what.

Your patch does not handle the case when `tramp-syntax' is
`separate'. There are no methods in the remote file name then, and you
must handle user and host names properly.

Finally, I have submitted a patch to master, which deactivates *default*
method, user and host name expansion in
`tramp-completion-handle-file-name-all-completions'. The difference to
your patch is, that it doesn't manipulate the results, but it
deactivates this expansion by usual Tramp means.

Could you pls check how it fits in your use case? My (limited) tests
along your examples look good.

Best regards, Michael.

bug#50387: Possible bug in Tramp or in completions

[Gregory Heytings]

>
> All of this does not fit to your commit message "Do not return methods 
> when the filename already contains a complete method.", I believe. But 
> so what.
>

It could be more precise indeed: "when the filename alread contains a 
complete method or does not contain the prefix of a method", or something 
like that.

>
> Your patch does not handle the case when `tramp-syntax' is `separate'. 
> There are no methods in the remote file name then, and you must handle 
> user and host names properly.
>

I guess adding a test to the conditional would be sufficient: (unless 
(string-empty-p method) -> (unless (or (string-empty-p method) (eq 
'tramp-syntax 'separate)).  I do know what the "separate" syntax is, so I 
did not test it.

>
> Finally, I have submitted a patch to master, which deactivates *default* 
> method, user and host name expansion in 
> `tramp-completion-handle-file-name-all-completions'. The difference to 
> your patch is, that it doesn't manipulate the results, but it 
> deactivates this expansion by usual Tramp means.
>
> Could you pls check how it fits in your use case? My (limited) tests 
> along your examples look good.
>

Alas it doesn't.

emacs -Q -l tramp --eval "(add-to-list 'completion-styles 'substring)"

C-x C-f /scp: TAB

still shows "Sole completion".

bug#50387: Possible bug in Tramp or in completions

[Gregory Heytings]

>
> Alas it doesn't.
>
> emacs -Q -l tramp --eval "(add-to-list 'completion-styles 'substring)"
>
> C-x C-f /scp: TAB
>
> still shows "Sole completion".
>

Sorry, I typed too fast.  It displays two completion candidates: "pscp:" 
and "scp:", which is not what one expects at that point.

With tramp-default-method set to "ssh", C-x C-f /ssh: TAB still displays 
"Sole completion".

bug#50387: Possible bug in Tramp or in completions

[Michael Albinus]

Gregory Heytings <gregory@heytings.org> writes:

>> Alas it doesn't.
>>
>> emacs -Q -l tramp --eval "(add-to-list 'completion-styles 'substring)"
>>
>> C-x C-f /scp: TAB
>>
>> still shows "Sole completion".

In order to see what Tramp does, I have called

--8<---------------cut here---------------start------------->8---
# emacs -Q -l tramp --eval "(add-to-list 'completion-styles 'substring)" --eval "(trace-function-background 'tramp-completion-handle-file-name-all-completions)"
--8<---------------cut here---------------end--------------->8---

> Sorry, I typed too fast.  It displays two completion candidates:
> "pscp:" and "scp:", which is not what one expects at that point.

When I type "C-x C-f /scp: TAB", I see the same result as you, "pscp:"
and "scp:". However, the buffer *trace-output* contains

--8<---------------cut here---------------start------------->8---
======================================================================
1 -> (tramp-completion-handle-file-name-all-completions "" "/")
1 <- tramp-completion-handle-file-name-all-completions: ("sudoedit:" "sshfs:" "smb:" "rcp:" "remcp:" "scp:" "scpx:" "rsync:" "rsh:" "remsh:" "ssh:" "sshx:" "telnet:" "nc:" "su:" "sg:" "sudo:" "doas:" "ksu:" "krlogin:" "plink:" "plinkx:" "pscp:" "psftp:" "fcp:" "rclone:" "afp:" "dav:" "davs:" "gdrive:" "mtp:" "nextcloud:" "sftp:" "ftp:" "adb:")
======================================================================
1 -> (tramp-completion-handle-file-name-all-completions "" "/")
1 <- tramp-completion-handle-file-name-all-completions: ("sudoedit:" "sshfs:" "smb:" "rcp:" "remcp:" "scp:" "scpx:" "rsync:" "rsh:" "remsh:" "ssh:" "sshx:" "telnet:" "nc:" "su:" "sg:" "sudo:" "doas:" "ksu:" "krlogin:" "plink:" "plinkx:" "pscp:" "psftp:" "fcp:" "rclone:" "afp:" "dav:" "davs:" "gdrive:" "mtp:" "nextcloud:" "sftp:" "ftp:" "adb:")
--8<---------------cut here---------------end--------------->8---

That means, "scp:" hasn't been passed to
tramp-completion-handle-file-name-all-completions, and the result of
that function looks proper to me.

> With tramp-default-method set to "ssh", C-x C-f /ssh: TAB still
> displays "Sole completion".

Now I have called

--8<---------------cut here---------------start------------->8---
# emacs -Q -l tramp --eval "(add-to-list 'completion-styles 'substring)" --eval "(trace-function-background 'tramp-completion-handle-file-name-all-completions)" --eval '(setq tramp-default-method "ssh")'
--8<---------------cut here---------------end--------------->8---

Calling "C-x C-f /ssh: TAB" gives me indeed "Sole completion", and
*trace-output* still contains

--8<---------------cut here---------------start------------->8---
======================================================================
1 -> (tramp-completion-handle-file-name-all-completions "" "/")
1 <- tramp-completion-handle-file-name-all-completions: ("sudoedit:" "sshfs:" "smb:" "rcp:" "remcp:" "scp:" "scpx:" "rsync:" "rsh:" "remsh:" "ssh:" "sshx:" "telnet:" "nc:" "su:" "sg:" "sudo:" "doas:" "ksu:" "krlogin:" "plink:" "plinkx:" "pscp:" "psftp:" "fcp:" "rclone:" "afp:" "dav:" "davs:" "gdrive:" "mtp:" "nextcloud:" "sftp:" "ftp:" "adb:")
--8<---------------cut here---------------end--------------->8---

This also looks proper to me.

From the Tramp POV, there's no problem I believe. How completion
packages call file-name-all-completions, and how they interpret the
result, is out of Tramp's responsibility.

Best regards, Michael.

bug#50387: Possible bug in Tramp or in completions

[Gregory Heytings]

>
> When I type "C-x C-f /scp: TAB", I see the same result as you, "pscp:" 
> and "scp:". However, the buffer *trace-output* contains
>
> ======================================================================
> 1 -> (tramp-completion-handle-file-name-all-completions "" "/")
> 1 <- tramp-completion-handle-file-name-all-completions: ("sudoedit:" "sshfs:" "smb:" "rcp:" "remcp:" "scp:" "scpx:" "rsync:" "rsh:" "remsh:" "ssh:" "sshx:" "telnet:" "nc:" "su:" "sg:" "sudo:" "doas:" "ksu:" "krlogin:" "plink:" "plinkx:" "pscp:" "psftp:" "fcp:" "rclone:" "afp:" "dav:" "davs:" "gdrive:" "mtp:" "nextcloud:" "sftp:" "ftp:" "adb:")
> ======================================================================
> 1 -> (tramp-completion-handle-file-name-all-completions "" "/")
> 1 <- tramp-completion-handle-file-name-all-completions: ("sudoedit:" "sshfs:" "smb:" "rcp:" "remcp:" "scp:" "scpx:" "rsync:" "rsh:" "remsh:" "ssh:" "sshx:" "telnet:" "nc:" "su:" "sg:" "sudo:" "doas:" "ksu:" "krlogin:" "plink:" "plinkx:" "pscp:" "psftp:" "fcp:" "rclone:" "afp:" "dav:" "davs:" "gdrive:" "mtp:" "nextcloud:" "sftp:" "ftp:" "adb:")
>
> That means, "scp:" hasn't been passed to 
> tramp-completion-handle-file-name-all-completions, and the result of 
> that function looks proper to me.
>

Yet it isn't.

By the way, before your attempt to fix the problem, you would have seen:

1 -> (tramp-completion-handle-file-name-all-completions "" "/")
1 <- tramp-completion-handle-file-name-all-completions: ("scp:" "scpx:")

IOW, one side effect of your patch is that C-x C-f / TAB now displays all 
Tramp methods.  And you said in your reply to Stefan: "For a completion of 
"/" Tramp tries to be silent, in order not to surprise the many users 
which don't use Tramp with candidates like "/adb:". They wouldn't know 
what's that."  I fully agree with that, and the problem is that Tramp 
tried to be "too smart" here, and to display completions that correspond 
to tramp-default-method, which confuses completion mechanisms.

IOW again:

- before your attempt to fix the problem, C-x C-f / TAB only displayed 
"scp:" and "scpx:" among the completion candidates (or e.g. "ssh:", 
"sshfs:" and "sshx:" with tramp-default-method set to "ssh");

- now C-x C-f / TAB displays all Tramp methods;

- with my patch, which actually fixes the bug, C-x C-f / TAB does not 
display any Tramp method, as it should.

bug#50387: Possible bug in Tramp or in completions

[Michael Albinus]

Gregory Heytings <gregory@heytings.org> writes:

Hi Gregory,

>> When I type "C-x C-f /scp: TAB", I see the same result as you,
>> "pscp:" and "scp:". However, the buffer *trace-output* contains
>>
>> ======================================================================
>> 1 -> (tramp-completion-handle-file-name-all-completions "" "/")
>> 1 <- tramp-completion-handle-file-name-all-completions: ("sudoedit:"
>> "sshfs:" "smb:" "rcp:" "remcp:" "scp:" "scpx:" "rsync:" "rsh:"
>> "remsh:" "ssh:" "sshx:" "telnet:" "nc:" "su:" "sg:" "sudo:" "doas:"
>> "ksu:" "krlogin:" "plink:" "plinkx:" "pscp:" "psftp:" "fcp:"
>> "rclone:" "afp:" "dav:" "davs:" "gdrive:" "mtp:" "nextcloud:"
>> "sftp:" "ftp:" "adb:")
>> ======================================================================
>> 1 -> (tramp-completion-handle-file-name-all-completions "" "/")
>> 1 <- tramp-completion-handle-file-name-all-completions: ("sudoedit:"
>> "sshfs:" "smb:" "rcp:" "remcp:" "scp:" "scpx:" "rsync:" "rsh:"
>> "remsh:" "ssh:" "sshx:" "telnet:" "nc:" "su:" "sg:" "sudo:" "doas:"
>> "ksu:" "krlogin:" "plink:" "plinkx:" "pscp:" "psftp:" "fcp:"
>> "rclone:" "afp:" "dav:" "davs:" "gdrive:" "mtp:" "nextcloud:"
>> "sftp:" "ftp:" "adb:")
>>
>> That means, "scp:" hasn't been passed to
>> tramp-completion-handle-file-name-all-completions, and the result of
>> that function looks proper to me.
>
> Yet it isn't.

Why not? In this example, `completion-styles' contains `substring'. And
then you ask for completion of "/scp:". Looking at the candidates
returned by `tramp-completion-handle-file-name-all-completions', only
"pscp:" and "scp:" have the substring "scp:".

Intuitively, I believe it is correct to show these two methods.

> By the way, before your attempt to fix the problem, you would have seen:
>
> 1 -> (tramp-completion-handle-file-name-all-completions "" "/")
> 1 <- tramp-completion-handle-file-name-all-completions: ("scp:" "scpx:")
>
> IOW, one side effect of your patch is that C-x C-f / TAB now displays
> all Tramp methods.

Yes.

> And you said in your reply to Stefan: "For a completion of "/" Tramp
> tries to be silent, in order not to surprise the many users which
> don't use Tramp with candidates like "/adb:". They wouldn't know
> what's that."  I fully agree with that, and the problem is that Tramp
> tried to be "too smart" here, and to display completions that
> correspond to tramp-default-method, which confuses completion
> mechanisms.

Indeed, I have changed my mind. The initial intention of Tramp's method
completion was to be not invasive, i.e. not to disturb Emacs users not
interested in Tramp.

Eli has reminded me, that we have changed Tramp later on such a way,
that its completion happens only when Tramp is loaded. This wasn't the
case before; Tramp was autoloaded just for its completion, which was not
appreciated by the users not interested in Tramp.

With that change, which is still preserved, there's no need for this
restriction any longer, because a user will see Tramp's completion only
after Tramp has been loaded.

> IOW again:
>
> - before your attempt to fix the problem, C-x C-f / TAB only displayed
>   "scp:" and "scpx:" among the completion candidates (or e.g. "ssh:",
>   "sshfs:" and "sshx:" with tramp-default-method set to "ssh");

Yes, this is changed.

> - now C-x C-f / TAB displays all Tramp methods;

When Tramp is loaded. I regard this to be correct.

> - with my patch, which actually fixes the bug, C-x C-f / TAB does not
>   display any Tramp method, as it should.

Again, C-x C-f / TAB displays a Tramp method only when Tramp is
loaded. What is wrong with this?

Best regards, Michael.

bug#50387: Possible bug in Tramp or in completions

[Gregory Heytings]

>>> That means, "scp:" hasn't been passed to 
>>> tramp-completion-handle-file-name-all-completions, and the result of 
>>> that function looks proper to me.
>>
>> Yet it isn't.
>
> Why not? In this example, `completion-styles' contains `substring'. And 
> then you ask for completion of "/scp:". Looking at the candidates 
> returned by `tramp-completion-handle-file-name-all-completions', only 
> "pscp:" and "scp:" have the substring "scp:".
>

Because after typing "/s" or "/sc" or "/scp" it makes sense to see that 
"/scp:" is among the completion candidates, but after typing "/scp:" it 
doesn't make sense anymore, the method has already been entered by the 
user, what they now want to see is a list of hosts, and there is no way to 
see it.

>> - before your attempt to fix the problem, C-x C-f / TAB only displayed 
>> "scp:" and "scpx:" among the completion candidates (or e.g. "ssh:", 
>> "sshfs:" and "sshx:" with tramp-default-method set to "ssh");
>
> Yes, this is changed.
>
>> - now C-x C-f / TAB displays all Tramp methods;
>
> When Tramp is loaded. I regard this to be correct.
>
>> - with my patch, which actually fixes the bug, C-x C-f / TAB does not 
>> display any Tramp method, as it should.
>
> Again, C-x C-f / TAB displays a Tramp method only when Tramp is loaded.
>

I filed a bug report, explained in every detail what the bug is, provided 
a patch, and the end result is a situation that is worse than earlier.  I 
expect other bug reports from confused users, and hope that you'll change 
your mind.

Closing.

bug#50387: Possible bug in Tramp or in completions

[Michael Albinus]

Gregory Heytings <gregory@heytings.org> writes:

Hi Gregory,

>> Why not? In this example, `completion-styles' contains
>> `substring'. And then you ask for completion of "/scp:". Looking at
>> the candidates returned by
>> `tramp-completion-handle-file-name-all-completions', only "pscp:"
>> and "scp:" have the substring "scp:".
>>
>
> Because after typing "/s" or "/sc" or "/scp" it makes sense to see
> that "/scp:" is among the completion candidates, but after typing
> "/scp:" it doesn't make sense anymore, the method has already been
> entered by the user, what they now want to see is a list of hosts, and
> there is no way to see it.

Type any first char of a user or host name, and you'll see it.

> I filed a bug report, explained in every detail what the bug is,
> provided a patch, and the end result is a situation that is worse than
> earlier.  I expect other bug reports from confused users,

We'll see. In my opinion, the situation isn't worse, but more
consistent, because there is no arbitrary filtering of method names
anymore (arbitrary in the eye of a user).

> and hope that you'll change your mind.

I will, when I'm convinced the current situation isn't acceptable. The
current situation (you must type a char in order to see the host name)
is described in the Tramp manual, and I haven't heard complaints about
since ever. (I know, nobody reads manuals ...)

> Closing.

Best regards, Michael.

bug#50387: Possible bug in Tramp or in completions

[Gregory Heytings]

>
> I will, when I'm convinced the current situation isn't acceptable. The 
> current situation (you must type a char in order to see the host name) 
> is described in the Tramp manual, and I haven't heard complaints about 
> since ever.
>

That might be what the manual says, but it's not what the current 
situation actually is.  Type C-x C-f /scp TAB and you'll see scp: scpx:. 
Type C-x C-f /scp: TAB and you'll see scp:foo: scp:bar:.  This is 
coherent, ':' acts like a directory separator, like the '://' in URLs.

But for those who use the substring and flex completion styles, typing C-x 
C-f /scp TAB displays pscp: scp: scpx:.  Typing C-x C-f /scp: TAB still 
displays scp: scpx:.  This isn't at all coherent.

bug#50387: Possible bug in Tramp or in completions

[Gregory Heytings]

[-- Attachment #1: Type: text/plain, Size: 785 bytes --]


>> I will, when I'm convinced the current situation isn't acceptable. The 
>> current situation (you must type a char in order to see the host name) 
>> is described in the Tramp manual, and I haven't heard complaints about 
>> since ever.
>
> That might be what the manual says
>

And in fact that's not what the manual says.  See (info "(tramp)File name 
completion"):

[After ‘/ssh:’,] typing ‘<TAB>’ shows host names TRAMP extracts from 
‘~/.ssh/config’ file, for example.

        ssh:127.0.0.1:                       ssh:192.168.0.1:
        ssh:[::1]:                           ssh:localhost:
        ssh:melancholia.danann.net:          ssh:melancholia:

Choose a host from the above list and then continue to complete file names 
on that host.

bug#50387: Possible bug in Tramp or in completions

[Michael Albinus]

Gregory Heytings <gregory@heytings.org> writes:

>>> I will, when I'm convinced the current situation isn't
>>> acceptable. The current situation (you must type a char in order to
>>> see the host name) is described in the Tramp manual, and I haven't
>>> heard complaints about since ever.
>>
>> That might be what the manual says
>>
>
> And in fact that's not what the manual says.  See (info "(tramp)File
> name completion"):

You're right, my answer was reckless. Sorry.

Best regards, Michael.

bug#50387: Possible bug in Tramp or in completions

[Michael Albinus]

Gregory Heytings <gregory@heytings.org> writes:

>>
>> I will, when I'm convinced the current situation isn't
>> acceptable. The current situation (you must type a char in order to
>> see the host name) is described in the Tramp manual, and I haven't
>> heard complaints about since ever.
>>
>
> That might be what the manual says, but it's not what the current
> situation actually is.  Type C-x C-f /scp TAB and you'll see scp:
> scpx:.

Sure. Completion style `substring' is not active, and so we see in the
traces buffer

--8<---------------cut here---------------start------------->8---
======================================================================
1 -> (tramp-completion-handle-file-name-all-completions "scp" "/")
1 <- tramp-completion-handle-file-name-all-completions: ("scp:" "scpx:")
======================================================================
1 -> (tramp-completion-handle-file-name-all-completions "scp" "/")
1 <- tramp-completion-handle-file-name-all-completions: ("scp:" "scpx:")
--8<---------------cut here---------------end--------------->8---

See, how `tramp-completion-handle-file-name-all-completions' is invoked
differently.

> Type C-x C-f /scp: TAB and you'll see scp:foo: scp:bar:.  This
> is coherent, ':' acts like a directory separator, like the '://' in
> URLs.

The traces show us

--8<---------------cut here---------------start------------->8---
======================================================================
1 -> (tramp-completion-handle-file-name-all-completions "scp:" "/")
1 <- tramp-completion-handle-file-name-all-completions: ("scp:test:" ... "scp:osboxes@")
======================================================================
1 -> (tramp-completion-handle-file-name-all-completions "scp:" "/")
1 <- tramp-completion-handle-file-name-all-completions: ("scp:test:" ... "scp:osboxes@")
--8<---------------cut here---------------end--------------->8---

(I've cut the result of the function, you don't want to see the zillions
of possible completions in my stanza :-) Again, see the arguments the
function is called with.

> But for those who use the substring and flex completion styles, typing
> C-x C-f /scp TAB displays pscp: scp: scpx:.  Typing C-x C-f /scp: TAB
> still displays scp: scpx:.  This isn't at all coherent.

I've said it several times: `tramp-completion-handle-file-name-all-completions'
is called with arguments, and it returns a corresponding result. It is
consistent. How completion styles interpret the result, is out of
Tramp's responsibility.

I don't say there's no bug. But as Tramp maintainer I believe there is
no bug in Tramp (anymore); I have no knowledge of completion styles in
order to help fixing it there.

Best regards, Michael.

bug#50387: Possible bug in Tramp or in completions

[Gregory Heytings]

>
> But as Tramp maintainer I believe there is no bug in Tramp (anymore); I 
> have no knowledge of completion styles in order to help fixing it there.
>

There is a bug, because the return values provided by Tramp to the 
(substring and flex) completion mechanisms are not what these mechanisms 
expect to do their job.  Tramp should not return methods when 
(tramp-completion-handle-file-name-all-completions "" "/") is called, 
because there are no methods in the root directory, what the root 
directory contains is files.  If Tramp was using another character instead 
of "/" before methods, the problem would not exist because there would be 
a clear way to differentiate files and methods.

Anyway, I think I've now made my point as clear as possible, and I use the 
correct bugfix in my local Emacs, so I won't continue this discussion.

bug#50387: Possible bug in Tramp or in completions

[Michael Albinus]

Gregory Heytings <gregory@heytings.org> writes:

Hi Gregory,

> Tramp should not return methods when
> (tramp-completion-handle-file-name-all-completions "" "/") is called,
> because there are no methods in the root directory, what the root
> directory contains is files.  If Tramp was using another character
> instead of "/" before methods, the problem would not exist because
> there would be a clear way to differentiate files and methods.

FTR, this is simply wrong. Remote file names, starting with
"/<method>:", are first class Emacs citizens, designed that way more
than twenty years ago.

Completion styles have been added later.

> Anyway, I think I've now made my point as clear as possible, and I use
> the correct bugfix in my local Emacs, so I won't continue this
> discussion.

As you like. Until this problem has been fixed, I'll add a note to
Tramp's manual, that user and host name completion with the `substring'
or `flex' style require an initial letter or digit of the name.

Best regards, Michael.

bug#50387: Possible bug in Tramp or in completions

[Lars Ingebrigtsen]

I'm getting this today:

1 unexpected results:
   FAILED  tramp-test26-file-name-completion

Test tramp-test26-file-name-completion condition:
    (ert-test-failed
     ((should
       (member
	(concat prefix-format method-marker tramp-postfix-method-format ipv6-prefix host ipv6-postfix tramp-postfix-host-format)
	(file-name-all-completions ... "/")))
      :form
      (member
       #("-:elva:" 2 6
	 (tramp-default t))
       nil)
      :value nil))


I'm guessing it has something to do with this bug report...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#50387: Possible bug in Tramp or in completions

[Michael Albinus]

Lars Ingebrigtsen <larsi@gnus.org> writes:

Hi Lars,

> 1 unexpected results:
>    FAILED  tramp-test26-file-name-completion

Oops, yes. The test for default method completion doesn't make sense
anymore. I've fixed this in master. Sorry for the trouble!

Best regards, Michael.

bug#50387: Possible bug in Tramp or in completions

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> Adding Stefan M in Cc.
> Yes, hopefully he can tell us what happens then.

The completion tables's API only directly support prefix completion.
So in order to implement `substring` style, when the minibuffer.el code
sees a request for completion of "/a" it will request all the
possible prefix-completions for "/" and then filter them based on the
presence of "a" in there.

Tramp can only affect the file name completion table, so all it gets to
see is a request for the set of prefix-completions of "/".
Maybe Tramp could include "/adb:" and friends in there, but Michael
probably has good reasons for not doing so.


        Stefan

bug#50387: Possible bug in Tramp or in completions

[Michael Albinus]

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

Hi Stefan,

> Tramp can only affect the file name completion table, so all it gets to
> see is a request for the set of prefix-completions of "/".
> Maybe Tramp could include "/adb:" and friends in there, but Michael
> probably has good reasons for not doing so.

For a completion of "/" Tramp tries to be silent, in order not to
surprise the many users which don't use Tramp with candidates like
"/adb:". They wouldn't know what's that.

The compromise is to show them only the default method "/scp:" (and
"/scpx:" due to completion), in order to give them an idea that there's
more in Emacs but the local file system.

>         Stefan

Best regards, Michael.

bug#50387: Possible bug in Tramp or in completions

[Gregory Heytings]

>
> The completion tables's API only directly support prefix completion. So 
> in order to implement `substring` style, when the minibuffer.el code 
> sees a request for completion of "/a" it will request all the possible 
> prefix-completions for "/" and then filter them based on the presence of 
> "a" in there.
>

Thank you.  Now I understand the bug better: when the user types / TAB, 
Tramp returns the default methods (scp: scpx:), when the user types /sc 
TAB, Tramp returns the methods starting with 'sc', i.e. scp: scpx:, and 
when the user types /scp: TAB, Tramp again returns the default methods 
scp: scpx:, because it is again at '/', and the substring completion 
backend concludes that "scp:" cannot be completed further, which is wrong.

With my patch, when the user types / TAB, Tramp does not return the 
default methods anymore, and therefore they are not returned after /scp: 
TAB either.  So IMO the patch does TRT, and fixes both the behavior with 
substring and flex completion styles.

bug#50387: Possible bug in Tramp or in completions

[Eli Zaretskii]

> Date: Mon, 06 Sep 2021 21:39:06 +0000
> From: Gregory Heytings <gregory@heytings.org>
> Cc: Michael Albinus <michael.albinus@gmx.de>, 50387@debbugs.gnu.org
> 
> With my patch, when the user types / TAB, Tramp does not return the 
> default methods anymore, and therefore they are not returned after /scp: 
> TAB either.  So IMO the patch does TRT, and fixes both the behavior with 
> substring and flex completion styles.

When the dust settles on this, please make sure typing "C-x C-f / TAB"
doesn't cause Tramp to be loaded into Emacs due to these changes.
(I'm not saying it should, but we had trouble with this before, and
Michael solved them.)

bug#50387: Possible bug in Tramp or in completions

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

Hi Eli,

> When the dust settles on this, please make sure typing "C-x C-f / TAB"
> doesn't cause Tramp to be loaded into Emacs due to these changes.
> (I'm not saying it should, but we had trouble with this before, and
> Michael solved them.)

This is guaranteed. tramp-completion-file-name-handler is added to
file-name-handler-alist only while Tramp is loading.

Best regards, Michael.