bug#73172: [PATCH] Move to start of current header in diff-{file, hunk}-prev

bug#73172: [PATCH] Move to start of current header in diff-{file, hunk}-prev

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

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

Tags: patch


(The following change is split across two patches; the first one, "move
easy-mmode", fixes an unrelated FIXME, which makes the diff in the
second patch simpler)

If point was after a file or hunk header, the diff-file-prev and
diff-hunk-prev commands would move to the start of that header.
But if point was *within* the header, they would not move, and
would report "No previous file" or "No previous hunk".  This
differs from the behavior of most other movement commands,
e.g. backward-sexp or backward-sentence.

This commit fixes diff-file-prev and diff-hunk-prev, as well as
other easy-mmode-define-navigation BASE-prev commands.  Now
these commands move to the start of the containing "thing" just
like other movement commands.

* lisp/emacs-lisp/easy-mmode.el (easy-mmode--prev): Move to
start of current match first.

Also discussed here:
https://lists.gnu.org/archive/html/help-gnu-emacs/2024-08/msg00367.html

In GNU Emacs 29.2.50 (build 17, x86_64-pc-linux-gnu, X toolkit, cairo
 version 1.15.12, Xaw scroll bars) of 2024-09-06 built on
 igm-qws-u22796a
Repository revision: e6d04c06a7eb6ce932b52a346368d02b7a811a00
Repository branch: emacs-29
Windowing system distributor 'The X.Org Foundation', version 11.0.12011000
System Description: Rocky Linux 8.10 (Green Obsidian)

Configured using:
 'configure --with-x-toolkit=lucid --without-gpm --without-gconf
 --without-selinux --without-imagemagick --with-modules --with-gif=no
 --with-cairo --with-rsvg --without-compress-install
 --with-native-compilation=aot --with-tree-sitter
 PKG_CONFIG_PATH=/usr/local/home/garnish/libtree-sitter/0.22.6-1/lib/pkgconfig/'


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Move-easy-mmode-define-navigation-logic-to-helper-fu.patch --]
[-- Type: text/x-patch, Size: 6106 bytes --]

From 93f50388bda9f986a5aa8e51378793031cfdce05 Mon Sep 17 00:00:00 2001
From: Spencer Baugh <sbaugh@janestreet.com>
Date: Tue, 10 Sep 2024 13:46:18 -0400
Subject: [PATCH] Move easy-mmode-define-navigation logic to helper functions

The functions defined by easy-mmode-define-navigation are useful
even if the easy-mmode-define-navigation macro is not used.
Let's take a step towards exposing them by moving them out as
helpers.

This also makes the macro much easier to modify and work on.

* lisp/emacs-lisp/easy-mmode.el (easy-mmode--prev)
(easy-mmode--next): Add.
(easy-mmode-define-navigation): Use easy-mmode--prev and
easy-mmode--next.
---
 lisp/emacs-lisp/easy-mmode.el | 86 ++++++++++++++++++++---------------
 1 file changed, 49 insertions(+), 37 deletions(-)

diff --git a/lisp/emacs-lisp/easy-mmode.el b/lisp/emacs-lisp/easy-mmode.el
index a140027839e..d3dcab899d6 100644
--- a/lisp/emacs-lisp/easy-mmode.el
+++ b/lisp/emacs-lisp/easy-mmode.el
@@ -763,6 +763,48 @@ easy-mmode-defsyntax
 ;;; easy-mmode-define-navigation
 ;;;
 
