From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED!not-for-mail From: Dima Kogan Newsgroups: gmane.emacs.bugs Subject: bug#17544: 24.3; [PATCH] Improved diff-mode navigation/manipulation Date: Sun, 06 Nov 2016 18:26:54 -0800 Message-ID: <87wpgge0kl.fsf@secretsauce.net> References: <87ha4jgw53.fsf@secretsauce.net> <87si0ion1c.fsf@gnus.org> <87k2et8hr4.fsf@secretsauce.net> <83a8fpe1dr.fsf@gnu.org> <87h99w8ynv.fsf@secretsauce.net> <87k2es9weh.fsf@users.sourceforge.net> <87oa4018r3.fsf@secretsauce.net> <87twdi6rm9.fsf@secretsauce.net> <87h997nkqp.fsf@secretsauce.net> <874m44tmge.fsf@users.sourceforge.net> <87h983rg9c.fsf@secretsauce.net> <87mvhvsrta.fsf@users.sourceforge.net> NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="=-=-=" X-Trace: blaine.gmane.org 1478485714 22770 195.159.176.226 (7 Nov 2016 02:28:34 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Mon, 7 Nov 2016 02:28:34 +0000 (UTC) User-Agent: mu4e 0.9.17; emacs 26.0.50.1 Cc: Andreas Schwab , 17544@debbugs.gnu.org To: npostavs@users.sourceforge.net Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Mon Nov 07 03:28:28 2016 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by blaine.gmane.org with esmtp (Exim 4.84_2) (envelope-from ) id 1c3ZfO-0003UZ-Jo for geb-bug-gnu-emacs@m.gmane.org; Mon, 07 Nov 2016 03:28:10 +0100 Original-Received: from localhost ([::1]:50460 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c3ZfR-0005e6-FP for geb-bug-gnu-emacs@m.gmane.org; Sun, 06 Nov 2016 21:28:13 -0500 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:58423) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c3ZfJ-0005dz-FJ for bug-gnu-emacs@gnu.org; Sun, 06 Nov 2016 21:28:07 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c3ZfG-0000mJ-5s for bug-gnu-emacs@gnu.org; Sun, 06 Nov 2016 21:28:05 -0500 Original-Received: from debbugs.gnu.org ([208.118.235.43]:60175) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1c3ZfG-0000mD-2W for bug-gnu-emacs@gnu.org; Sun, 06 Nov 2016 21:28:02 -0500 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1c3ZfF-0005TI-P8 for bug-gnu-emacs@gnu.org; Sun, 06 Nov 2016 21:28:01 -0500 X-Loop: help-debbugs@gnu.org Resent-From: Dima Kogan Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Mon, 07 Nov 2016 02:28:01 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 17544 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: patch Original-Received: via spool by 17544-submit@debbugs.gnu.org id=B17544.147848562320963 (code B ref 17544); Mon, 07 Nov 2016 02:28:01 +0000 Original-Received: (at 17544) by debbugs.gnu.org; 7 Nov 2016 02:27:03 +0000 Original-Received: from localhost ([127.0.0.1]:47341 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1c3ZeI-0005S3-VE for submit@debbugs.gnu.org; Sun, 06 Nov 2016 21:27:03 -0500 Original-Received: from out2-smtp.messagingengine.com ([66.111.4.26]:54114) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1c3ZeG-0005Rd-Im for 17544@debbugs.gnu.org; Sun, 06 Nov 2016 21:27:01 -0500 Original-Received: from compute5.internal (compute5.nyi.internal [10.202.2.45]) by mailout.nyi.internal (Postfix) with ESMTP id 596232068C; Sun, 6 Nov 2016 21:27:00 -0500 (EST) Original-Received: from frontend2 ([10.202.2.161]) by compute5.internal (MEProxy); Sun, 06 Nov 2016 21:27:00 -0500 DKIM-Signature: v=1; a=rsa-sha1; c=relaxed/relaxed; d=secretsauce.net; h=cc:content-type:date:from:in-reply-to:message-id:mime-version :references:subject:to:x-me-sender:x-me-sender:x-sasl-enc :x-sasl-enc; s=mesmtp; bh=V5cVhrxWeroSReZjnHyd7ZzHQFY=; b=vbR8EF ENAofu+WSu/1k13TkdNl0xie+rUyxtqCu4B3o0s9I+L7GHF4wCxL67lrqZjFrbd6 X4GoBKonckCA9Jw508UMomKguFWkrlfLNQ4d41KJ0H1Cf826+xO7hS2+tp9SRUju iKTkclbcNcxOhnTydYkPlipRddXx5XlZ7qD+k= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-type:date:from:in-reply-to :message-id:mime-version:references:subject:to:x-me-sender :x-me-sender:x-sasl-enc:x-sasl-enc; s=smtpout; bh=V5cVhrxWeroSRe ZjnHyd7ZzHQFY=; b=TliO57kwfDqPSjFMNblTpLpTkCAqUQkFokSHkQbmG0N8Yz F4dHKENzL+ZgbNGHfwrHfW1nmVwgnrwjZ5y1MPUAUFXnp6fen1bEfGMamcUyoMdK pgz7y1y++KLdo/tiSj1zM1GCoAxeqDA1RjcmN8ny/G5Q+U556iY+YR4h37eHw= X-ME-Sender: X-Sasl-enc: zmcv24EmnnTbqzHWLTgR8gKbD7dWTOAepl5z7IW5HL8t 1478485618 Original-Received: from shorty.local (c-73-140-59-251.hsd1.wa.comcast.net [73.140.59.251]) by mail.messagingengine.com (Postfix) with ESMTPA id 407D1CCF2D; Sun, 6 Nov 2016 21:26:58 -0500 (EST) Original-Received: from dima by shorty.local with local (Exim 4.87) (envelope-from ) id 1c3ZeA-0007lh-RM; Sun, 06 Nov 2016 18:26:54 -0800 In-reply-to: <87mvhvsrta.fsf@users.sourceforge.net> X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 208.118.235.43 X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Original-Sender: "bug-gnu-emacs" Xref: news.gmane.org gmane.emacs.bugs:125404 Archived-At: --=-=-= Content-Type: text/plain npostavs@users.sourceforge.net writes: > Dima Kogan writes: > >> Hi. I'm open to suggestions. The goal was to retain the previous logic >> for any existing code, but to provide improved user-facing behavior. Ack! So this is what happens when you submit a patch, and then talk about it more than 2 years later. Turns out preserving compatibility with existing code wasn't my goal at all. Rather, the goal was to avoid an infinite loop that results if (diff-hunk-next) unconditionally uses the new logic: diff-hunk-next calls diff--wrap-navigation calls diff-bounds-of-hunk calls diff-beginning-of-hunk calls diff-hunk-next > it looks like interactiveness is not what you actually care about. > You could do something like this instead: > > (defun diff-hunk-next (&optional count skip-hunk-start) > "Go to the next COUNT'th hunk" > (interactive (list (prefix-numeric-value current-prefix-arg) t)) > (diff--wrap-navigation > skip-hunk-start > "next hunk" > 'diff--internal-hunk-next > diff-hunk-header-re > (lambda () (goto-char (car (diff-bounds-of-hunk)))) > count)) > > Then instead of (call-interactively 'diff-hunk-next) you can just do > (diff-hunk-next nil t) Yes. Agreed. Revised patch attached. I'll try to respond to any comments quickly so that we all can remember what this is about. --=-=-= Content-Type: text/x-diff Content-Disposition: inline; filename=0001-Improved-diff-mode-navigation-manipulation.patch >From 4cd4f839dc840493ca14ff13a4f27cedca12a562 Mon Sep 17 00:00:00 2001 From: Dima Kogan Date: Wed, 14 Sep 2016 15:25:06 -0700 Subject: [PATCH] Improved diff-mode navigation/manipulation Navigation and use of diff buffers had several annoying corner cases that this patch fixes. These corner cases were largely due to inconsistent treatment of file headers. Say you have a diff such as this: --- aaa +++ bbb @@ -52,7 +52,7 @@ hunk1 @@ -74,7 +74,7 @@ hunk2 --- ccc +++ ddd @@ -608,6 +608,6 @@ hunk3 @@ -654,7 +654,7 @@ hunk4 The file headers here are the '---' and '+++' lines. With the point on such a line, hunk operations would sometimes refer to the next hunk and sometimes to the previous hunk. Most of the time it would be the previous hunk, which is not what the user would expect. This patch consistently treats such headers as the next hunk. So with this patch, if the point is on the '--- ccc' line, the point is seen as referring to hunk3. Specific behaviors this fixes are: 1. It should be possible to place the point in the middle of a diff buffer, and press M-k repeatedly to kill hunks in the order they appear in the buffer. With the point on hunk1, M-k M-k would kill hunk1 then hunk2. With the point on hunk3, it would kill hunk3 then hunk4; this is fine. However, with the point on hunk2, it'd kill hunk2 then hunk1. This is fixed by this patch. 2. Similarly, it should be possible to apply hunks in order. Previously with the point at the start, C-c C-a would apply the hunk1, then move the point to the first @@ header, and thus C-c C-a would try to apply the same hunk again. --- lisp/vc/diff-mode.el | 115 +++++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 102 insertions(+), 13 deletions(-) diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el index 58498cb..16d3f46 100644 --- a/lisp/vc/diff-mode.el +++ b/lisp/vc/diff-mode.el @@ -551,7 +551,7 @@ diff--auto-refine-data ;; Define diff-{hunk,file}-{prev,next} (easy-mmode-define-navigation - diff-hunk diff-hunk-header-re "hunk" diff-end-of-hunk diff-restrict-view + diff--internal-hunk diff-hunk-header-re "hunk" diff-end-of-hunk diff-restrict-view (when diff-auto-refine-mode (unless (prog1 diff--auto-refine-data (setq diff--auto-refine-data @@ -570,7 +570,88 @@ diff--auto-refine-data (diff-refine-hunk)))))))))))) (easy-mmode-define-navigation - diff-file diff-file-header-re "file" diff-end-of-file) + diff--internal-file diff-file-header-re "file" diff-end-of-file) + +(defun diff--wrap-navigation (skip-hunk-start + what orig + header-re goto-start-func count) + "I override the default diff-{hunk,file}-{next,prev} +implementation by skipping any lines that are associated with +this hunk/file but precede the hunk-start marker. For instance, a +diff file could contain + +diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el +index 923de9a..6b1c24f 100644 +--- a/lisp/vc/diff-mode.el ++++ b/lisp/vc/diff-mode.el +@@ -590,6 +590,22 @@ +....... + +If a point is on 'index', then the point is considered to be in +this first hunk. Here I move the point to the @@... marker before +executing the default diff-hunk-next/prev implementation to move +to the NEXT marker" + (if (not skip-hunk-start) + (funcall orig count) + + (let ((start (point))) + (funcall goto-start-func) + + ;; I trap the error + (condition-case nil + (funcall orig count) + (error nil)) + + (when (not (looking-at header-re)) + (goto-char start) + (user-error (format "No %s" what)))))) + +(defun diff-hunk-prev (&optional count skip-hunk-start) + "Go to the previous COUNT'th hunk" + (interactive (list (prefix-numeric-value current-prefix-arg) t)) + (diff--wrap-navigation + skip-hunk-start + "prev hunk" + 'diff--internal-hunk-prev + diff-hunk-header-re + (lambda () (goto-char (car (diff-bounds-of-hunk)))) + count)) + +(defun diff-hunk-next (&optional count skip-hunk-start) + "Go to the next COUNT'th hunk" + (interactive (list (prefix-numeric-value current-prefix-arg) t)) + (diff--wrap-navigation + skip-hunk-start + "next hunk" + 'diff--internal-hunk-next + diff-hunk-header-re + (lambda () (goto-char (car (diff-bounds-of-hunk)))) + count)) + +(defun diff-file-prev (&optional count skip-hunk-start) + "Go to the previous COUNT'th file" + (interactive (list (prefix-numeric-value current-prefix-arg) t)) + (diff--wrap-navigation + skip-hunk-start + "prev file" + 'diff--internal-file-prev + diff-file-header-re + (lambda () (goto-char (car (diff-bounds-of-file))) (diff--internal-hunk-next)) + count)) + +(defun diff-file-next (&optional count skip-hunk-start) + "Go to the next COUNT'th file" + (interactive (list (prefix-numeric-value current-prefix-arg) t)) + (diff--wrap-navigation + skip-hunk-start + "next file" + 'diff--internal-file-next + diff-file-header-re + (lambda () (goto-char (car (diff-bounds-of-file))) (diff--internal-hunk-next)) + count)) + + + (defun diff-bounds-of-hunk () "Return the bounds of the diff hunk at point. @@ -581,12 +662,13 @@ diff-bounds-of-hunk (let ((pos (point)) (beg (diff-beginning-of-hunk t)) (end (diff-end-of-hunk))) - (cond ((>= end pos) + (cond ((> end pos) (list beg end)) ;; If this hunk ends above POS, consider the next hunk. ((re-search-forward diff-hunk-header-re nil t) (list (match-beginning 0) (diff-end-of-hunk))) - (t (error "No hunk found")))))) + ;; There's no next hunk, so just take the one we have + (t (list beg end)))))) (defun diff-bounds-of-file () "Return the bounds of the file segment at point. @@ -1665,8 +1747,9 @@ diff-find-source-location SWITCHED is non-nil if the patch is already applied. NOPROMPT, if non-nil, means not to prompt the user." (save-excursion - (let* ((other (diff-xor other-file diff-jump-to-old-file)) - (char-offset (- (point) (diff-beginning-of-hunk t))) + (let* ((hunk-bounds (diff-bounds-of-hunk)) + (other (diff-xor other-file diff-jump-to-old-file)) + (char-offset (- (point) (goto-char (car hunk-bounds)))) ;; Check that the hunk is well-formed. Otherwise diff-mode and ;; the user may disagree on what constitutes the hunk ;; (e.g. because an empty line truncates the hunk mid-course), @@ -1675,7 +1758,7 @@ diff-find-source-location ;; Suppress check when NOPROMPT is non-nil (Bug#3033). (_ (unless noprompt (diff-sanity-check-hunk))) (hunk (buffer-substring - (point) (save-excursion (diff-end-of-hunk) (point)))) + (point) (cadr hunk-bounds))) (old (diff-hunk-text hunk reverse char-offset)) (new (diff-hunk-text hunk (not reverse) char-offset)) ;; Find the location specification. @@ -1783,8 +1866,13 @@ diff-apply-hunk ;; Display BUF in a window (set-window-point (display-buffer buf) (+ (car pos) (cdr new))) (diff-hunk-status-msg line-offset (diff-xor switched reverse) nil) + + ;; I advance to the next hunk interactively because I want the + ;; interactive behavior of moving to the next logical hunk, not + ;; the legacy behavior where were would sometimes sty on the + ;; curent hunk. See http://debbugs.gnu.org/17544 (when diff-advance-after-apply-hunk - (diff-hunk-next)))))) + (diff-hunk-next nil t)))))) (defun diff-test-hunk (&optional reverse) @@ -1865,14 +1953,15 @@ diff-current-defun (defun diff-ignore-whitespace-hunk () "Re-diff the current hunk, ignoring whitespace differences." (interactive) - (let* ((char-offset (- (point) (diff-beginning-of-hunk t))) + (let* ((hunk-bounds (diff-bounds-of-hunk)) + (char-offset (- (point) (goto-char (car hunk-bounds)))) (opts (pcase (char-after) (?@ "-bu") (?* "-bc") (_ "-b"))) (line-nb (and (or (looking-at "[^0-9]+\\([0-9]+\\)") (error "Can't find line number")) (string-to-number (match-string 1)))) (inhibit-read-only t) (hunk (delete-and-extract-region - (point) (save-excursion (diff-end-of-hunk) (point)))) + (point) (cadr hunk-bounds))) (lead (make-string (1- line-nb) ?\n)) ;Line nums start at 1. (file1 (make-temp-file "diff1")) (file2 (make-temp-file "diff2")) @@ -1959,8 +2048,8 @@ diff-refine-hunk (interactive) (require 'smerge-mode) (save-excursion - (diff-beginning-of-hunk t) - (let* ((start (point)) + (let* ((hunk-bounds (diff-bounds-of-hunk)) + (start (goto-char (car hunk-bounds))) (style (diff-hunk-style)) ;Skips the hunk header as well. (beg (point)) (props-c '((diff-mode . fine) (face diff-refine-changed))) @@ -1968,7 +2057,7 @@ diff-refine-hunk (props-a '((diff-mode . fine) (face diff-refine-added))) ;; Be careful to go back to `start' so diff-end-of-hunk gets ;; to read the hunk header's line info. - (end (progn (goto-char start) (diff-end-of-hunk) (point)))) + (end (goto-char (cadr hunk-bounds)))) (remove-overlays beg end 'diff-mode 'fine) -- 2.10.1 --=-=-=--