bug#62599: 30.0.50; Improve ibuffer-diff-with-file

bug#62599: 30.0.50; Improve ibuffer-diff-with-file

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

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

Byte-compiling ibuf-ext.el currently emits the following warning:

  In ibuffer-diff-buffer-with-file-1:
  ibuf-ext.el:1660:8: Warning: ‘unwind-protect’ without unwind forms

Trying out ibuffer-diff-with-file, I also see some undesirable
behaviour:

0. emacs -Q
1. M-x find-library RET ibuf-ext RET
2. C-o
3. M-x find-library RET ibuffer RET
4. C-o
5. M-x ibuffer RET
6. * u =

The resulting *Ibuffer Diff* buffer opens with two empty lines and point
on the second, and closes with two 'Diff finished' sentinel lines.

The attached patch fixes these issues, bringing ibuffer-diff-with-file
more in line with its apparent inspiration, diff-buffer-with-file.

WDYT?

BTW, just curious: why is diff-command called through the shell in
diff-no-select (and ibuffer-diff-with-file), rather than directly?

Thanks,

-- 
Basil


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Improve-ibuffer-diff-with-file.patch --]
[-- Type: text/x-diff, Size: 5611 bytes --]

From 9d6d39c237bbe4a4cb333d1258b4e732988ee8e0 Mon Sep 17 00:00:00 2001
From: "Basil L. Contovounesios" <contovob@tcd.ie>
Date: Sat, 1 Apr 2023 15:14:34 +0100
Subject: [PATCH] Improve ibuffer-diff-with-file

* lisp/ibuf-ext.el (ibuffer-diff-with-file): Link to diff-command in
docstring.  Make Diff buffer read-only from outset and inhibit as
needed to avoid surprises.  Check whether diff-command
supports --label.  Leave point at BOB and clean up any excess
newline inserted by ibuffer-diff-buffer-with-file-1.  Prefer
pop-to-buffer-same-window over switch-to-buffer.

(ibuffer-diff-buffer-with-file-1): Add docstring.  Remove unused
unwind-protect and copypasta from diff-no-select.  Use
diff-file-local-copy, string-join, and redisplay in place of
analogues.  Condition --label use on availability, and label buffers
consistently with diff-no-select.  Leave empty line between runs.
Let diff-sentinel delete temporary files.  Leave point at EOB for
next run.
---
 lisp/ibuf-ext.el | 93 ++++++++++++++++++++++++------------------------
 1 file changed, 46 insertions(+), 47 deletions(-)

diff --git a/lisp/ibuf-ext.el b/lisp/ibuf-ext.el
index ed4c8a04db7..550b5ed0e6a 100644
--- a/lisp/ibuf-ext.el
+++ b/lisp/ibuf-ext.el
@@ -1650,68 +1650,67 @@ ibuffer-jump-to-buffer
 	  (error "No buffer with name %s" name)
 	(goto-char buf-point)))))
 
+(declare-function diff-check-labels "diff" (&optional force))
+(declare-function diff-file-local-copy "diff" (file-or-buf))
 (declare-function diff-sentinel "diff"
 		  (code &optional old-temp-file new-temp-file))
 
 (defun ibuffer-diff-buffer-with-file-1 (buffer)
-  (let ((bufferfile (buffer-local-value 'buffer-file-name buffer))
-	(tempfile (make-temp-file "buffer-content-")))
-    (when bufferfile
-      (unwind-protect
-	  (progn
-	    (with-current-buffer buffer
-	      (write-region nil nil tempfile nil 'nomessage))
-	    (let* ((old (expand-file-name bufferfile))
-		   (new (expand-file-name tempfile))
-		   (oldtmp (file-local-copy old))
-		   (newtmp (file-local-copy new))
-		   (switches diff-switches)
-		   (command
-		    (mapconcat
-		     'identity
-		     `(,diff-command
-		       ;; Use explicitly specified switches
-		       ,@(if (listp switches) switches (list switches))
-		       ,@(if (or old new)
-			     (list "-L" (shell-quote-argument old)
-				   "-L" (shell-quote-argument
-					 (format "Buffer %s" (buffer-name buffer)))))
-		       ,(shell-quote-argument (or oldtmp old))
-		       ,(shell-quote-argument (or newtmp new)))
-		     " ")))
-	      (let ((inhibit-read-only t))
-		(insert command "\n")
-		(diff-sentinel
-		 (call-process shell-file-name nil
-			       (current-buffer) nil
-			       shell-command-switch command))
-		(insert "\n")))))
-      (sit-for 0)
-      (when (file-exists-p tempfile)
-	(delete-file tempfile)))))
+  "Compare BUFFER with its associated file, if any.
+Unlike `diff-no-select', insert output into current buffer
+without erasing it."
+  (when-let ((old (buffer-file-name buffer)))
+    (defvar diff-use-labels)
+    (let* ((new buffer)
+           (oldtmp (diff-file-local-copy old))
+           (newtmp (diff-file-local-copy new))
+           (switches diff-switches)
+           (command
+            (string-join
+             `(,diff-command
+               ,@(if (listp switches) switches (list switches))
+               ,@(and (eq diff-use-labels t)
+                      (list "--label" (shell-quote-argument old)
+                            "--label" (shell-quote-argument (format "%S" new))))
+               ,(shell-quote-argument (or oldtmp old))
+               ,(shell-quote-argument (or newtmp new)))
+             " "))
+           (inhibit-read-only t))
+      (insert ?\n command ?\n)
+      (diff-sentinel (call-process shell-file-name nil t nil
+                                   shell-command-switch command)
+                     oldtmp newtmp)
+      (goto-char (point-max)))
+    (redisplay)))
 
 ;;;###autoload
 (defun ibuffer-diff-with-file ()
   "View the differences between marked buffers and their associated files.
 If no buffers are marked, use buffer at point.
-This requires the external program \"diff\" to be in your `exec-path'."
+This requires the external program `diff-command' to be in your
+`exec-path'."
   (interactive)
   (require 'diff)
-  (let ((marked-bufs (ibuffer-get-marked-buffers)))
-    (when (null marked-bufs)
-      (setq marked-bufs (list (ibuffer-current-buffer t))))
-    (with-current-buffer (get-buffer-create "*Ibuffer Diff*")
-      (setq buffer-read-only nil)
-      (buffer-disable-undo (current-buffer))
-      (erase-buffer)
-      (buffer-enable-undo (current-buffer))
+  (let ((marked-bufs (or (ibuffer-get-marked-buffers)
+                         (list (ibuffer-current-buffer t))))
+        (diff-buf (get-buffer-create "*Ibuffer Diff*")))
+    (with-current-buffer diff-buf
+      (setq buffer-read-only t)
+      (buffer-disable-undo)
+      (let ((inhibit-read-only t))
+        (erase-buffer))
+      (buffer-enable-undo)
       (diff-mode)
+      (diff-check-labels)
       (dolist (buf marked-bufs)
 	(unless (buffer-live-p buf)
 	  (error "Buffer %s has been killed" buf))
-	(ibuffer-diff-buffer-with-file-1 buf))
-      (setq buffer-read-only t)))
-  (switch-to-buffer "*Ibuffer Diff*"))
+        (ibuffer-diff-buffer-with-file-1 buf))
+      (goto-char (point-min))
+      (when (= (following-char) ?\n)
+        (let ((inhibit-read-only t))
+          (delete-char 1))))
+    (pop-to-buffer-same-window diff-buf)))
 
 ;;;###autoload
 (defun ibuffer-copy-filename-as-kill (&optional arg)
-- 
2.39.2


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


In GNU Emacs 30.0.50 (build 1, x86_64-pc-linux-gnu, X toolkit, cairo
 version 1.16.0, Xaw3d scroll bars) of 2023-04-01 built on tia
Repository revision: 626f2f744104bc14c28456134cf75ff2f16d9901
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12101007
System Description: Debian GNU/Linux 12 (bookworm)

Configured using:
 'configure 'CFLAGS=-Og -ggdb3' -C --prefix=/home/blc/.local
 --enable-checking=structs --with-file-notification=yes
 --with-x-toolkit=lucid --with-x'

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

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

Major mode: Lisp Interaction

Minor modes in effect:
  tooltip-mode: t
  global-eldoc-mode: t
  eldoc-mode: t
  show-paren-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  line-number-mode: t
  indent-tabs-mode: t
  transient-mark-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message mailcap yank-media puny dired
dired-loaddefs rfc822 mml mml-sec password-cache epa derived epg rfc6068
epg-config gnus-util text-property-search time-date subr-x mm-decode
mm-bodies mm-encode mail-parse rfc2231 mailabbrev gmm-utils mailheader
cl-loaddefs cl-lib sendmail rfc2047 rfc2045 ietf-drums mm-util
mail-prsvr mail-utils rmc iso-transl tooltip cconv 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 nadvice seq simple cl-generic
indonesian philippine 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 abbrev obarray oclosure cl-preloaded button loaddefs
theme-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 x-toolkit
xinput2 x multi-tty make-network-process emacs)

Memory information:
((conses 16 36572 9347)
 (symbols 48 5171 0)
 (strings 32 13854 1584)
 (string-bytes 1 377429)
 (vectors 16 9294)
 (vector-slots 8 148577 13143)
 (floats 8 23 25)
 (intervals 56 245 0)
 (buffers 984 10))

bug#62599: 30.0.50; Improve ibuffer-diff-with-file

[Mattias Engdegård]

Thank you -- I'm no buffer expert but see no obvious flaws.

`verilog-diff-file-with-buffer` in verilog-mode.el looks vaguely similar, down to the dodgy unwind-protect, but it has stronger compatibility requirements so it probably can't use more recent diff machinery.

bug#62599: 30.0.50; Improve ibuffer-diff-with-file

[Mattias Engdegård]

> Thank you -- I'm no buffer expert but see no obvious flaws.

(No obvious flaws with your patch, that is.)

bug#62599: 30.0.50; Improve ibuffer-diff-with-file

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

Mattias Engdegård [2023-04-02 15:31 +0200] wrote:

>> Thank you -- I'm no buffer expert but see no obvious flaws.
>
> (No obvious flaws with your patch, that is.)

Good - I try to keep my obvious character flaws out of my patches ;).

-- 
Basil

bug#62599: 30.0.50; Improve ibuffer-diff-with-file

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

Mattias Engdegård [2023-04-02 10:24 +0200] wrote:

> Thank you -- I'm no buffer expert but see no obvious flaws.

Thanks for reviewing.  I'll wait a few more days for comments before
pushing.

> `verilog-diff-file-with-buffer` in verilog-mode.el looks vaguely
> similar, down to the dodgy unwind-protect, but it has stronger
> compatibility requirements so it probably can't use more recent diff
> machinery.

Oh, that reminded me that call-process has a DISPLAY parameter.  I guess
in ibuffer-diff-buffer-with-file-1 we're still interested in calling
redisplay after diff-sentinel, though:

Basil L. Contovounesios [2023-04-01 23:20 +0100] wrote:

> +      (diff-sentinel (call-process shell-file-name nil t nil
> +                                   shell-command-switch command)
> +                     oldtmp newtmp)
> +      (goto-char (point-max)))
> +    (redisplay)))

I opened https://bugs.gnu.org/62620 for the Verilog case.

Thanks,

-- 
Basil

bug#62599: 30.0.50; Improve ibuffer-diff-with-file

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

close 62599 30.1
quit

Basil Contovounesios [2023-04-02 17:44 +0100] wrote:

> Mattias Engdegård [2023-04-02 10:24 +0200] wrote:
>
>> Thank you -- I'm no buffer expert but see no obvious flaws.
>
> Thanks for reviewing.  I'll wait a few more days for comments before
> pushing.

Done and closing.

Improve ibuffer-diff-with-file
10b58633b56 2023-04-08 11:51:43 +0100
https://git.sv.gnu.org/cgit/emacs.git/commit/?id=10b58633b56

Thanks,

-- 
Basil