[-- 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
[-- 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 --]
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
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.
> 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
[-- 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
[-- 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
> 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
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
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
[-- 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
[-- 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
[-- 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
[-- 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
Thanks Matthias, this looks nice. Pushed to `master`. Stefan
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
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
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))
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
[-- 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
> 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
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
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