bug#54034: 29.0.50; Diff prettify broken for empty files

[Matthias Meulien <orontee@gmail.com>]

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

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

> [ It'd have been better to file a new bug report for this one, FWIW.  ]
>
> I installed the patch below, which seems safe, but is probably
> not optimal.  Matthias?

Wouldn't it be safer to simply disable prettification of the "diff
header" when diff-buffer-type isn't equal to git?

In case of the output of the command diff-buffers, I don't see what
would be a usefull prettification of that header.

For other cases, back in 2018 you wrote that "This has only been tested
with Git's diff output."  (and I extended diff--font-lock-prettify
without testing other outputs as Git's).  Note also that prettification
was already broken with emacs 27.1.  See the attached screenshot where
--- #<buffer *Messages*> has one of its minus sign in the fringe when
diff-font-lock-prettify is t.  (Your patch fixed that!)


[-- Attachment #2: Capture d’écran du 2022-06-29 19-43-23.png --]
[-- Type: image/png, Size: 156551 bytes --]

[-- Attachment #3: Type: text/plain, Size: 103 bytes --]


So I guess it won't make any harm to support "diff header"
prettification for Git diff output only: 


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0001-Disable-prettification-of-diff-header-for-non-Git-di.patch --]
[-- Type: text/x-diff, Size: 9777 bytes --]

From 48a4950b1f4a5d9f8397ff061bc8a3109f4421b7 Mon Sep 17 00:00:00 2001
From: Matthias Meulien <orontee@gmail.com>
Date: Wed, 29 Jun 2022 20:10:59 +0200
Subject: [PATCH] Disable prettification of diff header for non-Git diff output

---
 lisp/vc/diff-mode.el | 159 ++++++++++++++++++++++---------------------
 1 file changed, 80 insertions(+), 79 deletions(-)

diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index 3f3e503a3f..9873d85f55 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -2618,85 +2618,86 @@ diff--font-lock-prettify
     ;; Mimicks the output of Magit's diff.
     ;; FIXME: This has only been tested with Git's diff output.
     ;; FIXME: Add support for Git's "rename from/to"?
-    (while (re-search-forward "^diff " limit t)
-      ;; We split the regexp match into a search plus a looking-at because
-      ;; we want to use LIMIT for the search but we still want to match
-      ;; all the header's lines even if LIMIT falls in the middle of it.
-      (when (save-excursion
-              (forward-line 0)
-              (looking-at
-               (eval-when-compile
-                 (let* ((index "\\(?:index.*\n\\)?")
-                        (file4 (concat
-                                "\\(?:" null-device "\\|[ab]/\\(?4:.*\\)\\)"))
-                        (file5 (concat
-                                "\\(?:" null-device "\\|[ab]/\\(?5:.*\\)\\)"))
-                        (header (concat "--- " file4 "\n"
-                                        "\\+\\+\\+ " file5 "\n"))
-                        (binary (concat
-                                 "Binary files " file4
-                                 " and " file5 " \\(?7:differ\\)\n"))
-                        (horb (concat "\\(?:" header "\\|" binary "\\)?")))
-                   (concat "diff.*?\\(?: a/\\(.*?\\) b/\\(.*\\)\\)?\n"
-                           "\\(?:"
-                           ;; For new/deleted files, there might be no
-                           ;; header (and no hunk) if the file is/was empty.
-                           "\\(?3:new\\(?6:\\)\\|deleted\\) file mode \\(?10:[0-7]\\{6\\}\\)\n"
-                           index horb
-                           ;; Normal case. There might be no header
-                           ;; (and no hunk) if only the file mode
-                           ;; changed.
-                           "\\|"
-                           "\\(?:old mode \\(?8:[0-7]\\{6\\}\\)\n\\)?"
-                           "\\(?:new mode \\(?9:[0-7]\\{6\\}\\)\n\\)?"
-                           index horb "\\)")))))
-        ;; The file names can be extracted either from the `diff' line
-        ;; or from the two header lines.  Prefer the header line info if
-        ;; available since the `diff' line is ambiguous in case the
-        ;; file names include " b/" or " a/".
-        ;; FIXME: This prettification throws away all the information
-        ;; about the index hashes.
-        (let ((oldfile (or (match-string 4) (match-string 1)))
-              (newfile (or (match-string 5) (match-string 2)))
-              (kind (if (match-beginning 7) " BINARY"
-                      (unless (or (match-beginning 4)
-                                  (match-beginning 5)
-                                  (not (match-beginning 3)))
-                        " empty")))
-              (filemode
-               (cond
-                ((match-beginning 10)
-                 (concat " file with mode " (match-string 10) "  "))
-                ((and (match-beginning 8) (match-beginning 9))
-                 (concat " file (mode changed from "
-                         (match-string 8) " to " (match-string 9) ")  "))
-                (t " file  "))))
-          (add-text-properties
-           (match-beginning 0) (1- (match-end 0))
-           (list 'display
-                 (propertize
-                  (cond
-                   ((match-beginning 3)
-                    (concat (capitalize (match-string 3)) kind filemode
-                            (if (match-beginning 6) newfile oldfile)))
-                   ((and (null (match-string 4)) (match-string 5))
-                    (concat "New " kind filemode newfile))
-                   ((null (match-string 2))
-                    ;; We used to use
-                    ;;     (concat "Deleted" kind filemode oldfile)
-                    ;; here but that misfires for `diff-buffers'
-                    ;; (see 24 Jun 2022 message in bug#54034).
-                    ;; AFAIK if (match-string 2) is nil then so is
-                    ;; (match-string 1), so "Deleted" doesn't sound right,
-                    ;; so better just let the header in plain sight for now.
-                    ;; FIXME: `diff-buffers' should maybe try to better
-                    ;; mimic Git's format with "a/" and "b/" so prettification
-                    ;; can "just work!"
-                    nil)
-                   (t
-                    (concat "Modified" kind filemode oldfile)))
-                  'face '(diff-file-header diff-header))
-                 'font-lock-multiline t))))))
+    (when (eq diff-buffer-type 'git)
+      (while (re-search-forward "^diff " limit t)
+        ;; We split the regexp match into a search plus a looking-at because
+        ;; we want to use LIMIT for the search but we still want to match
+        ;; all the header's lines even if LIMIT falls in the middle of it.
+        (when (save-excursion
+                (forward-line 0)
+                (looking-at
+                 (eval-when-compile
+                   (let* ((index "\\(?:index.*\n\\)?")
+                          (file4 (concat
+                                  "\\(?:" null-device "\\|[ab]/\\(?4:.*\\)\\)"))
+                          (file5 (concat
+                                  "\\(?:" null-device "\\|[ab]/\\(?5:.*\\)\\)"))
+                          (header (concat "--- " file4 "\n"
+                                          "\\+\\+\\+ " file5 "\n"))
+                          (binary (concat
+                                   "Binary files " file4
+                                   " and " file5 " \\(?7:differ\\)\n"))
+                          (horb (concat "\\(?:" header "\\|" binary "\\)?")))
+                     (concat "diff.*?\\(?: a/\\(.*?\\) b/\\(.*\\)\\)?\n"
+                             "\\(?:"
+                             ;; For new/deleted files, there might be no
+                             ;; header (and no hunk) if the file is/was empty.
+                             "\\(?3:new\\(?6:\\)\\|deleted\\) file mode \\(?10:[0-7]\\{6\\}\\)\n"
+                             index horb
+                             ;; Normal case. There might be no header
+                             ;; (and no hunk) if only the file mode
+                             ;; changed.
+                             "\\|"
+                             "\\(?:old mode \\(?8:[0-7]\\{6\\}\\)\n\\)?"
+                             "\\(?:new mode \\(?9:[0-7]\\{6\\}\\)\n\\)?"
+                             index horb "\\)")))))
+          ;; The file names can be extracted either from the `diff' line
+          ;; or from the two header lines.  Prefer the header line info if
+          ;; available since the `diff' line is ambiguous in case the
+          ;; file names include " b/" or " a/".
+          ;; FIXME: This prettification throws away all the information
+          ;; about the index hashes.
+          (let ((oldfile (or (match-string 4) (match-string 1)))
+                (newfile (or (match-string 5) (match-string 2)))
+                (kind (if (match-beginning 7) " BINARY"
+                        (unless (or (match-beginning 4)
+                                    (match-beginning 5)
+                                    (not (match-beginning 3)))
+                          " empty")))
+                (filemode
+                 (cond
+                  ((match-beginning 10)
+                   (concat " file with mode " (match-string 10) "  "))
+                  ((and (match-beginning 8) (match-beginning 9))
+                   (concat " file (mode changed from "
+                           (match-string 8) " to " (match-string 9) ")  "))
+                  (t " file  "))))
+            (add-text-properties
+             (match-beginning 0) (1- (match-end 0))
+             (list 'display
+                   (propertize
+                    (cond
+                     ((match-beginning 3)
+                      (concat (capitalize (match-string 3)) kind filemode
+                              (if (match-beginning 6) newfile oldfile)))
+                     ((and (null (match-string 4)) (match-string 5))
+                      (concat "New " kind filemode newfile))
+                     ((null (match-string 2))
+                      ;; We used to use
+                      ;;     (concat "Deleted" kind filemode oldfile)
+                      ;; here but that misfires for `diff-buffers'
+                      ;; (see 24 Jun 2022 message in bug#54034).
+                      ;; AFAIK if (match-string 2) is nil then so is
+                      ;; (match-string 1), so "Deleted" doesn't sound right,
+                      ;; so better just let the header in plain sight for now.
+                      ;; FIXME: `diff-buffers' should maybe try to better
+                      ;; mimic Git's format with "a/" and "b/" so prettification
+                      ;; can "just work!"
+                      nil)
+                     (t
+                      (concat "Modified" kind filemode oldfile)))
+                    'face '(diff-file-header diff-header))
+                   'font-lock-multiline t)))))))
   nil)
 
 ;;; Syntax highlighting from font-lock
-- 
2.30.2


[-- Attachment #5: Type: text/plain, Size: 281 bytes --]


Then it would be possible to introduce a new value for diff-buffer-type,
dedicated to diff-buffers output, and provide a nice prettification
dedicated to that case 🤔 Would a single line like "Differences between buffer
*Help* and *scratch*" be a good idea?
-- 
Matthias