GNU bug report logs - #25524
26.0.50; diff-mode is broken

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 25524 in the body.
You can then email your comments to 25524 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Dima Kogan <dima <at> secretsauce.net>
To: bug-gnu-emacs <at> gnu.org
Subject: 26.0.50; diff-mode is broken
Date: Tue, 24 Jan 2017 22:42:08 -0800

[Message part 1 (text/plain, inline)]

I'm using a very recent build from git: 0a49f158f15. I see diff-mode
being broken in 2 ways.

I'm attaching a diff file, produced by C-x v D in a project using
subversion (then cut down and de-contented).

Breakage 1:

  1. emacs -Q /tmp/tst.patch
  2. M-k

I would expect this to kill the first hunk. Instead emacs barfs:
Args out of range: something something


Breakage 2:

  1. emacs -Q /tmp/tst.patch
  2. M-g g 13 RET     move point to the start of the 2nd hunk
  3. M-k              I would expect this to kill the hunk at point (2nd
                      hunk). I would then expect the point to remain at
                      the 2nd hunk
  4. M-k              Same as before. Should kill 2nd hunk
  5. M-k              Same as before. Should kill 2nd hunk

Instead, M-k #2 kills the 2nd hunk and then moves the point to the 1st
hunk. So that subsequent M-k #3 kills the 1st hunk.

[tst.patch (text/x-diff, inline)]

Index: Makefile.1
===================================================================
--- Makefile.1	(revision 81382)
+++ Makefile.1	(working copy)
@@ -220,2 +220,3 @@
 # 1111
+# 2222
 # 3333
Index: Makefile.2
===================================================================
--- Makefile.2	(revision 81382)
+++ Makefile.2	(working copy)
@@ -220,2 +220,3 @@
 # 2222
+# 3333
 # 4444
@@ -330,2 +330,3 @@
 # 3333
+# 4444
 # 5555
Index: Makefile.3
===================================================================
--- Makefile.3	(revision 81382)
+++ Makefile.3	(working copy)
@@ -220,2 +220,3 @@
 # 4444
+# 5555
 # 6666

Message #8 received at 25524 <at> debbugs.gnu.org (full text, mbox):

From: Tino Calancha <tino.calancha <at> gmail.com>
To: Dima Kogan <dima <at> secretsauce.net>
Cc: 25524 <at> debbugs.gnu.org, tino.calancha <at> gmail.com
Subject: Re: bug#25524: 26.0.50; diff-mode is broken
Date: Thu, 26 Jan 2017 01:32:56 +0900

Dima Kogan <dima <at> secretsauce.net> writes:

