Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

"ivy-alt-done" can complete to non-existent directory #1494

Closed
muirrn opened this issue Mar 20, 2018 · 12 comments

Comments

@muirrn
Copy link

commented Mar 20, 2018

Starting with file tree:

[muir@work-mb root]$ find .
.
./foo
./foo/foo_file.rb
./bar
./bar/start
./bar/foo
  1. Open bar/start
  2. Run counsel-find-file
  3. Press backspace to go up to /
  4. Type "foo" then <C-j> to complete into foo/
  5. Type "foo" then <C-j> to attempt to open foo_file.rb

I expect it to open foo_file.rb since it is not a directory, but it navigates into non-existent foo/foo directory.

I reproduced using the plain make target.

@muirrn muirrn changed the title `ivy-alt-done` can complete to non-existent directory "ivy-alt-done" can complete to non-existent directory Mar 20, 2018

@basil-conto

This comment has been minimized.

Copy link
Collaborator

commented Mar 20, 2018

What is your emacs-version and Swiper project version? I cannot reproduce this on GNU Emacs 27.0.50 (build 16, x86_64-pc-linux-gnu, X toolkit, Xaw3d scroll bars) of 2018-03-19 starting from make plain with latest Swiper master of 2018-03-16, commit b53ba0b. The given steps correctly open the foo/foo_file.rb file for me.

@muirrn

This comment has been minimized.

Copy link
Author

commented Mar 20, 2018

I am running GNU Emacs 26.0.91

[muir@work-mb swiper]$ git rev-parse HEAD
b53ba0be297a6bf22e0fab831eb1297c986bf774

[muir@work-mb swiper]$ emacs --version
GNU Emacs 26.0.91
Copyright (C) 2018 Free Software Foundation, Inc.
GNU Emacs comes with ABSOLUTELY NO WARRANTY.
You may redistribute copies of GNU Emacs
under the terms of the GNU General Public License.
For more information about these matters, see the file named COPYING.

[muir@work-mb swiper]$ emacs -Q -l colir.el -l ivy-overlay.el -l ivy.el -l swiper.el -l counsel.el -l targets/plain.el
@basil-conto

This comment has been minimized.

Copy link
Collaborator

commented Mar 20, 2018

I additionally cannot reproduce this on any one of Emacs 24, 25 or 26.

Can you please list your full emacs-version, i.e. M-xemacs-versionRET, rather than just emacs --version?

From your hostname I assume your system-type is darwin, which potentially implicates bug#30350.

@basil-conto

This comment has been minimized.

Copy link
Collaborator

commented Mar 20, 2018

I'm not sure to what extent this commit solves the aforementioned Emacs bug, and I'm afraid I have no way to test Swiper on macOS more generally.

@muirrn

This comment has been minimized.

Copy link
Author

commented Mar 20, 2018

emacs-version is GNU Emacs 26.0.91 (build 1, x86_64-apple-darwin13.4.0, NS appkit-1265.21 Version 10.9.5 (Build 13F1911)) of 2018-01-24

I am not seeing the buggy behavior you linked to. Let me see if I can reproduce on non-darwin.

@muirrn

This comment has been minimized.

Copy link
Author

commented Mar 20, 2018

I reproduced it on linux GNU Emacs 24.5.1 (x86_64-pc-linux-gnu, GTK+ Version 3.18.9) of 2017-09-20 on lcy01-07, modified by Debian under make plain.

Are you typing exactly "foo" before the final <C-j>? If you do "fo" or "foo_" for example, it won't reproduce.

@basil-conto

This comment has been minimized.

Copy link
Collaborator

commented Mar 20, 2018

Are you typing exactly "foo" before the final <C-j>?

Yes, I am following your recipe verbatim:

  1. emacs=emacs24 make plain
  2. M-xemacs-versionRET logs the following in *Messages*:
    GNU Emacs 24.5.1 (x86_64-pc-linux-gnu, X toolkit, Xaw3d scroll bars) of 2017-09-12 on hullmann, modified by Debian
  3. (let (;; New temporary directory
          (default-directory (file-name-as-directory (make-temp-file "" t)))
          (files '("bar/start" "bar/foo" "foo/foo_file.rb")))
      ;; Create sample directory tree
      (dolist (file files)
        (make-directory (file-name-directory file) t)
        (with-temp-file file))
      ;; Log directory structure
      (message "%s" (with-output-to-string
                      (call-process "tree" nil standard-output)))
      ;; Open `bar/start' via `counsel-find-file'
      (setq unread-command-events
            (nconc (listify-key-sequence (car files)) '(?\C-m)))
      (call-interactively #'counsel-find-file))
    C-j logs the following in *Messages*:
    .
    ├── bar
    │   ├── foo
    │   └── start
    └── foo
        └── foo_file.rb
    
    2 directories, 3 files
    
  4. C-xC-fDELfooC-jfooC-j
  5. M-:buffer-file-nameRET gives:
    "/tmp/15823JdI/foo/foo_file.rb"
@muirrn

This comment has been minimized.

Copy link
Author

commented Mar 20, 2018

Sorry, my steps are wrong/unclear due to the find output. bar/foo should be a directory. I changed your final file from "bar/foo" to "bar/foo/baz" to make "bar/foo" a directory and it reproduced the issue.

(files '("bar/start" "foo/foo_file.rb" "bar/foo/baz"))

@basil-conto

This comment has been minimized.

Copy link
Collaborator

commented Mar 20, 2018

Thanks, I can reproduce it in all Emacs versions between 24-27 now.

@basil-conto

This comment has been minimized.

Copy link
Collaborator

commented Mar 20, 2018

Does the following patch look right?

diff --git a/ivy.el b/ivy.el
index c48dc55..92b46c0 100644
--- a/ivy.el
+++ b/ivy.el
@@ -821,12 +821,13 @@ ivy--directory-done
         (setq dir (ivy-expand-file-if-directory (ivy-state-current ivy-last))))
        (ivy--cd dir)
        (ivy--exhibit))
-      ((and (not (string= ivy-text ""))
-            (ignore-errors (file-exists-p ivy-text)))
-       (if (file-directory-p ivy-text)
-           (ivy--cd (expand-file-name
-                     (file-name-as-directory ivy-text) ivy--directory))
-         (ivy-done)))
+      ((unless (string= ivy-text "")
+         (let ((file (expand-file-name ivy-text ivy--directory)))
+           (when (ignore-errors (file-exists-p file))
+             (if (file-directory-p file)
+                 (ivy--cd (file-name-as-directory file))
+               (ivy-done))
+             ivy-text))))
       ((or (and (equal ivy--directory "/")
                 (string-match "\\`[^/]+:.*:.*\\'" ivy-text))
            (string-match "\\`/[^/]+:.*:.*\\'" ivy-text))
@muirrn

This comment has been minimized.

Copy link
Author

commented Mar 20, 2018

I can confirm that patch fixes it for me.

@abo-abo

This comment has been minimized.

Copy link
Owner

commented Mar 20, 2018

@basil-conto Thanks, please PR.

basil-conto added a commit to basil-conto/swiper that referenced this issue Mar 20, 2018
ivy.el (ivy--directory-done): Fix file expansion
Expand ivy-text file path component relative to ivy--directory
before passing the result to file/directory predicates.

Fixes abo-abo#1494.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.