bbatsov / helm-projectile

Helm UI for Projectile
327 stars 71 forks source link

helm-projectile-ag royally broken, shows some top level dir on selecting an entry #124

Closed maindoor closed 5 years ago

maindoor commented 5 years ago

Expected behavior

Once the helm-projectile-ag results are shown, hitting tab on it or choosing a persistent action should open the entry in a new buffer.

Actual behavior

It just opens some top level directory buffer.

Steps to reproduce the problem

Only happens on large projects, I tried this on a small project with the exact same steps, works without any issues. I can invite the author to a Skype meeting to show the problem and do a live debug session if necessary.

Backtraces if necessary (M-x toggle-debug-on-error)

I enabled this toggle debug on error, but emacs does not see it as an error.

Environment & version information

helm-projectile version: 20190408.1508 helm version (in helm-pkg.el): 20190527.1253 projectile version (M-x projectile-version): 20190529.515 Emacs version (M-x emacs-version): 26.2 OS: RHEL7.5

xiongtx commented 5 years ago

Hitting TAB should open the actions menu, not a new buffer.

Can you add a GIF or something demonstrating the problem? And how big is a "large" project? Is there a repo I can clone? Would Linux source do?

maindoor commented 5 years ago

Here is a demo of the helm projectile ag issue

I don't upload videos to youtube often so please let me know if you have any clarifications. So I have narrowed down the problem in my case to the .projectile file being present, if it is not present helm-projectile-switch-project indexes all the files based on .git as root and works well. But if I even create a simple .projectile file then helm-projectile-ag breaks.

This is my projectile config:

   (use-package projectile
     :ensure    t
     :config
     (projectile-mode t)
     :init
     (progn
       ;; create projectile data folder if non-existent
       (sk/mkdir-p sk-projectile-folder)

       ;; set projectile custom variables
       (let* ((sk-projectile-dir (file-name-as-directory sk-projectile-folder))
              (sk-projectile-cache-file (concat sk-projectile-dir sk-projectile-cache-filename))
              (sk-projectile-bookmarks-file (concat sk-projectile-dir sk-projectile-bookmarks-filename)))
         (setq projectile-cache-file          sk-projectile-cache-file
               projectile-known-projects-file sk-projectile-bookmarks-file
               projectile-indexing-method     'native
               projectile-completion-system   'helm
               projectile-enable-caching      t)
         (helm-projectile-on))
       )
     :diminish
     projectile-mode)
xiongtx commented 5 years ago

Does this only happen when you have a + pattern in your .projectile?

IIUC this functionality actually doesn't work properly w/ helm-projectile-ag given the current implementation, as it would search only in those directories.

But I'm not sure that's the problem you're running into.

xiongtx commented 5 years ago

See if #129 solves your problem.

maindoor commented 5 years ago

Hi @xiongtx,

129 does not solve the problem.

If I include #129 it does ag in all the directories at project root, it completely ignores .projectile file, but only during ag. Although helm-projectile-find-file respects .projectile. In the example above my .projectile contains only +arm64

Here is my complete function with the modification you suggested:

(defun helm-projectile-ag (&optional options)
  "Helm version of `projectile-ag'."
  (interactive (if current-prefix-arg (list (helm-read-string "option: " "" 'helm-ag--extra-options-history))))
  (if (require 'helm-ag nil t)
      (if (projectile-project-p)
          (let* ((grep-find-ignored-files (cl-union (projectile-ignored-files-rel) grep-find-ignored-files))
                 (grep-find-ignored-directories (cl-union (projectile-ignored-directories-rel) grep-find-ignored-directories))
                 (ignored (mapconcat (lambda (i)
                                       (concat "--ignore " i))
                                     (append grep-find-ignored-files grep-find-ignored-directories (cadr (projectile-parse-dirconfig-file)))
                                     " "))
                 (helm-ag-base-command (concat helm-ag-base-command " " ignored " " options))
                 (current-prefix-arg nil)
                 (keep-paths (projectile-normalise-paths (car (projectile-parse-dirconfig-file)))))
            (helm-do-ag (projectile-project-root) (append (list (projectile-project-root)) keep-paths (projectile-paths-to-ensure))))
        (error "You're not in a project"))
    (when (yes-or-no-p "`helm-ag' is not installed. Install? ")
      (condition-case nil
          (progn
            (package-install 'helm-ag)
            (helm-projectile-ag options))
        (error (error "`helm-ag' is not available.  Is MELPA in your `package-archives'?"))))))
xiongtx commented 5 years ago

If your .projectile includes only +arm64, what do you mean by "it completely ignores .projectile file"? The +arm64 forces inclusion of that directory in ag. In fact, I'm not sure why you have it at all; its typical use case is to include a subdirectory of a directory you're excluding, e.g.

-/foo
+/foo/bar
xiongtx commented 5 years ago

I can't reproduce this. It's good that the investigation uncovered the problem #129 is intended to solve; unfortunately it's not your issue.

This behavior also doesn't make any sense; what is TAB mapped to in your setup? By default, it's supposed to bring up the actions menu, yet you're expecting it to open the result in a new buffer?

maindoor commented 5 years ago

Hi @xiongtx , When I said "it completely ignores .projectile file" it means the helm-projectile-ag does an ag on all the files in the project root. In the above video "arch" is my project root. Inside "arch" there are many directories like x86, powerpc, arm, arm64. But I did not want an ag on all the files, I only wanted ag to run in "arm64".

I think my .projectile file is valid because I want to index only the arm64 directory, and forget about all other directories. helm-projectile-find-file does the right thing by just showing the files under arm64.

It is common to have such a config in helm: ([tab] . helm-execute-persistent-action))

xiongtx commented 5 years ago

If you don't want to run ag on all the files / dirs, you need to exclude them in .projectile or the top-level .gitignore (the one in the same directory as .git).

Have you tried running a vanilla Emacs w/out all those custom configs?

maindoor commented 5 years ago

Here is where I picked up on how to set .projectile file: projectile ignoring files In particular: "If both directories to keep and ignore are specified, the directories to keep first apply, restricting what files are considered. The paths and patterns to ignore are then applied to that set. "

If I read helm-projectile-ag correctly, it is only considering ignored-files and ignored-directories. Maybe that assumption is not accurate ?

Yes, I am able to recreate this on vanilla emacs with just projectile and helm loaded. Shall I made a video of that ?

xiongtx commented 5 years ago

helm-projectile-ag, after #129, should consider not only ignored, but also files / directories to keep (i.e. subdirectories of ignored that you want to include) and ensure (ignored in .gitignore but you want to include).

Yes, I am able to recreate this on vanilla emacs with just projectile and helm loaded. Shall I made a video of that ?

Sure. Please upload in 1080p if you can, as it's hard to make out text in 720p. Also make sure to update your packages to latest.

maindoor commented 5 years ago

@xiongtx here is the video: helm projectile issue on vanilla emacs I recorded it in 4k but while uploading youtube reprocess it to only 720p but this time the video should be clear Pls let me know if you have any questions about the video.

xiongtx commented 5 years ago

OK, so this is about how .projectile works; it has nothing to do w/ the original issue of not being able to open the file, correct? Is that issue still occuring?

As for the .projectile interpretation issue, have a look at #130 and see if that solves your issue.

maindoor commented 5 years ago

@xiongtx, The original issue did not occur after #129. Instead of not being able to open, it started searching all the dirs. But with #130 it behaves as expected. Looks like #130 has fixed the problem. I will do more tests tomorrow and close this case. Thank you following up on this.

xiongtx commented 5 years ago

130 should have absolutely nothing to do with the original problem of not opening files.

That's likely due to some bad config that's no longer present for you.

I'll merge #130. You can close this 🎫 when you're done testing.