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

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

[Matthias Meulien]

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


When `diff-font-lock-prettify' is non-nil, the fontification of a diff
where an empty file is added to a Git project is broken:


[-- Attachment #2: broken_diff.png --]
[-- Type: image/png, Size: 13105 bytes --]

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


where I called `vc-root-diff' from the root of an clone of emacs Git
repository after I ran the following commands:

    matthias@carbon:~/Sources/emacs$ git status
    Sur la branche master
    Votre branche est à jour avec 'origin/master'.
    
    rien à valider, la copie de travail est propre
    matthias@carbon:~/Sources/emacs$ touch empty.py
    matthias@carbon:~/Sources/emacs$ git add empty.py

As one can see from the "git diff" output, there's no line with "---"
nor "+++" signs as expected in the implementation of
`diff--font-lock-prettify':

    matthias@carbon:~/Sources/emacs$ git diff --cached
    diff --git a/empty.py b/empty.py
    new file mode 100644
    index 0000000000..e69de29bb2

Note that empty "__init__.py" files are quite common in Python based
projects.



In GNU Emacs 29.0.50 (build 2, x86_64-pc-linux-gnu, GTK+ Version 3.24.24, cairo version 1.16.0)
 of 2022-02-15 built on carbon
Repository revision: c4ca19e7eb6ed25b1f38f614ef0c2bbb7df12bc3
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12011000
System Description: Debian GNU/Linux 11 (bullseye)

Configured using:
 'configure --with-native-compilation'

Configured features:
ACL CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GPM GSETTINGS HARFBUZZ JPEG
JSON LCMS2 LIBOTF LIBSELINUX LIBSYSTEMD LIBXML2 M17N_FLT MODULES
NATIVE_COMP NOTIFY INOTIFY PDUMPER PNG RSVG SECCOMP SOUND SQLITE3
THREADS TIFF TOOLKIT_SCROLL_BARS WEBP X11 XDBE XIM XPM GTK3 ZLIB

Important settings:
  value of $LANG: fr_FR.UTF-8
  value of $XMODIFIERS: @im=ibus
  locale-coding-system: utf-8-unix

Major mode: Summary

Minor modes in effect:
  highlight-changes-visible-mode: t
  shell-dirtrack-mode: t
  minions-mode: t
  global-company-mode: t
  company-mode: t
  desktop-save-mode: t
  save-place-mode: t
  electric-pair-mode: t
  icomplete-mode: t
  global-so-long-mode: t
  global-auto-revert-mode: t
  auto-insert-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  show-paren-mode: t
  electric-layout-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tab-bar-mode: t
  file-name-shadow-mode: t
  context-menu-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  window-divider-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  buffer-read-only: t
  line-number-mode: t
  indent-tabs-mode: t
  transient-mark-mode: t

Load-path shadows:
/home/matthias/.config/emacs/elpa/transient-20220130.1941/transient hides /usr/local/share/emacs/29.0.50/lisp/transient
/home/matthias/.config/emacs/elpa/dictionary-20201001.1727/dictionary hides /usr/local/share/emacs/29.0.50/lisp/net/dictionary

Features:
(shadow emacsbug sendmail sort smiley gnus-cite mm-archive mail-extr
textsec uni-scripts idna-mapping ucs-normalize uni-confusable
textsec-check gnus-bcklg gnus-async gnus-ml disp-table gnus-topic
nndraft nnmh nnfolder utf-7 epa-file gnutls network-stream nsm
gnus-agent gnus-srvr gnus-score score-mode nnvirtual gnus-msg gnus-cache
image-dired woman man dabbrev sh-script executable mhtml-mode css-mode
hideshow cap-words superword subword js generic meson-mode smie misearch
multi-isearch conf-mode yaml-mode make-mode rng-xsd xsd-regexp rng-cmpct
rng-nxml rng-valid nxml-mode nxml-outln nxml-rap sgml-mode facemenu rst
smerge-mode diff whitespace hl-line add-log log-view pcvs-util follow
mule-util bug-reference flyspell ox-odt rng-loc rng-uri rng-parse
rng-match rng-dt rng-util rng-pttrn nxml-parse nxml-ns nxml-enc xmltok
nxml-util ox-latex ox-icalendar org-agenda ox-html table ox-ascii
ox-publish ox goto-addr org-element avl-tree ol-eww eww xdg url-queue
mm-url ol-rmail ol-mhe ol-irc ol-info ol-gnus nnselect gnus-art mm-uu
mml2015 mm-view mml-smime smime dig gnus-sum shr pixel-fill kinsoku svg
dom ol-docview doc-view jka-compr image-mode exif ol-bibtex ol-bbdb
ol-w3m ol-doi org-link-doi dired-aux display-line-numbers hilit-chg
vc-mtn vc-hg vc-bzr vc-src vc-sccs vc-svn vc-cvs vc-rcs python vc-dir vc
bash-completion shell eglot array jsonrpc ert ewoc debug backtrace
flymake-proc flymake compile imenu company-oddmuse company-keywords
company-etags etags fileloop generator xref project company-gtags
company-dabbrev-code company-dabbrev company-files company-clang
company-capf company-cmake company-semantic company-template
company-bbdb avoid minions company pcase carbon-custom cus-edit cus-load
gnus-demon nntp gnus-group gnus-undo gnus-start gnus-dbus dbus xml
gnus-cloud nnimap nnmail mail-source utf7 netrc parse-time gnus-spec
gnus-win nnoo gnus-int gnus-range message yank-media rmc puny rfc822 mml
mml-sec epa derived epg rfc6068 epg-config mm-decode mm-bodies mm-encode
mail-parse rfc2231 rfc2047 rfc2045 ietf-drums mailabbrev gmm-utils
mailheader gnus nnheader gnus-util mail-utils range mm-util mail-prsvr
wid-edit gnus-dired dired-x dired dired-loaddefs org-capture org-refile
org ob ob-tangle ob-ref ob-lob ob-table ob-exp org-macro org-footnote
org-src ob-comint org-pcomplete pcomplete comint ansi-color ring
org-list org-faces org-entities org-version ob-emacs-lisp ob-core
ob-eval org-table oc-basic bibtex iso8601 time-date ol org-keys oc
org-compat org-macs org-loaddefs format-spec find-func cal-menu calendar
cal-loaddefs dictionary link connection advice markdown-mode
edit-indirect color thingatpt noutline outline skeleton find-file vc-git
diff-mode easy-mmode vc-dispatcher ispell comp comp-cstr warnings rx
cl-extra help-mode desktop frameset server bookmark text-property-search
pp saveplace elec-pair icomplete so-long autorevert filenotify
autoinsert cc-mode cc-fonts cc-guess cc-menus cc-cmds cc-styles cc-align
cc-engine cc-vars cc-defs generic-x face-remap proof-site
proof-autoloads info package browse-url url url-proxy url-privacy
url-expand url-methods url-history url-cookie url-domsuf url-util
mailcap url-handlers url-parse auth-source cl-seq eieio eieio-core
cl-macs eieio-loaddefs password-cache json map url-vars seq gv subr-x
byte-opt bytecomp byte-compile cconv cl-loaddefs cl-lib iso-transl
tooltip eldoc paren electric uniquify ediff-hook vc-hooks
lisp-float-type elisp-mode mwheel term/x-win x-win term/common-win x-dnd
tool-bar dnd fontset image regexp-opt fringe tabulated-list replace
newcomment text-mode lisp-mode prog-mode register page tab-bar menu-bar
rfn-eshadow isearch easymenu timer select scroll-bar mouse jit-lock
font-lock syntax font-core term/tty-colors frame minibuffer cl-generic
cham georgian utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao
korean japanese eucjp-ms cp51932 hebrew greek romanian slovak czech
european ethiopic indian cyrillic chinese composite emoji-zwj charscript
charprop case-table epa-hook jka-cmpr-hook help simple abbrev obarray
cl-preloaded nadvice button loaddefs faces cus-face macroexp files
window text-properties overlay sha1 md5 base64 format env code-pages
mule custom widget keymap hashtable-print-readable backquote threads
dbusbind inotify lcms2 dynamic-setting system-font-setting
font-render-setting cairo move-toolbar gtk x-toolkit x multi-tty
make-network-process native-compile emacs)

Memory information:
((conses 16 990058 38601)
 (symbols 48 42390 5)
 (strings 32 194841 12704)
 (string-bytes 1 6139210)
 (vectors 16 98218)
 (vector-slots 8 1847045 66873)
 (floats 8 689 257)
 (intervals 56 17287 491)
 (buffers 992 227))

-- 
Matthias

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

[Matthias Meulien]

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

Matthias Meulien <orontee@gmail.com> writes:

> When `diff-font-lock-prettify' is non-nil, the fontification of a diff
> where an empty file is added to a Git project is broken:

For binary files, there's an analog problem: One would expect
`diff-font-lock-prettify' to hide the "diff --git", "index", etc. lines
and display a user friendly line.


[-- Attachment #2: Capture d’écran du 2022-02-20 08-51-36.png --]
[-- Type: image/png, Size: 38847 bytes --]

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Matthias Meulien [2022-02-20 08:58:05] wrote:
> Matthias Meulien <orontee@gmail.com> writes:
>> When `diff-font-lock-prettify' is non-nil, the fontification of a diff
>> where an empty file is added to a Git project is broken:
> For binary files, there's an analog problem: One would expect
> `diff-font-lock-prettify' to hide the "diff --git", "index", etc. lines
> and display a user friendly line.

Ah... now I understand: when I looked at your previous email, I couldn't
see what you thought was wrong, it all looked correct to me, because
what you describe is "not a bug", it's a missing feature ;-)

It should be easy to handle those cases as well.
Can you suggest a good replacement?  (i.e. what should we display
instead of the "diff --git ..." stuff)


        Stefan

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

[Matthias Meulien]

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

> (...)  It should be easy to handle those cases as well.  Can you
> suggest a good replacement?  (i.e. what should we display instead of
> the "diff --git ..." stuff)

When the empty file empty.py is added, I expect to
read:

    new file  empty.py

Analogous expectations with "deleted " and "modified ", the file being a
binary file or not.

Thus my suggestion is to use exactly the same replacement as for
non-empty, text files!

It looks like being empty or binary, implies that the diff won't output
+++, nor --- lines, which makes current regex inappropriate.

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> Thus my suggestion is to use exactly the same replacement as for
> non-empty, text files!

I pushed a patch to `master` which should handle those cases correctly.
Please confirm that it also fixes the problem on your side,


        Stefan

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

[Matthias Meulien]

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

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

> I pushed a patch to `master` which should handle those cases correctly.
> Please confirm that it also fixes the problem on your side,

Yes, thanks a lot Stefan. The fix looks good to me!

I found one unhandled case. When a diff changes a file mode, like in the
following:

    diff --git a/argos/session.py b/argos/session.py
    old mode 100755
    new mode 100644

Note that it becomes "problematic" when the diff cumulates file
modification and mode change for the same file.  I understand that the
"old mode" and "new mode" lines are the culprit since they're not
handled by the regexp used by diff--font-lock-prettify:


[-- Attachment #2: Capture d’écran du 2022-02-21 23-42-45.png --]
[-- Type: image/png, Size: 59275 bytes --]

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


I also saw you added a comment saying "FIXME: Add support for Git's
"rename from/to"?".  I am curious: What do you have in mind?  What kind
of output of "git diff" do you want to support? Do you want to link the
related "New file" and "Deleted file" hunks?
-- 
Matthias

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

[Matthias Meulien]

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

Matthias Meulien <orontee@gmail.com> writes:

> I also saw you added a comment saying "FIXME: Add support for Git's
> "rename from/to"?".  I am curious: What do you have in mind?  What kind
> of output of "git diff" do you want to support? Do you want to link the
> related "New file" and "Deleted file" hunks?

Ok I guess I now understand what you had in mind:


[-- Attachment #2: Capture d’écran du 2022-02-22 00-08-43.png --]
[-- Type: image/png, Size: 65591 bytes --]

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


-- 
Matthias

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> I found one unhandled case. When a diff changes a file mode, like in the
> following:
>
>     diff --git a/argos/session.py b/argos/session.py
>     old mode 100755
>     new mode 100644

I just pushed a change for that as well.
Of course, there's always the problem that this prettification
hides info, so there's a tension here.


        Stefan

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

[Matthias Meulien]

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

> I just pushed a change for that as well.

Thank you!

> Of course, there's always the problem that this prettification
> hides info, so there's a tension here.

You're right.

Would changing the string "Modified file  " to "Modified file and file
mode  " be better when both the file and the file mode are changed?

We also could handle "mode changes" only hunks with the string "Modified
file mode ".

Does this proposal looks good to you? I can try to implement it.

Unfortunately for the last case we'll have to modify the regex to match
the file name from the "diff --git" line...

To improve the situation on hidden info, would it make sense to provide
a `diff-toggle-display-properties-at-point`?  Or/and provide the
original text as tooltip?  Just thinking.
-- 
Matthias

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

[Matthias Meulien]

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

> Of course, there's always the problem that this prettification
> hides info, so there's a tension here.

Not directly related but same tension with the motion commands
`diff-file-next', `diff-file-prev', `diff-hunk-next' and
`diff-hunk-prev'.  When the buffer contains a file mode only change,
this change is simply skipped by those motion commands!
-- 
Matthias

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

