dgutov/diff-hl

`diff-hl-after-revert` is completely broken

Closed this issue · 12 comments

dhanak commented

I'm sorry to say, but this change is complete nonsense. First, it changed the hook function it adds to after-revert-hook, but it completely ignored the removal of the same hook function a few lines below. So now this function remains part of the hook forever.

But a larger problem is that it defines a variable revert-buffer-preserve-modes inside the function definition, and then checks its value. The variable is thus never created, and invoking the function manually causes an error (void variable revert-buffer-preserve-modes). As a hook function, it doesn't actually do anything.

I strongly believe this change should be reverted.

dgutov commented

Hi!

Let's review this step-by-step.

First, it changed the hook function it adds to after-revert-hook, but it completely ignored the removal of the same hook function a few lines below. So now this function remains part of the hook forever.

That's helpful, thanks. This kind of thing can be hard to notice in practice, given that when a minor mode is enabled, it usually stays enabled, outside of experimentation. I've fixed it now.

But a larger problem is that it defines a variable revert-buffer-preserve-modes inside the function definition, and then checks its value

Not quite. When a defvar form is without the initvalue argument, the real effect is that the variable is declared dynamic (global), which is important to the byte-compiler when lexical-binding is enabled. This variable is set inside the function revert-buffer, which the issue referenced in the commit was about. And this variable's value determines what is the context that revert-buffer was called in, and in particular -- whether the major mode function would be called again, in which case calling diff-hl-mode from the hook would just be extra work. Hence the variable lookup. Calling this function outside of after-revert-hook is not supported, but that seems fine.

And having the defvar form only inside the function just makes the scope of the dynamic-ness of the variable be limited to that function. I think that's not well-documented, unfortunately.

dgutov commented

Was there a particular problem you were looking into?

dhanak commented

Hmmm, interesting. I didn't know defvar could work this way. Thanks for the detailed explanation.

The issue is that the diff-hl marks don't get updated upon auto-revert. I'm using projectile, which enables auto-revert-mode on the edited buffers. Then, I use CLI git (in a terminal, outside emacs) to pull and push changes. This updates the file, but doesn't update the markings. As far as I was able to tell, reverting this change solved my problem, but I'll continue observing its behavior and get back to you with updates.

dgutov commented

Okay.

I'm using projectile, which enables auto-revert-mode on the edited buffers.

Does it do that? I'm looking at https://github.com/bbatsov/projectile/blob/master/projectile.el, and it doesn't seem to have any references to revert-buffer or auto-revert-mode.

If you were using something else for reverting buffers, e.g. https://github.com/joaotavora/revbufs/blob/master/revbufs.el, it could be the place to look at to find the problem.

You can also put a breakpoint on auto-revert-mode (with C-u C-M-x) and when it's hit, press d to see the current call stack. That should tell us what arguments revert-buffer was called with (the 3rd argument in particular).

dhanak commented

My bad, it's not projectile, as I assumed, it's magit, magit-auto-revert-mode in particular (which is enabled by default, that's why I had to guess which of my packages is responsible for turning auto-revert-mode on).

It's odd, anyhow. Sometimes, it refreshes, other times, it doesn't. Perhaps it refreshes only when there is some incoming change as well. And when there are only my modifications, which are committed, then there is no revert, hence to diff-hl update.

Hmm. Before diff-hl, I used git-gutter, which was able to detect the refresh of the .git directory and update its coloring. I switched because it was slow over tramp, and I was happy to find that diff-hl solves this issue well. When I noticed that the colors don't always update, I figured that the issue was what I explained above. But perhaps not.

dhanak commented

Yeah, this is probably not a diff-hl issue. Sorry for the false alarm, and thank you for your patience!

It's a nifty little package!

  • ❤️1
dgutov commented

Okay, Magit has known problems (you can search the tracker for that term).

But this particular step could be improved.

@tarsius, what would you say about passing the 3rd argument t to revert-buffer in magit-find-file-noselect-1? I don't understand the whole picture, but the arg PRESERVE-MODES is what the diff-hl-after-revert checks for (otherwise, diff-hl-mode is supposed to be turned on by the major mode function that's re-invoked).

  • ❤️1