+(defun easy-mmode--prev (re name count &optional endfun narrowfun)
+  "Go to the previous COUNT'th occurence of RE.
+
+If none, error with NAME.
+
+ENDFUN and NARROWFUN are treated like in `easy-mmode-define-navigation'."
+  (unless count (setq count 1))
+  (if (< count 0) (easy-mmode--next re name (- count) endfun narrowfun)
+    (let ((re-narrow (and narrowfun (prog1 (buffer-narrowed-p) (widen)))))
+      (unless (re-search-backward re nil t count)
+        (user-error "No previous %s" name))
+      (when re-narrow (funcall narrowfun)))))
+
+(defun easy-mmode--next (re name count &optional endfun narrowfun)
+  "Go to the next COUNT'th occurence of RE.
+
+If none, error with NAME.
+
+ENDFUN and NARROWFUN are treated like in `easy-mmode-define-navigation'."
+  (unless count (setq count 1))
+  (if (< count 0) (easy-mmode--prev re name (- count) endfun narrowfun)
+    (if (looking-at re) (setq count (1+ count)))
+    (let ((re-narrow (and narrowfun (prog1 (buffer-narrowed-p) (widen)))))
+      (if (not (re-search-forward re nil t count))
+          (if (looking-at re)
+              (goto-char (or (if endfun (funcall endfun)) (point-max)))
+            (user-error "No next %s" name))
+        (goto-char (match-beginning 0))
+        (when (and (eq (current-buffer) (window-buffer))
+                   (called-interactively-p 'interactive))
+          (let ((endpt (or (save-excursion
+                             (if endfun (funcall endfun)
+                               (re-search-forward re nil t 2)))
+                           (point-max))))
+            (unless (pos-visible-in-window-p endpt nil t)
+              (let ((ws (window-start)))
+                (recenter '(0))
+                (if (< (window-start) ws)
+                    ;; recenter scrolled in the wrong direction!
+                    (set-window-start nil ws)))))))
+      (when re-narrow (funcall narrowfun)))))
+
 (defmacro easy-mmode-define-navigation (base re &optional name endfun narrowfun
                                              &rest body)
   "Define BASE-next and BASE-prev to navigate in the buffer.
@@ -780,53 +822,23 @@ easy-mmode-define-navigation
   (let* ((base-name (symbol-name base))
 	 (prev-sym (intern (concat base-name "-prev")))
 	 (next-sym (intern (concat base-name "-next")))
-         (when-narrowed
-          (lambda (body)
-            (if (null narrowfun) body
-              `(let ((was-narrowed (prog1 (buffer-narrowed-p) (widen))))
-                 ,body
-                 (when was-narrowed (funcall #',narrowfun)))))))
+         (endfun (when endfun `#',endfun))
+         (narrowfun (when narrowfun `#',narrowfun)))
     (unless name (setq name base-name))
-    ;; FIXME: Move most of those functions's bodies to helper functions!
     `(progn
        (defun ,next-sym (&optional count)
 	 ,(format "Go to the next COUNT'th %s.
 Interactively, COUNT is the prefix numeric argument, and defaults to 1." name)
 	 (interactive "p")
-	 (unless count (setq count 1))
-	 (if (< count 0) (,prev-sym (- count))
-	   (if (looking-at ,re) (setq count (1+ count)))
-           ,(funcall when-narrowed
-             `(if (not (re-search-forward ,re nil t count))
-                  (if (looking-at ,re)
-                      (goto-char (or ,(if endfun `(funcall #',endfun)) (point-max)))
-                    (user-error "No next %s" ,name))
-                (goto-char (match-beginning 0))
-                (when (and (eq (current-buffer) (window-buffer))
-                           (called-interactively-p 'interactive))
-                  (let ((endpt (or (save-excursion
-                                     ,(if endfun `(funcall #',endfun)
-                                        `(re-search-forward ,re nil t 2)))
-                                   (point-max))))
-                    (unless (pos-visible-in-window-p endpt nil t)
-                      (let ((ws (window-start)))
-                        (recenter '(0))
-                        (if (< (window-start) ws)
-                            ;; recenter scrolled in the wrong direction!
-                            (set-window-start nil ws))))))))
-           ,@body))
+         (easy-mmode--next ,re ,name count ,endfun ,narrowfun)
+         ,@body)
        (put ',next-sym 'definition-name ',base)
        (defun ,prev-sym (&optional count)
 	 ,(format "Go to the previous COUNT'th %s.
-Interactively, COUNT is the prefix numeric argument, and defaults to 1."
-                  (or name base-name))
+Interactively, COUNT is the prefix numeric argument, and defaults to 1." name)
 	 (interactive "p")
-	 (unless count (setq count 1))
-	 (if (< count 0) (,next-sym (- count))
-           ,(funcall when-narrowed
-             `(unless (re-search-backward ,re nil t count)
-                (user-error "No previous %s" ,name)))
-           ,@body))
+         (easy-mmode--prev ,re ,name count ,endfun ,narrowfun)
+         ,@body)
        (put ',prev-sym 'definition-name ',base))))
 
 ;; When deleting these two, also delete them from loaddefs-gen.el.
-- 
2.39.3


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0001-Move-to-start-of-current-header-in-diff-file-hunk-pr.patch --]
[-- Type: text/patch, Size: 2131 bytes --]

From 3bf96598caae3746001fdd5d4f4a4d5a14bdf717 Mon Sep 17 00:00:00 2001
From: Spencer Baugh <sbaugh@janestreet.com>
Date: Tue, 10 Sep 2024 14:18:39 -0400
Subject: [PATCH] Move to start of current header in diff-{file,hunk}-prev

If point was after a file or hunk header, the diff-file-prev and
diff-hunk-prev commands would move to the start of that header.
But if point was *within* the header, they would not move, and
would report "No previous file" or "No previous hunk".  This
differs from the behavior of most other movement commands,
e.g. backward-sexp or backward-sentence.

This commit fixes diff-file-prev and diff-hunk-prev, as well as
other easy-mmode-define-navigation BASE-prev commands.  Now
these commands move to the start of the containing "thing" just
like other movement commands.

* lisp/emacs-lisp/easy-mmode.el (easy-mmode--prev): Move to
start of current match first.
---
 lisp/emacs-lisp/easy-mmode.el | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/lisp/emacs-lisp/easy-mmode.el b/lisp/emacs-lisp/easy-mmode.el
index d3dcab899d6..7a94d832273 100644
--- a/lisp/emacs-lisp/easy-mmode.el
+++ b/lisp/emacs-lisp/easy-mmode.el
@@ -772,6 +772,17 @@ easy-mmode--prev
   (unless count (setq count 1))
   (if (< count 0) (easy-mmode--next re name (- count) endfun narrowfun)
     (let ((re-narrow (and narrowfun (prog1 (buffer-narrowed-p) (widen)))))
+      ;; If point is inside a match for RE, move to its beginning like
+      ;; `backward-sexp' and other movement commands.
+      (when (and (not (zerop count))
+                 (save-excursion
+                   ;; Make sure we're out of the current match if any.
+                   (goto-char (if (re-search-backward re nil t 1)
+                                  (match-end 0) (point-min)))
+                   (re-search-forward re nil t 1))
+                 (< (match-beginning 0) (point) (match-end 0)))
+        (goto-char (match-beginning 0))
+        (setq count (1- count)))
       (unless (re-search-backward re nil t count)
         (user-error "No previous %s" name))
       (when re-narrow (funcall narrowfun)))))
-- 
2.39.3

bug#73172: [PATCH] Move to start of current header in diff-{file, hunk}-prev

[Dmitry Gutov]

Hi Spencer and Stefan,

On 10/09/2024 21:40, Spencer Baugh via Bug reports for GNU Emacs, the 
Swiss army knife of text editors wrote:

> (The following change is split across two patches; the first one, "move
> easy-mmode", fixes an unrelated FIXME, which makes the diff in the
> second patch simpler)
> 
> If point was after a file or hunk header, the diff-file-prev and
> diff-hunk-prev commands would move to the start of that header.
> But if point was *within* the header, they would not move, and
> would report "No previous file" or "No previous hunk".  This
> differs from the behavior of most other movement commands,
> e.g. backward-sexp or backward-sentence.
> 
> This commit fixes diff-file-prev and diff-hunk-prev, as well as
> other easy-mmode-define-navigation BASE-prev commands.  Now
> these commands move to the start of the containing "thing" just
> like other movement commands.
> 
> * lisp/emacs-lisp/easy-mmode.el (easy-mmode--prev): Move to
> start of current match first.
> 
> Also discussed here:
> https://lists.gnu.org/archive/html/help-gnu-emacs/2024-08/msg00367.html

Patch#1 seems unequivocally a good thing (easier code iteration) and 
patch#2 seems good on balance.

It does introduce some backward incompatibility in rare cases where I 
have probably internalized the current behavior already -- for example 
in the vc-print-root-log output pressing 'p' while on the first line 
somewhere between the initial '*' and the end of the date dings with "No 
previous log message", and how will move to bol.

But it might be more consistent anyway, given that the there is no ding 
already if you start out inside the summary text.

There aren't many in-tree callers of easy-mmode-define-navigation 
(diff-mode, log-view, smerge and, uh, cvs-status), and few 3rd party 
callers, so this seems low-impact. diff-hl-mode is not affected either 
because it only uses diff-hunk-next, not diff-hunk-prev.

bug#73172: [PATCH] Move to start of current header in diff-{file, hunk}-prev

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

Dmitry Gutov <dmitry@gutov.dev> writes:
> Hi Spencer and Stefan,
>
> On 10/09/2024 21:40, Spencer Baugh via Bug reports for GNU Emacs, the
> Swiss army knife of text editors wrote:
>
>> (The following change is split across two patches; the first one, "move
>> easy-mmode", fixes an unrelated FIXME, which makes the diff in the
>> second patch simpler)
>> If point was after a file or hunk header, the diff-file-prev and
>> diff-hunk-prev commands would move to the start of that header.
>> But if point was *within* the header, they would not move, and
>> would report "No previous file" or "No previous hunk".  This
>> differs from the behavior of most other movement commands,
>> e.g. backward-sexp or backward-sentence.
>> This commit fixes diff-file-prev and diff-hunk-prev, as well as
>> other easy-mmode-define-navigation BASE-prev commands.  Now
>> these commands move to the start of the containing "thing" just
>> like other movement commands.
>> * lisp/emacs-lisp/easy-mmode.el (easy-mmode--prev): Move to
>> start of current match first.
>> Also discussed here:
>> https://lists.gnu.org/archive/html/help-gnu-emacs/2024-08/msg00367.html
>
> Patch#1 seems unequivocally a good thing (easier code iteration)

BTW, if we want to go ahead and install that patch on its own, that
would be fine with me.

> and patch#2 seems good on balance.
>
> It does introduce some backward incompatibility in rare cases where I
> have probably internalized the current behavior already -- for example
> in the vc-print-root-log output pressing 'p' while on the first line
> somewhere between the initial '*' and the end of the date dings with
> "No previous log message", and how will move to bol.
>
> But it might be more consistent anyway, given that the there is no
> ding already if you start out inside the summary text.

Yes.  It's definitely a change in behavior.

But, if someone has internalized:

  diff-file-prev usually acts like "backward-sexp for files" (it moves
  point to the start of the current file), except that if point is in
  the header of the first file in the buffer then diff-file-prev does
  nothing instead.

Then I think they'll be happy to be able to discard that knowledge
in favor of:

  diff-file-prev always acts like "backward-sexp for files" (it moves
  point to the start of the current file).

Especially because "does nothing in one weird corner case" is not useful
or deliberate behavior (it's just a bug).

> There aren't many in-tree callers of easy-mmode-define-navigation
> (diff-mode, log-view, smerge and, uh, cvs-status), and few 3rd party
> callers, so this seems low-impact. diff-hl-mode is not affected either
> because it only uses diff-hunk-next, not diff-hunk-prev.

Yep.  I think it will be a nice improvement for those few other commands
as well.

bug#73172: [PATCH] Move to start of current header in diff-{file, hunk}-prev

[Dmitry Gutov]

On 19/09/2024 21:41, Spencer Baugh wrote:

>>> Also discussed here:
>>> https://lists.gnu.org/archive/html/help-gnu-emacs/2024-08/msg00367.html
>>
>> Patch#1 seems unequivocally a good thing (easier code iteration)
> 
> BTW, if we want to go ahead and install that patch on its own, that
> would be fine with me.

I think we'll wait a week or so for somebody to protest, and install 
both if nobody does.

>> and patch#2 seems good on balance.
>>
>> It does introduce some backward incompatibility in rare cases where I
>> have probably internalized the current behavior already -- for example
>> in the vc-print-root-log output pressing 'p' while on the first line
>> somewhere between the initial '*' and the end of the date dings with
>> "No previous log message", and how will move to bol.
>>
>> But it might be more consistent anyway, given that the there is no
>> ding already if you start out inside the summary text.
> 
> Yes.  It's definitely a change in behavior.
> 
> But, if someone has internalized:
> 
>    diff-file-prev usually acts like "backward-sexp for files" (it moves
>    point to the start of the current file), except that if point is in
>    the header of the first file in the buffer then diff-file-prev does
>    nothing instead.
> 
> Then I think they'll be happy to be able to discard that knowledge
> in favor of:
> 
>    diff-file-prev always acts like "backward-sexp for files" (it moves
>    point to the start of the current file).
> 
> Especially because "does nothing in one weird corner case" is not useful
> or deliberate behavior (it's just a bug).

Yeah, probably.

It kind of indicated "no elements above" right away, which is a tad 
meaningful, but indeed I struggle to produce a scenario where that would 
make a big difference.

bug#73172: [PATCH] Move to start of current header in diff-{file, hunk}-prev

[Eli Zaretskii]

> Cc: Stefan Monnier <monnier@iro.umontreal.ca>, 73172@debbugs.gnu.org
> Date: Thu, 19 Sep 2024 23:46:00 +0300
> From: Dmitry Gutov <dmitry@gutov.dev>
> 
> On 19/09/2024 21:41, Spencer Baugh wrote:
> 
> >> and patch#2 seems good on balance.
> >>
> >> It does introduce some backward incompatibility in rare cases where I
> >> have probably internalized the current behavior already -- for example
> >> in the vc-print-root-log output pressing 'p' while on the first line
> >> somewhere between the initial '*' and the end of the date dings with
> >> "No previous log message", and how will move to bol.
> >>
> >> But it might be more consistent anyway, given that the there is no
> >> ding already if you start out inside the summary text.
> > 
> > Yes.  It's definitely a change in behavior.
> > 
> > But, if someone has internalized:
> > 
> >    diff-file-prev usually acts like "backward-sexp for files" (it moves
> >    point to the start of the current file), except that if point is in
> >    the header of the first file in the buffer then diff-file-prev does
> >    nothing instead.
> > 
> > Then I think they'll be happy to be able to discard that knowledge
> > in favor of:
> > 
> >    diff-file-prev always acts like "backward-sexp for files" (it moves
> >    point to the start of the current file).
> > 
> > Especially because "does nothing in one weird corner case" is not useful
> > or deliberate behavior (it's just a bug).
> 
> Yeah, probably.
> 
> It kind of indicated "no elements above" right away, which is a tad 
> meaningful, but indeed I struggle to produce a scenario where that would 
> make a big difference.

Please be sure to document all behavior changes in NEWS, if you
install such changes.

bug#73172: [PATCH] Move to start of current header in diff-{file, hunk}-prev

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

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

Eli Zaretskii <eliz@gnu.org> writes:

>> Cc: Stefan Monnier <monnier@iro.umontreal.ca>, 73172@debbugs.gnu.org
>> Date: Thu, 19 Sep 2024 23:46:00 +0300
>> From: Dmitry Gutov <dmitry@gutov.dev>
>> 
>> On 19/09/2024 21:41, Spencer Baugh wrote:
>> 
>> >> and patch#2 seems good on balance.
>> >>
>> >> It does introduce some backward incompatibility in rare cases where I
>> >> have probably internalized the current behavior already -- for example
>> >> in the vc-print-root-log output pressing 'p' while on the first line
>> >> somewhere between the initial '*' and the end of the date dings with
>> >> "No previous log message", and how will move to bol.
>> >>
>> >> But it might be more consistent anyway, given that the there is no
>> >> ding already if you start out inside the summary text.
>> > 
>> > Yes.  It's definitely a change in behavior.
>> > 
>> > But, if someone has internalized:
>> > 
>> >    diff-file-prev usually acts like "backward-sexp for files" (it moves
>> >    point to the start of the current file), except that if point is in
>> >    the header of the first file in the buffer then diff-file-prev does
>> >    nothing instead.
>> > 
>> > Then I think they'll be happy to be able to discard that knowledge
>> > in favor of:
>> > 
>> >    diff-file-prev always acts like "backward-sexp for files" (it moves
>> >    point to the start of the current file).
>> > 
>> > Especially because "does nothing in one weird corner case" is not useful
>> > or deliberate behavior (it's just a bug).
>> 
>> Yeah, probably.
>> 
>> It kind of indicated "no elements above" right away, which is a tad 
>> meaningful, but indeed I struggle to produce a scenario where that would 
>> make a big difference.
>
> Please be sure to document all behavior changes in NEWS, if you
> install such changes.

Updated patch to add to NEWS.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Move-to-start-of-current-header-in-diff-file-hunk-pr.patch --]
[-- Type: text/x-patch, Size: 2873 bytes --]

From d1481a60734c2ce7228cd356e7e0280e7e66cd34 Mon Sep 17 00:00:00 2001
From: Spencer Baugh <sbaugh@janestreet.com>
Date: Tue, 10 Sep 2024 14:18:39 -0400
Subject: [PATCH] Move to start of current header in diff-{file,hunk}-prev

If point was after a file or hunk header, the diff-file-prev and
diff-hunk-prev commands would move to the start of that header.
But if point was *within* the header, they would not move, and
would report "No previous file" or "No previous hunk".  This
differs from the behavior of most other movement commands,
e.g. backward-sexp or backward-sentence.

This commit fixes diff-file-prev and diff-hunk-prev, as well as
other easy-mmode-define-navigation BASE-prev commands.  Now
these commands move to the start of the containing "thing" just
like other movement commands.

* lisp/emacs-lisp/easy-mmode.el (easy-mmode--prev): Move to
start of current match first. (bug#73172)
* etc/NEWS: Document the behavior change.
---
 etc/NEWS                      |  6 ++++++
 lisp/emacs-lisp/easy-mmode.el | 11 +++++++++++
 2 files changed, 17 insertions(+)

diff --git a/etc/NEWS b/etc/NEWS
index 8bd5c7c9515..637a0e63acb 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -304,6 +304,12 @@ according to diffs in the current buffer, but without applying the diffs
 to the original text.  If the selected range extends a hunk, the
 command attempts to look up and copy the text in-between the hunks.
 
+---
+*** diff-file-prev and diff-hunk-prev reliably move to start of header.
+Previously, diff-file-prev and diff-hunk-prev would move when they were
+in the first header in the file, but not at the start of the header.
+Now they will reliably move to the start of the relevant header.
+
 ** php-ts-mode
 
 ---
diff --git a/lisp/emacs-lisp/easy-mmode.el b/lisp/emacs-lisp/easy-mmode.el
index d3dcab899d6..7a94d832273 100644
--- a/lisp/emacs-lisp/easy-mmode.el
+++ b/lisp/emacs-lisp/easy-mmode.el
@@ -772,6 +772,17 @@ easy-mmode--prev
   (unless count (setq count 1))
   (if (< count 0) (easy-mmode--next re name (- count) endfun narrowfun)
     (let ((re-narrow (and narrowfun (prog1 (buffer-narrowed-p) (widen)))))
+      ;; If point is inside a match for RE, move to its beginning like
+      ;; `backward-sexp' and other movement commands.
+      (when (and (not (zerop count))
+                 (save-excursion
+                   ;; Make sure we're out of the current match if any.
+                   (goto-char (if (re-search-backward re nil t 1)
+                                  (match-end 0) (point-min)))
+                   (re-search-forward re nil t 1))
+                 (< (match-beginning 0) (point) (match-end 0)))
+        (goto-char (match-beginning 0))
+        (setq count (1- count)))
       (unless (re-search-backward re nil t count)
         (user-error "No previous %s" name))
       (when re-narrow (funcall narrowfun)))))
-- 
2.39.3

bug#73172: [PATCH] Move to start of current header in diff-{file, hunk}-prev

[Dmitry Gutov]

On 20/09/2024 20:24, Spencer Baugh wrote:
> Updated patch to add to NEWS.

Thanks! Now pushed to master, both changes. Let's see if anybody noticed 
the difference.

I've taken the liberty to rewrite the NEWS entry a little so it makes 
more sense to me, but please feel free to update it more.