bug#33567: Syntactic fontification of diff hunks

bug#33567: Syntactic fontification of diff hunks

[Juri Linkov]

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

Tags: patch
Severity: wishlist

For a long time after announcing this feature in
https://lists.gnu.org/archive/html/emacs-devel/2018-08/msg00537.html
I received requests in private mails asking when I'll submit a complete patch.
I'm sorry, it took much time addressing all concerns raised in that thread,
and testing in many possible scenarios.  Based on the feedback, I rewrote it
several times, and now finally it's optimized to be fast and reliable.


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

diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index 4adef02984..68b2f9e522 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -103,6 +103,21 @@ diff-font-lock-prettify
   :version "27.1"
   :type 'boolean)
 
+(defcustom diff-font-lock-syntax 'vc
+  "If non-nil, diff hunk font-lock includes syntax highlighting.
+If `vc', highlight syntax only in Diff buffers created by a version control
+system that provides all necessary context for reliable highlighting.
+If t, additionally try to get more context from existing files, or when
+source files are not found, still try to highlight hunks without enough
+context that sometimes might result in wrong fontification.
+If `hunk-only', fontification is based on hunk alone, without full source.
+This is the fastest, but less reliable."
+  :version "27.1"
+  :type '(choice (const :tag "Don't highlight syntax" nil)
+                 (const :tag "Only under version control" vc)
+                 (const :tag "Hunk-based only" hunk-only)
+                 (const :tag "Without full source or get it from files" t)))
+
 (defvar diff-vc-backend nil
   "The VC backend that created the current Diff buffer, if any.")
 