> I'm using a very recent build from git: 0a49f158f15. I see diff-mode
> being broken in 2 ways.
Thank you for report this.  I confirm your observations.
> I'm attaching a diff file, produced by C-x v D in a project using
> subversion (then cut down and de-contented).
>
> Breakage 1:
>
>   1. emacs -Q /tmp/tst.patch
>   2. M-k
>
> I would expect this to kill the first hunk. Instead emacs barfs:
> Args out of range: something something
`diff-beginning-of-hunk' must return the pos of the beginning
of the hunk as stated in its docstring.

>
> Breakage 2:
>
>   1. emacs -Q /tmp/tst.patch
>   2. M-g g 13 RET     move point to the start of the 2nd hunk
>   3. M-k              I would expect this to kill the hunk at point (2nd
>                       hunk). I would then expect the point to remain at
>                       the 2nd hunk
>   4. M-k              Same as before. Should kill 2nd hunk
>   5. M-k              Same as before. Should kill 2nd hunk
>
> Instead, M-k #2 kills the 2nd hunk and then moves the point to the 1st
> hunk. So that subsequent M-k #3 kills the 1st hunk.
After recent changes `diff-file-junk-re' must be updated; currently,
it just contains Git diff keywords, i.e. `diff--at-diff-header-p'
with point in the first line of your example returns nil.

Following patch fix 1) and 2):
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
From 91351edff90628a1c134d79e13f3062467612478 Mon Sep 17 00:00:00 2001
From: Tino Calancha <tino.calancha <at> gmail.com>
Date: Thu, 26 Jan 2017 01:16:31 +0900
Subject: [PATCH] Fix Bug#25524

* lisp/vc/diff-mode.el (diff-beginning-of-hunk):
Return pos at beginning of hunk.
(diff-file-junk-re): Add SVN keywords.
---
 lisp/vc/diff-mode.el | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index b7ad8e8ebd..0de314df2d 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -499,9 +499,11 @@ diff-end-of-hunk
     (goto-char (or end (point-max)))))
 
 ;; "index ", "old mode", "new mode", "new file mode" and
-;; "deleted file mode" are output by git-diff.
+;; "deleted file mode" are output by git-diff; "Index: " and
+;; "========..." by SVN.
 (defconst diff-file-junk-re
-  "diff \\|index \\|\\(?:deleted file\\|new\\(?: file\\)?\\|old\\) mode\\|=== modified file")
+  (concat "Index: \\|" (make-string 67 ?=) "\\|"
+          "diff \\|index \\|\\(?:deleted file\\|new\\(?: file\\)?\\|old\\) mode\\|=== modified file"))
 
 ;; If point is in a diff header, then return beginning
 ;; of hunk position otherwise return nil.
@@ -545,7 +547,8 @@ diff-beginning-of-hunk
                (error "Can't find the beginning of the hunk")))
             ((re-search-backward regexp nil t)) ; In the middle of a hunk.
             ((re-search-forward regexp nil t) ; At first hunk header.
-             (forward-line 0))
+             (forward-line 0)
+             (point))
             (t (error "Can't find the beginning of the hunk"))))))
 
 (defun diff-unified-hunk-p ()
-- 
2.11.0

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
In GNU Emacs 26.0.50.1 (x86_64-pc-linux-gnu, GTK+ Version 3.22.6)
 of 2017-01-26
Repository revision: 6bfa9e9abca17290bc393d90aedb5abef83a3e06

Message #13 received at 25524 <at> debbugs.gnu.org (full text, mbox):

From: Noam Postavsky <npostavs <at> users.sourceforge.net>
To: Tino Calancha <tino.calancha <at> gmail.com>
Cc: 25524 <at> debbugs.gnu.org, Dima Kogan <dima <at> secretsauce.net>
Subject: Re: bug#25524: 26.0.50; diff-mode is broken
Date: Wed, 25 Jan 2017 15:47:53 -0500

On Wed, Jan 25, 2017 at 11:32 AM, Tino Calancha <tino.calancha <at> gmail.com> wrote:
>  ;; "index ", "old mode", "new mode", "new file mode" and
> -;; "deleted file mode" are output by git-diff.
> +;; "deleted file mode" are output by git-diff; "Index: " and
> +;; "========..." by SVN.
>  (defconst diff-file-junk-re
> -  "diff \\|index \\|\\(?:deleted file\\|new\\(?: file\\)?\\|old\\) mode\\|=== modified file")
> +  (concat "Index: \\|" (make-string 67 ?=) "\\|"
> +          "diff \\|index \\|\\(?:deleted file\\|new\\(?: file\\)?\\|old\\) mode\\|=== modified file"))

Is it safe to assume exactly 67 "="? Maybe it would be better to use
something like "=\\{20,\\}", i.e. a sequence of 20 or more "=".

Message #16 received at 25524 <at> debbugs.gnu.org (full text, mbox):

From: Dima Kogan <dima <at> secretsauce.net>
To: Noam Postavsky <npostavs <at> users.sourceforge.net>
Cc: 25524 <at> debbugs.gnu.org, Tino Calancha <tino.calancha <at> gmail.com>
Subject: Re: bug#25524: 26.0.50; diff-mode is broken
Date: Wed, 25 Jan 2017 13:25:04 -0800

Noam Postavsky <npostavs <at> users.sourceforge.net> writes:

> On Wed, Jan 25, 2017 at 11:32 AM, Tino Calancha <tino.calancha <at> gmail.com> wrote:
>>  ;; "index ", "old mode", "new mode", "new file mode" and
>> -;; "deleted file mode" are output by git-diff.
>> +;; "deleted file mode" are output by git-diff; "Index: " and
>> +;; "========..." by SVN.
>>  (defconst diff-file-junk-re
>> -  "diff \\|index \\|\\(?:deleted file\\|new\\(?: file\\)?\\|old\\) mode\\|=== modified file")
>> +  (concat "Index: \\|" (make-string 67 ?=) "\\|"
>> +          "diff \\|index \\|\\(?:deleted file\\|new\\(?: file\\)?\\|old\\) mode\\|=== modified file"))
>
> Is it safe to assume exactly 67 "="? Maybe it would be better to use
> something like "=\\{20,\\}", i.e. a sequence of 20 or more "=".

More broadly, are we sure we need to be touching this? The previous
approach and the one before that worked with the stock regexes.

Message #19 received at 25524 <at> debbugs.gnu.org (full text, mbox):

From: Tino Calancha <tino.calancha <at> gmail.com>
To: Noam Postavsky <npostavs <at> users.sourceforge.net>
Cc: 25524 <at> debbugs.gnu.org, Dima Kogan <dima <at> secretsauce.net>,
 tino.calancha <at> gmail.com
Subject: Re: bug#25524: 26.0.50; diff-mode is broken
Date: Thu, 26 Jan 2017 13:29:59 +0900

Noam Postavsky <npostavs <at> users.sourceforge.net> writes:

> On Wed, Jan 25, 2017 at 11:32 AM, Tino Calancha <tino.calancha <at> gmail.com> wrote:
>>  ;; "index ", "old mode", "new mode", "new file mode" and
>> -;; "deleted file mode" are output by git-diff.
>> +;; "deleted file mode" are output by git-diff; "Index: " and
>> +;; "========..." by SVN.
>>  (defconst diff-file-junk-re
>> -  "diff \\|index \\|\\(?:deleted file\\|new\\(?: file\\)?\\|old\\) mode\\|=== modified file")
>> +  (concat "Index: \\|" (make-string 67 ?=) "\\|"
>> +          "diff \\|index \\|\\(?:deleted file\\|new\\(?: file\\)?\\|old\\) mode\\|=== modified file"))
>
> Is it safe to assume exactly 67 "="? Maybe it would be better to use
> something like "=\\{20,\\}", i.e. a sequence of 20 or more "=".
Humm probably 20 is OK, but i am not using SVN in a while: i ignore if
such '====...' might be coustomizable.
Updated the patch to use >=20 ?=.

Dima Kogan <dima <at> secretsauce.net> writes:

> More broadly, are we sure we need to be touching this? The previous
> approach and the one before that worked with the stock regexes.
1) The previous approach touch more things that just and already
existant regexp: it caused several problems already well discussed
in Bug#25105.

2) In the original approach, the mere existance of 'diff-file-junk-re'
as it is, it looks weird.  The file want to support all kind VCS,
not just Git,  so it's unnatural that the regexp just contains words
from Git diff headers.
Indeed, you can see that there are some hardcoded "^Index: " around
the file to handle SVN.
IMO, it's more readable adding the necessary stuff to
`diff-file-junk-re' and perform such operations uniformly just with
same function and regexp.  Then, if later turns out that we need more
keyword to handle other VCS we just need to change on line of code,
i.e., add those words into `diff-file-junk-re'.

That said, you are very welcome to improve current implementation so
that any `diff-file-junk-re' is required at all, without changing
other desirable functionality of the file.


