bug#12747: 23.4; diff-auto-refine-mode process only last hunk in diff (must ALL).

bug#12747: 23.4; diff-auto-refine-mode process only last hunk in diff (must ALL).

[Oleksandr Gavenko]

In GNU Emacs 23.4.1 (x86_64-pc-linux-gnu, GTK+ Version 2.24.10) but seems that
current truck have same problem as related part of 'diff-mode.el' unchanged.

If I enable diff-auto-refine-mode in all diff-mode buffers:

  (defun my-diff-auto-refine-mode-on () (diff-auto-refine-mode 1))
  (add-hook 'diff-mode-hook 'my-diff-auto-refine-mode-on)

I see actions only on last hunk in diff. Look to definition:

  (define-minor-mode diff-auto-refine-mode
    (when diff-auto-refine-mode
      (condition-case-unless-debug nil (diff-refine-hunk) (error nil))))

and to doc string:

  diff-refine-hunk is an interactive compiled Lisp function in `diff-mode.el'.

  Highlight changes of hunk at point at a finer granularity.

So I think 'diff-auto-refine-mode' must iterate over all hunks and apply
'diff-refine-hunk' function on each of them.

-- 
Best regards!

bug#12747: 23.4; diff-auto-refine-mode process only last hunk in diff (must ALL).

[Stefan Monnier]

> If I enable diff-auto-refine-mode in all diff-mode buffers:
>   (defun my-diff-auto-refine-mode-on () (diff-auto-refine-mode 1))
>   (add-hook 'diff-mode-hook 'my-diff-auto-refine-mode-on)
> I see actions only on last hunk in diff.

I'm not sure I understand what you mean.  `diff-auto-refine-mode' does
not refine-highlight all the hunks at once (quoting the docstring):

   When enabled, Emacs automatically highlights
   changes in detail as the user visits hunks.

"as the user visits the hunks" means that it's only highlighted in
response to "n" and "p" (and a few related operations).
   
This is not a bug.  IIUC you'd like the refinement to be done in any
hunk that is ever displayed, right?
If so, that is a valid request for enhancement, and I fully agree.
If someone is interested in implementing it, here's how I think it would
have to work:
- add a font-lock-keywords rule in diff-mode which simply registers the
  region displayed in a buffer-local var `diff--regions-displayed'.
- have an idle timer that checks `diff--regions-displayed' and refines
  all the hunks in those regions (and it should also font-lock those
  hunks at the same time, so that if some of the hunk is not yet
  displayed and not yet font-locked, displaying it later on won't cause
  re-refining the hunk).


        Stefan

bug#12747: 23.4; diff-auto-refine-mode process only last hunk in diff (must ALL).

[Oleksandr Gavenko]

On 2012-10-28, Stefan Monnier wrote:

>> If I enable diff-auto-refine-mode in all diff-mode buffers:
>>   (defun my-diff-auto-refine-mode-on () (diff-auto-refine-mode 1))
>>   (add-hook 'diff-mode-hook 'my-diff-auto-refine-mode-on)
>> I see actions only on last hunk in diff.
>
> I'm not sure I understand what you mean.  `diff-auto-refine-mode' does
> not refine-highlight all the hunks at once (quoting the docstring):
>
>    When enabled, Emacs automatically highlights
>    changes in detail as the user visits hunks.
>
> "as the user visits the hunks" means that it's only highlighted in
> response to "n" and "p" (and a few related operations).
>
Ok. But it's difficult to understand. For example manual section
"22.10 Diff Mode" doesn't say anything about concepts of visiting hunks...

I forget add another essential details. I use this functionality with 'C-x v
=' (runs the command vc-diff). And got highlighting in detail only for last
hunk.

Next I save *vc-diff* buffer to file and reopen it. Now I got highlighting in
detail only in first hunk.

This behaviour frustrate me, until I got understanding of working model from
you.

> This is not a bug.  IIUC you'd like the refinement to be done in any
> hunk that is ever displayed, right?

Yes.

I never use "n" or "p" for moving around *diff* buffer. Just "arrows" and
"pages". So in my work-flow 'diff-auto-refine-mode' do nothing...

> If so, that is a valid request for enhancement, and I fully agree.

That is!

Also I would be glad to see alias for:

  (diff-auto-refine-mode 1)

in form:

  (defun diff-auto-refine-mode-on () (diff-auto-refine-mode 1))

to simplify usage in hooks...

> If someone is interested in implementing it, here's how I think it would
> have to work:
> - add a font-lock-keywords rule in diff-mode which simply registers the
>   region displayed in a buffer-local var `diff--regions-displayed'.
> - have an idle timer that checks `diff--regions-displayed' and refines
>   all the hunks in those regions (and it should also font-lock those
>   hunks at the same time, so that if some of the hunk is not yet
>   displayed and not yet font-locked, displaying it later on won't cause
>   re-refining the hunk).
>
Sound very good.

FYI I heard about refining in emacs-help:

  http://thread.gmane.org/gmane.emacs.help/87082/focus=87093
                Diff could also show the changes within lines

Tom suggest add to 'diff-mode-hook' such code:

  (save-excursion
    (goto-char (point-min))
    (while (not (eobp))
      (diff-hunk-next))))

As exercise I try make some code myself:

  (defvar diff-auto+-refine-idle-time 2
    "Time before highlighting take place XXX")

  (defvar diff-auto+-refine-idle-timer nil
    "Timer for highlighting XXX")

  (defun diff-auto+-refine-action ()
    "TODO how about preventing to process already processed
  regions??"
    (when (eq major-mode 'diff-mode)
      (message "%s: diff-auto+-refine-action" (format-time-string "%M:%S" (current-time)))
      (diff-refine-hunk)))

  (defun diff-auto+-refine-mode (&optional arg)
    "Auto refine current hunk in diff mode."
    (interactive "P")
    (if (not arg)
        (if diff-auto+-refine-idle-timer
            (diff-auto+-refine-mode -1)
          (diff-auto+-refine-mode 1))
      (if (and (number-or-marker-p arg) (< 0 (prefix-numeric-value arg)))
          (unless diff-auto+-refine-idle-timer
            (setq diff-auto+-refine-idle-timer
                  (run-with-idle-timer 2 t 'diff-auto+-refine-action)))
        (when diff-auto+-refine-idle-timer
          (cancel-timer diff-auto+-refine-idle-timer)
          (setq diff-auto+-refine-idle-timer nil)))))

which does not use 'font-lock-keywords'. I am inexperience Elisp hacker and
don't understand how can >>font-lock-keywords rule registers the region
displayed<<. Does this mean register MATCHER with always fail but perform side
effect by updating suggested 'diff--regions-displayed' variable?

Also I can't found function that return visible to user region to proper
highlight all visible hunks instead current...

As I have many question which involve teaching of me I ask them in
emacs-help:

  http://permalink.gmane.org/gmane.emacs.help/87474
                Function that return visible to user region and performing
                action on demand.
  http://permalink.gmane.org/gmane.emacs.help/87472
                idle-timer for processing all buffers with selected
                minor-mode.

-- 
Best regards!

bug#12747: 23.4; diff-auto-refine-mode process only last hunk in diff (must ALL).

[Stefan Monnier]

retitle 12747 Make diff-auto-refine-mode refine from jit/font-lock
severity 12747 wishlist
thanks

>> If so, that is a valid request for enhancement, and I fully agree.
> That is!

OK, title adjusted.  I also would like to see this, FWIW.

> Also I would be glad to see alias for:
>   (diff-auto-refine-mode 1)
> in form:
>   (defun diff-auto-refine-mode-on () (diff-auto-refine-mode 1))
> to simplify usage in hooks...

This is not needed any more.  Nowadays minor modes always enable the
mode if called without argument.  IOW you can just use
diff-auto-refine-mode in your hooks without risk of it toggling the
minor mode off.

>   (save-excursion
>     (goto-char (point-min))
>     (while (not (eobp))
>       (diff-hunk-next))))

Problem with this, is that it can be slow when visiting large patch
files, and it may fail to refine all hunks in *vc-diff* buffers because
that might be run before the process has finished returning the diff.

> Does this mean register MATCHER with always fail but perform side
> effect by updating suggested 'diff--regions-displayed' variable?

Yes.

> Also I can't found function that return visible to user region to proper
> highlight all visible hunks instead current...

The font-lock-keywords rule suggested before does more or less.

Otherwise, you can go through all windows, check if they're displaying
your buffer and then use window-start and window-end to figure out what
is displayed, but that approach generally leads to madness (compare
jit-lock to lazy-lock, or linum.el to nlinum.el).


        Stefan

bug#12747: 23.4; diff-auto-refine-mode process only last hunk in diff (must ALL).

[Noam Postavsky]

[-- Attachment #1: Type: text/plain, Size: 1355 bytes --]

merge 12747 16798 18128 21744
tags 12747 fixed
close 12747 27.1
quit

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> This is not a bug.  IIUC you'd like the refinement to be done in any
> hunk that is ever displayed, right?
> If so, that is a valid request for enhancement, and I fully agree.
> If someone is interested in implementing it, here's how I think it would
> have to work:
> - add a font-lock-keywords rule in diff-mode which simply registers the
>   region displayed in a buffer-local var `diff--regions-displayed'.
> - have an idle timer that checks `diff--regions-displayed' and refines
>   all the hunks in those regions (and it should also font-lock those
>   hunks at the same time, so that if some of the hunk is not yet
>   displayed and not yet font-locked, displaying it later on won't cause
>   re-refining the hunk).

I think you've implemented this now [1: f8b1e40fb6], though not quite in
the way you describe (I don't see any timers).

[1: f8b1e40fb6]: 2018-07-10 22:52:21 -0400
  * lisp/vc/diff-mode.el: Perform hunk refinement from font-lock
  https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=f8b1e40fb63b0a6bc6692cc0bc84e5f5e65c2644

This reminds that magit users found binding write-region-inhibit-fsync
around smerge-refine-regions made a noticable performance difference.
So should we add something like this?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch --]
[-- Type: text/x-diff, Size: 1311 bytes --]

From e5f3cf973c37ddaca92cc819d95d896ca0d869c7 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Wed, 11 Jul 2018 20:13:25 -0400
Subject: [PATCH] Speed up smerge-refine-regions by avoiding fsync

* lisp/vc/smerge-mode.el (smerge-refine-regions): Bind
write-region-inhibit-fsync to t.
---
 lisp/vc/smerge-mode.el | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/lisp/vc/smerge-mode.el b/lisp/vc/smerge-mode.el
index cb51fbab8e..cb9880c80d 100644
--- a/lisp/vc/smerge-mode.el
+++ b/lisp/vc/smerge-mode.el
@@ -1075,9 +1075,10 @@ smerge-refine-regions
           (if smerge-refine-weight-hack (make-hash-table :test #'equal))))
     (unless (markerp beg1) (setq beg1 (copy-marker beg1)))
     (unless (markerp beg2) (setq beg2 (copy-marker beg2)))
-    ;; Chop up regions into smaller elements and save into files.
-    (smerge--refine-chopup-region beg1 end1 file1 preproc)
-    (smerge--refine-chopup-region beg2 end2 file2 preproc)
+    (let ((write-region-inhibit-fsync t)) ; Don't fsync temp files.
+      ;; Chop up regions into smaller elements and save into files.
+      (smerge--refine-chopup-region beg1 end1 file1 preproc)
+      (smerge--refine-chopup-region beg2 end2 file2 preproc))
 
     ;; Call diff on those files.
     (unwind-protect
-- 
2.11.0

bug#12747: 23.4; diff-auto-refine-mode process only last hunk in diff (must ALL).

[Stefan Monnier]

> I think you've implemented this now [1: f8b1e40fb6], though not quite in
> the way you describe (I don't see any timers).

Indeed, thanks.

> This reminds that magit users found binding write-region-inhibit-fsync
> around smerge-refine-regions made a noticable performance difference.
> So should we add something like this?

Sounds good, yes (tho I'm surprised it'd make much of a difference,
when your /tmp is on some kind of tmpfs).  If you can add a URL
pointing to the discussion where they found the noticable performance
difference, that'd be even better.


        Stefan


> From e5f3cf973c37ddaca92cc819d95d896ca0d869c7 Mon Sep 17 00:00:00 2001
> From: Noam Postavsky <npostavs@gmail.com>
> Date: Wed, 11 Jul 2018 20:13:25 -0400
> Subject: [PATCH] Speed up smerge-refine-regions by avoiding fsync
>
> * lisp/vc/smerge-mode.el (smerge-refine-regions): Bind
> write-region-inhibit-fsync to t.
> ---
>  lisp/vc/smerge-mode.el | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/lisp/vc/smerge-mode.el b/lisp/vc/smerge-mode.el
> index cb51fbab8e..cb9880c80d 100644
> --- a/lisp/vc/smerge-mode.el
> +++ b/lisp/vc/smerge-mode.el
> @@ -1075,9 +1075,10 @@ smerge-refine-regions
>            (if smerge-refine-weight-hack (make-hash-table :test #'equal))))
>      (unless (markerp beg1) (setq beg1 (copy-marker beg1)))
>      (unless (markerp beg2) (setq beg2 (copy-marker beg2)))
> -    ;; Chop up regions into smaller elements and save into files.
> -    (smerge--refine-chopup-region beg1 end1 file1 preproc)
> -    (smerge--refine-chopup-region beg2 end2 file2 preproc)
> +    (let ((write-region-inhibit-fsync t)) ; Don't fsync temp files.
> +      ;; Chop up regions into smaller elements and save into files.
> +      (smerge--refine-chopup-region beg1 end1 file1 preproc)
> +      (smerge--refine-chopup-region beg2 end2 file2 preproc))
>  
>      ;; Call diff on those files.
>      (unwind-protect

bug#12747: 23.4; diff-auto-refine-mode process only last hunk in diff (must ALL).

[Noam Postavsky]

On 12 July 2018 at 09:28, Stefan Monnier <monnier@iro.umontreal.ca> wrote:

>> This reminds that magit users found binding write-region-inhibit-fsync
>> around smerge-refine-regions made a noticable performance difference.
>> So should we add something like this?
>
> Sounds good, yes (tho I'm surprised it'd make much of a difference,
> when your /tmp is on some kind of tmpfs).  If you can add a URL
> pointing to the discussion where they found the noticable performance
> difference, that'd be even better.

Hmm, actually there wasn't that much discussion about it, one person
reported it made a big difference:

https://github.com/magit/magit/pull/2834

There was previous discussion about smerge refinement being slow, but
nobody narrowed it down to fsync in particular, and the conclusion
there was just "call smerge-refine-subst less".

https://github.com/magit/magit/issues/1581

bug#12747: 23.4; diff-auto-refine-mode process only last hunk in diff (must ALL).

[Stefan Monnier]

>>> This reminds that magit users found binding write-region-inhibit-fsync
>>> around smerge-refine-regions made a noticable performance difference.
>>> So should we add something like this?
>> Sounds good, yes (tho I'm surprised it'd make much of a difference,
>> when your /tmp is on some kind of tmpfs).  If you can add a URL
>> pointing to the discussion where they found the noticable performance
>> difference, that'd be even better.
> Hmm, actually there wasn't that much discussion about it, one person
> reported it made a big difference:
> https://github.com/magit/magit/pull/2834

Seems like a good enough URL top put there.

IIUC magit's code uses a different approach from the one I installed in
diff-mode (i.e. it eagerly calls smerge-refine on all hunks), which
is likely to suffer much more severely from performance issues on large
diffs, so I think write-region-inhibit-fsync won't make a big difference
for diff-mode, but it's still a good change in any case.

Note that both approaches suffer identically when bumping into a large
hunk, OTOH.

> There was previous discussion about smerge refinement being slow, but
> nobody narrowed it down to fsync in particular, and the conclusion
> there was just "call smerge-refine-subst less".

Yes, I think this one is really case of Magit's approach being simply
too eager.


        Stefan

bug#12747: 23.4; diff-auto-refine-mode process only last hunk in diff (must ALL).

[Noam Postavsky]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

> Sounds good, yes (tho I'm surprised it'd make much of a difference,
> when your /tmp is on some kind of tmpfs).  If you can add a URL
> pointing to the discussion where they found the noticable performance
> difference, that'd be even better.

Pushed to master.

[1: 01dbf2a347]: 2018-07-12 21:45:31 -0400
  Speed up smerge-refine-regions by avoiding fsync
  https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=01dbf2a347944497fdcf2ec156f4605020d7ba2a