[Matthias Meulien]

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

Hi Stefan,

Stefan Monnier <monnier@iro.umontreal.ca> writes:
> Of course, there's always the problem that this prettification
> hides info, so there's a tension here.

Here is a patch that capture file mode information and displays that
information.

Screenshot:


[-- Attachment #2: Capture d’écran du 2022-04-07 00-36-38.png --]
[-- Type: image/png, Size: 107252 bytes --]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0001-Display-file-mode-information-when-diff-font-lock-pr.patch --]
[-- Type: text/x-diff, Size: 1102 bytes --]

From 09d1fa92b73ce956e75608f1a8a4c9fee8e7ca70 Mon Sep 17 00:00:00 2001
From: Matthias Meulien <orontee@gmail.com>
Date: Thu, 7 Apr 2022 00:47:31 +0200
Subject: [PATCH] Display file mode information when diff font lock prettify
 enabled

* lisp/vc/diff-mode.el (diff--font-lock-prettify): Make regexp capture
file mode information.
---
 lisp/vc/diff-mode.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index 251d3dc090..7de427d4fd 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -2665,7 +2665,7 @@ diff--font-lock-prettify
                             (concat " file with mode " (match-string 10) "  ")
                           " file  "))
               (modechanged (when (and (match-beginning 8) (match-beginning 9))
-                             (concat " mode changed from "
+                             (concat "mode changed from "
                                      (match-string 8) " to " (match-string 9)))))
           (add-text-properties
            (match-beginning 0) (1- (match-end 0))
-- 
2.30.2

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

[Matthias Meulien]

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

Here is an updated patch with a small display improvement: File name
doesn't appear in the middle of the line anymore.


[-- Attachment #2: Capture d’écran du 2022-04-07 09-07-46.png --]
[-- Type: image/png, Size: 116913 bytes --]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0001-Display-file-mode-information-when-diff-font-lock-pr.patch --]
[-- Type: text/x-diff, Size: 2110 bytes --]

From 35ae0c0d343cc530cb79ee08fbedd641beb212ee Mon Sep 17 00:00:00 2001
From: Matthias Meulien <orontee@gmail.com>
Date: Thu, 7 Apr 2022 00:47:31 +0200
Subject: [PATCH] Display file mode information when diff font lock prettify
 enabled

* lisp/vc/diff-mode.el (diff--font-lock-prettify): Make regexp capture
file mode information.
---
 lisp/vc/diff-mode.el | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index 251d3dc090..c6409d1677 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -2661,12 +2661,14 @@ diff--font-lock-prettify
                                   (match-beginning 5)
                                   (not (match-beginning 3)))
                         " empty")))
