bug#28513: 25.1; ido insists on guessing the wrong directory

bug#28513: 25.1; ido insists on guessing the wrong directory

[Guillaume Salagnac]

I want to reproduce the usual "save as..." feature that all GUI applications offer. But I fail to do so using ido-mode and I suspect a bug. Steps to reproduce are described below.

emacs -Q
M-x ido-mode
... I type stuff in the scratch buffer ...
C-x C-w
    (this brings the ido-write-file dialog in the minibuffer. the blue
     prompt is ~/ and all the subdirs are listed in red. fine so far)
... I type a new filename and then hit RETURN
    (this brings a [Confirm] notification in the minibuffer, so I have to hit
     RETURN once more to get the "Wrote /home/user/filename" message)
... I type some more stuff in the (now named) buffer ...
C-x C-w
(this brings the ido-write-file dialog once more, with the same
subdirectories in red)
... I type "k" then RETURN
(the blue prompt now says "Write file: ~/Desktop/" )
C-f
(the completions in red disappear ; the "~/Desktop" part of the prompt
becomes black and I can edit the path to my liking. but I don't)
RETURN

expected behaviour: emacs may or may not ask for a confirmation here, but eventually it should save my buffer as ~/Desktop/filename (as is the case with M-x write-file)

actual behaviour: emacs decides that I actually didn't want to change directories and says: File '~/filename' exists; overwrite ? I can choose either "cancel" or "confirm" but neither lead to the correct result.


Is this indeed a bug, or am I missing something obvious ?


In GNU Emacs 25.1.1 (x86_64-apple-darwin15.6.0, NS appkit-1404.47 Version 10.11.6 (Build 15G1217))
of 2017-02-18 built on elcapitan.internal.macports.net
Windowing system distributor 'Apple', version 10.3.1404
Configured using:
'configure --prefix=/opt/local --without-ns --without-dbus
--without-gconf --without-libotf --without-m17n-flt --without-gpm
--without-gnutls --with-xml2 --with-modules --infodir
/opt/local/share/info/emacs --with-ns CC=/usr/bin/clang 'CFLAGS=-pipe
-Os -arch x86_64' 'LDFLAGS=-L/opt/local/lib
-Wl,-headerpad_max_install_names -Wl,-no_pie -arch x86_64'
CPPFLAGS=-I/opt/local/include'

Configured features:
NOTIFY ACL LIBXML2 ZLIB TOOLKIT_SCROLL_BARS NS MODULES

Important settings:
  value of $LC_CTYPE: en_US.UTF-8
  value of $LANG: C
  locale-coding-system: utf-8-unix

Major mode: Fundamental

Minor modes in effect:
  tooltip-mode: t
  global-eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-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 messages:
Saving file /Users/gsalagnac/bla...
Wrote /Users/gsalagnac/bla
Mark set
next-line: End of buffer [3 times]
Mark set
next-line: End of buffer
Saving file /Users/gsalagnac/new-file...
Wrote /Users/gsalagnac/new-file
File ‘~/new-file’ exists; overwrite? (y or n) n
user-error: Canceled

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message dired format-spec rfc822 mml
mml-sec password-cache epg epg-config gnus-util mm-decode mm-bodies
mm-encode mail-parse rfc2231 mailabbrev gmm-utils mailheader sendmail
rfc2047 rfc2045 ietf-drums mm-util help-fns mail-prsvr mail-utils ido
seq byte-opt gv bytecomp byte-compile cl-extra help-mode easymenu cconv
cl-loaddefs pcase cl-lib time-date mule-util tooltip eldoc electric
uniquify ediff-hook vc-hooks lisp-float-type mwheel ns-win ucs-normalize
term/common-win tool-bar dnd fontset image regexp-opt fringe
tabulated-list newcomment elisp-mode lisp-mode prog-mode register page
menu-bar rfn-eshadow timer select scroll-bar mouse jit-lock font-lock
syntax facemenu font-core frame cl-generic cham georgian utf-8-lang
misc-lang vietnamese tibetan thai tai-viet lao korean japanese eucjp-ms
cp51932 hebrew greek romanian slovak czech european ethiopic indian
cyrillic chinese charscript case-table epa-hook jka-cmpr-hook help
simple abbrev minibuffer cl-preloaded nadvice loaddefs button faces
cus-face macroexp files text-properties overlay sha1 md5 base64 format
env code-pages mule custom widget hashtable-print-readable backquote
kqueue cocoa ns multi-tty make-network-process emacs)

Memory information:
((conses 16 207881 7570)
(symbols 48 20460 0)
(miscs 40 66 194)
(strings 32 19472 6289)
(string-bytes 1 589390)
(vectors 16 34429)
(vector-slots 8 666731 6726)
(floats 8 186 153)
(intervals 56 284 16)
(buffers 976 20))