tarsius commented

magit-auto-revert-mode in particular (which is enabled by default, that's why I had to guess which of my packages is responsible for turning auto-revert-mode on).

How did you determine that? Can I see the call-stack please.

what would you say about passing the 3rd argument t to revert-buffer in magit-find-file-noselect-1?

  • That's not related to magit-auto-revert-mode.
  • Also we really should call normal-mode here because when the buffer visits, say, "file as it was in HEAD" and HEAD does not point at the same commit as before, then we might be looking at a very different file, which might even require a different major-mode.

@dhanak did you follow the instructions at https://github.com/dgutov/diff-hl#magit?

The configuration I personally use is

(use-package diff-hl
  :config
  (global-diff-hl-mode)
  (add-hook 'magit-post-refresh-hook 'diff-hl-magit-post-refresh t))
tarsius commented

(Off-topic)

And having the defvar form only inside the function just makes the scope of the dynamic-ness of the variable be limited to that function. I think that's not well-documented, unfortunately.

If you find any documentation about that, please point me to it.

dgutov commented

@tarsius

Also we really should call normal-mode here because when the buffer visits, say, "file as it was in HEAD" and HEAD does not point at the same commit as before, then we might be looking at a very different file, which might even require a different major-mode.

Well, actually since you're calling (revert-buffer t t) (no third argument), it sets revert-buffer-preserve-modes to nil, and consequently later normal-mode gets called (in after-find-file). I'm not sure why diff-hl-update doesn't get invoked then; hard to say without backtraces.

did you follow the instructions at ...

I thought the Magit integration "never worked properly"? According to you in #171 (comment), that is.

I'll be happy to hear otherwise.

If you find any documentation about <defvar>, please point me to it.

A couple of places I found in the manual:

The doc for defvar here: https://www.gnu.org/software/emacs/manual/html_node/elisp/Defining-Variables.html

Whereas if value is omitted then the variable is only marked special locally (i.e. within the current lexical scope, or file if at the top-level).

And the section "Using Lexical Binding": https://www.gnu.org/software/emacs/manual/html_node/elisp/Using-Lexical-Binding.html

See the code example and the small paragraph right before it.

dhanak commented

magit-auto-revert-mode in particular (which is enabled by default, that's why I had to guess which of my packages is responsible for turning auto-revert-mode on).

How did you determine that? Can I see the call-stack please.

Isn't it magit-auto-revert-mode which turns on auto-revert-mode in my buffers of git managed projects? That's what the source code suggests, and in magit-turn-on-auto-revert-mode-if-desired, it says (auto-revert-mode 1) explicitly. I could be wrong, though. Mind you, I really like this feature, there is nothing wrong with it. I was just wrong in my original post in saying that it was projectile which turned auto-revert-mode on, that's all.

@dhanak did you follow the instructions at https://github.com/dgutov/diff-hl#magit?

The configuration I personally use is

(use-package diff-hl
  :config
  (global-diff-hl-mode)
  (add-hook 'magit-post-refresh-hook 'diff-hl-magit-post-refresh t))

Sort of. I definitely added the diff-hl functions to the appropriate magit hooks. And they work like a charm when the file itself is changed on the disk and auto-revert kicks in. I'm missing a diff-hl-update only when the .git directory changes but the file itself doesn't. Most notably, when I commit my changes, but from the CLI, not from magit. IIRC, this used to work fine with git-gutter, but I could be wrong. It's not a big deal.

dgutov commented

I'm missing a diff-hl-update only when the .git directory changes but the file itself doesn't. Most notably, when I commit my changes, but from the CLI, not from magit.

I guess that's simply not supported by either of the auto-revert mechanisms. Could be added, I guess -- I'll have to see how git-gutter does that. It might make more sense in the context of diff-hl-flydiff-mode, too.

EDIT: Note that this feature would either slow everything down over Tramp as well, or would need to be disabled at least for remote buffers.