bug#19412: 24.3; ido-write-file sometimes writes to a different directory than it says it will

bug#19412: 24.3; ido-write-file sometimes writes to a different directory than it says it will

[Don Morrison]

This bug report will be sent to the Bug-GNU-Emacs mailing list
and the GNU bug tracker at debbugs.gnu.org.  Please check that
the From: line contains a valid email address.  After a delay of up
to one day, you should receive an acknowledgment at that address.

Please write in English if possible, as the Emacs maintainers
usually do not have translators for other languages.

Please describe exactly what actions triggered the bug, and
the precise symptoms of the bug.  If you can, give a recipe
starting from `emacs -Q':



Create a file, ~/mumble.frotz, containing some text.

emacs -Q

M-x ido-mode

M-x ido-everywhere

C-x C-f ~/mumble.frotz

C-x C-w /tmp/ C-f

You are now sitting at a prompt that appears to be saying if confirmed
it will write the file into /tmp/, with the file name mumble.frotz
implied.

Hit the carriage return key to confirm it.

Note that it is trying to write it into ~/mumble.frotz, not /tmp/mumble.frotz




If Emacs crashed, and you have the Emacs process in the gdb debugger,
please include the output from the following gdb commands:
    `bt full' and `xbacktrace'.
For information about debugging Emacs, please read the file
/usr/share/emacs/24.3/etc/DEBUG.


In GNU Emacs 24.3.1 (x86_64-pc-linux-gnu, GTK+ Version 3.10.7)
 of 2014-03-07 on lamiak, modified by Debian
Windowing system distributor `The X.Org Foundation', version 11.0.11501000
System Description: Linux Mint 17 Qiana

Configured using:
 `configure '--build' 'x86_64-linux-gnu' '--build' 'x86_64-linux-gnu'
 '--prefix=/usr' '--sharedstatedir=/var/lib' '--libexecdir=/usr/lib'
 '--localstatedir=/var/lib' '--infodir=/usr/share/info'
 '--mandir=/usr/share/man' '--with-pop=yes'
 '--enable-locallisppath=/etc/emacs24:/etc/emacs:/usr/local/share/emacs/24.3/site-lisp:/usr/local/share/emacs/site-lisp:/usr/share/emacs/24.3/site-lisp:/usr/share/emacs/site-lisp'
 '--with-crt-dir=/usr/lib/x86_64-linux-gnu' '--with-x=yes'
 '--with-x-toolkit=gtk3' '--with-toolkit-scroll-bars'
 'build_alias=x86_64-linux-gnu' 'CFLAGS=-g -O2 -fstack-protector
 --param=ssp-buffer-size=4 -Wformat -Werror=format-security -Wall'
 'LDFLAGS=-Wl,-Bsymbolic-functions -Wl,-z,relro'
 'CPPFLAGS=-D_FORTIFY_SOURCE=2''

Important settings:
  value of $LANG: en_US.UTF-8
  locale-coding-system: utf-8-unix
  default enable-multibyte-characters: t

Major mode: Fundamental

Minor modes in effect:
  shell-dirtrack-mode: t
  ido-everywhere: t
  tooltip-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t
  transient-mark-mode: t

Recent input:
<help-echo> <help-echo> M-x i d o - e v e r y w h e
r e <return> <backspace> <backspace> <backspace> <backspace>
<backspace> <backspace> <backspace> <backspace> <backspace>
<backspace> m o d e <return> M-x i d o - e v e r y
w h e r e <return> C-x C-f m u m <return> C-x C-w /
t m p / C-f <return> C-g <help-echo> M-x r e p o r
t - e m a c s - b u g <return>

Recent messages:
For information about GNU Emacs and the GNU system, type C-h C-a.
Ido mode enabled
Ido-Everywhere mode enabled
mumble.frotz has auto save data; consider M-x recover-this-file
Quit
Type C-x 1 to delete the help window.

Load-path shadows:
None found.

