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

Counsel compile further fixes #2077

Closed
wants to merge 3 commits into from

Conversation

@stsquad
Copy link
Contributor

@stsquad stsquad commented May 23, 2019

A small number of tweaks and fixes for better handling builds in subdirectories and recent history.

stsquad added 3 commits May 20, 2019
…subdirs

It is possible the user might actually just have a single build
directory rather than a hierarchy. We can handle this case by simply
adding the probed directory to the list if we detect any non-directory
files in the directory. While we are at it we shouldn't be returning
any non-directories so filter those out.
This ensures the most recent history items appears first on the
suggestions list.
We can't recover the current build directory when the user invokes M-i
so let's track it in a variable. The user still has to delete the
extra verbiage but at least won't be surprised about where the command
is run.
@abo-abo abo-abo closed this in b65cdb5 May 23, 2019
@abo-abo
Copy link
Owner

@abo-abo abo-abo commented May 23, 2019

Thanks.

@basil-conto
Copy link
Collaborator

@basil-conto basil-conto commented May 27, 2019

@stsquad Can you please check the simplification I pushed in bc2dcbe?

How important is it to stat each file only once? If the answer is "not very", I think we can further simplify as follows:

diff --git a/counsel.el b/counsel.el
index 86e5264..339eb51 100644
--- a/counsel.el
+++ b/counsel.el
@@ -5425,15 +5425,14 @@ counsel--get-build-subdirs
   "Return all subdirs under BLDDIR sorted by modification time.
 If there are non-directory files in BLDDIR, include BLDDIR in the
 list as it may also be a build directory."
-  (let* ((files (directory-files-and-attributes
+  (let* ((files (directory-files
                  blddir t directory-files-no-dot-files-regexp t))
-         (dirs (cl-remove-if-not #'cl-second files)))
+         (dirs (cl-remove-if-not #'file-directory-p files)))
     ;; Any non-dir files?
     (when (< (length dirs)
              (length files))
-      (push (cons blddir (file-attributes blddir)) dirs))
-    (mapcar #'car (sort dirs (lambda (x y)
-                               (time-less-p (nth 6 y) (nth 6 x)))))))
+      (push blddir dirs))
+    (sort dirs #'file-newer-than-file-p)))
 
 (defun counsel-compile-get-build-directories (&optional dir)
   "Return a list of potential build directories."

@stsquad
Copy link
Contributor Author

@stsquad stsquad commented May 28, 2019

@basil-conto I'm probably not the best person to ask. stat on Linux is super cheap (and cached) but it might matter more for Windows users. Anyway we'll see if anyone complains about performance. Thanks for the clean-up.

@basil-conto
Copy link
Collaborator

@basil-conto basil-conto commented May 28, 2019

@stsquad I was more worried about, say, remote filesystems, but either way it's probably best to keep the current implementation (the minor cleanup I pushed yesterday shouldn't affect performance). Thanks.

astoff added a commit to astoff/swiper that referenced this issue Jan 1, 2021
We can't recover the current build directory when the user invokes M-i
so let's track it in a variable. The user still has to delete the
extra verbiage but at least won't be surprised about where the command
is run.

Fixes abo-abo#2077
astoff added a commit to astoff/swiper that referenced this issue Jan 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants