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

Warning when using counsel-find-file in conjunction with diff-hl #627

Closed
drot opened this Issue Aug 18, 2016 · 10 comments

Comments

Projects
None yet
3 participants
@drot

drot commented Aug 18, 2016

GNU Emacs 25.1.1 (x86_64-unknown-linux-gnu, X toolkit, Xaw scroll bars) of 2016-07-24

Enabling the following modes:

(global-diff-hl-mode 1)
(counsel-mode 1)

I get the following warning on the first file I visit after Emacs has started (init.el in this example):
Making find-file-hook local to init.el while let-bound!

The warning only displays once, after that I can visit other files and it will not display again.

@abo-abo

This comment has been minimized.

Owner

abo-abo commented Aug 18, 2016

This is OK, just a regular Emacs message. What happens here, is that counsel-find-file temporarily sets find-file-hook to nil for remote files. This should result in a remote file opening faster.

If you don't like this behavior, you can disable it with:

(setq counsel-find-file-speedup-remote nil)
@drot

This comment has been minimized.

drot commented Aug 18, 2016

Thank you for the clarification, though the warning still appears after setting counsel-find-file-speedup-remote to nil.

@abo-abo

This comment has been minimized.

Owner

abo-abo commented Aug 18, 2016

I can't reproduce your issue actually. It has to be something in your config.

@drot

This comment has been minimized.

drot commented Aug 18, 2016

This is all I have in my init file started from scratch with a blank file:

(package-initialize)
(add-to-list 'package-archives '("melpa" . "https://melpa.org/packages/"))

(global-diff-hl-mode 1)
(counsel-mode 1)
(setq counsel-find-file-speedup-remote nil)

(custom-set-variables
 ;; custom-set-variables was added by Custom.
 ;; If you edit it by hand, you could mess it up, so be careful.
 ;; Your init file should contain only one such instance.
 ;; If there is more than one, they won't work right.
 '(package-selected-packages (quote (diff-hl counsel))))
(custom-set-faces
 ;; custom-set-faces was added by Custom.
 ;; If you edit it by hand, you could mess it up, so be careful.
 ;; Your init file should contain only one such instance.
 ;; If there is more than one, they won't work right.
 )
@abo-abo

This comment has been minimized.

Owner

abo-abo commented Aug 18, 2016

I see the problem now: even with counsel-find-file-speedup-remote off, counsel-find-file let-binds find-file-hook to itself.

And diff-hl-mode has this code:

(add-hook (if vc-mode
                      ;; Defer until the end of this hook, so that its
                      ;; elements can modify the update behavior.
                      'diff-hl-mode-on-hook
                    ;; If we're only opening the file now,
                    ;; `vc-find-file-hook' likely hasn't run yet, so
                    ;; let's wait until the state information is
                    ;; saved, in order not to fetch it twice.
                    'find-file-hook)
                  'diff-hl-update t t)

Which means that it locally sets find-file-hook if the file isn't version controlled. Which is pretty weird, if you ask me. I suggest you report this as an issue vs diff-hl.

@drot

This comment has been minimized.

drot commented Aug 18, 2016

Thank you for the pointers once again, I will file an issue for diff-hl.

@dgutov

This comment has been minimized.

dgutov commented Aug 18, 2016

Which is pretty weird, if you ask me. I suggest you report this as an issue vs diff-hl.

I'm within my rights to do that, and it serves a purpose. What makes you say it's a bug?

@abo-abo

This comment has been minimized.

Owner

abo-abo commented Aug 18, 2016

@dgutov It seems strange for the minor mode to locally set find-file-hook: the file was already opened, why bother setting the hook locally for the exact same file? It won't be called again. It becomes weirder when globalized.

I would suggest for the globalized mode to set find-file-hook not in a buffer-local way, and for the plain minor mode to not touch it at all.

@dgutov

This comment has been minimized.

dgutov commented Aug 18, 2016

If vc-mode is nil, we're in the process of running find-file-hook, and vc-refresh-state hasn't been called yet.

@drot

This comment has been minimized.

drot commented Aug 19, 2016

Same warning happens also when using thegit-gutter package.

errge added a commit to nilcons-contrib/swiper that referenced this issue Jul 25, 2017

abo-abo added a commit that referenced this issue Jul 25, 2017

Do not let bind find-file-hook if not necessary
Fixes #627
Fixes #1118

We only do the let binding here for find-file-hook if we are actually
going to override it, there are some other modes (git-gutter and
diff-hl) that cause user visible warnings if find-file-hook is let
bound.  This way the user only has the warning in case of remote files
and if she is annoyed with that, can set the
counsel-find-file-speedup-remote variable to nil to get rid of them.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment