bling / fzf.el

A front-end for fzf
GNU General Public License v3.0
364 stars 49 forks source link

fzf-git-files works same as normal fzf function when starting in eshell buffer #35

Closed tttuuu888 closed 3 years ago

tttuuu888 commented 6 years ago

If I start fzf-git-files in eshell buffer, it works same as normal fzf. This is happening only in eshell buffer as far as I know. I tested this on Emacs version 25.3.1.

Thanks.

tttuuu888 commented 6 years ago

I reported this on GNU Bug tracker related to this issue and unfortunately this looks like unavoidable. ( bug#30134 )

The problem is that 'fzf-git-files' is using process-environment and I cannot make term with modified process-environment variable if i start 'fzf-git-files' in eshell buffer.

I found one way worth to try. It is wraping the code with 'with-temp-buffer'. In this case, 'fzf-git-files' will always start in temp buffer and modified process-environment variable would be affected.

However I cannot use this directly because 'fzf/after-term-handle-exit' function will try to restore windows after fzf is finished. After fzf is finished, an error occurs and the temp buffer looks like is a matter at that moment.

I should look into this little more. Any other approaches would be welcomed.

tttuuu888 commented 6 years ago

Refering to above bug report, eshell buffer make process-environment variable buffer local to prevent changing a variable affect all of emacs. and that's why fzf-git-files work like normal fzf in eshell buffer. (modified process-environment was ignored in fzf buffer)

It looks like wrapping code with 'with-temp-buffer' is the quick solution. process-environment should be modified in new temp buffer with this solution.

I tried as below and fzf-git-files worked fine in eshell buffer as well.

(defcustom fzf/default-command ""
  "Default command for fzf to use when input is tty."
  :type 'string
  :group 'fzf)

(defun fzf/start (directory)
  (require 'term)
  (window-configuration-to-register :fzf-windows)
  (advice-add 'term-handle-exit :after #'fzf/after-term-handle-exit)
  (with-temp-buffer
    (let* ((process-environment
            (cons (format "FZF_DEFAULT_COMMAND=%s" fzf/default-command)
                  process-environment))
           (buf (get-buffer-create "*fzf*"))
           (min-height (min fzf/window-height (/ (window-height) 2)))
           (window-height (if fzf/position-bottom (- min-height) min-height))
           (window-system-args (when window-system " --margin=1,0"))
           (fzf-args (concat fzf/args window-system-args)))
      (with-current-buffer buf
        (setq default-directory directory))
      (split-window-vertically window-height)
      (when fzf/position-bottom (other-window 1))
      (apply 'make-term "fzf" fzf/executable nil (split-string fzf-args))
      (switch-to-buffer buf)
      (linum-mode 0)
      (visual-line-mode 0)

      ;; disable various settings known to cause artifacts, see #1 for more details
      (setq-local scroll-margin 0)
      (setq-local scroll-conservatively 0)
      (setq-local term-suppress-hard-newline t) ;for paths wider than the window
      (setq-local show-trailing-whitespace nil)
      (face-remap-add-relative 'mode-line '(:box nil))

      (term-char-mode)
      (setq mode-line-format (format "   FZF  %s" directory)))))

(defun fzf/git-files ()
  (let ((fzf/default-command "git ls-files")
        (path (locate-dominating-file default-directory ".git")))
    (if path
        (fzf/start path)
      (user-error "Not inside a Git repository"))))

I am still not happy with adding additional code only for 'fzf-git-files' so I didn't make pull request yet. Need some opinions.

Thanks.

tttuuu888 commented 3 years ago

@bling @rolag Thanks a lot for recent major update. However the the last commit(23c09c9) make this issue happens again. As far as I remember eshell treats process-environment a little different way so modifying process-environment didn't work properly but piping worked. Thanks.

bling commented 3 years ago

@tttuuu888 this is the MR you created a while back to fix this: https://github.com/bling/fzf.el/pull/53/files. can you please confirm whether calling (/fzf/start path "git ls-files") directly instead works for you?

rolag commented 3 years ago

@bling That won't work as the original merge request I based mine on removed the command argument from fzf/start in favor of fzf-with-command. @tttuuu888 I'm not sure I fully understand how to reproduce the issue but can you try https://github.com/rolag/fzf.el/tree/without-process-environment and see if that works for you?

tttuuu888 commented 3 years ago

@rolag I tried and found it works for eshell. But I also found it emit an error whenever I finished fzf by pressing ESC. I'm sending "\e" to term buffer when I press ESC key though. Except for that, fzf-git-files works properly in eshell too.

Error message is as below:

error in process sentinel: advice-remove: Symbol’s value as variable is void: action

rolag commented 3 years ago

Can you show me the stack trace by setting (setq debug-on-error t) and retry?

tttuuu888 commented 3 years ago

Below is the stack trace of it.

Debugger entered--Lisp error: (void-variable action)
  (fzf/after-term-handle-exit action)
  (advice-remove 'term-handle-exit (fzf/after-term-handle-exit action))
  (lambda (process-name msg) (let ((exit-code (fzf/exit-code-from-event msg))) (message (format "exit code %s" exit-code)) (if (string= "0" exit-code) (let* ((text (buffer-substring-no-properties ... ...)) (lines (split-string text "\n" t " *> +")) (target (car ...))) (kill-buffer fzf/buffer-name) (jump-to-register fzf/window-register) (message (format "target %s" target)) (funcall action target)) (kill-buffer fzf/buffer-name) (jump-to-register fzf/window-register) (message (format "FZF exited with code %s" exit-code)))) (advice-remove 'term-handle-exit (fzf/after-term-handle-exit action)))("fzf" "exited abnormally with code 130\n")
  apply((lambda (process-name msg) (let ((exit-code (fzf/exit-code-from-event msg))) (message (format "exit code %s" exit-code)) (if (string= "0" exit-code) (let* ((text (buffer-substring-no-properties ... ...)) (lines (split-string text "\n" t " *> +")) (target (car ...))) (kill-buffer fzf/buffer-name) (jump-to-register fzf/window-register) (message (format "target %s" target)) (funcall action target)) (kill-buffer fzf/buffer-name) (jump-to-register fzf/window-register) (message (format "FZF exited with code %s" exit-code)))) (advice-remove 'term-handle-exit (fzf/after-term-handle-exit action))) ("fzf" "exited abnormally with code 130\n"))
  term-handle-exit("fzf" "exited abnormally with code 130\n")
  term-sentinel(#<process fzf> "exited abnormally with code 130\n")