Features:
(shadow sort mail-extr help-mode emacsbug message rfc822 mml easymenu
mml-sec mm-decode mm-bodies mm-encode mail-parse rfc2231 mailabbrev
gmm-utils mailheader sendmail rfc2047 rfc2045 ietf-drums mail-utils
tramp-cache tramp tramp-compat auth-source eieio byte-opt bytecomp
byte-compile cconv gnus-util mm-util mail-prsvr password-cache
tramp-loaddefs shell pcomplete comint ansi-color ring format-spec advice
help-fns cl-lib advice-preload cus-start cus-load ido time-date tooltip
ediff-hook vc-hooks lisp-float-type mwheel x-win x-dnd tool-bar dnd
fontset image regexp-opt fringe tabulated-list newcomment lisp-mode
register page menu-bar rfn-eshadow timer select scroll-bar mouse
jit-lock font-lock syntax facemenu font-core frame cham georgian
utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao korean
japanese hebrew greek romanian slovak czech european ethiopic indian
cyrillic chinese case-table epa-hook jka-cmpr-hook help simple abbrev
minibuffer loaddefs button faces cus-face macroexp files text-properties
overlay sha1 md5 base64 format env code-pages mule custom widget
hashtable-print-readable backquote make-network-process dbusbind
dynamic-setting system-font-setting font-render-setting move-toolbar gtk
x-toolkit x multi-tty emacs)

bug#19412: 24.3; ido-write-file sometimes writes to a different directory than, it says it will

[Ryan C. Thompson]

I believe I have discovered the cause of this bug. It can be reproduced 
by evaluating the following code and then hitting RET:

(read-file-name-default "Write file: " "/tmp/" "~/mumble.frotz" nil nil nil)

This will return "~/mumble.frotz" rather than "/tmp/". Ido triggers this 
issue by causing "read-file-name-default" to be called with arguments 
like the above when triggered to fall back to non-ido completion as 
described in the inital report. The crux of the issue is that the 
initial directory, "/tmp/", is treated as *not* user-entered, so 
pressing RET on it returns the default filename instead (which is 
populated from the buffer file name).

This is not trivial to fix, because ido isn't calling 
"read-file-name-default" directly. In the example given, it's 
let-binding "default-directory" to "/tmp/" and then doing:

(call-interactively 'write-file)

And then the interactive form calls:

(read-file-name "Write file: ")

which then picks up DIR and DEFAULT-FILENAME from "default-directory" 
and "buffer-file-name" respectively. So this isn't a case of just fixing 
a function call somewhere. One possible solution would be to also 
let-bind "buffer-file-name" to nil, in which case DEFAULT-FILENAME gets 
set to DIR. That would work for the case of "write-file", but I don't 
know if it would work for other functions that read file names.

Lastly, I'm guessing that the original reporter ran into this issue 
because they were using C-f RET to select "/tmp/" within ido completion, 
since RET would just select the first file or subdirectory on the list. 
They should be using C-j to do that.

I might add a hack to my ido-completing-read-plus package to fix this 
edge case, if I can figure out a reasonably clean way to fix it.

bug#19412: 24.3; ido-write-file sometimes writes to a different directory than, it says it will

[Ryan C. Thompson]

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

I think I have found a fix for this issue. A patch for ido.el is 
attached. The solution is essentially to simulate re-typing the current 
ido input into the fallback command's prompt rather than modifying 
dynamic variables to trick the fallback command into starting in the 
right place.

Note that this is NOT thoroughly tested yet. It seems to work for the 
specific case described in this bug (ido-write-file), but I need to test 
it for some time to make sure it isn't breaking other cases at the same 
time. I will use this fix in my Emacs for some time and report back.

[-- Attachment #2: 0001-Ensure-correct-behavior-in-ido-file-fallback-complet.patch --]
[-- Type: text/plain, Size: 2165 bytes --]

From 11f7ce6952550cbcec35492f41baa14025911cfa Mon Sep 17 00:00:00 2001
From: "Ryan C. Thompson" <rct@thompsonclan.org>
Date: Sun, 3 Nov 2019 23:40:13 -0800
Subject: [PATCH] Ensure correct behavior in ido file fallback completion

Fixes bug #19412. Previously, when falling back, ido would let-bind
default-directory to ido-current-directory so that read-file-name
would start in the right location. However, this has an unexpected
side effect (see the bug report). Instead, ido now leaves
default-directory alone and instead uses minibuffer-with-setup-hook to
simulate the user deleting the initial input and typing
ido-current-directory instead.
---
 lisp/ido.el | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/lisp/ido.el b/lisp/ido.el
index 2a660e6b0c..c17972c1cc 100644
--- a/lisp/ido.el
+++ b/lisp/ido.el
@@ -2428,11 +2428,23 @@ If cursor is not at the end of the user input, move to end of input."
        ((eq ido-exit 'fallback)
 	;; Need to guard setting of default-directory here, since
 	;; we don't want to change directory of current buffer.
-	(let ((default-directory ido-current-directory)
-	      (read-file-name-function nil))
+	(let ((default-directory default-directory)
+              (read-file-name-function nil))
 	  (setq this-command (or ido-fallback fallback 'find-file))
 	  (run-hook-with-args 'ido-before-fallback-functions this-command)
-	  (call-interactively this-command)))
+          ;; Workaround for bug #19412: ensure that pressing RET
+          ;; immediately after falling back with C-f will select the
+          ;; input rather than use the default (which is
+          ;; `default-directory').
+          (minibuffer-with-setup-hook
+              (:append
+               (lambda ()
+                 ;; Clear out whatever started in the minibuffer and
+                 ;; replace it with what the user had already entered
+                 ;; into ido.
+                 (delete-minibuffer-contents)
+                 (insert ido-current-directory)))
+            (call-interactively this-command))))
 
        ((eq ido-exit 'switch-to-buffer)
 	(ido-buffer-internal
-- 
2.23.0

bug#19412: 24.3; ido-write-file sometimes writes to a different directory than, it says it will

[Ryan C. Thompson]

On further testing, I've determined that this patch does not handle all 
the relevant code paths. At least ido-file-internal, ido-read-file-name, 
and ido-read-directory-name all need to be handled, each in a slightly 
different way. I'll work on it.

In any case, please DO NOT install the current patch as is.

bug#19412: 24.3; ido-write-file sometimes writes to a different directory than, it says it will

[Ryan C. Thompson]

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

Ok, I think I have a working patch for this issue. I patched 
ido-file-internal, ido-read-file-name, and ido-read-directory-name, and 
I think that's all the code paths that need to be fixed. Now the test 
case described in the original report produces the correct result for 
me: writing to /tmp/mumble.frotz.

However, I should note that this is still relatively untested. I will 
test it out and try to make sure it doesn't cause any unexpected issues 
before I recommend merging it.


[-- Attachment #2: 0001-Fix-default-directory-handling-in-ido-file-fallback-.patch --]
[-- Type: text/plain, Size: 5683 bytes --]

From 4a9443ddd8be57b2a74cdabf5aa0d2cfbba35053 Mon Sep 17 00:00:00 2001
From: "Ryan C. Thompson" <rct@thompsonclan.org>
Date: Wed, 11 Mar 2020 12:24:24 -0400
Subject: [PATCH] Fix default directory handling in ido file fallback
 (bug#19412)

Briefly, when falling back from ido file completion to normal file
completion, previously the current directory at the time of falling
back was treated as the default directory, which was wrong and caused
unintuitive edge cases. Now, when falling back for file completion,
ido uses the original default directory that ido was called with and
then uses `minibuffer-with-setup-hook' to "simulate" typing in the
currently entered directory, so that it is not treated as the
default. See the bug description for more information.
---
 lisp/ido.el | 54 +++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 48 insertions(+), 6 deletions(-)

diff --git a/lisp/ido.el b/lisp/ido.el
index 81883402ad..bf35df9547 100644
--- a/lisp/ido.el
+++ b/lisp/ido.el
@@ -2355,11 +2355,23 @@ If cursor is not at the end of the user input, move to end of input."
        ((eq ido-exit 'fallback)
 	;; Need to guard setting of default-directory here, since
 	;; we don't want to change directory of current buffer.
-	(let ((default-directory ido-current-directory)
-	      (read-file-name-function nil))
+	(let ((default-directory default-directory)
+              (read-file-name-function nil))
 	  (setq this-command (or ido-fallback fallback 'find-file))
 	  (run-hook-with-args 'ido-before-fallback-functions this-command)
-	  (call-interactively this-command)))
+          ;; Workaround for bug#19412: ensure that pressing RET
+          ;; immediately after falling back with C-f will select the
+          ;; input rather than use the default (which is
+          ;; `default-directory').
+          (minibuffer-with-setup-hook
+              (:append
+               (lambda ()
+                 ;; Clear out whatever started in the minibuffer and
+                 ;; replace it with what the user had already entered
+                 ;; into ido.
+                 (delete-minibuffer-contents)
+                 (insert (abbreviate-file-name ido-current-directory))))
+            (call-interactively this-command))))
 
        ((eq ido-exit 'switch-to-buffer)
 	(ido-buffer-internal
@@ -4841,7 +4853,8 @@ buffers that can be considered."
   "Ido replacement for the built-in `read-file-name'.
 Read file name, prompting with PROMPT and completing in directory DIR.
 See `read-file-name' for additional parameters."
-  (let (filename)
+  (let (filename
+        (orig-dir dir))
     (cond
      ((and (not (memq this-command ido-read-file-name-non-ido))
            (or (eq predicate 'file-directory-p)
@@ -4895,7 +4908,21 @@ See `read-file-name' for additional parameters."
     (if (eq filename 'fallback)
 	(let ((read-file-name-function nil))
 	  (run-hook-with-args 'ido-before-fallback-functions 'read-file-name)
-	  (read-file-name prompt dir default-filename mustmatch initial predicate))
+          ;; Bug#19412: need to pass original DIR to `read-file-name'
+          ;; but start with current value of DIR entered in
+          ;; minibuffer, so that it correctly handles a default that
+          ;; is not in the current directory. See also bug#1516.
+          ;; (ido-trace "read-file-name fallback" (list prompt orig-dir default-filename mustmatch initial predicate))
+          ;; (ido-trace "read-file-name fallback initial" dir)
+          (minibuffer-with-setup-hook
+              (:append
+               (lambda ()
+                 ;; Clear out whatever started in the minibuffer and
+                 ;; replace it with what the user had already entered
+                 ;; into ido.
+                 (delete-minibuffer-contents)
+                 (insert (abbreviate-file-name dir))))
+            (read-file-name prompt orig-dir default-filename mustmatch initial predicate)))
       filename)))
 
 ;;;###autoload
@@ -4904,6 +4931,7 @@ See `read-file-name' for additional parameters."
 Read directory name, prompting with PROMPT and completing in directory DIR.
 See `read-directory-name' for additional parameters."
   (let* (filename
+         (orig-dir dir)
 	 (minibuffer-completing-file-name t)
 	 (ido-context-switch-command 'ignore)
 	 ido-saved-vc-hb
@@ -4920,11 +4948,25 @@ See `read-directory-name' for additional parameters."
 			    (expand-file-name initial ido-current-directory)
 			  ido-current-directory))
 		    mustmatch initial))
+    (setq dir ido-current-directory)
     (cond
      ((eq ido-exit 'fallback)
       (let ((read-file-name-function nil))
 	(run-hook-with-args 'ido-before-fallback-functions 'read-directory-name)
-	(read-directory-name prompt ido-current-directory
+        ;; Bug#19412: need to pass original DIR to `read-file-name'
+        ;; but start with current value of DIR entered in minibuffer,
+        ;; so that it correctly handles a default that is not in the
+        ;; current directory.
+        (minibuffer-with-setup-hook
+            (:append
+             (lambda ()
+               ;; Clear out whatever started in the minibuffer and
+               ;; replace it with what the user had already entered
+               ;; into ido.
+               (delete-minibuffer-contents)
+               (insert (abbreviate-file-name dir))))
+          (read-directory-name prompt orig-dir default-filename mustmatch initial predicate))
+        (read-directory-name prompt ido-current-directory
 			     default-dirname mustmatch initial)))
      ((equal filename ".") ido-current-directory)
      (t (concat ido-current-directory filename)))))
-- 
2.25.0

bug#19412: 24.3; ido-write-file sometimes writes to a different directory than, it says it will

[Stefan Kangas]

"Ryan C. Thompson" <rct@thompsonclan.org> writes:

> Ok, I think I have a working patch for this issue. I patched ido-file-internal,
> ido-read-file-name, and ido-read-directory-name, and I think that's all the code
> paths that need to be fixed. Now the test case described in the original report
> produces the correct result for me: writing to /tmp/mumble.frotz.
>
> However, I should note that this is still relatively untested. I will test it
> out and try to make sure it doesn't cause any unexpected issues before I
> recommend merging it.

Any updates here?  Did you give it more testing?

Best regards,
Stefan Kangas

bug#19412: 24.3; ido-write-file sometimes writes to a different directory than, it says it will

[Ryan C. Thompson]

Hi Stefan,

It's been a while, but I've fixed up my patch and given it some testing, 
and it seems to work on for me. However, in the meantime, this issue has 
recently been "fixed" by special-casing write-file in ido.el, as seen in 
#28513. So if you want to install my patch now, you'll need to install 
the version attached to that thread. That version reverts the other fix, 
since of course they are not compatible, and would be redundant even if 
they were.

Regards,

Ryan Thompson

On 8/12/20 12:44 PM, Stefan Kangas wrote:
> "Ryan C. Thompson" <rct@thompsonclan.org> writes:
>
>> Ok, I think I have a working patch for this issue. I patched ido-file-internal,
>> ido-read-file-name, and ido-read-directory-name, and I think that's all the code
>> paths that need to be fixed. Now the test case described in the original report
>> produces the correct result for me: writing to /tmp/mumble.frotz.
>>
>> However, I should note that this is still relatively untested. I will test it
>> out and try to make sure it doesn't cause any unexpected issues before I
>> recommend merging it.
> Any updates here?  Did you give it more testing?
>
> Best regards,
> Stefan Kangas

bug#19412: 24.3; ido-write-file sometimes writes to a different directory than, it says it will

[Lars Ingebrigtsen]

"Ryan C. Thompson" <rct@thompsonclan.org> writes:

> It's been a while, but I've fixed up my patch and given it some
> testing, and it seems to work on for me. However, in the meantime,
> this issue has recently been "fixed" by special-casing write-file in
> ido.el, as seen in #28513. So if you want to install my patch now,
> you'll need to install the version attached to that thread. That
> version reverts the other fix, since of course they are not
> compatible, and would be redundant even if they were.

It looks like a more thorough fix.  However:

+          (minibuffer-with-setup-hook
+              (:append
+               (lambda ()
+                 ;; Clear out whatever started in the minibuffer and
+                 ;; replace it with what the user had already entered
+                 ;; into ido.
+                 (delete-minibuffer-contents)
+                 (insert (abbreviate-file-name ido-current-directory))))
+            (call-interactively this-command))))

I'd be worried that this would step on other modifications the user may
be doing from the minibuffer setup.

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

bug#19412: 24.3; ido-write-file sometimes writes to a different directory than, it says it will

[Ryan C. Thompson]

On 1/11/21 9:14 AM, Lars Ingebrigtsen wrote:
> "Ryan C. Thompson" <rct@thompsonclan.org> writes:
>
>> It's been a while, but I've fixed up my patch and given it some
>> testing, and it seems to work on for me. However, in the meantime,
>> this issue has recently been "fixed" by special-casing write-file in
>> ido.el, as seen in #28513. So if you want to install my patch now,
>> you'll need to install the version attached to that thread. That
>> version reverts the other fix, since of course they are not
>> compatible, and would be redundant even if they were.
> It looks like a more thorough fix.  However:
>
> +          (minibuffer-with-setup-hook
> +              (:append
> +               (lambda ()
> +                 ;; Clear out whatever started in the minibuffer and
> +                 ;; replace it with what the user had already entered
> +                 ;; into ido.
> +                 (delete-minibuffer-contents)
> +                 (insert (abbreviate-file-name ido-current-directory))))
> +            (call-interactively this-command))))
>
> I'd be worried that this would step on other modifications the user may
> be doing from the minibuffer setup.

The only case where this would step on other minibuffer setup code is 
when that code makes modifications to the initial input in the 
minibuffer, since those modifications would be deleted and replaced. 
However, in this case I'd argue that is the correct behavior. The point 
of this code is to fall back from ido completion to standard emacs 
completion while preserving the current input. That means that any setup 
hook that modifies the initial contents of the minibuffer has *already* 
run at the start of ido completion and should not run *again* here. 
Effectively, we want to pretend that we are continuing the same 
completion session with a different completion system, even though we 
are actually starting a new completion session. And to do that, we need 
to preserve the user's current input verbatim when falling back.

bug#19412: 24.3; ido-write-file sometimes writes to a different directory than, it says it will

[Lars Ingebrigtsen]

"Ryan C. Thompson" <rct@thompsonclan.org> writes:

> And to do that, we need to preserve the user's current input
> verbatim when falling back.

Sounds reasonable, but who knows what people are doing in the setup
hooks there?  So I worry about making a change like this, perhaps overly
much.

I guess the only way to find out whether it breaks a lot of stuff or
not is to try it out and get feedback.

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

bug#19412: 24.3; ido-write-file sometimes writes to a different directory than, it says it will

[Ryan C. Thompson]

On 1/11/21 1:43 PM, Lars Ingebrigtsen wrote:
> "Ryan C. Thompson" <rct@thompsonclan.org> writes:
>
>> And to do that, we need to preserve the user's current input
>> verbatim when falling back.
> Sounds reasonable, but who knows what people are doing in the setup
> hooks there?  So I worry about making a change like this, perhaps overly
> much.
>
> I guess the only way to find out whether it breaks a lot of stuff or
> not is to try it out and get feedback.
>
Yes, it's definitely an invasive change, and more properly characterized 
as a work-around than a true fix, so the worry isn't unwarranted. I'm 
pretty sure the code is correct (or as close to correct as possible 
given the "API" limitations we're working with), but it's always 
possible there's some edge case I haven't anticipated.