bug#28513: 25.1; ido insists on guessing the wrong directory

[Lars Ingebrigtsen]

Guillaume Salagnac <guillaume.salagnac@gmail.com> writes:

> emacs -Q
> M-x ido-mode

[...]

> expected behaviour: emacs may or may not ask for a confirmation here, but eventually it should save my buffer as ~/Desktop/filename (as is the case with M-x write-file)
>
> actual behaviour: emacs decides that I actually didn't want to change directories and says: File '~/filename' exists; overwrite ? I can choose either "cancel" or "confirm" but neither lead to the correct result.
>
> Is this indeed a bug, or am I missing something obvious ?

(This bug report unfortunately didn't get any response at the time.)

I can confirm that this odd behaviour is still present in Emacs 28.  It
seems like a bug to me -- the prompt says "Write file: ~/Documents/",
but hitting RET insists on writing to `default-directory' in the buffer.

I don't use ido-mode for files normally -- are there any ido users here
that do?  If so, does this seem like expected behaviour to you?

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

bug#28513: 25.1; ido insists on guessing the wrong directory

[Dmitry Gutov]

On 12.12.2020 14:10, Lars Ingebrigtsen wrote:
> I don't use ido-mode for files normally -- are there any ido users here
> that do?  If so, does this seem like expected behaviour to you?

Looks like a bug, yes.

The scenario looks pretty specific, though, that's probably why it 
hasn't come up before.

bug#28513: 25.1; ido insists on guessing the wrong directory

[Lars Ingebrigtsen]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 12.12.2020 14:10, Lars Ingebrigtsen wrote:
>> I don't use ido-mode for files normally -- are there any ido users here
>> that do?  If so, does this seem like expected behaviour to you?
>
> Looks like a bug, yes.
>
> The scenario looks pretty specific, though, that's probably why it
> hasn't come up before.

I have a slightly simpler reproduction case:

emacs -Q -f ido-mode lisp/abbrev.el
C-x C-w RET C-f RET

This should write the file to calc/abbrev.el, but prompts for
overwriting.

Here's the backtrace with debug-on-quit:

Debugger entered--Lisp error: (quit)
  read-from-minibuffer("File ‘~/src/emacs/trunk/lisp/abbrev.el’ exists; ov.
  y-or-n-p("File ‘~/src/emacs/trunk/lisp/abbrev.el’ exists; ov...")
  write-file("~/src/emacs/trunk/lisp/abbrev.el" t)
  funcall-interactively(write-file "~/src/emacs/trunk/lisp/abbrev.el" t)
  call-interactively(write-file)
  ido-file-internal(write write-file nil "Write file: " nil nil ignore)
  ido-write-file()

I was momentarily puzzled about why that call-interactively to
write-file didn't re-prompt about the location, but:

       ((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))
	  (setq this-command (or ido-fallback fallback 'find-file))
	  (run-hook-with-args 'ido-before-fallback-functions this-command)
	  (call-interactively this-command)))

So hitting `C-f' makes ido go into `fallback' mode?  Yes!

    (define-key map "\C-f" 'ido-magic-forward-char)

(defun ido-magic-forward-char (arg)
  "Move forward in user input or perform magic action.
If no user input is present, or at end of input, perform magic actions:
C-x C-b ... C-f  switch to `ido-find-file'.
C-x C-f ... C-f  fallback to non-Ido `find-file'.
C-x C-d ... C-f  fallback to non-Ido brief `dired'.
C-x d ... C-f    fallback to non-Ido `dired'."
  (interactive "P")
  (cond
   ((or arg (not (eobp)))
    (forward-char (min (prefix-numeric-value arg)
		       (- (point-max) (point)))))
   ((memq ido-cur-item '(file dir))
    (ido-fallback-command))

So...  this is apparently a feature?  Hitting `C-f' disables ido and
calls the fallback command, which is `write-region' in this case.

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

bug#28513: 25.1; ido insists on guessing the wrong directory

[Dmitry Gutov]

On 13.12.2020 15:12, Lars Ingebrigtsen wrote:
> So...  this is apparently a feature?  Hitting `C-f' disables ido and
> calls the fallback command, which is `write-region' in this case.
                                         ^ write-file, right?

Okay, but why does the fallback command end up trying to overwrite the 
original file, even though, in your scenario, input ends with /vc/?

bug#28513: 25.1; ido insists on guessing the wrong directory

[Lars Ingebrigtsen]

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

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 13.12.2020 15:12, Lars Ingebrigtsen wrote:
>> So...  this is apparently a feature?  Hitting `C-f' disables ido and
>> calls the fallback command, which is `write-region' in this case.
>                                         ^ write-file, right?

Yup.

> Okay, but why does the fallback command end up trying to overwrite the
> original file, even though, in your scenario, input ends with /vc/?

In essence, it's doing this (if we say we've navigated to "/tmp/" before
`C-f'):

(let ((default-directory "/tmp/"))
  (call-interactively 'write-file))

This gives you a prompt of

Write file: /tmp/

If you then hit RET, then:


[-- Attachment #2: Type: image/png, Size: 12708 bytes --]

[-- Attachment #3: Type: text/plain, Size: 816 bytes --]


That is, hitting RET in the `write-file' dialogue gives you
buffer-file-name, and ignores whatever is in the prompt.  This seems
contrary to what the doc string says:

---
Interactively, prompt for FILENAME.
If you specify just a directory name as FILENAME, that means to write
to a file in that directory.  In this case, the base name of the file
is the same as that of the file visited in the buffer, or the buffer
name sans leading directories, if any, if the buffer is not already
visiting a file.
---

So this isn't an ido problem at all -- it's a bug in `write-file'?  Or
rather...

(let ((default-directory "/tmp/")) (read-file-name "Foo: "))

If you just hit RET there, it'll return `buffer-file-name'.

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

bug#28513: 25.1; ido insists on guessing the wrong directory

[Dmitry Gutov]

On 14.12.2020 18:42, Lars Ingebrigtsen wrote:

>> Okay, but why does the fallback command end up trying to overwrite the
>> original file, even though, in your scenario, input ends with /vc/?
> 
> In essence, it's doing this (if we say we've navigated to "/tmp/" before
> `C-f'):
> 
> (let ((default-directory "/tmp/"))
>    (call-interactively 'write-file))
> 
> This gives you a prompt of
> 
> Write file: /tmp/
> 
> If you then hit RET, then:
> 
> 
> 
> That is, hitting RET in the `write-file' dialogue gives you
> buffer-file-name, and ignores whatever is in the prompt.  This seems
> contrary to what the doc string says:
> 
> ---
> Interactively, prompt for FILENAME.
> If you specify just a directory name as FILENAME, that means to write
> to a file in that directory.  In this case, the base name of the file
> is the same as that of the file visited in the buffer, or the buffer
> name sans leading directories, if any, if the buffer is not already
> visiting a file.
> ---
> 
> So this isn't an ido problem at all -- it's a bug in `write-file'?  Or
> rather...
> 
> (let ((default-directory "/tmp/")) (read-file-name "Foo: "))
> 
> If you just hit RET there, it'll return `buffer-file-name'.

But there is a difference between having default-directory set to /tmp/ 
and typing /tmp/ yourself.

I think when the user calls the escape hatch command, they don't expect 
their current input to translate to the new default-directory value. 
Rather, it should be the input in the new prompt.

Might not be easy to fix, however, given that the current code in Ido 
tries to do that in the most generic way:

         (let ((default-directory ido-current-directory)
	      (read-file-name-function nil))
	  (setq this-command (or ido-fallback fallback 'find-file))
	  ...
	  (call-interactively this-command))

And since that this feature is an escape hatch and, say, fido-mode 
(which everyone will migrate to any year now) shouldn't need anything 
like it, maybe it's not worth the effort fixing.

bug#28513: 25.1; ido insists on guessing the wrong directory

[Lars Ingebrigtsen]

Dmitry Gutov <dgutov@yandex.ru> writes:

>> So this isn't an ido problem at all -- it's a bug in `write-file'?
>> Or
>> rather...
>> (let ((default-directory "/tmp/")) (read-file-name "Foo: "))
>> If you just hit RET there, it'll return `buffer-file-name'.
>
> But there is a difference between having default-directory set to
> /tmp/ and typing /tmp/ yourself.

There is.  However, I don't think the way

(let ((default-directory "/tmp/")) (read-file-name "Foo: "))

works is logical.  We're clearly presenting the user with an interface
that seems like we're doing something in /tmp/, but RET returns
buffer-file-name.

The following would be more logical, in my opinion.  But it's a very
low-level change, so it's...  ticklish.  It should give the same results
99% of the time (because binding default-directory and then calling
read-file-name isn't the usual pattern, I think?), but would fix this
issue.

Opinions?

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index 456193d52e..d1f1bf3758 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -2814,7 +2814,9 @@ read-file-name-default
   (unless default-filename
     (setq default-filename
           (cond
-           ((null initial) buffer-file-name)
+           ((null initial)
+            (expand-file-name (file-name-nondirectory buffer-file-name)
+                              default-directory))
            ;; Special-case "" because (expand-file-name "" "/tmp/") returns
            ;; "/tmp" rather than "/tmp/" (bug#39057).
            ((equal "" initial) dir)

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

bug#28513: 25.1; ido insists on guessing the wrong directory

[Lars Ingebrigtsen]

I've read the doc string again -- I didn't think the current behaviour
was documented, but it is:

---
If DEFAULT-FILENAME is omitted or
nil, then if INITIAL is non-nil, the default is DIR combined with
INITIAL; otherwise, if the current buffer is visiting a file,
that file serves as the default; otherwise, the default is simply
the string inserted into the minibuffer.
---

So it's documented that buffer-file-name is the default value, no matter
what default-directory is.

I guess the fix here has to be in ido itself, and it'll have to
special-case write-file.

[time passes]

OK, now done in Emacs 28, and the test case now works better, and there
probably shouldn't be a lot of regressions here...

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

bug#28513: 25.1; ido insists on guessing the wrong directory

[Dmitry Gutov]

On 17.12.2020 13:33, Lars Ingebrigtsen wrote:
> I've read the doc string again -- I didn't think the current behaviour
> was documented, but it is:
> 
> ---
> If DEFAULT-FILENAME is omitted or
> nil, then if INITIAL is non-nil, the default is DIR combined with
> INITIAL; otherwise, if the current buffer is visiting a file,
> that file serves as the default; otherwise, the default is simply
> the string inserted into the minibuffer.
> ---

That's the doc for read-file-name, so another option would be to pass an 
explicit third argument from write-file to read-file-name.

> So it's documented that buffer-file-name is the default value, no matter
> what default-directory is.
> 
> I guess the fix here has to be in ido itself, and it'll have to
> special-case write-file.
> 
> [time passes]
> 
> OK, now done in Emacs 28, and the test case now works better, and there
> probably shouldn't be a lot of regressions here...

But this works too. Thanks!

bug#28513:

[Ryan C. Thompson]

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

Hello,

I believe this bug (#28513) is a duplicate of #19412. In the discussion 
for #19412 I have investigated the problem and proposed a fix that 
doesn't require special-casing anything. However, due to being quite 
busy since then I have not had the time to properly test my fix. 
Briefly, the fix is to pass all the original args to the fallback 
function unchanged and then use "minibuffer-with-setup-hook" to simulate 
deleting the initial input and then typing whatever the user currently 
had typed into ido before they triggered the fallback.

I have rebased the patch from that thread onto the current master and 
attached it. To reiterate, this patch should be tested before merging.

Regards,

Ryan


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

From 41feb86c67390ce6c17fd59ddfc6e7288aa47d64 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.

Note: This also reverts commit 526abadd07, which is intended to be a fix
for bug #28513 which I believe is a duplicate of bug #19412. I believe
the fix in this patch is better because it should fix every case
without the need to special-case specific functions.
---
 lisp/ido.el | 63 +++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 47 insertions(+), 16 deletions(-)

diff --git a/lisp/ido.el b/lisp/ido.el
index 89b6a62f5a..2d0a9e69e2 100644
--- a/lisp/ido.el
+++ b/lisp/ido.el
@@ -2363,20 +2363,23 @@ ido-file-internal
        ((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)
-          (if (eq this-command 'write-file)
-              (write-file (read-file-name
-                           "Write file: "
-                           default-directory
-                           (and buffer-file-name
-                                (expand-file-name
-                                 (file-name-nondirectory buffer-file-name)
-                                 default-directory)))
-                          t)
-	    (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
@@ -4871,7 +4874,8 @@ ido-read-file-name
   "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)
@@ -4925,7 +4929,21 @@ ido-read-file-name
     (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
@@ -4934,6 +4952,7 @@ ido-read-directory-name
 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
@@ -4950,12 +4969,24 @@ ido-read-directory-name
 			    (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
-			     default-dirname mustmatch initial)))
+        ;; 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-dirname mustmatch initial))))
      ((equal filename ".") ido-current-directory)
      (t (concat ido-current-directory filename)))))
 
-- 
2.29.2

bug#19412: bug#28513

[Ryan C. Thompson]

Hello again,

I finally got around to testing this patch. I've been using it for over 
a week now and I haven't observed any unexpected behaviors or issues as 
a result. And of course, it does indeed resolve the bug as reported 
here. I believe my patch is ready to merge. Again, for more details on 
how my patch works, see my investigation in #19412.

Regards,

Ryan Thompson

On 1/1/21 4:51 PM, Ryan C. Thompson wrote:
> Hello,
>
> I believe this bug (#28513) is a duplicate of #19412. In the 
> discussion for #19412 I have investigated the problem and proposed a 
> fix that doesn't require special-casing anything. However, due to 
> being quite busy since then I have not had the time to properly test 
> my fix. Briefly, the fix is to pass all the original args to the 
> fallback function unchanged and then use "minibuffer-with-setup-hook" 
> to simulate deleting the initial input and then typing whatever the 
> user currently had typed into ido before they triggered the fallback.
>
> I have rebased the patch from that thread onto the current master and 
> attached it. To reiterate, this patch should be tested before merging.
>
> Regards,
>
> Ryan
>