Skip to content

Commit

Permalink
Implement integration with Magit 2.4
Browse files Browse the repository at this point in the history 
Closes #60

magit/magit#2491
magit/magit#2530
  • Loading branch information
@dgutov
dgutov committed Jan 31, 2016
1 parent 1c996a0 commit fdbf34a
Show file tree
Hide file tree
Showing 2 changed files with 36 additions and 4 deletions.
10 changes: 8 additions & 2 deletions README.md
View file
Expand Up @@ -90,5 +90,11 @@ psvn
Magit
-----

If you have a recent enough version installed, it defines
`magit-revert-buffer-hook` (or `magit-not-reverted-hook`), which we use.
If you're using a version before 2.4.0, it defines `magit-revert-buffer-hook`
(or `magit-not-reverted-hook`), which we use.

When using Magit 2.4 or newer, add this to your init script:

```lisp
(add-hook 'magit-post-refresh-hook 'diff-hl-magit-post-refresh t)
```
30 changes: 28 additions & 2 deletions diff-hl.el
View file
Expand Up @@ -488,8 +488,8 @@ in the source file, or the last line of the hunk above it."
;; doesn't care about changed VC state.
;; https://github.com/magit/magit/issues/603
(add-hook 'magit-revert-buffer-hook 'diff-hl-update nil t)
;; Magit 2+ doesn't do the above and calls this instead,
;; but only when it doesn't call `revert-buffer':
;; Magit versions 2.0-2.3 don't do the above and call this
;; instead, but only when they dosn't call `revert-buffer':
(add-hook 'magit-not-reverted-hook 'diff-hl-update nil t)
(add-hook 'auto-revert-mode-hook 'diff-hl-update nil t)
(add-hook 'text-scale-mode-hook 'diff-hl-define-bitmaps nil t))
Expand Down Expand Up @@ -517,6 +517,32 @@ in the source file, or the last line of the hunk above it."
(scan diff-hl-command-map)
(smartrep-define-key diff-hl-mode-map diff-hl-command-prefix smart-keys))))

(declare-function magit-toplevel "magit-git")
(declare-function magit-unstaged-files "magit-git")

(defun diff-hl-magit-post-refresh ()
(let* ((topdir (magit-toplevel))
(modified-files
(mapcar (lambda (file) (expand-file-name file topdir))
(magit-unstaged-files t)))
(unmodified-states '(up-to-date ignored unregistered)))
(dolist (buf (buffer-list))
(when (and (buffer-local-value 'diff-hl-mode buf)
(not (buffer-modified-p buf))
(file-in-directory-p (buffer-file-name buf) topdir))

This comment has been minimized.

Sign in to view

Copy link
@tarsius

tarsius Feb 29, 2016

Contributor

I have run into cases where (buffer-file-name buf) returned nil but the preceding conditions where non-nil. This happens in buffers created with magit-find-file. I am not sure where diff-hl gets turned on in those files and whether that is desirable or not.

I'll investigate some other time but thought I would let you know. Meanwhile you might wanna add an additional condition (buffer-file-name buf).

This comment has been minimized.

Sign in to view

Copy link
@dgutov

dgutov Feb 29, 2016

Author Owner

I am not sure where diff-hl gets turned on in those files and whether that is desirable or not.

Non-nil buffer-file-name is a precondition for having diff-hl-mode enabled from the globalized mode. diff-hl-changes also returns nil if buffer-file-name is nil.

So I think we're safe here.

This comment has been minimized.

Sign in to view

Copy link
@tarsius

tarsius Apr 5, 2016

Contributor

I intend to implement a proper file-handler for visiting blobs, so I am well aware that the current solution in Magit is a huge hack, but this is how a blob is currently being visited. The following part causes diff-hl-mode to be turned on:

          (let ((buffer-file-name magit-buffer-file-name))
            (normal-mode t))

For now I have changed this to:

          (let ((buffer-file-name magit-buffer-file-name)
                (after-change-major-mode-hook
                 (remq 'global-diff-hl-mode-enable-in-buffers
                       after-change-major-mode-hook)))
            (normal-mode t))

Any suggestions? (I don't intend to make any risky changes until I actually implement the blob file-handler.)

This comment has been minimized.

Sign in to view

Copy link
@dgutov

dgutov Apr 5, 2016

Author Owner

Is turning on diff-hl-mode in those files an actual problem? If so, can we identify a new class of buffers it should skip? Maybe there's a meaningful predicate we can add in turn-on-diff-hl-mode.

This comment has been minimized.

Sign in to view

Copy link
@tarsius

tarsius Aug 19, 2016

Contributor

Magit's blob buffers are no longer an issue because I have made the change mentioned above.

can we identify a new class of buffers it should skip?

Indirect buffers are a problem. If diff-hl-mode is enabled in the base buffer, then it's also enabled in the indirect buffer. I believe reset_buffer_local_variables is taking care of that and that turn-on-diff-hl-mode isn't involved here.

buffer-file-name is nil in indirect buffers. So when diff-hl-magit-post-refresh tries to revert an indirect buffer before the base buffer (afterwards there would be nothing left to revert), then it performs (file-in-directory-p nil topdir) but the first argument cannot be nil here.
This could be fixed by first making sure (buffer-file-name buf) does not return nil. Maybe there's another approach, but this one is cheap and a quick fix.

This comment has been minimized.

Sign in to view

Copy link
@dgutov

dgutov Aug 20, 2016

Author Owner

If diff-hl-mode is enabled in the base buffer, then it's also enabled in the indirect buffer.

I've just tried creating an indirect buffer of a file buffer with diff-hl-mode enabled, and that doesn't seem to be true.

How did you test that?

This comment has been minimized.

Sign in to view

Copy link
@tarsius

tarsius Aug 27, 2016 •
edited

Contributor

It is important that the indirect buffer is processed before the respective file-visiting buffer.

  1. M-x clone-indirect-buffer
  2. M-x magit-refresh-all

Is enough to reproduce this. Just make sure you don't do anything in between these two steps that might change the order of buffers as returned by buffer-list.

This comment has been minimized.

Sign in to view

Copy link
@dgutov

dgutov Aug 28, 2016

Author Owner

Ah, OK. So this can happen when the indirect buffer is created with the non-nil clone argument. I've pushed the proposed workaround.

Do you have a particular situation in mind, though, where clone-indirect-buffer is used? Maybe diff-hl-mode should instead disable itself in clone-indirect-buffer-hook.

This comment has been minimized.

Sign in to view

Copy link
@tarsius

tarsius Aug 29, 2016

Contributor

I use indirect buffers when I want to narrow to different parts of a long file. (I usually use outline-minor-mode not actual narrowing for that.)

I don't use indirect buffers often and am not familiar with the inner workings, so maybe I make some incorrect assumptions here:

Explicitly turning diff-hl-mode off might be a good idea; I think indirect buffers otherwise end up with stale diff-hl information. Even better though would be if the diff-hl information in indirect buffer did get updated, but that would have to happen after the file-visiting buffer has already been reverted (and therefor the indirect buffer too).

This comment has been minimized.

Sign in to view

Copy link
@dgutov

dgutov Aug 29, 2016

Author Owner

Even better though would be if the diff-hl information in indirect buffer did get updated, but that would have to happen after the file-visiting buffer has already been reverted (and therefor the indirect buffer too).

I doubt there would be one good way to behave there: people create indirect buffers for different purposes.
(with-current-buffer buf
(let* ((file buffer-file-name)
(backend (vc-backend file)))
(when backend
(cond
((member file modified-files)
(when (memq (vc-state file) unmodified-states)
(vc-state-refresh file backend))
(diff-hl-update))
((not (memq (vc-state file backend) unmodified-states))
(vc-state-refresh file backend)
(diff-hl-update))))))))))

(defun diff-hl-dir-update ()
(dolist (pair (if (vc-dir-marked-files)
(vc-dir-marked-only-files-and-states)
Expand Down

2 comments on commit fdbf34a

@manuel-uberti
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this. 👍

@cmacrae
Copy link

@cmacrae cmacrae commented on fdbf34a Feb 3, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! 👍

Please sign in to comment.