-              (filemode (if (match-beginning 10)
-                            (concat " file with mode " (match-string 10) "  ")
-                          " file  "))
-              (modechanged (when (and (match-beginning 8) (match-beginning 9))
-                             (concat " mode changed from "
-                                     (match-string 8) " to " (match-string 9)))))
+              (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
@@ -2680,7 +2682,7 @@ diff--font-lock-prettify
                    ((null (match-string 2))
                     (concat "Deleted" kind filemode oldfile))
                    (t
-                    (concat "Modified" kind " file  " oldfile modechanged)))
+                    (concat "Modified" kind filemode oldfile)))
                   'face '(diff-file-header diff-header))
                  'font-lock-multiline t))))))
   nil)
-- 
2.30.2

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

[Matthias Meulien]

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

Matthias Meulien <orontee@gmail.com> writes:

> Here is an updated patch with a small display improvement: File name
> doesn't appear in the middle of the line anymore.

Arfff... Forgot that I had a work in progress commit on the head of my
branch...

Here is a complete patch. Sorry for the noise.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Display-file-mode-information-when-diff-font-lock-pr.patch --]
[-- Type: text/x-diff, Size: 4516 bytes --]

From fc7a87d739877368227ab5c6206e771e0044b45e Mon Sep 17 00:00:00 2001
From: Matthias Meulien <orontee@gmail.com>
Date: Thu, 7 Apr 2022 00:11:55 +0200
Subject: [PATCH] Display file mode information when diff font lock prettify
 enabled

* lisp/vc/diff-mode.el (diff--font-lock-prettify): Make regexp capture
file mode information.
---
 lisp/vc/diff-mode.el | 43 ++++++++++++++++++++++++++++---------------
 1 file changed, 28 insertions(+), 15 deletions(-)

diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index 511cc89778..c6409d1677 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -2634,42 +2634,55 @@ diff--font-lock-prettify
                         (binary (concat
                                  "Binary files " file4
                                  " and " file5 " \\(?7:differ\\)\n"))
-                        (horb (concat "\\(?:" header "\\|" binary "\\)")))
+                        (horb (concat "\\(?:" header "\\|" binary "\\)?")))
                    (concat "diff.*?\\(?: a/\\(.*?\\) b/\\(.*\\)\\)?\n"
-                           "\\(?:\\(?:old\\|new\\) mode .*\n\\)*"
                            "\\(?:"
                            ;; For new/deleted files, there might be no
                            ;; header (and no hunk) if the file is/was empty.
-                           "\\(?3:new\\(?6:\\)\\|deleted\\) file.*\n"
-                           index "\\(?:" horb "\\)?"
-                           ;; Normal case.
-                           "\\|" index horb "\\)")))))
+                           "\\(?3:new\\(?6:\\)\\|deleted\\) file mode \\(?10:[0-9]\\{6\\}\\)\n"
+                           index horb
+                           ;; Normal case. There might be no header
+                           ;; (and no hunk) if only the file mode
+                           ;; changed.
+                           "\\|"
+                           "\\(?:old mode \\(?8:[0-9]\\{6\\}\\)\n\\)?"
+                           "\\(?:new mode \\(?9:[0-9]\\{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 file modes (and the index hashes).
+        ;; 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))
-                       " empty"))))
+                      (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 " file"
-                            "  "
+                    (concat (capitalize (match-string 3)) kind filemode
                             (if (match-beginning 6) newfile oldfile)))
-                   ((null (match-string 4))
-                    (concat "New" kind " file  " newfile))
+                   ((and (null (match-string 4)) (match-string 5))
+                    (concat "New " kind filemode newfile))
                    ((null (match-string 2))
-                    (concat "Deleted" kind " file  " oldfile))
+                    (concat "Deleted" kind filemode oldfile))
                    (t
-                    (concat "Modified" kind " file  " oldfile)))
+                    (concat "Modified" kind filemode oldfile)))
                   'face '(diff-file-header diff-header))
                  'font-lock-multiline t))))))
   nil)
-- 
2.30.2

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

[Matthias Meulien]

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

As seen in git-diff man page, file modes are printed as 6-digit octal
numbers.  I changed the corresponding regexp accordingly.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Display-file-mode-information-when-diff-font-lock-pr.patch --]
[-- Type: text/x-diff, Size: 4516 bytes --]

From 6ada335f2ff58bc9dd9cf9cf8d2a571087b49600 Mon Sep 17 00:00:00 2001
From: Matthias Meulien <orontee@gmail.com>
Date: Thu, 7 Apr 2022 00:11:55 +0200
Subject: [PATCH] Display file mode information when diff font lock prettify
 enabled

* lisp/vc/diff-mode.el (diff--font-lock-prettify): Make regexp capture
file mode information.
---
 lisp/vc/diff-mode.el | 43 ++++++++++++++++++++++++++++---------------
 1 file changed, 28 insertions(+), 15 deletions(-)

diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index 511cc89778..5c13c7fc38 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -2634,42 +2634,55 @@ diff--font-lock-prettify
                         (binary (concat
                                  "Binary files " file4
                                  " and " file5 " \\(?7:differ\\)\n"))
-                        (horb (concat "\\(?:" header "\\|" binary "\\)")))
+                        (horb (concat "\\(?:" header "\\|" binary "\\)?")))
                    (concat "diff.*?\\(?: a/\\(.*?\\) b/\\(.*\\)\\)?\n"
-                           "\\(?:\\(?:old\\|new\\) mode .*\n\\)*"
                            "\\(?:"
                            ;; For new/deleted files, there might be no
                            ;; header (and no hunk) if the file is/was empty.
-                           "\\(?3:new\\(?6:\\)\\|deleted\\) file.*\n"
-                           index "\\(?:" horb "\\)?"
-                           ;; Normal case.
-                           "\\|" index horb "\\)")))))
+                           "\\(?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 file modes (and the index hashes).
+        ;; 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))
-                       " empty"))))
+                      (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 " file"
-                            "  "
+                    (concat (capitalize (match-string 3)) kind filemode
                             (if (match-beginning 6) newfile oldfile)))
-                   ((null (match-string 4))
-                    (concat "New" kind " file  " newfile))
+                   ((and (null (match-string 4)) (match-string 5))
+                    (concat "New " kind filemode newfile))
                    ((null (match-string 2))
-                    (concat "Deleted" kind " file  " oldfile))
+                    (concat "Deleted" kind filemode oldfile))
                    (t
-                    (concat "Modified" kind " file  " oldfile)))
+                    (concat "Modified" kind filemode oldfile)))
                   'face '(diff-file-header diff-header))
                  'font-lock-multiline t))))))
   nil)
-- 
2.30.2

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Thanks Matthias, this looks nice.
Pushed to `master`.


        Stefan

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

[Matthias Meulien]

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

> Thanks Matthias, this looks nice.
> Pushed to `master`.

Thank you Stefan.

I'll create a new bug report for the bug I found related to the motion
commands `diff-file-next', `diff-file-prev', `diff-hunk-next' and
`diff-hunk-prev': When the buffer contains a file mode only change, this
change is simply skipped by those motion commands.  I think it's better
since it isn't related to diff prettify.
-- 
Matthias

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

[Basil L. Contovounesios via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Matthias Meulien [2022-04-07 14:15 +0200] wrote:

> From: Matthias Meulien <orontee@gmail.com>
> Date: Thu, 7 Apr 2022 00:11:55 +0200
> Subject: [PATCH] Display file mode information when diff font lock prettify
>  enabled
>
> * lisp/vc/diff-mode.el (diff--font-lock-prettify): Make regexp capture
> file mode information.

I think this gave rise to the following regression in the header when
diff-font-lock-prettify is enabled:

0. emacs -Q
1. M-x diff-buffers RET RET RET
   [ Header now comprises 'diff -u ...'. ]
2. (setq diff-font-lock-prettify t) C-x C-e
3. C-x o g
   [ Header is now 'Deleted file'. ]

It's not particularly jarring to see a header like that when diffing
non-file-visiting buffers, but more importantly the same header appears
when diffing any regular files via M-x diff.

Thanks,

-- 
Basil

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Basil L. Contovounesios [2022-06-24 01:36:15] wrote:
> I think this gave rise to the following regression in the header when
> diff-font-lock-prettify is enabled:
>
> 0. emacs -Q
> 1. M-x diff-buffers RET RET RET
>    [ Header now comprises 'diff -u ...'. ]
> 2. (setq diff-font-lock-prettify t) C-x C-e
> 3. C-x o g
>    [ Header is now 'Deleted file'. ]
>
> It's not particularly jarring to see a header like that when diffing
> non-file-visiting buffers, but more importantly the same header appears
> when diffing any regular files via M-x diff.

[ 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?


        Stefan


diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index 0fd67422d55..3f3e503a3f3 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -2682,7 +2682,17 @@ diff--font-lock-prettify
                    ((and (null (match-string 4)) (match-string 5))
                     (concat "New " kind filemode newfile))
                    ((null (match-string 2))
-                    (concat "Deleted" kind filemode oldfile))
+                    ;; 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))

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

[Basil L. Contovounesios via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Stefan Monnier [2022-06-29 11:47 -0400] wrote:

> [ It'd have been better to file a new bug report for this one, FWIW.  ]

I would have done that next, but you're right, I should have done that
to begin with, sorry.

> I installed the patch below, which seems safe, but is probably
> not optimal.

WFM in any case.

Thanks,

-- 
Basil

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

[Matthias Meulien]

[-- 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

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

Could be, tho I think the current code ends up doing something similar.

FWIW, I tend to prefer using the buffer's content rather than the value
of a buffer-local variable (to the extend possible) when deciding
whether a given diff header can be prettified.

[ After all, some diffs may be the result of running several commands,
  some of which are Git but not all, so the "type" may be different for
  different headers in the same buffer.  ]


        Stefan

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

[Basil L. Contovounesios via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Matthias Meulien [2022-06-29 20:22 +0200] wrote:

> 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?

Note that the problem is not specific to 'diff-buffers', but applies
more generally to non-Git clients of 'diff', including file-backed ones
like 'M-x diff' and Dired's '=' binding, 'dired-diff'.

Thanks,

-- 
Basil

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

[Matthias Meulien]

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

>> Wouldn't it be safer to simply disable prettification of the "diff
>> header" when diff-buffer-type isn't equal to git?
>
> Could be, tho I think the current code ends up doing something similar.
>
> FWIW, I tend to prefer using the buffer's content rather than the value
> of a buffer-local variable (to the extend possible) when deciding
> whether a given diff header can be prettified.

The buffer-local variable is currently initialized from the buffer
content!

>
> [ After all, some diffs may be the result of running several commands,
>   some of which are Git but not all, so the "type" may be different for
>   different headers in the same buffer.  ]

Ok, I understand your point.

I can't see how a regex can distinguish diff output generated in
the context of a call to diff-buffers (where compared files are
temporary files and a prettified message should refer to the
corresponding buffer names) and a generic diff output where a user may
have used any diff option including --label for whatever reason.
-- 
Matthias