@@ -406,6 +421,7 @@ diff-font-lock-keywords
      (1 font-lock-comment-delimiter-face)
      (2 font-lock-comment-face))
     ("^[^-=+*!<>#].*\n" (0 'diff-context))
+    (,#'diff--font-lock-syntax)
     (,#'diff--font-lock-prettify)
     (,#'diff--font-lock-refined)))
 
@@ -2316,6 +2333,195 @@ diff--font-lock-prettify
                              'display "")))))
   nil)
 
+;;; Syntax highlighting from font-lock
+
+(defun diff--font-lock-syntax (max)
+  "Syntax highlighting from font-lock."
+  (when diff-font-lock-syntax
+    (when (get-char-property (point) 'diff--font-lock-syntax)
+      (goto-char (next-single-char-property-change
+                  (point) 'diff--font-lock-syntax nil max)))
+    (let* ((min (point))
+           (beg (or (ignore-errors (diff-beginning-of-hunk))
+                    (ignore-errors (diff-hunk-next) (point))
+                    max)))
+      (while (< beg max)
+        (let ((end
+               (save-excursion (goto-char beg) (diff-end-of-hunk) (point))))
+          (if (< end min) (setq beg min))
+          (unless (or (< end beg)
+                      (get-char-property beg 'diff--font-lock-syntax))
+            (diff-syntax-fontify beg end)
+            (let ((ol (make-overlay beg end)))
+              (overlay-put ol 'diff--font-lock-syntax t)
+              (overlay-put ol 'diff-mode 'syntax)
+              (overlay-put ol 'evaporate t)
+              (overlay-put ol 'modification-hooks
+                           '(diff--font-lock-syntax--refresh))))
+          (goto-char (max beg end))
+          (setq beg (or (ignore-errors (diff-hunk-next) (point)) max))))))
+  nil)
+
+(defun diff--font-lock-syntax--refresh (ol _after _beg _end &optional _len)
+  (delete-overlay ol))
+
+(defun diff-syntax-fontify (start end)
+  (save-excursion
+    (diff-syntax-fontify-hunk start end t)
+    (diff-syntax-fontify-hunk start end nil)))
+
+(defvar diff-syntax-fontify-revisions (make-hash-table :test 'equal))
+
+(defun diff-syntax-fontify-hunk (beg end old)
+  "Highlight language syntax in diff hunks."
+  (remove-overlays beg end 'diff-mode 'syntax)
+  (goto-char beg)
+  (let* ((hunk (buffer-substring-no-properties beg end))
+         (text (or (ignore-errors (diff-hunk-text hunk (not old) nil)) ""))
+	 (line (if (looking-at "\\(?:\\*\\{15\\}.*\n\\)?[-@* ]*\\([0-9,]+\\)\\([ acd+]+\\([0-9,]+\\)\\)?")
+		   (if old (match-string 1)
+		     (if (match-end 3) (match-string 3) (match-string 1)))))
+         (line-nb (and line (string-match "\\([0-9]+\\),\\([0-9]+\\)" line)
+                       (list (string-to-number (match-string 1 line))
+                             (string-to-number (match-string 2 line)))))
+         props)
+    (cond
+     ((and diff-vc-backend (not (eq diff-font-lock-syntax 'hunk-only)))
+      (let* ((file (diff-find-file-name old t))
+             (revision (and file (if (not old) (nth 1 diff-vc-revisions)
+                                   (or (nth 0 diff-vc-revisions)
+                                       (vc-working-revision file))))))
+        (if file
+            (if (not revision)
+                ;; Get properties from the current working file
+                (when (and (not old) (file-exists-p file))
+                  ;; Try to reuse an existing buffer
+                  (if (get-file-buffer (expand-file-name file))
+                      (with-current-buffer (get-file-buffer (expand-file-name file))
+                        (setq props (diff-syntax-fontify-props nil text line-nb t)))
+                    ;; Get properties from the file
+                    (with-temp-buffer
+                      (insert-file-contents file t)
+                      (setq props (diff-syntax-fontify-props file text line-nb)))))
+              ;; Get properties from a cached revision
+              (let* ((buffer-name (format " diff-syntax:%s.~%s~"
+                                          (expand-file-name file) revision))
+                     (buffer (gethash buffer-name diff-syntax-fontify-revisions))
+                     (no-init t))
+                (unless (and buffer (buffer-live-p buffer))
+                  (let* ((vc-find-revision-no-save t)
+                         (vc-buffer (save-window-excursion
+                                      ;; Restore restore previous window configuration
+                                      ;; because when vc-find-revision can't find a revision
+                                      ;; (e.g. for /dev/null), it jumps to another window
+                                      ;; using pop-to-buffer in vc-do-command when
+                                      ;; the buffer name doesn't begin with a space char.
+                                      (ignore-errors
+                                        (vc-find-revision (expand-file-name file)
+                                                          revision diff-vc-backend)))))
+                    (when vc-buffer
+                      (with-current-buffer (get-buffer-create buffer-name)
+                        (insert-buffer-substring-no-properties vc-buffer)
+                        (setq buffer (current-buffer) no-init nil))
+                      (puthash buffer-name buffer diff-syntax-fontify-revisions)
+                      (kill-buffer vc-buffer))))
+                (when buffer
+                  (with-current-buffer buffer
+                    (setq props (diff-syntax-fontify-props file text line-nb no-init))))))
+          ;; If file is unavailable, get properties from the hunk alone
+          (setq file (car (diff-hunk-file-names old)))
+          (with-temp-buffer
+            (insert text)
+            (setq props (diff-syntax-fontify-props file text line-nb nil t))))))
+     ((eq diff-font-lock-syntax 'hunk-only)
+      (setq file (car (diff-hunk-file-names old)))
+      (with-temp-buffer
+        (insert text)
+        (setq props (diff-syntax-fontify-props file text line-nb nil t))))
+     ((not (eq diff-font-lock-syntax 'vc))
+      (let ((file (car (diff-hunk-file-names old))))
+        (if (and file (file-exists-p file))
+            ;; Try to get full text from the file
+            (with-temp-buffer
+              (insert-file-contents file t)
+              (setq props (diff-syntax-fontify-props file text line-nb)))
+          ;; Otherwise, get properties from the hunk alone
+          (with-temp-buffer
+            (insert text)
+            (setq props (diff-syntax-fontify-props file text line-nb nil t)))))))
+
+    ;; Put properties over the hunk text
+    (when props
+      (goto-char beg)
+      (while (< (progn (forward-line 1) (point)) end)
+        (when (or (and (not old) (not (looking-at-p "[-<]")))
+                  (and      old  (not (looking-at-p "[+>]"))))
+          (if (and old (not (looking-at-p "[-<]")))
+              ;; Fontify context lines only from new source,
+              ;; don't refontify context lines from old source.
+              (pop props)
+            (let ((line-props (pop props))
+                  (bol (1+ (point))))
+              (dolist (prop line-props)
+                (let ((ol (make-overlay (+ bol (nth 0 prop))
+                                        (+ bol (nth 1 prop))
+                                        nil 'front-advance nil)))
+                  (overlay-put ol 'evaporate t)
+                  (overlay-put ol 'face (nth 2 prop)))))))))))
+
+(defun diff-syntax-fontify-props (file text line-nb &optional no-init hunk-only)
+  "Get font-lock properties from the source code."
+  (unless no-init
+    (buffer-disable-undo)
+    (font-lock-mode -1)
+    (let ((enable-local-variables :safe) ;; to find `mode:'
+          (buffer-file-name file))
+      (set-auto-mode)
+      (generic-mode-find-file-hook)))
+
+  (let ((font-lock-defaults (or font-lock-defaults '(nil t)))
+        props beg end)
+    (goto-char (point-min))
+    (if hunk-only
+        (setq beg (point-min) end (point-max))
+      (forward-line (1- (nth 0 line-nb)))
+      ;; non-regexp looking-at to compare hunk text for verification
+      (if (search-forward text (+ (point) (length text)) t)
+          (setq beg (- (point) (length text)) end (point))
+        (goto-char (point-min))
+        (if (search-forward text nil t)
+            (setq beg (- (point) (length text)) end (point)))))
+
+    (when (and beg end)
+      (goto-char beg)
+      (when (text-property-not-all beg end 'fontified t)
+        (if file
+            ;; In a temporary or cached buffer
+            (save-excursion
+              (font-lock-fontify-region beg end)
+              (put-text-property beg end 'fontified t))
+          ;; In an existing buffer
+          (font-lock-ensure beg end)))
+
+      (while (< (point) end)
+        (let* ((bol (point))
+               (eol (line-end-position))
+               line-props
+               (searching t)
+               (from (point)) to
+               (val (get-text-property from 'face)))
+          (while searching
+            (setq to (next-single-property-change from 'face nil eol))
+            (when val (push (list (- from bol) (- to bol) val) line-props))
+            (setq val (get-text-property to 'face) from to)
+            (unless (< to eol) (setq searching nil)))
+          (when val (push (list from eol val) line-props))
+          (push (nreverse line-props) props))
+        (forward-line 1)))
+    (set-buffer-modified-p nil)
+    (nreverse props)))
+
+
 (defun diff--filter-substring (str)
   (when diff-font-lock-prettify
     ;; Strip the `display' properties added by diff-font-lock-prettify,

bug#33567: Syntactic fontification of diff hunks

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Date: Sat, 01 Dec 2018 23:55:40 +0200
> 
> For a long time after announcing this feature in
> https://lists.gnu.org/archive/html/emacs-devel/2018-08/msg00537.html
> I received requests in private mails asking when I'll submit a complete patch.
> I'm sorry, it took much time addressing all concerns raised in that thread,
> and testing in many possible scenarios.  Based on the feedback, I rewrote it
> several times, and now finally it's optimized to be fast and reliable.

Thank you for working on this.  A few comments below.

> +(defcustom diff-font-lock-syntax 'vc
> +  "If non-nil, diff hunk font-lock includes syntax highlighting.
> +If `vc', highlight syntax only in Diff buffers created by a version control
> +system that provides all necessary context for reliable highlighting.
> +If t, additionally try to get more context from existing files, or when
> +source files are not found, still try to highlight hunks without enough
> +context that sometimes might result in wrong fontification.
> +If `hunk-only', fontification is based on hunk alone, without full source.
> +This is the fastest, but less reliable."

I don't think one can understand what the feature does just by reading
the doc string.  I think something very basic is missing here, without
which the rest of the doc text cannot be unlocked.  Perhaps just
elaborating on what exactly "syntax highlighting" means in this
context would be enough.

Also, judging by my reading of the code, the description of what the
various non-nil values do is not entirely accurate, and might not be
what the user expects by reading the above description.

> +  :type '(choice (const :tag "Don't highlight syntax" nil)
> +                 (const :tag "Only under version control" vc)

The "under" part of "Under version control" seems to say something
very different from what the doc string says about this value.

> +                 (const :tag "Without full source or get it from files" t)))

This description is backwards, I think: you should start with "with
source files".  (But maybe I misunderstand the whole issue, see
above.)

> +                                      ;; Restore restore previous window configuration
> +                                      ;; because when vc-find-revision can't find a revision
> +                                      ;; (e.g. for /dev/null), it jumps to another window
> +                                      ;; using pop-to-buffer in vc-do-command when
> +                                      ;; the buffer name doesn't begin with a space char.

Nitpicking: can this comment please be refilled to not exceed "normal"
line width?

> +     ((not (eq diff-font-lock-syntax 'vc))
> +      (let ((file (car (diff-hunk-file-names old))))
> +        (if (and file (file-exists-p file))

This assumes that the file name is relative to the default-directory
of the buffer with the diffs, right?  How reasonable is such an
assumption for when browsing diffs?  Should we perhaps allow the user
to specify the directory of the sources?

Also, if the diffs are from Git, they begin with a/, b/, etc. dummy
directories, which usually don't exist in the file system.

Finally, please include the necessary documentation changes with the
final changeset.

bug#33567: Syntactic fontification of diff hunks

[Juri Linkov]

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

> I don't think one can understand what the feature does just by reading
> the doc string.  I think something very basic is missing here, without
> which the rest of the doc text cannot be unlocked.  Perhaps just
> elaborating on what exactly "syntax highlighting" means in this
> context would be enough.
>
> Also, judging by my reading of the code, the description of what the
> various non-nil values do is not entirely accurate, and might not be
> what the user expects by reading the above description.

I tried to explain this more thoughtfully in a new version of the docstring.

>> +                 (const :tag "Without full source or get it from files" t)))
>
> This description is backwards, I think: you should start with "with
> source files".  (But maybe I misunderstand the whole issue, see
> above.)

Fixed in a new version.  Also please note why `t' is not the default.
This is to avoid trying to read local files while a received patch
is displayed in the mail attachment.  I think to avoid such situations
when it will try to read random files, better to use the same method
as is used in diff buffers created by a VC command - it sets a special
variable `diff-vc-backend' that guarantees that diff buffer is created
with file paths relative to the process that created these buffers.
I propose for commands that compare files like diff, diff-backup,
dired-diff, dired-backup-diff also to set a similar variable e.g.
`diff-files' that will guarantee that file paths are valid to read.

>> +                                      ;; Restore restore previous window configuration
>> +                                      ;; because when vc-find-revision can't find a revision
>> +                                      ;; (e.g. for /dev/null), it jumps to another window
>> +                                      ;; using pop-to-buffer in vc-do-command when
>> +                                      ;; the buffer name doesn't begin with a space char.
>
> Nitpicking: can this comment please be refilled to not exceed "normal"
> line width?

Fixing the described problem will remove this comment,
but I have no idea how better to do this.  The problem is that
we need to provide own created buffer to the call to `vc-find-revision'.
Currently it has the following function signature:

  (defun vc-find-revision (file revision &optional backend)

But VC API in the comments in the beginning of vc.el
is documented with a different function signature:

  ;; * find-revision (file rev buffer)
  ;;
  ;;   Fetch revision REV of file FILE and put it into BUFFER.
  ;;   If REV is the empty string, fetch the head of the trunk.
  ;;   The implementation should pass the value of vc-checkout-switches
  ;;   to the backend command.

This means that to fix the problem we need the call as is documented
with the argument BUFFER, but the current implementation without
such argument doesn't correspond to the documentation.

BTW, while deciding what to do with this, could you please confirm
if I correctly fixed another problem in vc-find-revision-no-save.
Recently in bug#33319 I added this function but now discovered
a problem with encoding.  A vc process outputs lines to the buffer
with no-conversion, so in the patch below I added recode-region
to convert output to the buffer's encoding.  coding-system-for-write
that I removed was copied from vc-find-revision-save where is was
needed for write-region called from the macro with-temp-file,
but vc-find-revision-no-save doesn't write output to the file.

>> +     ((not (eq diff-font-lock-syntax 'vc))
>> +      (let ((file (car (diff-hunk-file-names old))))
>> +        (if (and file (file-exists-p file))
>
> This assumes that the file name is relative to the default-directory
> of the buffer with the diffs, right?  How reasonable is such an
> assumption for when browsing diffs?  Should we perhaps allow the user
> to specify the directory of the sources?

This assumption should be true for all cases when the diff buffer is created
using commands like dired-diff, dired-backup-diff, diff, diff-backup.

But when navigating a diff output saved to a file that was moved to
another directory, currently diff-mode asks for a directory interactively,
that is not possible to do for non-interactive fontification.

As a general solution is should be possible to specify the default
directory in the local variables at the first line of the diff files
as currently already is used in compilation/grep buffers like

-*- mode: diff-mode; default-directory: "..." -*-

> Also, if the diffs are from Git, they begin with a/, b/, etc. dummy
> directories, which usually don't exist in the file system.

This is not a problem because diff-find-file-name used in the patch
strips such a/, b/ prefixes to get the existing file name.


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

diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index de43544864..00ec1226d7 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -2008,8 +2008,7 @@ vc-find-revision-no-save
       (with-current-buffer filebuf
 	(let ((failed t))
 	  (unwind-protect
-	      (let ((coding-system-for-read 'no-conversion)
-		    (coding-system-for-write 'no-conversion))
+	      (let ((coding-system-for-read 'no-conversion))
 		(with-current-buffer (create-file-buffer filename)
                   (setq buffer-file-name filename)
 		  (let ((outbuf (current-buffer)))
@@ -2019,6 +2018,9 @@ vc-find-revision-no-save
 			(vc-call find-revision file revision outbuf))))
                   (goto-char (point-min))
                   (normal-mode)
+                  (recode-region (point-min) (point-max)
+                                 (car (detect-coding-region (point-min) (point-max)))
+                                 'no-conversion)
 	          (set-buffer-modified-p nil)
                   (setq buffer-read-only t))
 		(setq failed nil))
diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index 4adef02984..02421e2630 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -103,6 +103,31 @@ diff-font-lock-prettify
   :version "27.1"
   :type 'boolean)
 
+(defcustom diff-font-lock-syntax 'vc
+  "If non-nil, diff hunk's font-lock includes language syntax highlighting.
+This highlighting is the same as added by `font-lock-mode' when
+corresponding source files are visited from the diff buffer.
+In diff hunks syntax highlighting is added over diff own
+highlighted changes.
+
+If `vc', highlight syntax only in Diff buffers created by a version control
+system that provides all necessary context for reliable highlighting.
+For working revisions get highlighting according to the working
+copy of the file.
+
+If `hunk-only', fontification is based on hunk alone, without full source.
+It tries to highlight hunks without enough context that sometimes might result
+in wrong fontification.  This is the fastest option, but less reliable.
+
+If t, additionally to trying to use a version control system to get
+old revisions for fontification, also try to get fontification based
+on existing files, and on failure get fontification from hunk alone."
+  :version "27.1"
+  :type '(choice (const :tag "Don't highlight syntax" nil)
+                 (const :tag "Use version control" vc)
+                 (const :tag "Hunk-based only" hunk-only)
+                 (const :tag "Try everything including files" t)))
+
 (defvar diff-vc-backend nil
   "The VC backend that created the current Diff buffer, if any.")
 
@@ -406,6 +431,7 @@ diff-font-lock-keywords
      (1 font-lock-comment-delimiter-face)
      (2 font-lock-comment-face))
     ("^[^-=+*!<>#].*\n" (0 'diff-context))
+    (,#'diff--font-lock-syntax)
     (,#'diff--font-lock-prettify)
     (,#'diff--font-lock-refined)))
 
@@ -1348,6 +1374,7 @@ diff-next-error
 
 (defun diff--font-lock-cleanup ()
   (remove-overlays nil nil 'diff-mode 'fine)
+  (remove-overlays nil nil 'diff-mode 'syntax)
   (when font-lock-mode
     (make-local-variable 'font-lock-extra-managed-props)
     ;; Added when diff--font-lock-prettify is non-nil!
@@ -1748,7 +1775,7 @@ diff-find-source-location
                                 (vc-working-revision file)))))
 	   (buf (if revision
                     (let ((vc-find-revision-no-save t))
-                      (vc-find-revision file revision diff-vc-backend))
+                      (vc-find-revision (expand-file-name file) revision diff-vc-backend))
                   (find-file-noselect file))))
       ;; Update the user preference if he so wished.
       (when (> (prefix-numeric-value other-file) 8)
@@ -2316,6 +2343,197 @@ diff--font-lock-prettify
                              'display "")))))
   nil)
 
+;;; Syntax highlighting from font-lock
+
+(defun diff--font-lock-syntax (max)
+  "Syntax highlighting from font-lock."
+  (when diff-font-lock-syntax
+    (when (get-char-property (point) 'diff--font-lock-syntax)
+      (goto-char (next-single-char-property-change
+                  (point) 'diff--font-lock-syntax nil max)))
+    (let* ((min (point))
+           (beg (or (ignore-errors (diff-beginning-of-hunk))
+                    (ignore-errors (diff-hunk-next) (point))
+                    max)))
+      (while (< beg max)
+        (let ((end
+               (save-excursion (goto-char beg) (diff-end-of-hunk) (point))))
+          (if (< end min) (setq beg min))
+          (unless (or (< end beg)
+                      (get-char-property beg 'diff--font-lock-syntax))
+            (diff-syntax-fontify beg end)
+            (let ((ol (make-overlay beg end)))
+              (overlay-put ol 'diff--font-lock-syntax t)
+              (overlay-put ol 'diff-mode 'syntax)
+              (overlay-put ol 'evaporate t)
+              (overlay-put ol 'modification-hooks
+                           '(diff--font-lock-syntax--refresh))))
+          (goto-char (max beg end))
+          (setq beg (or (ignore-errors (diff-hunk-next) (point)) max))))))
+  nil)
+
+(defun diff--font-lock-syntax--refresh (ol _after _beg _end &optional _len)
+  (delete-overlay ol))
+
+(defun diff-syntax-fontify (start end)
+  (save-excursion
+    (diff-syntax-fontify-hunk start end t)
+    (diff-syntax-fontify-hunk start end nil)))
+
+(defvar diff-syntax-fontify-revisions (make-hash-table :test 'equal))
+
+(defun diff-syntax-fontify-hunk (beg end old)
+  "Highlight language syntax in diff hunks."
+  (remove-overlays beg end 'diff-mode 'syntax)
+  (goto-char beg)
+  (let* ((hunk (buffer-substring-no-properties beg end))
+         (text (or (ignore-errors (diff-hunk-text hunk (not old) nil)) ""))
+	 (line (if (looking-at "\\(?:\\*\\{15\\}.*\n\\)?[-@* ]*\\([0-9,]+\\)\\([ acd+]+\\([0-9,]+\\)\\)?")
+		   (if old (match-string 1)
+		     (if (match-end 3) (match-string 3) (match-string 1)))))
+         (line-nb (and line (string-match "\\([0-9]+\\),\\([0-9]+\\)" line)
+                       (list (string-to-number (match-string 1 line))
+                             (string-to-number (match-string 2 line)))))
+         props)
+    (cond
+     ((and diff-vc-backend (not (eq diff-font-lock-syntax 'hunk-only)))
+      (let* ((file (diff-find-file-name old t))
+             (revision (and file (if (not old) (nth 1 diff-vc-revisions)
+                                   (or (nth 0 diff-vc-revisions)
+                                       (vc-working-revision file))))))
+        (if file
+            (if (not revision)
+                ;; Get properties from the current working revision
+                (when (and (not old) (file-exists-p file))
+                  ;; Try to reuse an existing buffer
+                  (if (get-file-buffer (expand-file-name file))
+                      (with-current-buffer (get-file-buffer (expand-file-name file))
+                        (setq props (diff-syntax-fontify-props nil text line-nb t)))
+                    ;; Get properties from the file
+                    (with-temp-buffer
+                      (insert-file-contents file t)
+                      (setq props (diff-syntax-fontify-props file text line-nb)))))
+              ;; Get properties from a cached revision
+              (let* ((buffer-name (format " diff-syntax:%s.~%s~"
+                                          (expand-file-name file) revision))
+                     (buffer (gethash buffer-name diff-syntax-fontify-revisions))
+                     (no-init t))
+                (unless (and buffer (buffer-live-p buffer))
+                  (let* ((vc-find-revision-no-save t)
+                         (vc-buffer (save-window-excursion
+                                      ;; Restore restore previous window configuration
+                                      ;; because when vc-find-revision can't find a revision
+                                      ;; (e.g. for /dev/null), it jumps to another window
+                                      ;; using pop-to-buffer in vc-do-command when
+                                      ;; the buffer name doesn't begin with a space char.
+                                      (ignore-errors
+                                        (vc-find-revision (expand-file-name file)
+                                                          revision diff-vc-backend)))))
+                    (when vc-buffer
+                      (with-current-buffer (get-buffer-create buffer-name)
+                        (insert-buffer-substring-no-properties vc-buffer)
+                        (setq buffer (current-buffer) no-init nil))
+                      (puthash buffer-name buffer diff-syntax-fontify-revisions)
+                      (kill-buffer vc-buffer))))
+                (when buffer
+                  (with-current-buffer buffer
+                    (setq props (diff-syntax-fontify-props file text line-nb no-init))))))
+          ;; If file is unavailable, get properties from the hunk alone
+          (setq file (car (diff-hunk-file-names old)))
+          (with-temp-buffer
+            (insert text)
+            (setq props (diff-syntax-fontify-props file text line-nb nil t))))))
+     ((eq diff-font-lock-syntax 'hunk-only)
+      (let ((file (car (diff-hunk-file-names old))))
+        (with-temp-buffer
+          (insert text)
+          (setq props (diff-syntax-fontify-props file text line-nb nil t)))))
+     ((not (eq diff-font-lock-syntax 'vc))
+      (let ((file (car (diff-hunk-file-names old))))
+        (if (and file (file-exists-p file))
+            ;; Try to get full text from the file
+            (with-temp-buffer
+              (insert-file-contents file t)
+              (setq props (diff-syntax-fontify-props file text line-nb)))
+          ;; Otherwise, get properties from the hunk alone
+          (with-temp-buffer
+            (insert text)
+            (setq props (diff-syntax-fontify-props file text line-nb nil t)))))))
+
+    ;; Put properties over the hunk text
+    (when props
+      (goto-char beg)
+      (while (< (progn (forward-line 1) (point)) end)
+        (when (or (and (not old) (not (looking-at-p "[-<]")))
+                  (and      old  (not (looking-at-p "[+>]"))))
+          (if (and old (not (looking-at-p "[-<]")))
+              ;; Fontify context lines only from new source,
+              ;; don't refontify context lines from old source.
+              (pop props)
+            (let ((line-props (pop props))
+                  (bol (1+ (point))))
+              (dolist (prop line-props)
+                (let ((ol (make-overlay (+ bol (nth 0 prop))
+                                        (+ bol (nth 1 prop))
+                                        nil 'front-advance nil)))
+                  (overlay-put ol 'evaporate t)
+                  (overlay-put ol 'face (nth 2 prop)))))))))))
+
+(defun diff-syntax-fontify-props (file text line-nb &optional no-init hunk-only)
+  "Get font-lock properties from the source code."
+  (unless no-init
+    (buffer-disable-undo)
+    (font-lock-mode -1)
+    (let ((enable-local-variables :safe) ;; to find `mode:'
+          (buffer-file-name file))
+      (set-auto-mode)
+      (when (and (memq 'generic-mode-find-file-hook find-file-hook)
+                 (fboundp 'generic-mode-find-file-hook))
+        (generic-mode-find-file-hook))))
+
+  (let ((font-lock-defaults (or font-lock-defaults '(nil t)))
+        props beg end)
+    (goto-char (point-min))
+    (if hunk-only
+        (setq beg (point-min) end (point-max))
+      (forward-line (1- (nth 0 line-nb)))
+      ;; non-regexp looking-at to compare hunk text for verification
+      (if (search-forward text (+ (point) (length text)) t)
+          (setq beg (- (point) (length text)) end (point))
+        (goto-char (point-min))
+        (if (search-forward text nil t)
+            (setq beg (- (point) (length text)) end (point)))))
+
+    (when (and beg end)
+      (goto-char beg)
+      (when (text-property-not-all beg end 'fontified t)
+        (if file
+            ;; In a temporary or cached buffer
+            (save-excursion
+              (font-lock-fontify-region beg end)
+              (put-text-property beg end 'fontified t))
+          ;; In an existing buffer
+          (font-lock-ensure beg end)))
+
+      (while (< (point) end)
+        (let* ((bol (point))
+               (eol (line-end-position))
+               line-props
+               (searching t)
+               (from (point)) to
+               (val (get-text-property from 'face)))
+          (while searching
+            (setq to (next-single-property-change from 'face nil eol))
+            (when val (push (list (- from bol) (- to bol) val) line-props))
+            (setq val (get-text-property to 'face) from to)
+            (unless (< to eol) (setq searching nil)))
+          (when val (push (list from eol val) line-props))
+          (push (nreverse line-props) props))
+        (forward-line 1)))
+    (set-buffer-modified-p nil)
+    (nreverse props)))
+
+
 (defun diff--filter-substring (str)
   (when diff-font-lock-prettify
     ;; Strip the `display' properties added by diff-font-lock-prettify,

bug#33567: Syntactic fontification of diff hunks

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: 33567@debbugs.gnu.org
> Date: Mon, 03 Dec 2018 02:34:14 +0200
> 
> > Nitpicking: can this comment please be refilled to not exceed "normal"
> > line width?
> 
> Fixing the described problem will remove this comment,
> but I have no idea how better to do this.  The problem is that
> we need to provide own created buffer to the call to `vc-find-revision'.
> Currently it has the following function signature:
> 
>   (defun vc-find-revision (file revision &optional backend)
> 
> But VC API in the comments in the beginning of vc.el
> is documented with a different function signature:
> 
>   ;; * find-revision (file rev buffer)
>   ;;
>   ;;   Fetch revision REV of file FILE and put it into BUFFER.
>   ;;   If REV is the empty string, fetch the head of the trunk.
>   ;;   The implementation should pass the value of vc-checkout-switches
>   ;;   to the backend command.
> 
> This means that to fix the problem we need the call as is documented
> with the argument BUFFER, but the current implementation without
> such argument doesn't correspond to the documentation.

I'm not an expert on those details, sorry.  Perhaps someone else
(Dmitry? Martin?) could help you.

> BTW, while deciding what to do with this, could you please confirm
> if I correctly fixed another problem in vc-find-revision-no-save.
> Recently in bug#33319 I added this function but now discovered
> a problem with encoding.  A vc process outputs lines to the buffer
> with no-conversion, so in the patch below I added recode-region
> to convert output to the buffer's encoding.  coding-system-for-write
> that I removed was copied from vc-find-revision-save where is was
> needed for write-region called from the macro with-temp-file,
> but vc-find-revision-no-save doesn't write output to the file.

vc-find-revision disables encoding/decoding because it wants to
create an identical copy of the checked-out file, and doesn't want to
be tripped by encoding/decoding issues.  But in your case you don't
write the buffer to a file, so why do you need to bind
coding-system-for-read at all?  I say leave it unbound, and let Emacs
do its job decoding the text as usual.  Does that not work?

> >> +     ((not (eq diff-font-lock-syntax 'vc))
> >> +      (let ((file (car (diff-hunk-file-names old))))
> >> +        (if (and file (file-exists-p file))
> >
> > This assumes that the file name is relative to the default-directory
> > of the buffer with the diffs, right?  How reasonable is such an
> > assumption for when browsing diffs?  Should we perhaps allow the user
> > to specify the directory of the sources?
> 
> This assumption should be true for all cases when the diff buffer is created
> using commands like dired-diff, dired-backup-diff, diff, diff-backup.
> 
> But when navigating a diff output saved to a file that was moved to
> another directory, currently diff-mode asks for a directory interactively,
> that is not possible to do for non-interactive fontification.
> 
> As a general solution is should be possible to specify the default
> directory in the local variables at the first line of the diff files
> as currently already is used in compilation/grep buffers like
> 
> -*- mode: diff-mode; default-directory: "..." -*-

This is all fine, but I think we should document that files are
visited relative to default-directory of the buffer, so that users
could invoke "cd" to change that as needed.

> > Also, if the diffs are from Git, they begin with a/, b/, etc. dummy
> > directories, which usually don't exist in the file system.
> 
> This is not a problem because diff-find-file-name used in the patch
> strips such a/, b/ prefixes to get the existing file name.

Not in my testing, but maybe I tried in the wrong Emacs version.  Is
this feature new with Emacs 27?

> @@ -2008,8 +2008,7 @@ vc-find-revision-no-save
>        (with-current-buffer filebuf
>  	(let ((failed t))
>  	  (unwind-protect
> -	      (let ((coding-system-for-read 'no-conversion)
> -		    (coding-system-for-write 'no-conversion))
> +	      (let ((coding-system-for-read 'no-conversion))
>  		(with-current-buffer (create-file-buffer filename)
>                    (setq buffer-file-name filename)
>  		  (let ((outbuf (current-buffer)))
> @@ -2019,6 +2018,9 @@ vc-find-revision-no-save
>  			(vc-call find-revision file revision outbuf))))
>                    (goto-char (point-min))
>                    (normal-mode)
> +                  (recode-region (point-min) (point-max)
> +                                 (car (detect-coding-region (point-min) (point-max)))
> +                                 'no-conversion)

See above: I think this manual decoding is unnecessary.

> +(defcustom diff-font-lock-syntax 'vc
> +  "If non-nil, diff hunk's font-lock includes language syntax highlighting.
> +This highlighting is the same as added by `font-lock-mode' when
> +corresponding source files are visited from the diff buffer.

Thanks, this is much better than the original text, but there are
still unclear corners.  One such corner is the "visited from the diff
buffer" part -- what is its significance?  Can we just say "when the
corresponding source files are visited normally"?

> +In diff hunks syntax highlighting is added over diff own
> +highlighted changes.

What is the significance of the "In diff hunks" part here?  Apart of
diff hunks, we have just headers, where this feature is irrelevant,
right?

> +If `vc', highlight syntax only in Diff buffers created by a version control
> +system that provides all necessary context for reliable highlighting.

I would use "in Diff buffers created by VC commands" instead.  I would
also add this text (assuming it is correct):

  This value requires support from a VC backend to find the files
  being compared.

This should tell users that they could in principle set up things
manually even for buffers that were not created by VC commands.

Please also indicate that `vc' is the default.

> +For working revisions get highlighting according to the working
> +copy of the file.

I don't understand the significance of this comment.  If you want to
say that the produced highlighting might be wrong if the working
version has changed since it was compared, then let's say that
explicitly.

> +If t, additionally to trying to use a version control system to get
> +old revisions for fontification, also try to get fontification based
> +on existing files, and on failure get fontification from hunk alone."

What is the difference between using a VCS to get old revisions, and
using existing files?

Also, does it mean `vc' will not fall back to `hunk-only'?  Why not?

Thanks.

bug#33567: Syntactic fontification of diff hunks

[Juri Linkov]

>> BTW, while deciding what to do with this, could you please confirm
>> if I correctly fixed another problem in vc-find-revision-no-save.
>> Recently in bug#33319 I added this function but now discovered
>> a problem with encoding.  A vc process outputs lines to the buffer
>> with no-conversion, so in the patch below I added recode-region
>> to convert output to the buffer's encoding.  coding-system-for-write
>> that I removed was copied from vc-find-revision-save where is was
>> needed for write-region called from the macro with-temp-file,
>> but vc-find-revision-no-save doesn't write output to the file.
>
> vc-find-revision disables encoding/decoding because it wants to
> create an identical copy of the checked-out file, and doesn't want to
> be tripped by encoding/decoding issues.  But in your case you don't
> write the buffer to a file, so why do you need to bind
> coding-system-for-read at all?  I say leave it unbound, and let Emacs
> do its job decoding the text as usual.  Does that not work?

I tried to remove coding-system-for-read binding from
vc-find-revision-no-save, but it still fails to get the buffer
in the right encoding.  Then I discovered that vc-git-find-revision
and also some other VC backend API implementations of find-revision
bind coding-system-for-read too.  It seems that removing
coding-system-for-read from vc-git-find-revision will cause
a lot of breakage.

bug#33567: Syntactic fontification of diff hunks

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: 33567@debbugs.gnu.org
> Date: Tue, 04 Dec 2018 01:36:52 +0200
> 
> > vc-find-revision disables encoding/decoding because it wants to
> > create an identical copy of the checked-out file, and doesn't want to
> > be tripped by encoding/decoding issues.  But in your case you don't
> > write the buffer to a file, so why do you need to bind
> > coding-system-for-read at all?  I say leave it unbound, and let Emacs
> > do its job decoding the text as usual.  Does that not work?
> 
> I tried to remove coding-system-for-read binding from
> vc-find-revision-no-save, but it still fails to get the buffer
> in the right encoding.

What is "the right encoding", and what did Emacs think the encoding
was, when you didn't bind coding-system-for-read?  These details are
necessary to understand what exactly happens there and how to solve
it.

> Then I discovered that vc-git-find-revision and also some other VC
> backend API implementations of find-revision bind
> coding-system-for-read too.  It seems that removing
> coding-system-for-read from vc-git-find-revision will cause a lot of
> breakage.

How do you know vc-git-find-revision doesn't have a subtle bug as
well, e.g. when file names in the repository are encoded in some
non-trivial, non-UTF-8 encoding?

And anyway, we are not talking about changing vc-git-find-revision or
affecting it, we are talking about your vc-find-revision-no-save,
which does a different job.  For the latter, I'd prefer not to decode
by hand, as that might have subtle issues and will require much more
testing in all kinds of environments and OSes.  I prefer to rely on
the usual decoding machinery, which we know works well.

bug#33567: Syntactic fontification of diff hunks

[Juri Linkov]

>> > vc-find-revision disables encoding/decoding because it wants to
>> > create an identical copy of the checked-out file, and doesn't want to
>> > be tripped by encoding/decoding issues.  But in your case you don't
>> > write the buffer to a file, so why do you need to bind
>> > coding-system-for-read at all?  I say leave it unbound, and let Emacs
>> > do its job decoding the text as usual.  Does that not work?
>>
>> I tried to remove coding-system-for-read binding from
>> vc-find-revision-no-save, but it still fails to get the buffer
>> in the right encoding.
>
> What is "the right encoding",

By the right encoding I meant the same encoding that is detected
when write-region saves the file, e.g. when using the macro
with-temp-file in vc-find-revision-save.  I don't know how
write-region detects the encoding for the saved file, but we need
the same encoding for the buffer that is not saved to the file
in vc-find-revision-no-save.

> and what did Emacs think the encoding was, when you didn't bind
> coding-system-for-read?  These details are necessary to understand
> what exactly happens there and how to solve it.

vc-git-find-revision binds coding-system-for-read to `binary'.

>> Then I discovered that vc-git-find-revision and also some other VC
>> backend API implementations of find-revision bind
>> coding-system-for-read too.  It seems that removing
>> coding-system-for-read from vc-git-find-revision will cause a lot of
>> breakage.
>
> How do you know vc-git-find-revision doesn't have a subtle bug as
> well, e.g. when file names in the repository are encoded in some
> non-trivial, non-UTF-8 encoding?

This is why vc-git-find-revision does nothing with its output
when it binds coding-system-for-read to `binary',
and doesn't try to encode/decode the git output.

> And anyway, we are not talking about changing vc-git-find-revision or
> affecting it, we are talking about your vc-find-revision-no-save,
> which does a different job.  For the latter, I'd prefer not to decode
> by hand, as that might have subtle issues and will require much more
> testing in all kinds of environments and OSes.

vc-git-find-revision returns the output undecoded.  I don't know
other way to decode it to the default coding other than recode-region.

> I prefer to rely on the usual decoding machinery, which we know
> works well.

Maybe I missed a separate function that can use the decoding machinery
like is used in write-region, but without writing a buffer to the file?

bug#33567: Syntactic fontification of diff hunks

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: 33567@debbugs.gnu.org
> Date: Wed, 05 Dec 2018 01:16:48 +0200
> 
> >> I tried to remove coding-system-for-read binding from
> >> vc-find-revision-no-save, but it still fails to get the buffer
> >> in the right encoding.
> >
> > What is "the right encoding",
> 
> By the right encoding I meant the same encoding that is detected
> when write-region saves the file, e.g. when using the macro
> with-temp-file in vc-find-revision-save.  I don't know how
> write-region detects the encoding for the saved file, but we need
> the same encoding for the buffer that is not saved to the file
> in vc-find-revision-no-save.
> 
> > and what did Emacs think the encoding was, when you didn't bind
> > coding-system-for-read?  These details are necessary to understand
> > what exactly happens there and how to solve it.
> 
> vc-git-find-revision binds coding-system-for-read to `binary'.

I see that vc-hg-find-revision does the same.  Sigh.  I guess the
find-revision API was never meant to process the resulting buffer
normally.  My advice would be to reimplement your
vc-find-revision-no-save function differently, without trying to
piggy-back the fact that vc-find-revision inserts the contents into a
buffer.  That is, let the code write the contents to a temporary file,
like vc-find-revision does, then call insert-file-contents to re-read
that file normally.  It would be slightly less efficient, but I think
the result will be much simpler, so a net win.

If you still want to reuse the literal contents of the file, as
inserted by vc-git-find-revision etc., then you will have to duplicate
what insert-file-contents does internally.  I suggest to look at how
this is done in archive-set-buffer-as-visiting-file.

> > How do you know vc-git-find-revision doesn't have a subtle bug as
> > well, e.g. when file names in the repository are encoded in some
> > non-trivial, non-UTF-8 encoding?
> 
> This is why vc-git-find-revision does nothing with its output
> when it binds coding-system-for-read to `binary',
> and doesn't try to encode/decode the git output.

vc-git-find-revision does _something_ with Git's output: it uses the
file name returned by Git.  That file name could have a non-trivial
encoding.

> vc-git-find-revision returns the output undecoded.  I don't know
> other way to decode it to the default coding other than recode-region.

See above.

bug#33567: Syntactic fontification of diff hunks

[Juri Linkov]

>> vc-git-find-revision binds coding-system-for-read to `binary'.
>
> I see that vc-hg-find-revision does the same.  Sigh.  I guess the
> find-revision API was never meant to process the resulting buffer
> normally.  My advice would be to reimplement your
> vc-find-revision-no-save function differently, without trying to
> piggy-back the fact that vc-find-revision inserts the contents into a
> buffer.  That is, let the code write the contents to a temporary file,
> like vc-find-revision does, then call insert-file-contents to re-read
> that file normally.  It would be slightly less efficient, but I think
> the result will be much simpler, so a net win.

The whole purpose of creating vc-find-revision-no-save function was
to improve the performance of vc-find-revision-save to avoid the need
to write files.  It would significantly degrade performance of
diff syntax fontification if it will write files for every hunk.

> If you still want to reuse the literal contents of the file, as
> inserted by vc-git-find-revision etc., then you will have to duplicate
> what insert-file-contents does internally.  I suggest to look at how
> this is done in archive-set-buffer-as-visiting-file.

I see it does something like I was trying to do.  I will use it
when failing to use the third possible solution I proposed below.

>> > How do you know vc-git-find-revision doesn't have a subtle bug as
>> > well, e.g. when file names in the repository are encoded in some
>> > non-trivial, non-UTF-8 encoding?
>>
>> This is why vc-git-find-revision does nothing with its output
>> when it binds coding-system-for-read to `binary',
>> and doesn't try to encode/decode the git output.
>
> vc-git-find-revision does _something_ with Git's output: it uses the
> file name returned by Git.  That file name could have a non-trivial
> encoding.

I'm thinking about overriding coding in vc-git-find-revision like

diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index f317400530..e5f44524df 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -838,8 +838,8 @@ vc-git-checkin
 
 (defun vc-git-find-revision (file rev buffer)
   (let* (process-file-side-effects
-	 (coding-system-for-read 'binary)
-	 (coding-system-for-write 'binary)
+	 (coding-system-for-read (or coding-system-for-read 'binary))
+	 (coding-system-for-write (or coding-system-for-write 'binary))
 	 (fullname
 	  (let ((fn (vc-git--run-command-string
 		     file "ls-files" "-z" "--full-name" "--")))

then a caller function could set its own value of this dynamic binding.
But I haven't tested yet if it works with UTF-8 file names.

bug#33567: Syntactic fontification of diff hunks

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: 33567@debbugs.gnu.org
> Date: Thu, 06 Dec 2018 01:25:46 +0200
> 
> I'm thinking about overriding coding in vc-git-find-revision like
> 
> diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
> index f317400530..e5f44524df 100644
> --- a/lisp/vc/vc-git.el
> +++ b/lisp/vc/vc-git.el
> @@ -838,8 +838,8 @@ vc-git-checkin
>  
>  (defun vc-git-find-revision (file rev buffer)
>    (let* (process-file-side-effects
> -	 (coding-system-for-read 'binary)
> -	 (coding-system-for-write 'binary)
> +	 (coding-system-for-read (or coding-system-for-read 'binary))
> +	 (coding-system-for-write (or coding-system-for-write 'binary))
>  	 (fullname
>  	  (let ((fn (vc-git--run-command-string
>  		     file "ls-files" "-z" "--full-name" "--")))
> 
> then a caller function could set its own value of this dynamic binding.
> But I haven't tested yet if it works with UTF-8 file names.

This is something that we might want to do anyway (it is IMO rude to
unconditionally bind these symbols), but I'm not sure it helps you,
because the level you work on is not vc-git.  The right value to bind
coding-system-for-read here is vc-git-log-output-coding-system, but
that is a Git specific value, whereas you are trying to provide a
VCS-independent feature, which should work with any supported VCS.

bug#33567: Syntactic fontification of diff hunks

[Juri Linkov]

> If you still want to reuse the literal contents of the file, as
> inserted by vc-git-find-revision etc., then you will have to duplicate
> what insert-file-contents does internally.  I suggest to look at how
> this is done in archive-set-buffer-as-visiting-file.

I looked at archive-set-buffer-as-visiting-file, and it seems it could
be simplified based on the assumption that the backend inserts output
in the binary coding.  I tried and this fixes the problem:

diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index 5ff9f4d5be..c980369fa9 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -2042,6 +2042,7 @@ vc-find-revision-no-save
 		      (if backend
 			  (vc-call-backend backend 'find-revision file revision outbuf)
 			(vc-call find-revision file revision outbuf))))
+                  (recode-region (point-min) (point-max) buffer-file-coding-system 'binary)
                   (goto-char (point-min))
                   (if buffer (let ((buffer-file-name file)) (normal-mode)) (normal-mode))
 	          (set-buffer-modified-p nil)

bug#33567: Syntactic fontification of diff hunks

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: 33567@debbugs.gnu.org
> Date: Tue, 11 Dec 2018 02:38:18 +0200
> 
> > If you still want to reuse the literal contents of the file, as
> > inserted by vc-git-find-revision etc., then you will have to duplicate
> > what insert-file-contents does internally.  I suggest to look at how
> > this is done in archive-set-buffer-as-visiting-file.
> 
> I looked at archive-set-buffer-as-visiting-file, and it seems it could
> be simplified based on the assumption that the backend inserts output
> in the binary coding.

"Binary coding" means what we have in the buffer is exactly what we
had on disk.  IOW, we have there the contents of the file in its
original encoding, and so decoding it correctly needs to use the same
facilities we normally use when visiting a file with the likes of
find-file.  archive-set-buffer-as-visiting-file solves precisely the
same problem.

> I tried and this fixes the problem:
> 
> diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
> index 5ff9f4d5be..c980369fa9 100644
> --- a/lisp/vc/vc.el
> +++ b/lisp/vc/vc.el
> @@ -2042,6 +2042,7 @@ vc-find-revision-no-save
>  		      (if backend
>  			  (vc-call-backend backend 'find-revision file revision outbuf)
>  			(vc-call find-revision file revision outbuf))))
> +                  (recode-region (point-min) (point-max) buffer-file-coding-system 'binary)

Where does the value of buffer-file-coding-system come from here?
Isn't that just (default-value 'buffer-file-coding-system)?  If so,
you were just lucky that it worked for you; in general, if the
encoding of the file is different from your locale-derived defaults,
the above won't DTRT.

In any case, really don't think recode-region is TRT in this case, for
several reasons:

 . recode-region is a command, so it wastes cycles checking the
   argument coding-system, which is entirely unnecessary in this case
 . it narrows the buffer, something that again is a waste of cycles
   for your case
 . it wastes even more cycles for "encoding" with 'binary', which in
   this case is a no-op, since the text was not decoded on reading
 . it doesn't use the following facilities for determining the right
   encoding, where you use buffer-file-coding-system:
    - auto-coding-function, which is where we detect the 'coding:'
      cookies in the first line and in the local vars, and use the
      data in auto-coding-alist and auto-coding-regexp-alist, and also
      call auto-coding-functions if needed
    - find-operation-coding-system by file name, which uses the data
      in file-coding-system-alist to determine the appropriate
      encoding given the file's name

The hard problem here is to determine what coding-system to use for
decoding a region that was inserted without any conversions; once the
encoding is determined, the rest boils down to calling
decode-coding-region with that encoding.  The method used by
archive-set-buffer-as-visiting-file solves that very problem, whereas
recode-region does not, because it is a command that relies on the
caller to specify the encoding, and is otherwise nothing more than a
thin wrapper around decode-coding-region.

If you need further help understanding what
archive-set-buffer-as-visiting-file does, and which parts might not be
relevant to the function you are writing, please ask specific
questions and I will gladly help as much as I can.  But recode-region
is IMO not the right tool for this job.

Thanks.

bug#33567: Syntactic fontification of diff hunks

[Juri Linkov]

>  . it doesn't use the following facilities for determining the right
>    encoding, where you use buffer-file-coding-system:
>     - auto-coding-function, which is where we detect the 'coding:'
>       cookies in the first line and in the local vars, and use the
>       data in auto-coding-alist and auto-coding-regexp-alist, and also
>       call auto-coding-functions if needed
>     - find-operation-coding-system by file name, which uses the data
>       in file-coding-system-alist to determine the appropriate
>       encoding given the file's name
>
> The hard problem here is to determine what coding-system to use for
> decoding a region that was inserted without any conversions; once the
> encoding is determined, the rest boils down to calling
> decode-coding-region with that encoding.  The method used by
> archive-set-buffer-as-visiting-file solves that very problem, whereas
> recode-region does not, because it is a command that relies on the
> caller to specify the encoding, and is otherwise nothing more than a
> thin wrapper around decode-coding-region.

Thanks for the explanation.  I explored more on this subject, and found
the most suitable existing function: `decode-coding-inserted-region'.
I tested it with different codings and everything works well:

diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index 5ff9f4d5be..127661a039 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -2042,6 +2042,8 @@ vc-find-revision-no-save
 		      (if backend
 			  (vc-call-backend backend 'find-revision file revision outbuf)
 			(vc-call find-revision file revision outbuf))))
+                  (decode-coding-inserted-region (point-min) (point-max) file)
+                  (after-insert-file-set-coding (- (point-max) (point-min)))
                   (goto-char (point-min))
                   (if buffer (let ((buffer-file-name file)) (normal-mode)) (normal-mode))
 	          (set-buffer-modified-p nil)

bug#33567: Syntactic fontification of diff hunks

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: 33567@debbugs.gnu.org
> Date: Wed, 12 Dec 2018 02:28:08 +0200
> 
> Thanks for the explanation.  I explored more on this subject, and found
> the most suitable existing function: `decode-coding-inserted-region'.
> I tested it with different codings and everything works well:

Thanks, this LGTM.

bug#33567: Syntactic fontification of diff hunks

[Juri Linkov]

>> As a general solution is should be possible to specify the default
>> directory in the local variables at the first line of the diff files
>> as currently already is used in compilation/grep buffers like
>> 
>> -*- mode: diff-mode; default-directory: "..." -*-
>
> This is all fine, but I think we should document that files are
> visited relative to default-directory of the buffer, so that users
> could invoke "cd" to change that as needed.

For more safety, I propose to set a new buffer-local variable
`diff-default-directory' by such commands as diff, diff-backup,
dired-diff, dired-backup-diff.  The existence of such variable
should guarantee that the referenced files really exist.
This variable will be like `diff-vc-backend' that says that
the diff-mode buffer is created by the VCS command.
Then anyone who want to visit a diff file in another directory,
could add it to the first line:

-*- mode: diff-mode; diff-default-directory: "..." -*-

>> > Also, if the diffs are from Git, they begin with a/, b/, etc. dummy
>> > directories, which usually don't exist in the file system.
>> 
>> This is not a problem because diff-find-file-name used in the patch
>> strips such a/, b/ prefixes to get the existing file name.
>
> Not in my testing, but maybe I tried in the wrong Emacs version.  Is
> this feature new with Emacs 27?

For testing better try to eval e.g. `(diff-find-file-name nil t)'
on a hunk in a diff-mode buffer created by git.

>> +(defcustom diff-font-lock-syntax 'vc
>> +  "If non-nil, diff hunk's font-lock includes language syntax highlighting.
>> +This highlighting is the same as added by `font-lock-mode' when
>> +corresponding source files are visited from the diff buffer.
>
> Thanks, this is much better than the original text, but there are
> still unclear corners.  One such corner is the "visited from the diff
> buffer" part -- what is its significance?  Can we just say "when the
> corresponding source files are visited normally"?

Changed, will send a patch with more changes later.

>> +In diff hunks syntax highlighting is added over diff own
>> +highlighted changes.
>
> What is the significance of the "In diff hunks" part here?  Apart of
> diff hunks, we have just headers, where this feature is irrelevant,
> right?

Removed "In diff hunks".

>> +If `vc', highlight syntax only in Diff buffers created by a version control
>> +system that provides all necessary context for reliable highlighting.
>
> I would use "in Diff buffers created by VC commands" instead.  I would
> also add this text (assuming it is correct):
>
>   This value requires support from a VC backend to find the files
>   being compared.
>
> This should tell users that they could in principle set up things
> manually even for buffers that were not created by VC commands.

Changed.

> Please also indicate that `vc' is the default.

After adding another safe option that uses `diff-default-directory',
we could combine it with `vc' for the safe default.

>> +For working revisions get highlighting according to the working
>> +copy of the file.
>
> I don't understand the significance of this comment.  If you want to
> say that the produced highlighting might be wrong if the working
> version has changed since it was compared, then let's say that
> explicitly.

This means that working revisions can't be extracted from the repository.
Until committed, they reside in files that are visited with find-file.

>> +If t, additionally to trying to use a version control system to get
>> +old revisions for fontification, also try to get fontification based
>> +on existing files, and on failure get fontification from hunk alone."
>
> What is the difference between using a VCS to get old revisions, and
> using existing files?

This means that when a diff-mode buffer is not created by a VCS,
then it tries to read files with find-file.

> Also, does it mean `vc' will not fall back to `hunk-only'?  Why not?

Actually, it already falls back to `hunk-only', this is what
"on failure get fontification from hunk alone." tries to say.

bug#33567: Syntactic fontification of diff hunks

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: 33567@debbugs.gnu.org
> Date: Tue, 04 Dec 2018 01:59:07 +0200
> 
> >> As a general solution is should be possible to specify the default
> >> directory in the local variables at the first line of the diff files
> >> as currently already is used in compilation/grep buffers like
> >> 
> >> -*- mode: diff-mode; default-directory: "..." -*-
> >
> > This is all fine, but I think we should document that files are
> > visited relative to default-directory of the buffer, so that users
> > could invoke "cd" to change that as needed.
> 
> For more safety, I propose to set a new buffer-local variable
> `diff-default-directory' by such commands as diff, diff-backup,
> dired-diff, dired-backup-diff.  The existence of such variable
> should guarantee that the referenced files really exist.
> This variable will be like `diff-vc-backend' that says that
> the diff-mode buffer is created by the VCS command.
> Then anyone who want to visit a diff file in another directory,
> could add it to the first line:
> 
> -*- mode: diff-mode; diff-default-directory: "..." -*-

I'm not sure this is a step in the right direction.  What is the
advantage of having a separate variable?  How is it "safer"?

The advantage of default-directory is that it is well-known, and the
command to change it, 'cd', is easier to type and invoke than setting
diff-default-directory manually.

> 
> >> > Also, if the diffs are from Git, they begin with a/, b/, etc. dummy
> >> > directories, which usually don't exist in the file system.
> >> 
> >> This is not a problem because diff-find-file-name used in the patch
> >> strips such a/, b/ prefixes to get the existing file name.
> >
> > Not in my testing, but maybe I tried in the wrong Emacs version.  Is
> > this feature new with Emacs 27?
> 
> For testing better try to eval e.g. `(diff-find-file-name nil t)'
> on a hunk in a diff-mode buffer created by git.

I did, but I guess this must be done inside the repository to work,
does it?  If I put the output of "git diff" on a file in some
arbitrary directory, then visit that file and evaluate
(diff-find-file-name nil t), I get nil.

> >> +For working revisions get highlighting according to the working
> >> +copy of the file.
> >
> > I don't understand the significance of this comment.  If you want to
> > say that the produced highlighting might be wrong if the working
> > version has changed since it was compared, then let's say that
> > explicitly.
> 
> This means that working revisions can't be extracted from the repository.
> Until committed, they reside in files that are visited with find-file.

We need to describe the implications of that to the users.  Does the
following text capture the issue?

  For diffs against the working-tree version of a file, the
  highlighting is based on the current file contents, which could be
  different from the contents when the diffs were taken.  In such
  cases, the produced highlighting might be wrong.

> >> +If t, additionally to trying to use a version control system to get
> >> +old revisions for fontification, also try to get fontification based
> >> +on existing files, and on failure get fontification from hunk alone."
> >
> > What is the difference between using a VCS to get old revisions, and
> > using existing files?
> 
> This means that when a diff-mode buffer is not created by a VCS,
> then it tries to read files with find-file.

If so, I suggest the following wording:

  If t, try to infer fontification from the compared files, if they
  exist in the filesystem, when accessing their contents via VC
  fails.

> > Also, does it mean `vc' will not fall back to `hunk-only'?  Why not?
> 
> Actually, it already falls back to `hunk-only', this is what
> "on failure get fontification from hunk alone." tries to say.

There's no such text in the description of 'vc', only in the
description of t, which is why I asked.

bug#33567: Syntactic fontification of diff hunks

[Juri Linkov]

>> For more safety, I propose to set a new buffer-local variable
>> `diff-default-directory' by such commands as diff, diff-backup,
>> dired-diff, dired-backup-diff.  The existence of such variable
>> should guarantee that the referenced files really exist.
>> This variable will be like `diff-vc-backend' that says that
>> the diff-mode buffer is created by the VCS command.
>> Then anyone who want to visit a diff file in another directory,
>> could add it to the first line:
>>
>> -*- mode: diff-mode; diff-default-directory: "..." -*-
>
> I'm not sure this is a step in the right direction.  What is the
> advantage of having a separate variable?  How is it "safer"?

When this special variable is set by a diff command, it's safe to assume
that the files referenced from the diff buffer really exist, so it's
safe to try reading them.  I don't want a patch in a mail attachment
to try reading files mentioned in the patch attachment.

>> >> > Also, if the diffs are from Git, they begin with a/, b/, etc. dummy
>> >> > directories, which usually don't exist in the file system.
>> >>
>> >> This is not a problem because diff-find-file-name used in the patch
>> >> strips such a/, b/ prefixes to get the existing file name.
>> >
>> > Not in my testing, but maybe I tried in the wrong Emacs version.  Is
>> > this feature new with Emacs 27?
>>
>> For testing better try to eval e.g. `(diff-find-file-name nil t)'
>> on a hunk in a diff-mode buffer created by git.
>
> I did, but I guess this must be done inside the repository to work,
> does it?  If I put the output of "git diff" on a file in some
> arbitrary directory, then visit that file and evaluate
> (diff-find-file-name nil t), I get nil.

Yes, it finds only the existing files inside the repository.

>> >> +For working revisions get highlighting according to the working
>> >> +copy of the file.
>> >
>> > I don't understand the significance of this comment.  If you want to
>> > say that the produced highlighting might be wrong if the working
>> > version has changed since it was compared, then let's say that
>> > explicitly.
>>
>> This means that working revisions can't be extracted from the repository.
>> Until committed, they reside in files that are visited with find-file.
>
> We need to describe the implications of that to the users.  Does the
> following text capture the issue?
>
>   For diffs against the working-tree version of a file, the
>   highlighting is based on the current file contents, which could be
>   different from the contents when the diffs were taken.  In such
>   cases, the produced highlighting might be wrong.

Such problem is very rare because highlighting is added usually
immediately after creating a diff.  When the file contents changes,
there is no highlighting at all - it verifies if text of the hunk
exist in the file, so highlighting never is wrong.

>> >> +If t, additionally to trying to use a version control system to get
>> >> +old revisions for fontification, also try to get fontification based
>> >> +on existing files, and on failure get fontification from hunk alone."
>> >
>> > What is the difference between using a VCS to get old revisions, and
>> > using existing files?
>>
>> This means that when a diff-mode buffer is not created by a VCS,
>> then it tries to read files with find-file.
>
> If so, I suggest the following wording:
>
>   If t, try to infer fontification from the compared files, if they
>   exist in the filesystem, when accessing their contents via VC
>   fails.

Will add in the final patch.

>> > Also, does it mean `vc' will not fall back to `hunk-only'?  Why not?
>>
>> Actually, it already falls back to `hunk-only', this is what
>> "on failure get fontification from hunk alone." tries to say.
>
> There's no such text in the description of 'vc', only in the
> description of t, which is why I asked.

Maybe then better to add text common for all cases, e.g.

"If some method fails, get fontification from hunk alone."

bug#33567: Syntactic fontification of diff hunks

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: 33567@debbugs.gnu.org
> Date: Wed, 05 Dec 2018 01:28:45 +0200
> 
> >> For more safety, I propose to set a new buffer-local variable
> >> `diff-default-directory' by such commands as diff, diff-backup,
> >> dired-diff, dired-backup-diff.  The existence of such variable
> >> should guarantee that the referenced files really exist.
> >> This variable will be like `diff-vc-backend' that says that
> >> the diff-mode buffer is created by the VCS command.
> >> Then anyone who want to visit a diff file in another directory,
> >> could add it to the first line:
> >>
> >> -*- mode: diff-mode; diff-default-directory: "..." -*-
> >
> > I'm not sure this is a step in the right direction.  What is the
> > advantage of having a separate variable?  How is it "safer"?
> 
> When this special variable is set by a diff command, it's safe to assume
> that the files referenced from the diff buffer really exist, so it's
> safe to try reading them.  I don't want a patch in a mail attachment
> to try reading files mentioned in the patch attachment.

I think you lean heavily towards the use case where the diffs were
produced by some Emacs command.  Whereas what I have in mind is the
use case where the diffs came from some other source, like manual
invocation of shell commands.  I'm saying that in the absence of
diff-default-directory, using default-directory and 'cd' will be much
easier for users.

> >> This means that working revisions can't be extracted from the repository.
> >> Until committed, they reside in files that are visited with find-file.
> >
> > We need to describe the implications of that to the users.  Does the
> > following text capture the issue?
> >
> >   For diffs against the working-tree version of a file, the
> >   highlighting is based on the current file contents, which could be
> >   different from the contents when the diffs were taken.  In such
> >   cases, the produced highlighting might be wrong.
> 
> Such problem is very rare because highlighting is added usually
> immediately after creating a diff.  When the file contents changes,
> there is no highlighting at all - it verifies if text of the hunk
> exist in the file, so highlighting never is wrong.

The use case I have in mind is that some time passes between the
generation of the diffs and the time the diffs are visited and
fontified.  During that time, the working revision could have been
changed.  Isn't that what this issue is about?  If so, we should
explain why in that case, rare as it could be, the fontification might
be wrong.

> >> > Also, does it mean `vc' will not fall back to `hunk-only'?  Why not?
> >>
> >> Actually, it already falls back to `hunk-only', this is what
> >> "on failure get fontification from hunk alone." tries to say.
> >
> > There's no such text in the description of 'vc', only in the
> > description of t, which is why I asked.
> 
> Maybe then better to add text common for all cases, e.g.
> 
> "If some method fails, get fontification from hunk alone."

That'd be fine as well.

Thanks.

bug#33567: Syntactic fontification of diff hunks

[Juri Linkov]

>> When this special variable is set by a diff command, it's safe to assume
>> that the files referenced from the diff buffer really exist, so it's
>> safe to try reading them.  I don't want a patch in a mail attachment
>> to try reading files mentioned in the patch attachment.
>
> I think you lean heavily towards the use case where the diffs were
> produced by some Emacs command.  Whereas what I have in mind is the
> use case where the diffs came from some other source, like manual
> invocation of shell commands.  I'm saying that in the absence of
> diff-default-directory, using default-directory and 'cd' will be much
> easier for users.

Users still can use all diff-mode commands to visit source files from them.
Only language syntax fontification in diffs will fall back to hunk-only
when a diff buffer is not created by a diff command.  This is to avoid all
possible dangers of automatically visiting files in arbitrary diff buffers.

> The use case I have in mind is that some time passes between the
> generation of the diffs and the time the diffs are visited and
> fontified.  During that time, the working revision could have been
> changed.  Isn't that what this issue is about?  If so, we should
> explain why in that case, rare as it could be, the fontification might
> be wrong.

Fontification can't be wrong because code verifies if hunks from diff
still exist in the updated file, and doesn't highlight changed hunks.

bug#33567: Syntactic fontification of diff hunks

[Juri Linkov]

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

> Users still can use all diff-mode commands to visit source files from them.
> Only language syntax fontification in diffs will fall back to hunk-only
> when a diff buffer is not created by a diff command.  This is to avoid all
> possible dangers of automatically visiting files in arbitrary diff buffers.

Due to the new variable `diff-default-directory', we can simplify the
customization options of `diff-font-lock-syntax'.  The default `t' is
the safest and the most reliable: it extracts revisions from VC when
the Diff buffer is created by a VC command, and extracts files from the
file system only in case the Diff buffer is created by a file-based
Diff command that sets the new variable `diff-default-directory'.

Here is a more less final version that I tested on many different diffs:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: diff-font-lock-syntax.3.patch --]
[-- Type: text/x-diff, Size: 12671 bytes --]

diff --git a/etc/NEWS b/etc/NEWS
index decc5e3954..1024e9e9f5 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -273,6 +273,8 @@ To disable it, set the new defcustom 'diff-font-lock-refine' to nil.
 *** File headers can be shortened, mimicking Magit's diff format
 To enable it, set the new defcustom 'diff-font-lock-prettify to t.
 
+*** Source language syntax is highlighted in diff hunks when 'diff-font-lock-syntax' is t.
+
 ** Browse-url
 
 *** The function 'browse-url-emacs' can now visit a URL in selected window.
diff --git a/doc/emacs/files.texi b/doc/emacs/files.texi
index b47be51e24..07f311c24e 100644
--- a/doc/emacs/files.texi
+++ b/doc/emacs/files.texi
@@ -1617,6 +1617,9 @@ Diff Mode
 modify the original (``old'') source files rather than the patched
 (``new'') source files.
 
+@vindex diff-font-lock-syntax
+  If non-nil, diff hunk font-lock includes source language syntax highlighting.
+
 @node Copying and Naming
 @section Copying, Naming and Renaming Files
 
diff --git a/lisp/vc/diff.el b/lisp/vc/diff.el
index ac94586cac..93fa54b1fb 100644
--- a/lisp/vc/diff.el
+++ b/lisp/vc/diff.el
@@ -165,6 +167,7 @@ diff-no-select
            (lambda (_ignore-auto _noconfirm)
              (diff-no-select old new switches no-async (current-buffer))))
       (setq default-directory thisdir)
+      (set (make-local-variable 'diff-default-directory) default-directory)
       (let ((inhibit-read-only t))
 	(insert command "\n"))
       (if (and (not no-async) (fboundp 'make-process))
diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index 4adef02984..99ddb19f87 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -103,12 +104,41 @@ diff-font-lock-prettify
   :version "27.1"
   :type 'boolean)
 
+(defcustom diff-font-lock-syntax t
+  "If non-nil, diff hunk font-lock includes source language syntax highlighting.
+This highlighting is the same as added by `font-lock-mode'
+when corresponding source files are visited normally.
+Syntax highlighting is added over diff own highlighted changes.
+
+If t, the default, highlight syntax only in Diff buffers created by Diff
+commands that compare files or by VC commands that compare revisions.
+These provide all necessary context for reliable highlighting.  This value
+requires support from a VC backend to find the files being compared.
+For diffs against the working-tree version of a file, the highlighting is
+based on the current file contents.  File-based fontification tries to
+infer fontification from the compared files.
+
+If some method fails, get fontification from hunk alone if the value is
+`hunk-also'.
+
+If `hunk-only', fontification is based on hunk alone, without full source.
+It tries to highlight hunks without enough context that sometimes might result
+in wrong fontification.  This is the fastest option, but less reliable."
+  :version "27.1"
+  :type '(choice (const :tag "Don't highlight syntax" nil)
+                 (const :tag "Hunk-based also" hunk-also)
+                 (const :tag "Hunk-based only" hunk-only)
+                 (const :tag "Highlight syntax" t)))
+
 (defvar diff-vc-backend nil
   "The VC backend that created the current Diff buffer, if any.")
 
 (defvar diff-vc-revisions nil
   "The VC revisions compared in the current Diff buffer, if any.")
 
+(defvar diff-default-directory nil
+  "The default directory where the current Diff buffer was created.")
+
 (defvar diff-outline-regexp
   "\\([*+][*+][*+] [^0-9]\\|@@ ...\\|\\*\\*\\* [0-9].\\|--- [0-9]..\\)")
 
@@ -406,6 +436,7 @@ diff-font-lock-keywords
      (1 font-lock-comment-delimiter-face)
      (2 font-lock-comment-face))
     ("^[^-=+*!<>#].*\n" (0 'diff-context))
+    (,#'diff--font-lock-syntax)
     (,#'diff--font-lock-prettify)
     (,#'diff--font-lock-refined)))
 
@@ -2316,6 +2348,189 @@ diff--font-lock-prettify
                              'display "")))))
   nil)
 
+;;; Syntax highlighting from font-lock
+
+(defun diff--font-lock-syntax (max)
+  "Syntax highlighting from font-lock."
+  (when diff-font-lock-syntax
+    (when (get-char-property (point) 'diff--font-lock-syntax)
+      (goto-char (next-single-char-property-change
+                  (point) 'diff--font-lock-syntax nil max)))
+    (let* ((min (point))
+           (beg (or (ignore-errors (diff-beginning-of-hunk))
+                    (ignore-errors (diff-hunk-next) (point))
+                    max)))
+      (while (< beg max)
+        (let ((end
+               (save-excursion (goto-char beg) (diff-end-of-hunk) (point))))
+          (if (< end min) (setq beg min))
+          (unless (or (< end beg)
+                      (get-char-property beg 'diff--font-lock-syntax))
+            (diff-syntax-fontify beg end)
+            (let ((ol (make-overlay beg end)))
+              (overlay-put ol 'diff--font-lock-syntax t)
+              (overlay-put ol 'diff-mode 'syntax)
+              (overlay-put ol 'evaporate t)
+              (overlay-put ol 'modification-hooks
+                           '(diff--font-lock-syntax--refresh))))
+          (goto-char (max beg end))
+          (setq beg (or (ignore-errors (diff-hunk-next) (point)) max))))))
+  nil)
+
+(defun diff--font-lock-syntax--refresh (ol _after _beg _end &optional _len)
+  (delete-overlay ol))
+
+(defun diff-syntax-fontify (start end)
+  (save-excursion
+    (diff-syntax-fontify-hunk start end t)
+    (diff-syntax-fontify-hunk start end nil)))
+
+(defvar diff-syntax-fontify-revisions (make-hash-table :test 'equal))
+
+(defun diff-syntax-fontify-hunk (beg end old)
+  "Highlight source language syntax in diff hunks."
+  (remove-overlays beg end 'diff-mode 'syntax)
+  (goto-char beg)
+  (let* ((hunk (buffer-substring-no-properties beg end))
+         (text (or (ignore-errors (diff-hunk-text hunk (not old) nil)) ""))
+	 (line (if (looking-at "\\(?:\\*\\{15\\}.*\n\\)?[-@* ]*\\([0-9,]+\\)\\([ acd+]+\\([0-9,]+\\)\\)?")
+		   (if old (match-string 1)
+		     (if (match-end 3) (match-string 3) (match-string 1)))))
+         (line-nb (and line (string-match "\\([0-9]+\\),\\([0-9]+\\)" line)
+                       (list (string-to-number (match-string 1 line))
+                             (string-to-number (match-string 2 line)))))
+         props)
+    (cond
+     ((and diff-vc-backend (not (eq diff-font-lock-syntax 'hunk-only)))
+      (let* ((file (diff-find-file-name old t))
+             (revision (and file (if (not old) (nth 1 diff-vc-revisions)
+                                   (or (nth 0 diff-vc-revisions)
+                                       (vc-working-revision file))))))
+        (if file
+            (if (not revision)
+                ;; Get properties from the current working revision
+                (when (and (not old) (file-exists-p file) (file-regular-p file))
+                  ;; Try to reuse an existing buffer
+                  (if (get-file-buffer (expand-file-name file))
+                      (with-current-buffer (get-file-buffer (expand-file-name file))
+                        (setq props (diff-syntax-fontify-props nil text line-nb t)))
+                    ;; Get properties from the file
+                    (with-temp-buffer
+                      (insert-file-contents file t)
+                      (setq props (diff-syntax-fontify-props file text line-nb)))))
+              ;; Get properties from a cached revision
+              (let* ((buffer-name (format " diff-syntax:%s.~%s~"
+                                          (expand-file-name file) revision))
+                     (buffer (gethash buffer-name diff-syntax-fontify-revisions)))
+                (unless (and buffer (buffer-live-p buffer))
+                  (let* ((vc-buffer (ignore-errors
+                                      (vc-find-revision-no-save
+                                       (expand-file-name file) revision
+                                       diff-vc-backend
+                                       (get-buffer-create buffer-name)))))
+                    (when vc-buffer
+                      (setq buffer vc-buffer)
+                      (puthash buffer-name buffer diff-syntax-fontify-revisions))))
+                (when buffer
+                  (with-current-buffer buffer
+                    (setq props (diff-syntax-fontify-props file text line-nb t))))))
+          ;; If file is unavailable, get properties from the hunk alone
+          (setq file (car (diff-hunk-file-names old)))
+          (with-temp-buffer
+            (insert text)
+            (setq props (diff-syntax-fontify-props file text line-nb nil t))))))
+     ((and diff-default-directory (not (eq diff-font-lock-syntax 'hunk-only)))
+      (let ((file (car (diff-hunk-file-names old))))
+        (if (and file (file-exists-p file) (file-regular-p file))
+            ;; Try to get full text from the file
+            (with-temp-buffer
+              (insert-file-contents file t)
+              (setq props (diff-syntax-fontify-props file text line-nb)))
+          ;; Otherwise, get properties from the hunk alone
+          (with-temp-buffer
+            (insert text)
+            (setq props (diff-syntax-fontify-props file text line-nb nil t))))))
+     ((memq diff-font-lock-syntax '(hunk-also hunk-only))
+      (let ((file (car (diff-hunk-file-names old))))
+        (with-temp-buffer
+          (insert text)
+          (setq props (diff-syntax-fontify-props file text line-nb nil t))))))
+
+    ;; Put properties over the hunk text
+    (goto-char beg)
+    (when (and props (eq (diff-hunk-style) 'unified))
+      (while (< (progn (forward-line 1) (point)) end)
+        (when (or (and (not old) (not (looking-at-p "[-<]")))
+                  (and      old  (not (looking-at-p "[+>]"))))
+          (if (and old (not (looking-at-p "[-<]")))
+              ;; Fontify context lines only from new source,
+              ;; don't refontify context lines from old source.
+              (pop props)
+            (let ((line-props (pop props))
+                  (bol (1+ (point))))
+              (dolist (prop line-props)
+                (let ((ol (make-overlay (+ bol (nth 0 prop))
+                                        (+ bol (nth 1 prop))
+                                        nil 'front-advance nil)))
+                  (overlay-put ol 'evaporate t)
+                  (overlay-put ol 'face (nth 2 prop)))))))))))
+
+(defun diff-syntax-fontify-props (file text line-nb &optional no-init hunk-only)
+  "Get font-lock properties from the source code."
+  (unless no-init
+    (buffer-disable-undo)
+    (font-lock-mode -1)
+    (let ((enable-local-variables :safe) ;; to find `mode:'
+          (buffer-file-name file))
+      (set-auto-mode)
+      (when (and (memq 'generic-mode-find-file-hook find-file-hook)
+                 (fboundp 'generic-mode-find-file-hook))
+        (generic-mode-find-file-hook))))
+
+  (let ((font-lock-defaults (or font-lock-defaults '(nil t)))
+        (inhibit-read-only t)
+        props beg end)
+    (goto-char (point-min))
+    (if hunk-only
+        (setq beg (point-min) end (point-max))
+      (forward-line (1- (nth 0 line-nb)))
+      ;; non-regexp looking-at to compare hunk text for verification
+      (if (search-forward text (+ (point) (length text)) t)
+          (setq beg (- (point) (length text)) end (point))
+        (goto-char (point-min))
+        (if (search-forward text nil t)
+            (setq beg (- (point) (length text)) end (point)))))
+
+    (when (and beg end)
+      (goto-char beg)
+      (when (text-property-not-all beg end 'fontified t)
+        (if file
+            ;; In a temporary or cached buffer
+            (save-excursion
+              (font-lock-fontify-region beg end)
+              (put-text-property beg end 'fontified t))
+          ;; In an existing buffer
+          (font-lock-ensure beg end)))
+
+      (while (< (point) end)
+        (let* ((bol (point))
+               (eol (line-end-position))
+               line-props
+               (searching t)
+               (from (point)) to
+               (val (get-text-property from 'face)))
+          (while searching
+            (setq to (next-single-property-change from 'face nil eol))
+            (when val (push (list (- from bol) (- to bol) val) line-props))
+            (setq val (get-text-property to 'face) from to)
+            (unless (< to eol) (setq searching nil)))
+          (when val (push (list from eol val) line-props))
+          (push (nreverse line-props) props))
+        (forward-line 1)))
+    (set-buffer-modified-p nil)
+    (nreverse props)))
+
+
 (defun diff--filter-substring (str)
   (when diff-font-lock-prettify
     ;; Strip the `display' properties added by diff-font-lock-prettify,

bug#33567: Syntactic fontification of diff hunks

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: 33567@debbugs.gnu.org
> Date: Thu, 13 Dec 2018 01:17:52 +0200
> 
> Here is a more less final version that I tested on many different diffs:

Thanks, I have a few minor comments, mainly to the documentation
parts.

> diff --git a/etc/NEWS b/etc/NEWS
> index decc5e3954..1024e9e9f5 100644
> --- a/etc/NEWS
> +++ b/etc/NEWS
> @@ -273,6 +273,8 @@ To disable it, set the new defcustom 'diff-font-lock-refine' to nil.
>  *** File headers can be shortened, mimicking Magit's diff format
>  To enable it, set the new defcustom 'diff-font-lock-prettify to t.
>  
> +*** Source language syntax is highlighted in diff hunks when 'diff-font-lock-syntax' is t.

This line is too long, suggest to rephrase:

  *** Better syntax highlighting of Diff hunks.
  Fragments of source in Diff hunks are now by default highlighted
  according to the appropriate major mode.  Customize the new option
  'diff-font-lock-syntax' to nil to disable this.

> +@vindex diff-font-lock-syntax
> +  If non-nil, diff hunk font-lock includes source language syntax highlighting.

"nil" should be in @code.

> diff --git a/lisp/vc/diff.el b/lisp/vc/diff.el
> index ac94586cac..93fa54b1fb 100644
> --- a/lisp/vc/diff.el
> +++ b/lisp/vc/diff.el
> @@ -165,6 +167,7 @@ diff-no-select
>             (lambda (_ignore-auto _noconfirm)
>               (diff-no-select old new switches no-async (current-buffer))))
>        (setq default-directory thisdir)
> +      (set (make-local-variable 'diff-default-directory) default-directory)

Any reason not to use setq-local?

> +(defun diff--font-lock-syntax (max)
> +  "Syntax highlighting from font-lock."

Although an internal function, could it have a slightly more detailed
do string, please?

> +(defun diff-syntax-fontify-hunk (beg end old)
> +  "Highlight source language syntax in diff hunks."

This is for a single hunk, not "hunks", right?  Also, please mention
the arguments in the doc string.

> +(defun diff-syntax-fontify-props (file text line-nb &optional no-init hunk-only)
> +  "Get font-lock properties from the source code."

Please mention the arguments ion the doc string.

bug#33567: Syntactic fontification of diff hunks

[Juri Linkov]

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

>> Here is a more less final version that I tested on many different diffs:
>
> Thanks, I have a few minor comments, mainly to the documentation
> parts.
>
>> diff --git a/etc/NEWS b/etc/NEWS
>> index decc5e3954..1024e9e9f5 100644
>> --- a/etc/NEWS
>> +++ b/etc/NEWS
>> @@ -273,6 +273,8 @@ To disable it, set the new defcustom 'diff-font-lock-refine' to nil.
>>  *** File headers can be shortened, mimicking Magit's diff format
>>  To enable it, set the new defcustom 'diff-font-lock-prettify to t.
>>
>> +*** Source language syntax is highlighted in diff hunks when 'diff-font-lock-syntax' is t.
>
> This line is too long, suggest to rephrase:
>
>   *** Better syntax highlighting of Diff hunks.
>   Fragments of source in Diff hunks are now by default highlighted
>   according to the appropriate major mode.  Customize the new option
>   'diff-font-lock-syntax' to nil to disable this.

Changed.

>> +@vindex diff-font-lock-syntax
>> +  If non-nil, diff hunk font-lock includes source language syntax highlighting.
>
> "nil" should be in @code.

Changed.

>> diff --git a/lisp/vc/diff.el b/lisp/vc/diff.el
>> index ac94586cac..93fa54b1fb 100644
>> --- a/lisp/vc/diff.el
>> +++ b/lisp/vc/diff.el
>> @@ -165,6 +167,7 @@ diff-no-select
>>             (lambda (_ignore-auto _noconfirm)
>>               (diff-no-select old new switches no-async (current-buffer))))
>>        (setq default-directory thisdir)
>> +      (set (make-local-variable 'diff-default-directory) default-directory)
>
> Any reason not to use setq-local?

Only for consistency with other variables in the same file that still
don't use make-local-variable.  But now I added automatically buffer-local
(make-variable-buffer-local 'diff-default-directory) for consistency
with `default-directory'.

>> +(defun diff--font-lock-syntax (max)
>> +  "Syntax highlighting from font-lock."
>
> Although an internal function, could it have a slightly more detailed
> do string, please?

Done.

>> +(defun diff-syntax-fontify-hunk (beg end old)
>> +  "Highlight source language syntax in diff hunks."
>
> This is for a single hunk, not "hunks", right?  Also, please mention
> the arguments in the doc string.

Done.

>> +(defun diff-syntax-fontify-props (file text line-nb &optional no-init hunk-only)
>> +  "Get font-lock properties from the source code."
>
> Please mention the arguments ion the doc string.

Done.

Also with more highlighting in diff hunks now diff indicators need more
distinctive colors that I also tested.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: diff-font-lock-syntax.4.patch --]
[-- Type: text/x-diff, Size: 15067 bytes --]

diff --git a/etc/NEWS b/etc/NEWS
index c88f6ef5ca..7351f5cdd2 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -412,6 +412,12 @@ and compares their entire trees.
 *** Hunks are now automatically refined by default.
 To disable it, set the new defcustom 'diff-font-lock-refine' to nil.
 
++++
+*** Better syntax highlighting of Diff hunks.
+Fragments of source in Diff hunks are now by default highlighted
+according to the appropriate major mode.  Customize the new option
+'diff-font-lock-syntax' to nil to disable this.
+
 *** File headers can be shortened, mimicking Magit's diff format.
 To enable it, set the new defcustom 'diff-font-lock-prettify' to t.
 
diff --git a/doc/emacs/files.texi b/doc/emacs/files.texi
index b47be51e24..6e1faf84dc 100644
--- a/doc/emacs/files.texi
+++ b/doc/emacs/files.texi
@@ -1617,6 +1617,10 @@ Diff Mode
 modify the original (``old'') source files rather than the patched
 (``new'') source files.
 
+@vindex diff-font-lock-syntax
+  If non-@code{nil}, fragments of source in hunks are highlighted
+according to the appropriate major mode.
+
 @node Copying and Naming
 @section Copying, Naming and Renaming Files
 
diff --git a/lisp/vc/diff.el b/lisp/vc/diff.el
index ac94586cac..ed5b49d3bf 100644
--- a/lisp/vc/diff.el
+++ b/lisp/vc/diff.el
@@ -165,6 +167,7 @@ diff-no-select
            (lambda (_ignore-auto _noconfirm)
              (diff-no-select old new switches no-async (current-buffer))))
       (setq default-directory thisdir)
+      (setq diff-default-directory default-directory)
       (let ((inhibit-read-only t))
 	(insert command "\n"))
       (if (and (not no-async) (fboundp 'make-process))
diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index 4adef02984..7116d74db1 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -103,12 +104,42 @@ diff-font-lock-prettify
   :version "27.1"
   :type 'boolean)
 
+(defcustom diff-font-lock-syntax t
+  "If non-nil, diff hunk font-lock includes source language syntax highlighting.
+This highlighting is the same as added by `font-lock-mode'
+when corresponding source files are visited normally.
+Syntax highlighting is added over diff own highlighted changes.
+
+If t, the default, highlight syntax only in Diff buffers created by Diff
+commands that compare files or by VC commands that compare revisions.
+These provide all necessary context for reliable highlighting.  This value
+requires support from a VC backend to find the files being compared.
+For diffs against the working-tree version of a file, the highlighting is
+based on the current file contents.  File-based fontification tries to
+infer fontification from the compared files.
+
+If revision-based or file-based method fails, use hunk-based method to get
+fontification from hunk alone if the value is `hunk-also'.
+
+If `hunk-only', fontification is based on hunk alone, without full source.
+It tries to highlight hunks without enough context that sometimes might result
+in wrong fontification.  This is the fastest option, but less reliable."
+  :version "27.1"
+  :type '(choice (const :tag "Don't highlight syntax" nil)
+                 (const :tag "Hunk-based also" hunk-also)
+                 (const :tag "Hunk-based only" hunk-only)
+                 (const :tag "Highlight syntax" t)))
+
 (defvar diff-vc-backend nil
   "The VC backend that created the current Diff buffer, if any.")
 
 (defvar diff-vc-revisions nil
   "The VC revisions compared in the current Diff buffer, if any.")
 
+(defvar diff-default-directory nil
+  "The default directory where the current Diff buffer was created.")
+(make-variable-buffer-local 'diff-default-directory)
+
 (defvar diff-outline-regexp
   "\\([*+][*+][*+] [^0-9]\\|@@ ...\\|\\*\\*\\* [0-9].\\|--- [0-9]..\\)")
 
@@ -295,19 +326,25 @@ diff-changed
   :version "25.1")
 
 (defface diff-indicator-removed
-  '((t :inherit diff-removed))
+  '((default :inherit diff-removed)
+    (((class color) (min-colors 88))
+     :foreground "#aa2222"))
   "`diff-mode' face used to highlight indicator of removed lines (-, <)."
   :version "22.1")
 (defvar diff-indicator-removed-face 'diff-indicator-removed)
 
 (defface diff-indicator-added
-  '((t :inherit diff-added))
+  '((default :inherit diff-added)
+    (((class color) (min-colors 88))
+     :foreground "#22aa22"))
   "`diff-mode' face used to highlight indicator of added lines (+, >)."
   :version "22.1")
 (defvar diff-indicator-added-face 'diff-indicator-added)
 
 (defface diff-indicator-changed
-  '((t :inherit diff-changed))
+  '((default :inherit diff-changed)
+    (((class color) (min-colors 88))
+     :foreground "#aaaa22"))
   "`diff-mode' face used to highlight indicator of changed lines."
   :version "22.1")
 (defvar diff-indicator-changed-face 'diff-indicator-changed)
@@ -317,10 +354,7 @@ diff-function
   "`diff-mode' face used to highlight function names produced by \"diff -p\".")
 
 (defface diff-context
-  '((((class color grayscale) (min-colors 88) (background light))
-     :foreground "#333333")
-    (((class color grayscale) (min-colors 88) (background dark))
-     :foreground "#dddddd"))
+  '((t nil))
   "`diff-mode' face used to highlight context and other side-information."
   :version "25.1")
 
@@ -406,6 +440,7 @@ diff-font-lock-keywords
      (1 font-lock-comment-delimiter-face)
      (2 font-lock-comment-face))
     ("^[^-=+*!<>#].*\n" (0 'diff-context))
+    (,#'diff--font-lock-syntax)
     (,#'diff--font-lock-prettify)
     (,#'diff--font-lock-refined)))
 
@@ -2316,6 +2352,199 @@ diff--font-lock-prettify
                              'display "")))))
   nil)
 
+;;; Syntax highlighting from font-lock
+
+(defun diff--font-lock-syntax (max)
+  "Apply source language syntax highlighting from font-lock.
+Calls `diff-syntax-fontify' on every hunk found between point
+and the position in MAX."
+  (when diff-font-lock-syntax
+    (when (get-char-property (point) 'diff--font-lock-syntax)
+      (goto-char (next-single-char-property-change
+                  (point) 'diff--font-lock-syntax nil max)))
+    (let* ((min (point))
+           (beg (or (ignore-errors (diff-beginning-of-hunk))
+                    (ignore-errors (diff-hunk-next) (point))
+                    max)))
+      (while (< beg max)
+        (let ((end
+               (save-excursion (goto-char beg) (diff-end-of-hunk) (point))))
+          (if (< end min) (setq beg min))
+          (unless (or (< end beg)
+                      (get-char-property beg 'diff--font-lock-syntax))
+            (diff-syntax-fontify beg end)
+            (let ((ol (make-overlay beg end)))
+              (overlay-put ol 'diff--font-lock-syntax t)
+              (overlay-put ol 'diff-mode 'syntax)
+              (overlay-put ol 'evaporate t)
+              (overlay-put ol 'modification-hooks
+                           '(diff--font-lock-syntax--refresh))))
+          (goto-char (max beg end))
+          (setq beg (or (ignore-errors (diff-hunk-next) (point)) max))))))
+  nil)
+
+(defun diff--font-lock-syntax--refresh (ol _after _beg _end &optional _len)
+  (delete-overlay ol))
+
+(defun diff-syntax-fontify (beg end)
+  "Highlight source language syntax in diff hunk between BEG and END."
+  (save-excursion
+    (diff-syntax-fontify-hunk beg end t)
+    (diff-syntax-fontify-hunk beg end nil)))
+
+(defvar diff-syntax-fontify-revisions (make-hash-table :test 'equal))
+
+(defun diff-syntax-fontify-hunk (beg end old)
+  "Highlight source language syntax in diff hunk between BEG and END.
+When OLD is non-nil, highlight the hunk from the old source."
+  (remove-overlays beg end 'diff-mode 'syntax)
+  (goto-char beg)
+  (let* ((hunk (buffer-substring-no-properties beg end))
+         (text (or (ignore-errors (diff-hunk-text hunk (not old) nil)) ""))
+	 (line (if (looking-at "\\(?:\\*\\{15\\}.*\n\\)?[-@* ]*\\([0-9,]+\\)\\([ acd+]+\\([0-9,]+\\)\\)?")
+		   (if old (match-string 1)
+		     (if (match-end 3) (match-string 3) (match-string 1)))))
+         (line-nb (and line (string-match "\\([0-9]+\\),\\([0-9]+\\)" line)
+                       (list (string-to-number (match-string 1 line))
+                             (string-to-number (match-string 2 line)))))
+         props)
+    (cond
+     ((and diff-vc-backend (not (eq diff-font-lock-syntax 'hunk-only)))
+      (let* ((file (diff-find-file-name old t))
+             (revision (and file (if (not old) (nth 1 diff-vc-revisions)
+                                   (or (nth 0 diff-vc-revisions)
+                                       (vc-working-revision file))))))
+        (if file
+            (if (not revision)
+                ;; Get properties from the current working revision
+                (when (and (not old) (file-exists-p file) (file-regular-p file))
+                  ;; Try to reuse an existing buffer
+                  (if (get-file-buffer (expand-file-name file))
+                      (with-current-buffer (get-file-buffer (expand-file-name file))
+                        (setq props (diff-syntax-fontify-props nil text line-nb t)))
+                    ;; Get properties from the file
+                    (with-temp-buffer
+                      (insert-file-contents file t)
+                      (setq props (diff-syntax-fontify-props file text line-nb)))))
+              ;; Get properties from a cached revision
+              (let* ((buffer-name (format " diff-syntax:%s.~%s~"
+                                          (expand-file-name file) revision))
+                     (buffer (gethash buffer-name diff-syntax-fontify-revisions)))
+                (unless (and buffer (buffer-live-p buffer))
+                  (let* ((vc-buffer (ignore-errors
+                                      (vc-find-revision-no-save
+                                       (expand-file-name file) revision
+                                       diff-vc-backend
+                                       (get-buffer-create buffer-name)))))
+                    (when vc-buffer
+                      (setq buffer vc-buffer)
+                      (puthash buffer-name buffer diff-syntax-fontify-revisions))))
+                (when buffer
+                  (with-current-buffer buffer
+                    (setq props (diff-syntax-fontify-props file text line-nb t))))))
+          ;; If file is unavailable, get properties from the hunk alone
+          (setq file (car (diff-hunk-file-names old)))
+          (with-temp-buffer
+            (insert text)
+            (setq props (diff-syntax-fontify-props file text line-nb nil t))))))
+     ((and diff-default-directory (not (eq diff-font-lock-syntax 'hunk-only)))
+      (let ((file (car (diff-hunk-file-names old))))
+        (if (and file (file-exists-p file) (file-regular-p file))
+            ;; Try to get full text from the file
+            (with-temp-buffer
+              (insert-file-contents file t)
+              (setq props (diff-syntax-fontify-props file text line-nb)))
+          ;; Otherwise, get properties from the hunk alone
+          (with-temp-buffer
+            (insert text)
+            (setq props (diff-syntax-fontify-props file text line-nb nil t))))))
+     ((memq diff-font-lock-syntax '(hunk-also hunk-only))
+      (let ((file (car (diff-hunk-file-names old))))
+        (with-temp-buffer
+          (insert text)
+          (setq props (diff-syntax-fontify-props file text line-nb nil t))))))
+
+    ;; Put properties over the hunk text
+    (goto-char beg)
+    (when (and props (eq (diff-hunk-style) 'unified))
+      (while (< (progn (forward-line 1) (point)) end)
+        (when (or (and (not old) (not (looking-at-p "[-<]")))
+                  (and      old  (not (looking-at-p "[+>]"))))
+          (if (and old (not (looking-at-p "[-<]")))
+              ;; Fontify context lines only from new source,
+              ;; don't refontify context lines from old source.
+              (pop props)
+            (let ((line-props (pop props))
+                  (bol (1+ (point))))
+              (dolist (prop line-props)
+                (let ((ol (make-overlay (+ bol (nth 0 prop))
+                                        (+ bol (nth 1 prop))
+                                        nil 'front-advance nil)))
+                  (overlay-put ol 'evaporate t)
+                  (overlay-put ol 'face (nth 2 prop)))))))))))
+
+(defun diff-syntax-fontify-props (file text line-nb &optional no-init hunk-only)
+  "Get font-lock properties from the source code.
+FILE is the name of the source file.  TEXT is the literal source text from
+hunk.  LINE-NB is a pair of numbers: start line number and the number of
+lines in the hunk.  NO-INIT means no initialization is needed to set major
+mode.  When HUNK-ONLY is non-nil, then don't verify the existence of the
+hunk text in the source file.  Otherwise, don't highlight the hunk if the
+hunk text is not found in the source file."
+  (unless no-init
+    (buffer-disable-undo)
+    (font-lock-mode -1)
+    (let ((enable-local-variables :safe) ;; to find `mode:'
+          (buffer-file-name file))
+      (set-auto-mode)
+      (when (and (memq 'generic-mode-find-file-hook find-file-hook)
+                 (fboundp 'generic-mode-find-file-hook))
+        (generic-mode-find-file-hook))))
+
+  (let ((font-lock-defaults (or font-lock-defaults '(nil t)))
+        (inhibit-read-only t)
+        props beg end)
+    (goto-char (point-min))
+    (if hunk-only
+        (setq beg (point-min) end (point-max))
+      (forward-line (1- (nth 0 line-nb)))
+      ;; non-regexp looking-at to compare hunk text for verification
+      (if (search-forward text (+ (point) (length text)) t)
+          (setq beg (- (point) (length text)) end (point))
+        (goto-char (point-min))
+        (if (search-forward text nil t)
+            (setq beg (- (point) (length text)) end (point)))))
+
+    (when (and beg end)
+      (goto-char beg)
+      (when (text-property-not-all beg end 'fontified t)
+        (if file
+            ;; In a temporary or cached buffer
+            (save-excursion
+              (font-lock-fontify-region beg end)
+              (put-text-property beg end 'fontified t))
+          ;; In an existing buffer
+          (font-lock-ensure beg end)))
+
+      (while (< (point) end)
+        (let* ((bol (point))
+               (eol (line-end-position))
+               line-props
+               (searching t)
+               (from (point)) to
+               (val (get-text-property from 'face)))
+          (while searching
+            (setq to (next-single-property-change from 'face nil eol))
+            (when val (push (list (- from bol) (- to bol) val) line-props))
+            (setq val (get-text-property to 'face) from to)
+            (unless (< to eol) (setq searching nil)))
+          (when val (push (list from eol val) line-props))
+          (push (nreverse line-props) props))
+        (forward-line 1)))
+    (set-buffer-modified-p nil)
+    (nreverse props)))
+
+
 (defun diff--filter-substring (str)
   (when diff-font-lock-prettify
     ;; Strip the `display' properties added by diff-font-lock-prettify,

bug#33567: Syntactic fontification of diff hunks

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: 33567@debbugs.gnu.org
> Date: Mon, 17 Dec 2018 01:27:35 +0200
> 
> Also with more highlighting in diff hunks now diff indicators need more
> distinctive colors that I also tested.

Thanks, this LGTM.

bug#33567: Syntactic fontification of diff hunks

[Juri Linkov]

>> Also with more highlighting in diff hunks now diff indicators need more
>> distinctive colors that I also tested.
>
> Thanks, this LGTM.

Pushed to master and closed.

bug#33567: Syntactic fontification of diff hunks

[Juri Linkov]

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

>> Thanks, this LGTM.
>
> Pushed to master and closed.

Here's an additional patch to support diffs with files that contain
just one line as documented in (info "(diffutils) Detailed Unified")
and also without the final newline.  Such diffs have the text:

\ No newline at end of file


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

diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index ed953deb21..8c18f69e8c 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -1697,10 +1697,11 @@ diff-hunk-text
 	      (delete-region divider-pos (point-max)))
 	    (delete-region (point-min) keep))
 	  ;; Remove line-prefix characters, and unneeded lines (unified diffs).
-	  (let ((kill-char (if destp ?- ?+)))
+          ;; Also skip lines like "\ No newline at end of file"
+	  (let ((kill-chars (list (if destp ?- ?+) ?\\)))
 	    (goto-char (point-min))
 	    (while (not (eobp))
-	      (if (eq (char-after) kill-char)
+	      (if (memq (char-after) kill-chars)
 		  (delete-region (point) (progn (forward-line 1) (point)))
 		(delete-char num-pfx-chars)
 		(forward-line 1)))))
@@ -2394,19 +2395,23 @@ diff-syntax-fontify
 
 (defvar diff-syntax-fontify-revisions (make-hash-table :test 'equal))
 
+(eval-when-compile (require 'subr-x)) ; for string-trim-right
+
 (defun diff-syntax-fontify-hunk (beg end old)
   "Highlight source language syntax in diff hunk between BEG and END.
 When OLD is non-nil, highlight the hunk from the old source."
   (remove-overlays beg end 'diff-mode 'syntax)
   (goto-char beg)
   (let* ((hunk (buffer-substring-no-properties beg end))
-         (text (or (ignore-errors (diff-hunk-text hunk (not old) nil)) ""))
+         (text (string-trim-right (or (ignore-errors (diff-hunk-text hunk (not old) nil)) "")))
 	 (line (if (looking-at "\\(?:\\*\\{15\\}.*\n\\)?[-@* ]*\\([0-9,]+\\)\\([ acd+]+\\([0-9,]+\\)\\)?")
 		   (if old (match-string 1)
 		     (if (match-end 3) (match-string 3) (match-string 1)))))
-         (line-nb (and line (string-match "\\([0-9]+\\),\\([0-9]+\\)" line)
+         (line-nb (when line
+                    (if (string-match "\\([0-9]+\\),\\([0-9]+\\)" line)
                         (list (string-to-number (match-string 1 line))
-                             (string-to-number (match-string 2 line)))))
+                             (string-to-number (match-string 2 line)))
+                      (list (string-to-number line) 1)))) ; One-line diffs
          props)
     (cond
      ((and diff-vc-backend (not (eq diff-font-lock-syntax 'hunk-only)))
@@ -2470,6 +2475,7 @@ diff-syntax-fontify-hunk
       (while (< (progn (forward-line 1) (point)) end)
         (when (or (and (not old) (not (looking-at-p "[-<]")))
                   (and      old  (not (looking-at-p "[+>]"))))
+          (unless (looking-at-p "\\\\") ; skip "\ No newline at end of file"
             (if (and old (not (looking-at-p "[-<]")))
                 ;; Fontify context lines only from new source,
                 ;; don't refontify context lines from old source.
@@ -2481,7 +2487,7 @@ diff-syntax-fontify-hunk
                                           (+ bol (nth 1 prop))
                                           nil 'front-advance nil)))
                     (overlay-put ol 'evaporate t)
-                  (overlay-put ol 'face (nth 2 prop)))))))))))
+                    (overlay-put ol 'face (nth 2 prop))))))))))))
 
 (defun diff-syntax-fontify-props (file text line-nb &optional no-init hunk-only)
   "Get font-lock properties from the source code.

bug#33567: Syntactic fontification of diff hunks

[Dmitry Gutov]

On 18.12.2018 1:11, Juri Linkov wrote:
>>> Also with more highlighting in diff hunks now diff indicators need more
>>> distinctive colors that I also tested.
>>
>> Thanks, this LGTM.
> 
> Pushed to master and closed.

Juri, thank you. Looks like an helpful feature.

Have you considered bringing over the "default" face foregrounds as 
well, though, not just the special highlighting?

My brain sees syntax highlighting in some places and keeps wanting to 
interpret the green and red text and something syntactically meaningful 
as well.

Maybe we could add extra ancestors for diff-added and diff-removed 
faces, with default foregrounds, and use them for the base highlighting 
of diff chunks, aside from +'s and -'s.

Another thing that messes up the display a bit, is that comments and 
docstrings also have customized backgrounds in my theme, but that's less 
essential and seems harder to fix.

bug#33567: Syntactic fontification of diff hunks

[Juri Linkov]

> Have you considered bringing over the "default" face foregrounds as well,
> though, not just the special highlighting?
>
> My brain sees syntax highlighting in some places and keeps wanting to
> interpret the green and red text and something syntactically meaningful
> as well.
>
> Maybe we could add extra ancestors for diff-added and diff-removed faces,
> with default foregrounds, and use them for the base highlighting of diff
> chunks, aside from +'s and -'s.
>
> Another thing that messes up the display a bit, is that comments and
> docstrings also have customized backgrounds in my theme, but that's less
> essential and seems harder to fix.

I tried to replicate your settings: in emacs -Q I change the default
foreground with e.g. M-x set-foreground-color RET

A side note: M-x set-foreground-color RET TAB shows a list
of colors with different backgrounds, not foregrounds
as it would be natural to expect, so it is difficult to decide
what foreground color to select by looking at background colors.
This needs a separate bug#.

Ok, let's say I chose M-x set-foreground-color RET LightSeaGreen

Now in diff-mode I see exactly the same fontification with the same
foreground colors and as seen in the original buffer visited from
diff-mode by `C-c C-c', plus red/green background added by diff-mode.

bug#33567: Syntactic fontification of diff hunks

[Dmitry Gutov]

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

On 19.12.2018 0:35, Juri Linkov wrote:

> I tried to replicate your settings: in emacs -Q I change the default
> foreground with e.g. M-x set-foreground-color RET

That only seems to change the default's foreground color. Which affects 
how the context looks, but not lines with changes.

> A side note: M-x set-foreground-color RET TAB shows a list
> of colors with different backgrounds, not foregrounds
> as it would be natural to expect, so it is difficult to decide
> what foreground color to select by looking at background colors.
> This needs a separate bug#.

Indeed, it only gives a rough idea.

> Ok, let's say I chose M-x set-foreground-color RET LightSeaGreen
> 
> Now in diff-mode I see exactly the same fontification with the same
> foreground colors and as seen in the original buffer visited from
> diff-mode by `C-c C-c', plus red/green background added by diff-mode.

Let me show you a screenshot. I've set some purple-ish foreground using 
'M-x set-foreground-color', and you can see it in certain places. But 
where there was the "default" face in the original buffers, in the 
changes lines there is now green4 or red4 in the foreground, coming from 
the respective faces diff-added and diff-removed.

(You can also see the white from the docstring and comment faces, but 
that's a smaller issue, and I don't mind changing the theme if it comes 
to that).

[-- Attachment #2: Screenshot from 2018-12-19 01-25-11.png --]
[-- Type: image/png, Size: 280061 bytes --]

bug#33567: Syntactic fontification of diff hunks

[Juri Linkov]

>> I tried to replicate your settings: in emacs -Q I change the default
>> foreground with e.g. M-x set-foreground-color RET
>
> That only seems to change the default's foreground color. Which affects how
> the context looks, but not lines with changes.
>
>> A side note: M-x set-foreground-color RET TAB shows a list
>> of colors with different backgrounds, not foregrounds
>> as it would be natural to expect, so it is difficult to decide
>> what foreground color to select by looking at background colors.
>> This needs a separate bug#.
>
> Indeed, it only gives a rough idea.

Reported to bug#33799.

>> Ok, let's say I chose M-x set-foreground-color RET LightSeaGreen
>>
>> Now in diff-mode I see exactly the same fontification with the same
>> foreground colors and as seen in the original buffer visited from
>> diff-mode by `C-c C-c', plus red/green background added by diff-mode.
>
> Let me show you a screenshot. I've set some purple-ish foreground using
> 'M-x set-foreground-color', and you can see it in certain places. But where
> there was the "default" face in the original buffers, in the changes lines
> there is now green4 or red4 in the foreground, coming from the respective
> faces diff-added and diff-removed.

Thanks, I see now.

Does it help to remove foreground colors from diff-added and diff-removed?

Neither GitHub nor GitLab have foreground green/red colors, only background
green/red colors.

bug#33567: Syntactic fontification of diff hunks

[Dmitry Gutov]

On 19.12.2018 2:11, Juri Linkov wrote:

> Does it help to remove foreground colors from diff-added and diff-removed?

Yup. Except if I customize those and restart, I think the +'s and -'s on 
the left will become black as well, which is somewhat of a loss. So the 
colors might need to be moved to other definitions, e.g. 
diff-indicator-added.

Further, I'm not sure if we should do the same to the look of the 
diff-mode buffers when diff-font-lock-syntax is nil. Just something to 
consider.

Third, third-party code might rely on those faces looking as they do 
now. E.g. in diff-hl, diff-hl-insert inherits from diff-added, although 
this package likely won't be affected by this change (the faces define 
their own :foreground, for reasons lost to history).

> Neither GitHub nor GitLab have foreground green/red colors, only background
> green/red colors.

Those are the interfaces that built up my expectations as well.

bug#33567: Syntactic fontification of diff hunks

[Dmitry Gutov]

On 19.12.2018 2:48, Dmitry Gutov wrote:
> On 19.12.2018 2:11, Juri Linkov wrote:
> 
>> Does it help to remove foreground colors from diff-added and 
>> diff-removed?
> 
> Yup. Except if I customize those and restart, I think the +'s and -'s on 
> the left will become black as well, which is somewhat of a loss.
(Maybe not on colorful enough displays, though).

Anyway, to be clear, and other considerations aside, this works:

diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index ed953deb21..8a41e365da 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -304,9 +304,7 @@ diff-removed
      (((class color) (min-colors 88) (background light))
       :background "#ffdddd")
      (((class color) (min-colors 88) (background dark))
-     :background "#553333")
-    (((class color))
-     :foreground "red"))
+     :background "#553333"))
    "`diff-mode' face used to highlight removed lines.")

  (defface diff-added
@@ -315,9 +313,7 @@ diff-added
      (((class color) (min-colors 88) (background light))
       :background "#ddffdd")
      (((class color) (min-colors 88) (background dark))
-     :background "#335533")
-    (((class color))
-     :foreground "green"))
+     :background "#335533"))
    "`diff-mode' face used to highlight added lines.")

  (defface diff-changed
@@ -328,7 +324,9 @@ diff-changed
  (defface diff-indicator-removed
    '((default :inherit diff-removed)
      (((class color) (min-colors 88))
-     :foreground "#aa2222"))
+     :foreground "#aa2222")
+    (((class color))
+     :foreground "red"))
    "`diff-mode' face used to highlight indicator of removed lines (-, <)."
    :version "22.1")
  (defvar diff-indicator-removed-face 'diff-indicator-removed)
@@ -336,7 +334,9 @@ diff-indicator-removed-face
  (defface diff-indicator-added
    '((default :inherit diff-added)
      (((class color) (min-colors 88))
-     :foreground "#22aa22"))
+     :foreground "#22aa22")
+    (((class color))
+     :foreground "green"))
    "`diff-mode' face used to highlight indicator of added lines (+, >)."
    :version "22.1")
  (defvar diff-indicator-added-face 'diff-indicator-added)

bug#33567: Syntactic fontification of diff hunks

[Juri Linkov]

> Anyway, to be clear, and other considerations aside, this works:
>[...]
>  (defface diff-added
> @@ -315,9 +313,7 @@ diff-added
>      (((class color) (min-colors 88) (background light))
>       :background "#ddffdd")
>      (((class color) (min-colors 88) (background dark))
> -     :background "#335533")
> -    (((class color))
> -     :foreground "green"))
> +     :background "#335533"))
>    "`diff-mode' face used to highlight added lines.")
>[...]
>  (defface diff-indicator-added
>    '((default :inherit diff-added)
>      (((class color) (min-colors 88))
> -     :foreground "#22aa22"))
> +     :foreground "#22aa22")
> +    (((class color))
> +     :foreground "green"))
>    "`diff-mode' face used to highlight indicator of added lines (+, >)."
>    :version "22.1")
>  (defvar diff-indicator-added-face 'diff-indicator-added)

This looks good.

For the same reason we have the face font-lock-comment-delimiter-face
separate from font-lock-comment-face to use colors only on the former,
but not on the latter on tty with 8 colors to make easier to read comments.

bug#33567: Syntactic fontification of diff hunks

[Dmitry Gutov]

On 19.12.2018 23:49, Juri Linkov wrote:
>> Anyway, to be clear, and other considerations aside, this works:
>> [...]
>>   (defface diff-added
>> @@ -315,9 +313,7 @@ diff-added
>>       (((class color) (min-colors 88) (background light))
>>        :background "#ddffdd")
>>       (((class color) (min-colors 88) (background dark))
>> -     :background "#335533")
>> -    (((class color))
>> -     :foreground "green"))
>> +     :background "#335533"))
>>     "`diff-mode' face used to highlight added lines.")
>> [...]
>>   (defface diff-indicator-added
>>     '((default :inherit diff-added)
>>       (((class color) (min-colors 88))
>> -     :foreground "#22aa22"))
>> +     :foreground "#22aa22")
>> +    (((class color))
>> +     :foreground "green"))
>>     "`diff-mode' face used to highlight indicator of added lines (+, >)."
>>     :version "22.1")
>>   (defvar diff-indicator-added-face 'diff-indicator-added)
> 
> This looks good.

Should I install it? Nobody has commented on my earlier stated concerns, 
but maybe we should just push it and see how it plays out.

> For the same reason we have the face font-lock-comment-delimiter-face
> separate from font-lock-comment-face to use colors only on the former,
> but not on the latter on tty with 8 colors to make easier to read comments.

Yeah, it's totally fine to use separate faces. And I was happy to see 
diff-indicator-* were already defined and in use.

My concerns were different, though:

1. Is it okay to use the black foreground inside diff hunks even when 
diff-font-lock-syntax is nil? It's an incompatible change.

2. Even if we change the default in diff-added and diff-removed, some 
themes might have their foregrounds customized, so those users won't 
notice the change. It will trickle down to the themes eventually, I 
think, but it's unclear how the theme authors will choose to deal with 
this change while keeping compatibility with previous Emacs releases.

bug#33567: Syntactic fontification of diff hunks

[Juri Linkov]

>>> Anyway, to be clear, and other considerations aside, this works:
>>> [...]
>>>   (defface diff-added
>>> @@ -315,9 +313,7 @@ diff-added
>>>       (((class color) (min-colors 88) (background light))
>>>        :background "#ddffdd")
>>>       (((class color) (min-colors 88) (background dark))
>>> -     :background "#335533")
>>> -    (((class color))
>>> -     :foreground "green"))
>>> +     :background "#335533"))
>>>     "`diff-mode' face used to highlight added lines.")
>>> [...]
>>>   (defface diff-indicator-added
>>>     '((default :inherit diff-added)
>>>       (((class color) (min-colors 88))
>>> -     :foreground "#22aa22"))
>>> +     :foreground "#22aa22")
>>> +    (((class color))
>>> +     :foreground "green"))
>>>     "`diff-mode' face used to highlight indicator of added lines (+, >)."
>>>     :version "22.1")
>>>   (defvar diff-indicator-added-face 'diff-indicator-added)
>>
>> This looks good.
>
> Should I install it? Nobody has commented on my earlier stated concerns,
> but maybe we should just push it and see how it plays out.

I don't know.  We have two options for tty: highlight indicators only
or use red/green foreground without syntax highlighting.

>> For the same reason we have the face font-lock-comment-delimiter-face
>> separate from font-lock-comment-face to use colors only on the former,
>> but not on the latter on tty with 8 colors to make easier to read comments.
>
> Yeah, it's totally fine to use separate faces. And I was happy to see
> diff-indicator-* were already defined and in use.
>
> My concerns were different, though:
>
> 1. Is it okay to use the black foreground inside diff hunks even when
> diff-font-lock-syntax is nil? It's an incompatible change.

By default it used the black foreground.  Only Magit uses red/green foreground.

> 2. Even if we change the default in diff-added and diff-removed, some
> themes might have their foregrounds customized, so those users won't notice
> the change. It will trickle down to the themes eventually, I think, but
> it's unclear how the theme authors will choose to deal with this change
> while keeping compatibility with previous Emacs releases.

Maybe with conditional face definitions like

(if (boundp 'diff-font-lock-syntax)
    (defface blabla))

bug#33567: Syntactic fontification of diff hunks

[Dmitry Gutov]

On 21.12.2018 0:00, Juri Linkov wrote:

> I don't know.  We have two options for tty: highlight indicators only
> or use red/green foreground without syntax highlighting.

Ouch. All this time I've been talking about a personal customization 
that's too old for me to remember about. Sorry about that, I'm dropping 
that part of the proposal.

Speaking of ttys, green and red look foreground looks kind of okay for 
me in that context. However, diff-refine-* faces are simply defined as 
:inverse-video. And now, with the new feature, when e.g. there is a face 
that spans the whole line (like a comment), both diff-refine-added and 
diff-refine-removed faces look the same. Which is a problem. I don't 
have easy suggestions except defining specific background for them 
instead of inverse-video. And at that point we can drop the green/red 
foregrounds if people prefer.

> By default it used the black foreground.  Only Magit uses red/green foreground.

So I'd propose Magit to follow the core here. And, like we discussed, 
other popular tools.

> Maybe with conditional face definitions like
> 
> (if (boundp 'diff-font-lock-syntax)
>      (defface blabla))

Fair enough.

bug#33567: Syntactic fontification of diff hunks

[Juri Linkov]

> Speaking of ttys, green and red look foreground looks kind of okay for me
> in that context. However, diff-refine-* faces are simply defined
> as :inverse-video. And now, with the new feature, when e.g. there is a face
> that spans the whole line (like a comment), both diff-refine-added and
> diff-refine-removed faces look the same. Which is a problem. I don't have
> easy suggestions except defining specific background for them instead of
> inverse-video. And at that point we can drop the green/red foregrounds if
> people prefer.

What do you think about this solution that combines both
green/red foreground colors with syntax colors:

https://gist.github.com/skanev/0eeb943e3111a1df55fd

bug#33567: Syntactic fontification of diff hunks

[Dmitry Gutov]

On 25.12.2018 22:35, Juri Linkov wrote:

> What do you think about this solution that combines both
> green/red foreground colors with syntax colors:
> 
> https://gist.github.com/skanev/0eeb943e3111a1df55fd

This is almost the look I complained about previously, but if the green 
and red are not as sharp, I suppose it's not too terrible. At least the 
lighter red doesn't scream "errors" to me.

But note that this example does not show any "refined" functionality.

bug#33567: Syntactic fontification of diff hunks

[Juri Linkov]

>> What do you think about this solution that combines both
>> green/red foreground colors with syntax colors:
>>
>> https://gist.github.com/skanev/0eeb943e3111a1df55fd
>
> This is almost the look I complained about previously, but if the green and
> red are not as sharp, I suppose it's not too terrible. At least the lighter
> red doesn't scream "errors" to me.
>
> But note that this example does not show any "refined" functionality.

There are some underlines on this screenshot, maybe use underlines for
refined diffs on tty?

bug#33567: Syntactic fontification of diff hunks

[Dmitry Gutov]

On 27.12.2018 0:49, Juri Linkov wrote:

> There are some underlines on this screenshot, maybe use underlines for
> refined diffs on tty?

You mean colored underlines for every line? Umm, not sure I'd like that.

But I never use Emacs in tty, so maybe better ask on emacs-devel.

bug#33567: Syntactic fontification of diff hunks

[Juri Linkov]

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

>> There are some underlines on this screenshot, maybe use underlines for
>> refined diffs on tty?
>
> You mean colored underlines for every line? Umm, not sure I'd like that.

Actually I meant underlines instead of inverse-video.  Underlined refinement
looks much better than in inverse-video.

Also I propose to not use this feature on tty because there is no good
solution how to combine diff foreground colors with syntax foreground colors:


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

diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index a4c0618c67..7ab64fa4aa 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -104,7 +104,7 @@ diff-font-lock-prettify
   :version "27.1"
   :type 'boolean)
 
-(defcustom diff-font-lock-syntax t
+(defcustom diff-font-lock-syntax (>= (display-color-cells) 88)
   "If non-nil, diff hunk font-lock includes source language syntax highlighting.
 This highlighting is the same as added by `font-lock-mode'
 when corresponding source files are visited normally.
@@ -2019,7 +2019,7 @@ diff-refine-changed
      :background "#ffff55")
     (((class color) (min-colors 88) (background dark))
      :background "#aaaa22")
-    (t :inverse-video t))
+    (t :underline t))
   "Face used for char-based changes shown by `diff-refine-hunk'.")
 
 (defface diff-refine-removed

bug#33567: Syntactic fontification of diff hunks

[Dmitry Gutov]

On 27.12.2018 2:18, Juri Linkov wrote:

> Actually I meant underlines instead of inverse-video.  Underlined refinement
> looks much better than in inverse-video.

OK. But I went to try it out, and looks like underlines don't work on 
tty. Or at least not on mine.

> Also I propose to not use this feature on tty because there is no good
> solution how to combine diff foreground colors with syntax foreground colors:

*Shrug*, personally I'd leave the config as it is now. The current 
behavior might still be more useful than simply green or red lines.

bug#33567: Syntactic fontification of diff hunks

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Date: Thu, 27 Dec 2018 02:18:14 +0200
> Cc: 33567@debbugs.gnu.org
> 
> > You mean colored underlines for every line? Umm, not sure I'd like that.
> 
> Actually I meant underlines instead of inverse-video.  Underlined refinement
> looks much better than in inverse-video.
> 
> Also I propose to not use this feature on tty because there is no good
> solution how to combine diff foreground colors with syntax foreground colors:

Please don't.

bug#33567: Syntactic fontification of diff hunks

[Eli Zaretskii]

> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Thu, 27 Dec 2018 01:16:09 +0200
> Cc: 33567@debbugs.gnu.org
> 
> On 27.12.2018 0:49, Juri Linkov wrote:
> 
> > There are some underlines on this screenshot, maybe use underlines for
> > refined diffs on tty?
> 
> You mean colored underlines for every line? Umm, not sure I'd like that.

I _know_ I won't.  It's ugly and, frankly, inappropriate for syntax
highlighting.

bug#33567: Syntactic fontification of diff hunks

[Juri Linkov]

>> Does it help to remove foreground colors from diff-added and diff-removed?
>
> Yup. Except if I customize those and restart, I think the +'s and -'s on
> the left will become black as well, which is somewhat of a loss. So the
> colors might need to be moved to other definitions,
> e.g. diff-indicator-added.
>
> Further, I'm not sure if we should do the same to the look of the diff-mode
> buffers when diff-font-lock-syntax is nil. Just something to consider.
>
> Third, third-party code might rely on those faces looking as they do
> now. E.g. in diff-hl, diff-hl-insert inherits from diff-added, although
> this package likely won't be affected by this change (the faces define
> their own :foreground, for reasons lost to history).

A worse situation with Magit - I was asked to update Magit
to use this feature, but I don't know where to start:
Magit uses green/red colors both for background and foreground.

>> Neither GitHub nor GitLab have foreground green/red colors, only background
>> green/red colors.
>
> Those are the interfaces that built up my expectations as well.

bug#33567: Syntactic fontification of diff hunks

[Dmitry Gutov]

On 19.12.2018 23:51, Juri Linkov wrote:

> A worse situation with Magit - I was asked to update Magit
> to use this feature, but I don't know where to start:
> Magit uses green/red colors both for background and foreground.

Are there cases where it uses only red/green foreground to show a hunk? 
I would honestly just skip those cases, if Magit insists on doing things 
in several different ways.

Not sure why you need to port the new feature yourself, though.

bug#33567: Syntactic fontification of diff hunks

[Juri Linkov]

>> A worse situation with Magit - I was asked to update Magit
>> to use this feature, but I don't know where to start:
>> Magit uses green/red colors both for background and foreground.
>
> Are there cases where it uses only red/green foreground to show a hunk?
> I would honestly just skip those cases, if Magit insists on doing things in
> several different ways.
>
> Not sure why you need to port the new feature yourself, though.

I see no such cases with only red/green foreground, so it's safe to remove
red/green foreground, but probably I'll let Magit developers port it
themselves because I don't understand many their design decisions.

bug#33567: Syntactic fontification of diff hunks

[Dmitry Gutov]

On 19.12.2018 2:11, Juri Linkov wrote:

> Neither GitHub nor GitLab have foreground green/red colors, only background
> green/red colors.

Speaking of GitHub and GitLab, I wonder if we should take another cue 
from them and tone down our background colors as well.

This makes the new diffs more readable for me:

(eval-after-load 'diff-mode
   '(progn
      (set-face-background 'diff-added "#eeffee")
      (set-face-background 'diff-removed "#ffeeee")
      (set-face-background 'diff-refine-added "#d0ffd0")
      (set-face-background 'diff-refine-removed "#ffd0d0")
      ))

bug#33567: Syntactic fontification of diff hunks

[Juri Linkov]

>> Neither GitHub nor GitLab have foreground green/red colors, only background
>> green/red colors.
>
> Speaking of GitHub and GitLab, I wonder if we should take another cue from
> them and tone down our background colors as well.
>
> This makes the new diffs more readable for me:
>
> (eval-after-load 'diff-mode
>   '(progn
>      (set-face-background 'diff-added "#eeffee")
>      (set-face-background 'diff-removed "#ffeeee")
>      (set-face-background 'diff-refine-added "#d0ffd0")
>      (set-face-background 'diff-refine-removed "#ffd0d0")
>      ))

Good idea.

But please see in https://debbugs.gnu.org/10181#68
why red and green colors are not symmetrical and
how is was wrong on GitHub.

After my post GitHub realized their mistake and
now they use #fbb instead of #faa.

So instead of #cfc a better color for refine-added is #bfb.

Here's a table with additional columns labeled "appr" (approximated)
where colors are rounded to the closest shorthand hex triplet:

               GitHub  appr  GitLab  appr  current new     better
       removed #ffeef0 #fee  #fbe9eb #fee  #ffdddd #ffeeee
         added #e6ffed #efe  #ecfdf0 #efe  #ddffdd #eeffee
refine-removed #fdb8c0 #fbb  #fac5cd #fcc  #ffbbbb #ffd0d0 #ffcccc
  refine-added #acf2bd #aeb  #c7f0d2 #cec  #aaffaa #d0ffd0 #bbffbb

Your proposed new colors for added/removed are the same that are used
GitHub/GitLab, so this should be a good change.  For refine-removed
better to use GitLab's color #ffcccc that is very close to the
color you proposed.  But for refine-added GitLab made the same mistake
that GitHub already fixed.  So the best color for refine-added is #bbffbb.

While at it, please also tone down file/hunk header colors:
diff-header from grey80 to grey90 and
diff-file-header from grey70 to grey80

This will produce nice-looking colors.

bug#33567: Syntactic fontification of diff hunks

[Juri Linkov]

> Your proposed new colors for added/removed are the same that are used
> GitHub/GitLab, so this should be a good change.  For refine-removed
> better to use GitLab's color #ffcccc that is very close to the
> color you proposed.  But for refine-added GitLab made the same mistake
> that GitHub already fixed.  So the best color for refine-added is #bbffbb.

After trying to use there colors, I see that their shade is too subtle.
They might look better on large hunks, and I'm not sure why they look ok
in the browser, but in Emacs refined colors for small changes are almost
not noticeable.  However, please change them if majority agrees.

bug#33567: Syntactic fontification of diff hunks

[Dmitry Gutov]

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

On 25.12.2018 22:39, Juri Linkov wrote:
>> Your proposed new colors for added/removed are the same that are used
>> GitHub/GitLab, so this should be a good change.  For refine-removed
>> better to use GitLab's color #ffcccc that is very close to the
>> color you proposed.  But for refine-added GitLab made the same mistake
>> that GitHub already fixed.  So the best color for refine-added is #bbffbb.
> 
> After trying to use there colors, I see that their shade is too subtle.
> They might look better on large hunks, and I'm not sure why they look ok
> in the browser, but in Emacs refined colors for small changes are almost
> not noticeable.  However, please change them if majority agrees.

To my eyes, that's a surprising conclusion.

I wasn't going to argue with your correction to refine-added, even 
though I might prefer a slightly lighter variation (because I end up 
looking at larger refined regions often). Are you now saying that 
#ffcccc for refine-removed (or #d0ffd0, the difference is visible only 
on large regions) and #bbffbb for refine-added are hard for you to 
notice on smaller regions?

It most likely depends on the choice of the font, the contrast levels of 
the monitor and similar stuff, but during the same several days I have 
not met the same experience.

Before we get into deeper discussion (as well as discussing how one 
finds out majority's opinion), I have to ask: did you make sure to use 
the new refined colors with the new diff-added and diff-removed 
background colors?

Attached are the screenshots of the default theme with my preferred font 
(Inconsolata LGC) and refined colors from your counter-proposal. The 
default Ubuntu's monospaced font is even more prominent and arguably 
readable, I can make screenshots with it as well if you like.

This is commit a94ac604d8. We can also note that GitHub only refines 
smaller chunks: 
https://github.com/emacs-mirror/emacs/commit/a94ac604d8c9848b0414ade80a1920b345161656, 
so its use of darker backgrounds is more justifiable.

What do you think of the screenshots? Are the small refined regions hard 
for you to see? Or do they look very different in your Emacs?

[-- Attachment #2: Screenshot from 2018-12-26 03-28-06.png --]
[-- Type: image/png, Size: 432848 bytes --]

[-- Attachment #3: Screenshot from 2018-12-26 03-28-44.png --]
[-- Type: image/png, Size: 364018 bytes --]

[-- Attachment #4: Screenshot from 2018-12-26 03-29-20.png --]
[-- Type: image/png, Size: 363806 bytes --]

bug#33567: Syntactic fontification of diff hunks

[Juri Linkov]

>>> Your proposed new colors for added/removed are the same that are used
>>> GitHub/GitLab, so this should be a good change.  For refine-removed
>>> better to use GitLab's color #ffcccc that is very close to the
>>> color you proposed.  But for refine-added GitLab made the same mistake
>>> that GitHub already fixed.  So the best color for refine-added is #bbffbb.
>>
>> After trying to use there colors, I see that their shade is too subtle.
>> They might look better on large hunks, and I'm not sure why they look ok
>> in the browser, but in Emacs refined colors for small changes are almost
>> not noticeable.  However, please change them if majority agrees.
>
> To my eyes, that's a surprising conclusion.
>
> I wasn't going to argue with your correction to refine-added, even though
> I might prefer a slightly lighter variation (because I end up looking at
> larger refined regions often). Are you now saying that #ffcccc for
> refine-removed (or #d0ffd0, the difference is visible only on large
> regions) and #bbffbb for refine-added are hard for you to notice on
> smaller regions?

I meant that added/removed #eeffee/#ffeeee are harder to notice,
and really only on distant corners of the monitor.  Also looking down
at the LCD screen from a sharp angle can see the inverted colors:
green instead of red, and red instead of green :)

So color choice is not the exact sciences.

> Before we get into deeper discussion (as well as discussing how one finds
> out majority's opinion), I have to ask: did you make sure to use the new
> refined colors with the new diff-added and diff-removed background colors?

Since #eeffee/#ffeeee colors were tested on many users of
GitHub/GitLab, please keep them in diff-mode and also install
the accompanying change of refine-added/refine-removed.

> This is commit a94ac604d8. We can also note that GitHub only refines
> smaller chunks:

GitHub only refines smaller chunks, but refining large chunks often
helps to see real changes with code indentation, e.g. when a let-binding
form is added and thus whitespace of indentation shifts the code block.

> https://github.com/emacs-mirror/emacs/commit/a94ac604d8c9848b0414ade80a1920b345161656,
> so its use of darker backgrounds is more justifiable.
>
> What do you think of the screenshots? Are the small refined regions hard
> for you to see? Or do they look very different in your Emacs?

I think this is fine, I have no problems with refine-added/refine-removed,
please install refine-added/refine-removed as well.

bug#33567: Syntactic fontification of diff hunks

[Dmitry Gutov]

On 27.12.2018 0:59, Juri Linkov wrote:

> I meant that added/removed #eeffee/#ffeeee are harder to notice,
> and really only on distant corners of the monitor.  Also looking down
> at the LCD screen from a sharp angle can see the inverted colors:
> green instead of red, and red instead of green :)

Oh, I see. IIRC, TN screens can exhibit this problem (poorer viewing 
angles), especially the older ones.

> So color choice is not the exact sciences.

It's subjective, of course.

> Since #eeffee/#ffeeee colors were tested on many users of
> GitHub/GitLab, please keep them in diff-mode and also install
> the accompanying change of refine-added/refine-removed.

OK, installed. Thanks for testing!

> GitHub only refines smaller chunks, but refining large chunks often
> helps to see real changes with code indentation, e.g. when a let-binding
> form is added and thus whitespace of indentation shifts the code block.

Of course. It's definitely useful (though it can incur some noticeable 
delay then a refined region is big enough).

What I'm saying is, reading text on a darker red background is not very 
easy or enjoyable. So I think we want to have the diff-refine-* faces to 
be as light as possible while still easy enough to discern from 
diff-added and diff-removed.

But GitHub and GitLab don't have that urgency. That's why our colors are 
lighter already.

With that in mind, I'm going to experiment with even lighter colors, 
similar to what I originally proposed, in my private config:

      (set-face-background 'diff-refine-added "#d0ffd0")
      (set-face-background 'diff-refine-removed "#ffd6d6")

Maybe try them out sometime.

bug#33567: Syntactic fontification of diff hunks

[Juri Linkov]

> With that in mind, I'm going to experiment with even lighter colors,
> similar to what I originally proposed, in my private config:
>
>      (set-face-background 'diff-refine-added "#d0ffd0")
>      (set-face-background 'diff-refine-removed "#ffd6d6")
>
> Maybe try them out sometime.

These are better on large chunks, but harder to notice on smaller ones.

bug#33567: Syntactic fontification of diff hunks

[Juri Linkov]

>> With that in mind, I'm going to experiment with even lighter colors,
>> similar to what I originally proposed, in my private config:
>>
>>      (set-face-background 'diff-refine-added "#d0ffd0")
>>      (set-face-background 'diff-refine-removed "#ffd6d6")
>>
>> Maybe try them out sometime.
>
> These are better on large chunks, but harder to notice on smaller ones.

Or we could add more faces: lighter refine-added/removed for multi-line chunks,
and darker refine-added/removed faces for one-line differences.

bug#33567: Syntactic fontification of diff hunks

[Dmitry Gutov]

On 30.12.2018 2:07, Juri Linkov wrote:

> Or we could add more faces: lighter refine-added/removed for multi-line chunks,
> and darker refine-added/removed faces for one-line differences.

It could work. But I'm not the best judge of the result since the 
lighter ones work fine for me in all contexts.

Hopefully somebody else decides to join the discussion.

bug#33567: Syntactic fontification of diff hunks

[Dmitry Gutov]

On 21.12.2018 0:17, Juri Linkov wrote:

> But please see in https://debbugs.gnu.org/10181#68
> why red and green colors are not symmetrical and
> how is was wrong on GitHub.

Thanks! (TBC in another email).

> Your proposed new colors for added/removed are the same that are used
> GitHub/GitLab, so this should be a good change.

Now pushed, thank you.

> While at it, please also tone down file/hunk header colors:
> diff-header from grey80 to grey90 and
> diff-file-header from grey70 to grey80

I see you have done that yourself, looks good.

bug#33567: Syntactic fontification of diff hunks

[Dmitry Gutov]

On 03.12.2018 2:34, Juri Linkov wrote:

> Fixing the described problem will remove this comment,
> but I have no idea how better to do this.  The problem is that
> we need to provide own created buffer to the call to `vc-find-revision'.
> Currently it has the following function signature:
> 
>    (defun vc-find-revision (file revision &optional backend)
> 
> But VC API in the comments in the beginning of vc.el
> is documented with a different function signature:
> 
>    ;; * find-revision (file rev buffer)
>    ;;
>    ;;   Fetch revision REV of file FILE and put it into BUFFER.
>    ;;   If REV is the empty string, fetch the head of the trunk.
>    ;;   The implementation should pass the value of vc-checkout-switches
>    ;;   to the backend command.

That is the VC backend API, not the signatures of public functions in 
vc.el (which would be kinda pointless).

vc-git-find-revision and friends do indeed have this signature.

Take a look at the uses of vc-call-backend in 
vc-find-revision[-no]-save. Maybe you need to write a new 
???-find-revision function that does what you need.

bug#33567: Syntactic fontification of diff hunks

[Juri Linkov]

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

>> Fixing the described problem will remove this comment,
>> but I have no idea how better to do this.  The problem is that
>> we need to provide own created buffer to the call to `vc-find-revision'.
>> Currently it has the following function signature:
>>
>>    (defun vc-find-revision (file revision &optional backend)
>>
>> But VC API in the comments in the beginning of vc.el
>> is documented with a different function signature:
>>
>>    ;; * find-revision (file rev buffer)
>>    ;;
>>    ;;   Fetch revision REV of file FILE and put it into BUFFER.
>>    ;;   If REV is the empty string, fetch the head of the trunk.
>>    ;;   The implementation should pass the value of vc-checkout-switches
>>    ;;   to the backend command.
>
> That is the VC backend API, not the signatures of public functions in vc.el
> (which would be kinda pointless).
>
> vc-git-find-revision and friends do indeed have this signature.
>
> Take a look at the uses of vc-call-backend in
> vc-find-revision[-no]-save. Maybe you need to write a new ???-find-revision
> function that does what you need.

Thanks, I see now that we can freely add a new optional arg:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: vc-find-revision-no-save-buffer.patch --]
[-- Type: text/x-diff, Size: 2569 bytes --]

diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index de43544864..4976deefef 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -1998,33 +1998,37 @@ vc-find-revision-save
 	(set (make-local-variable 'vc-parent-buffer) filebuf))
       result-buf)))
 
-(defun vc-find-revision-no-save (file revision &optional backend)
-  "Read REVISION of FILE into a buffer and return the buffer.
+(defun vc-find-revision-no-save (file revision &optional backend buffer)
+  "Read REVISION of FILE into a BUFFER and return the buffer.
 Unlike `vc-find-revision-save', doesn't save the created buffer to file."
-  (let ((filebuf (or (get-file-buffer file) (current-buffer)))
-        (filename (vc-version-backup-file-name file revision 'manual)))
-    (unless (or (get-file-buffer filename)
-                (file-exists-p filename))
+  (let ((filebuf (or buffer (get-file-buffer file) (current-buffer)))
+        (filename (unless buffer (vc-version-backup-file-name file revision 'manual))))
+    (unless (and (not buffer)
+                 (or (get-file-buffer filename)
+                     (file-exists-p filename)))
       (with-current-buffer filebuf
 	(let ((failed t))
 	  (unwind-protect
 	      (let ((coding-system-for-read 'no-conversion)
 		    (coding-system-for-write 'no-conversion))
-		(with-current-buffer (create-file-buffer filename)
-                  (setq buffer-file-name filename)
+		(with-current-buffer (or buffer (create-file-buffer filename))
+                  (unless buffer (setq buffer-file-name filename))
 		  (let ((outbuf (current-buffer)))
 		    (with-current-buffer filebuf
 		      (if backend
 			  (vc-call-backend backend 'find-revision file revision outbuf)
 			(vc-call find-revision file revision outbuf))))
                   (goto-char (point-min))
-                  (normal-mode)
+                  (if buffer (let ((buffer-file-name file)) (normal-mode)) (normal-mode))
 	          (set-buffer-modified-p nil)
                   (setq buffer-read-only t))
 		(setq failed nil))
-	    (when (and failed (get-file-buffer filename))
+	    (when (and failed (unless buffer (get-file-buffer filename)))
+	      (with-current-buffer (get-file-buffer filename)
+		(set-buffer-modified-p nil))
 	      (kill-buffer (get-file-buffer filename)))))))
-    (let ((result-buf (or (get-file-buffer filename)
+    (let ((result-buf (or buffer
+                          (get-file-buffer filename)
                           (find-file-noselect filename))))
       (with-current-buffer result-buf
 	(set (make-local-variable 'vc-parent-buffer) filebuf))

bug#33567: Syntactic fontification of diff hunks

[Dmitry Gutov]

On 04.12.2018 1:24, Juri Linkov wrote:
> Thanks, I see now that we can freely add a new optional arg:

*shrug* or that.

> Unlike `vc-find-revision-save', doesn't save the created buffer to file."

This part of the docstring will probably need to be updated, too.

bug#33567: Syntactic fontification of diff hunks

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: Eli Zaretskii <eliz@gnu.org>,  33567@debbugs.gnu.org
> Date: Tue, 04 Dec 2018 01:24:52 +0200
> 
> -(defun vc-find-revision-no-save (file revision &optional backend)
> -  "Read REVISION of FILE into a buffer and return the buffer.
> +(defun vc-find-revision-no-save (file revision &optional backend buffer)
> +  "Read REVISION of FILE into a BUFFER and return the buffer.

Since you use BUFFER, please drop the "a" part.

Also, the doc string should tell what BUFFER defaults to if omitted or
nil.

> +  (let ((filebuf (or buffer (get-file-buffer file) (current-buffer)))
                     ^^^^^^^^^
I would use bufferp here.  What if someone calls the function with
something that is not a buffer?  And, btw, are buffer names allowed?

Similarly with other places where you make the same test (why not make
it once and bind some local variable to the result, btw?).

Thanks.

bug#33567: Syntactic fontification of diff hunks

[Juri Linkov]

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

>> -(defun vc-find-revision-no-save (file revision &optional backend)
>> -  "Read REVISION of FILE into a buffer and return the buffer.
>> +(defun vc-find-revision-no-save (file revision &optional backend buffer)
>> +  "Read REVISION of FILE into a BUFFER and return the buffer.
>
> Since you use BUFFER, please drop the "a" part.
>
> Also, the doc string should tell what BUFFER defaults to if omitted or
> nil.

Updated.

>> +  (let ((filebuf (or buffer (get-file-buffer file) (current-buffer)))
>                      ^^^^^^^^^
> I would use bufferp here.  What if someone calls the function with
> something that is not a buffer?

Added buffer-live-p.

> And, btw, are buffer names allowed?

I see that most VC functions work with buffer objects only.
So buffer names could be allowed only when really necessary.

> Similarly with other places where you make the same test (why not make
> it once and bind some local variable to the result, btw?).

Done:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: vc-find-revision-no-save-buffer.2.patch --]
[-- Type: text/x-diff, Size: 2919 bytes --]

diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index de43544864..dbbc3e2038 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -1998,33 +1998,41 @@ vc-find-revision-save
 	(set (make-local-variable 'vc-parent-buffer) filebuf))
       result-buf)))
 
-(defun vc-find-revision-no-save (file revision &optional backend)
-  "Read REVISION of FILE into a buffer and return the buffer.
-Unlike `vc-find-revision-save', doesn't save the created buffer to file."
-  (let ((filebuf (or (get-file-buffer file) (current-buffer)))
-        (filename (vc-version-backup-file-name file revision 'manual)))
-    (unless (or (get-file-buffer filename)
-                (file-exists-p filename))
+(defun vc-find-revision-no-save (file revision &optional backend buffer)
+  "Read REVISION of FILE into BUFFER and return the buffer.
+If BUFFER omitted or nil, this function creates a new buffer and sets
+`buffer-file-name' to the name constructed from the file name and the
+revision number.
+Unlike `vc-find-revision-save', doesn't save the buffer to the file."
+  (let* ((buffer (when (buffer-live-p buffer) buffer))
+         (filebuf (or buffer (get-file-buffer file) (current-buffer)))
+         (filename (unless buffer (vc-version-backup-file-name file revision 'manual))))
+    (unless (and (not buffer)
+                 (or (get-file-buffer filename)
+                     (file-exists-p filename)))
       (with-current-buffer filebuf
 	(let ((failed t))
 	  (unwind-protect
 	      (let ((coding-system-for-read 'no-conversion)
-		    (coding-system-for-write 'no-conversion))
-		(with-current-buffer (create-file-buffer filename)
-                  (setq buffer-file-name filename)
+                    (coding-system-for-write 'no-conversion))
+		(with-current-buffer (or buffer (create-file-buffer filename))
+                  (unless buffer (setq buffer-file-name filename))
 		  (let ((outbuf (current-buffer)))
 		    (with-current-buffer filebuf
 		      (if backend
 			  (vc-call-backend backend 'find-revision file revision outbuf)
 			(vc-call find-revision file revision outbuf))))
                   (goto-char (point-min))
-                  (normal-mode)
+                  (if buffer (let ((buffer-file-name file)) (normal-mode)) (normal-mode))
 	          (set-buffer-modified-p nil)
                   (setq buffer-read-only t))
 		(setq failed nil))
-	    (when (and failed (get-file-buffer filename))
+	    (when (and failed (unless buffer (get-file-buffer filename)))
+	      (with-current-buffer (get-file-buffer filename)
+		(set-buffer-modified-p nil))
 	      (kill-buffer (get-file-buffer filename)))))))
-    (let ((result-buf (or (get-file-buffer filename)
+    (let ((result-buf (or buffer
+                          (get-file-buffer filename)
                           (find-file-noselect filename))))
       (with-current-buffer result-buf
 	(set (make-local-variable 'vc-parent-buffer) filebuf))