bug#53053: 28.0.90; Tramp completion bug of path /sudo::~/

bug#53053: 28.0.90; Tramp completion bug of path /sudo::~/

[Daniel Mendler]

1. emacs -Q
2. C-x f /sudo::~/ (alternatively use an ssh path of a similar form)
3. Open the completion buffer by pressing ?
4. Click/select a directory in the completions buffer

The resulting path looks like this:

/sudo::~dir/

The underlying issue is that somewhere a wrong completion boundary is
reported by the Emacs completion table for non-normalized file paths
which contain ~. This issue also affects other completion UIs. The issue
is present on both 27 and 28.

(Original report: https://github.com/minad/vertico/issues/174)

In GNU Emacs 28.0.90 (build 3, x86_64-pc-linux-gnu, GTK+ Version 3.24.5,
cairo version 1.16.0)
 of 2022-01-01 built on projects
Repository revision: efb1c7ec379430f560c5b801969ae43023c52734
Repository branch: emacs-28
Windowing system distributor 'The X.Org Foundation', version 11.0.12004000
System Description: Debian GNU/Linux 10 (buster)

bug#53053: 28.0.90; Tramp completion bug of path /sudo::~/

[Michael Albinus]

Daniel Mendler <mail@daniel-mendler.de> writes:

Hi Daniel,

> 1. emacs -Q
> 2. C-x f /sudo::~/ (alternatively use an ssh path of a similar form)
> 3. Open the completion buffer by pressing ?
> 4. Click/select a directory in the completions buffer
>
> The resulting path looks like this:
>
> /sudo::~dir/
>
> The underlying issue is that somewhere a wrong completion boundary is
> reported by the Emacs completion table for non-normalized file paths
> which contain ~. This issue also affects other completion UIs. The issue
> is present on both 27 and 28.

I can confirm this. Tested with recent Emacs 29.0.50. As far as Tramp is
concerned, I see

--8<---------------cut here---------------start------------->8---
1 -> (file-name-all-completions "" #("/sudo:root@gandalf:/root/" 6 10 (tramp-default t) 11 18 (tramp-default t)))
1 <- file-name-all-completions: (".tcshrc" ".bash_profile" ".viminfo" ".cshrc" "tmp/" "../" ".cache/" ".local/" ".tramp_history" ".bash_history" "anaconda-ks.cfg" "./" ".bashrc" ".history" ".emacs.d/" ".config/" ".bash_logout" ".dbus/" ".ssh/")
--8<---------------cut here---------------end--------------->8---

This looks proper. However, when clicking on "tmp/" (as said in your
recipe), I get "/sudo::~tmp/".

I'm not an expert in the completion machinery, so I cannot contribute
much more to this problem.

Best regards, Michael.

bug#53053: 28.0.90; Tramp completion bug of path /sudo::~/

[Daniel Mendler]

Hi Michael,

thank you for your response! The issue came up again in
https://github.com/minad/vertico/issues/193. The issue is that the
completion boundaries are reported incorrectly by the file completion
table or the Tramp backend.

For example both

(completion-boundaries "/sudo::~/" #'read-file-name-internal nil "")
(completion-boundaries "/sudo::/" #'read-file-name-internal nil "")

incorrectly return (8 . 0) as boundaries. These values are reported by
the 'boundaries action of the completion table. I am not sure if the bug
is on the layer of Tramp or on a level above. I cc'ed Stefan, he
probably knows the precise location.

Daniel

bug#53053: 28.0.90; Tramp completion bug of path /sudo::~/

[Michael Albinus]

Daniel Mendler <mail@daniel-mendler.de> writes:

> Hi Michael,

Hi Daniel,

> thank you for your response! The issue came up again in
> https://github.com/minad/vertico/issues/193. The issue is that the
> completion boundaries are reported incorrectly by the file completion
> table or the Tramp backend.
>
> For example both
>
> (completion-boundaries "/sudo::~/" #'read-file-name-internal nil "")
> (completion-boundaries "/sudo::/" #'read-file-name-internal nil "")
>
> incorrectly return (8 . 0) as boundaries. These values are reported by
> the 'boundaries action of the completion table. I am not sure if the bug
> is on the layer of Tramp or on a level above. I cc'ed Stefan, he
> probably knows the precise location.

Tramp doesn't know anything about completion-boundaries.

> Daniel

Best regards, Michael.

bug#53053: 28.0.90; Tramp completion bug of path /sudo::~/

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

> I cc'ed Stefan, he probably knows the precise location.

I'll ask him as son as I can get ahold of him,


        Stefan

bug#53053: 28.0.90; Tramp completion bug of path /sudo::~/

[Daniel Mendler]

On 2/5/22 19:10, Stefan Monnier wrote:
>> I cc'ed Stefan, he probably knows the precise location.
> 
> I'll ask him as son as I can get ahold of him,

So did you find him? His wisdom is dearly needed.

bug#53053: 28.0.90; Tramp completion bug of path /sudo::~/

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

> For example both
>
> (completion-boundaries "/sudo::~/" #'read-file-name-internal nil "")
> (completion-boundaries "/sudo::/" #'read-file-name-internal nil "")
>
> incorrectly return (8 . 0) as boundaries.

The problem comes from the quoting/unquoting.

`read-file-name-internal` is used for file names that will pass through
`substitute-in-file-name` (as is the case for file names read from
`read-file-name`).

So when you ask to complete "/sudo::~/", Emacs first expands it with
`substitute-in-file-name` and then asks for the completion.
The problem is that the expansion of "/sudo::~/" is exactly the same as
the expansion of "/sudo::~"

So maybe the "right fix" is to change Tramp's handling of
`substitute-in-file-name` such that
"/sudo::~" returns "/sudo:root@<host>:~" instead of
"/sudo:root@<host>:~/",
but in the mean time I installed the patch below which should avoid
this problem at least in the original recipe.  You can still bump into
side effects of the underlying problem, of course.

As pointed out in the comment in `completion--sifn-requote`, this
function is fundamentally asked to do the impossible.


        Stefan


diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index c9f58239403..36b8d808417 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -2932,6 +2932,10 @@
   (let* ((ustr (substitute-in-file-name qstr))
          (uprefix (substring ustr 0 upos))
          qprefix)
+    (if (eq upos (length ustr))
+        ;; Easy and common case.  This not only speed things up in a very
+        ;; common case but it also avoids problems in some cases (bug#53053).
+        (cons (length qstr) #'minibuffer-maybe-quote-filename)
     ;; Main assumption: nothing after qpos should affect the text before upos,
     ;; so we can work our way backward from the end of qstr, one character
     ;; at a time.
@@ -2951,7 +2955,7 @@
                                    (substitute-in-file-name
                                     (substring qstr 0 (1- qpos)))))
         (setq qpos (1- qpos)))
-      (cons qpos #'minibuffer-maybe-quote-filename))))
+        (cons qpos #'minibuffer-maybe-quote-filename)))))
 
 (defalias 'completion--file-name-table
   (completion-table-with-quoting #'completion-file-name-table

bug#53053: 28.0.90; Tramp completion bug of path /sudo::~/

[Michael Albinus]

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

Hi,

> So maybe the "right fix" is to change Tramp's handling of
> `substitute-in-file-name` such that
> "/sudo::~" returns "/sudo:root@<host>:~" instead of
> "/sudo:root@<host>:~/",
> but in the mean time I installed the patch below which should avoid
> this problem at least in the original recipe.  You can still bump into
> side effects of the underlying problem, of course.

In tramp-handle-substitute-in-file-name, there is the code

--8<---------------cut here---------------start------------->8---
      ;; "/m:h:~" does not work for completion.  We use "/m:h:~/".
      (if (and (stringp localname) (string-equal "~" localname))
	  (concat filename "/")
	filename))))
--8<---------------cut here---------------end--------------->8---

The ChangeLog does not give a reasoning. So I've removed this, and
testing with Emacs 28.0.50 shows proper behavior now with the recipe.

I've pushed it to master. The patch from Stefan could be reverted I
believe, unless it is also good for something else.

The Tramp change will also appear in Tramp 2.5.2.2, which is the
upcoming version on GNU ELPA. Later, it will be merged into Emacs 28.2.

>         Stefan

Best regards, Michael.

bug#53053: 28.0.90; Tramp completion bug of path /sudo::~/

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

> The ChangeLog does not give a reasoning. So I've removed this, and
> testing with Emacs 28.0.50 shows proper behavior now with the recipe.

Thanks.  Crossing fingers!

> I've pushed it to master. The patch from Stefan could be reverted I
> believe, unless it is also good for something else.

I think it's good in general: that function does a heuristic job, and
this heuristic is cheap and can still be useful in other cases.


        Stefan

bug#53053: 28.0.90; Tramp completion bug of path /sudo::~/

[Michael Albinus]

Version: 28.2

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

Hi Stefan,

>> The ChangeLog does not give a reasoning. So I've removed this, and
>> testing with Emacs 28.0.50 shows proper behavior now with the recipe.
>
> Thanks.  Crossing fingers!
>
>> I've pushed it to master. The patch from Stefan could be reverted I
>> believe, unless it is also good for something else.
>
> I think it's good in general: that function does a heuristic job, and
> this heuristic is cheap and can still be useful in other cases.

Well, nothing left to do then, I'm closing the bug.

>         Stefan

Best regards, Michael.