;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
From c3788c73140af02d6fc2b414ddcdbaf273890cdf Mon Sep 17 00:00:00 2001
From: Tino Calancha <tino.calancha <at> gmail.com>
Date: Thu, 26 Jan 2017 13:22:46 +0900
Subject: [PATCH] Fix Bug#25524

* lisp/vc/diff-mode.el (diff-beginning-of-hunk):
Return pos at beginning of hunk.
(diff-file-junk-re): Add SVN keywords.
---
 lisp/vc/diff-mode.el | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index b7ad8e8ebd..e609ca9f94 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -501,7 +501,8 @@ diff-end-of-hunk
 ;; "index ", "old mode", "new mode", "new file mode" and
 ;; "deleted file mode" are output by git-diff.
 (defconst diff-file-junk-re
-  "diff \\|index \\|\\(?:deleted file\\|new\\(?: file\\)?\\|old\\) mode\\|=== modified file")
+  (concat "Index: \\|=\\{20,\\}\\|" ; SVN
+          "diff \\|index \\|\\(?:deleted file\\|new\\(?: file\\)?\\|old\\) mode\\|=== modified file"))
 
 ;; If point is in a diff header, then return beginning
 ;; of hunk position otherwise return nil.
@@ -545,7 +546,8 @@ diff-beginning-of-hunk
                (error "Can't find the beginning of the hunk")))
             ((re-search-backward regexp nil t)) ; In the middle of a hunk.
             ((re-search-forward regexp nil t) ; At first hunk header.
-             (forward-line 0))
+             (forward-line 0)
+             (point))
             (t (error "Can't find the beginning of the hunk"))))))
 
 (defun diff-unified-hunk-p ()
-- 
2.11.0

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
In GNU Emacs 26.0.50.1 (x86_64-pc-linux-gnu, GTK+ Version 3.22.6)
 of 2017-01-26 built
Repository revision: 44765de2005fb56c5930383d6bd1e959a0102a45

Message #24 received at 25524-done <at> debbugs.gnu.org (full text, mbox):

From: Tino Calancha <tino.calancha <at> gmail.com>
To: 25524-done <at> debbugs.gnu.org
Subject: Re: bug#25524: 26.0.50; diff-mode is broken
Date: Sun, 29 Jan 2017 18:53:22 +0900

Tino Calancha <tino.calancha <at> gmail.com> writes:

> Noam Postavsky <npostavs <at> users.sourceforge.net> writes:
>
>> On Wed, Jan 25, 2017 at 11:32 AM, Tino Calancha <tino.calancha <at> gmail.com> wrote:
>>>  ;; "index ", "old mode", "new mode", "new file mode" and
>>> -;; "deleted file mode" are output by git-diff.
>>> +;; "deleted file mode" are output by git-diff; "Index: " and
>>> +;; "========..." by SVN.
>>>  (defconst diff-file-junk-re
>>> -  "diff \\|index \\|\\(?:deleted file\\|new\\(?: file\\)?\\|old\\) mode\\|=== modified file")
>>> +  (concat "Index: \\|" (make-string 67 ?=) "\\|"
>>> +          "diff \\|index \\|\\(?:deleted file\\|new\\(?: file\\)?\\|old\\) mode\\|=== modified file"))
>>
>> Is it safe to assume exactly 67 "="? Maybe it would be better to use
>> something like "=\\{20,\\}", i.e. a sequence of 20 or more "=".
> Humm probably 20 is OK, but i am not using SVN in a while: i ignore if
> such '====...' might be coustomizable.
> Updated the patch to use >=20 ?=.
Pushed fix to master branch as commit 0073223c23749ffd6bd3f882bc30a82cc37efd2a

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.