GNU bug report logs - #37395
Diff-mode doesn't take into account patch-separators as produced by git-format-patch

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 37395 in the body.
You can then email your comments to 37395 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: Konstantin Kharlamov <hi-angel <at> yandex.ru>
To: bug-gnu-emacs <at> gnu.org
Subject: Diff-mode doesn't take into account patch-separators as produced by
 git-format-patch
Date: Fri, 13 Sep 2019 00:33:05 +0300

As title says. As a follow-up to this report I'm gonna send a fix to 
this problem.

Observable result of the problem is that Emacs thinks `-- ` patch line 
is a "deleted line", whereas it actually is end of the patch.

The follow-up patch I successfully used to edit patch-series of 12 
patches to libinput 
https://gitlab.freedesktop.org/libinput/libinput/merge_requests/288#note_223871

# Steps to reproduce

1. In terminal, go to Emacs git repository
2. Execute `git format-patch -1 --stdout > 1.patch`
3. Open 1.patch in Emacs (diff-mode should automatically get enabled)
4. Replace at the beginning of a "deleted line" the `-` with space ` `.

## Expected

The 4-digit header (which looks like `@@ -561,7 +569,8`) should have 
first 2 digits (561 and 7 in example) unchanged.

## Actual

The 4-digit header increases count of 2nd digit (7 in example) by one.

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

From: Konstantin Kharlamov <Hi-Angel <at> yandex.ru>
To: 37395 <at> debbugs.gnu.org
Subject: [PATCH] diff-mode.el: take into account patch separators
Date: Fri, 13 Sep 2019 00:34:45 +0300

* lisp/vc/diff-mode.el (diff-goto-line-before-patch-separator): an
inline function to check if prev. line was git-format-patch separator,
in which case go there.
(diff-end-of-hunk): make use of (diff-goto-line-before-patch-separator)
---
 lisp/vc/diff-mode.el | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index 19f9c802d4..a3e4923fb4 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -511,6 +511,14 @@ diff-hunk-style
     (goto-char (match-end 0)))
   style)
 
+(defsubst diff-goto-line-before-patch-separator ()
+  "Go to prev. line, then if it has patch separator as produced
+by git-format-patch, stay there. Otherwise go back."
+  (previous-line)
+  (when (not (looking-at "-- "))
+      (next-line))
+  (point))
+
 (defun diff-end-of-hunk (&optional style donttrustheader)
   "Advance to the end of the current hunk, and return its position."
   (let (end)
@@ -561,7 +569,8 @@ diff-end-of-hunk
         (goto-char (or end (point-max)))
         (while (eq ?\n (char-before (1- (point))))
           (forward-char -1)
-          (setq end (point)))))
+          (setq end (point))))
+      (setq end (diff-goto-line-before-patch-separator)))
     ;; The return value is used by easy-mmode-define-navigation.
     (goto-char (or end (point-max)))))
 
-- 
2.23.0

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Konstantin Kharlamov <Hi-Angel <at> yandex.ru>
Cc: 37395 <at> debbugs.gnu.org
Subject: Re: bug#37395: [PATCH] diff-mode.el: take into account patch
 separators
Date: Fri, 13 Sep 2019 09:14:03 +0300

> From: Konstantin Kharlamov <Hi-Angel <at> yandex.ru>
> Date: Fri, 13 Sep 2019 00:34:45 +0300
> 
> * lisp/vc/diff-mode.el (diff-goto-line-before-patch-separator): an
> inline function to check if prev. line was git-format-patch separator,
> in which case go there.
> (diff-end-of-hunk): make use of (diff-goto-line-before-patch-separator)

The descriptions of changes should start with a capital letter.  Also,
your lines in the commit log message are too long, they should not
exceed 61 characters (because in the release tarball we create a
ChangeLog file from them, which prepends a TAB character to each
line).

> +(defsubst diff-goto-line-before-patch-separator ()
> +  "Go to prev. line, then if it has patch separator as produced
> +by git-format-patch, stay there. Otherwise go back."

The first line of a doc string should be a complete sentence.  I
suggest to rephrase as follows:

  Return buffer position before patch separator produced by git-format-patch.

> +  (previous-line)
> +  (when (not (looking-at "-- "))
> +      (next-line))
> +  (point))

Btw, Diff mode is more general than just Git-produced diffs.  Is there
any possibility that this change will misfire in diffs produced by
other tools?  If so, perhaps we should also verify the buffer is under
Git control.

Thanks.

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

From: Konstantin Kharlamov <hi-angel <at> yandex.ru>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 37395 <at> debbugs.gnu.org
Subject: Re: bug#37395: [PATCH] diff-mode.el: take into account patch
 separators
Date: Fri, 13 Sep 2019 09:58:27 +0300

On Пт, сен 13, 2019 at 09:14, Eli Zaretskii <eliz <at> gnu.org> wrote:
>>  From: Konstantin Kharlamov <Hi-Angel <at> yandex.ru>
>>  Date: Fri, 13 Sep 2019 00:34:45 +0300
>> 
>>  * lisp/vc/diff-mode.el (diff-goto-line-before-patch-separator): an
>>  inline function to check if prev. line was git-format-patch 
>> separator,
>>  in which case go there.
>>  (diff-end-of-hunk): make use of 
>> (diff-goto-line-before-patch-separator)
> 
> The descriptions of changes should start with a capital letter.  Also,
> your lines in the commit log message are too long, they should not
> exceed 61 characters (because in the release tarball we create a
> ChangeLog file from them, which prepends a TAB character to each
> line).
> 
>>  +(defsubst diff-goto-line-before-patch-separator ()
>>  +  "Go to prev. line, then if it has patch separator as produced
>>  +by git-format-patch, stay there. Otherwise go back."
> 
> The first line of a doc string should be a complete sentence.  I
> suggest to rephrase as follows:
> 
>   Return buffer position before patch separator produced by 
> git-format-patch.

Thank you, I'll address the comments a bit later this day!

>>  +  (previous-line)
>>  +  (when (not (looking-at "-- "))
>>  +      (next-line))
>>  +  (point))
> 
> Btw, Diff mode is more general than just Git-produced diffs.  Is there
> any possibility that this change will misfire in diffs produced by
> other tools?  If so, perhaps we should also verify the buffer is under
> Git control.

Oh, while writing this, I figured I should change the regexp from `-- ` 
to `^-- $`. Will do.

With that addressed: very unlikely. A miscalculation could happen if 
all of the following holds true *simultaneously*:

	* The diff removes a line with the exact text `- `, i.e. two 
characters {dash,space}. That would result in {dash,dash,space} diff, 
same as patch separator.
	* The line was the last line removed in the hunk.
	* The hunk has no context after the removed line

Note, these need to hold true simultaneously. Give the low probability 
of this compared to git-format-patch that is used literally everywhere 
(and that without this patch diff-mode can't handle it), I think this 
is a reasonable trade-off.

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

From: Konstantin Kharlamov <Hi-Angel <at> yandex.ru>
To: 37395 <at> debbugs.gnu.org
Subject: [PATCH v2] diff-mode.el: take into account patch separators
Date: Fri, 13 Sep 2019 12:19:21 +0300

* lisp/vc/diff-mode.el (diff-prev-line-if-patch-separator):
An inline function to return prev. line if it has
git-format-patch separator.
(diff-end-of-hunk): Make use of
diff-prev-line-if-patch-separator
---

v2:
* Start commit description with capital letter
* Limit commit description to 61 character line length
* Rename diff-goto-line-before-patch-separator, to
  diff-prev-line-if-patch-separator and rephrase the doc-string.
* diff-prev-line-if-patch-separator now does not mutate point.
* Make sure we're at line beginning before (looking-at …)

 lisp/vc/diff-mode.el | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index 19f9c802d4..6702614472 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -511,6 +511,17 @@ diff-hunk-style
     (goto-char (match-end 0)))
   style)
 
+(defsubst diff-prev-line-if-patch-separator ()
+  "Return previous line if it has patch separator produced by
+git-format-patch."
+  (save-excursion
+    (let ((old-point (point)))
+      (previous-line)
+      (beginning-of-line)
+      (if (looking-at "^-- $")
+          (point)
+        old-point))))
+
 (defun diff-end-of-hunk (&optional style donttrustheader)
   "Advance to the end of the current hunk, and return its position."
   (let (end)
@@ -561,7 +572,8 @@ diff-end-of-hunk
         (goto-char (or end (point-max)))
         (while (eq ?\n (char-before (1- (point))))
           (forward-char -1)
-          (setq end (point)))))
+          (setq end (point))))
+      (setq end (diff-prev-line-if-patch-separator)))
     ;; The return value is used by easy-mmode-define-navigation.
     (goto-char (or end (point-max)))))
 
-- 
2.23.0

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

From: Konstantin Kharlamov <Hi-Angel <at> yandex.ru>
To: 37395 <at> debbugs.gnu.org
Subject: [PATCH v3] diff-mode.el: take into account patch separators
Date: Mon, 16 Sep 2019 23:26:43 +0300

* lisp/vc/diff-mode.el (diff-prev-line-if-patch-separator):
An inline function to return prev. line if it has
git-format-patch separator.
(diff-end-of-hunk): Make use of
diff-prev-line-if-patch-separator
diff-buffer-type: whether a buffer is a git-diff
(define-derived-mode): set diff-buffer-type to appropriate
value
---

v3: detect whether buffer has git-diff format.


 lisp/vc/diff-mode.el | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index 19f9c802d4..5e74dbc18c 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -511,6 +511,20 @@ diff-hunk-style
     (goto-char (match-end 0)))
   style)
 
+(defsubst diff-prev-line-if-patch-separator ()
+  "Return previous line if it has patch separator produced by
+git-format-patch."
+  (pcase diff-buffer-type
+    ('git
+     (save-excursion
+       (let ((old-point (point)))
+         (previous-line)
+         (beginning-of-line)
+         (if (looking-at "^-- $")
+             (point)
+           old-point))))
+    (_ (point))))
+
 (defun diff-end-of-hunk (&optional style donttrustheader)
   "Advance to the end of the current hunk, and return its position."
   (let (end)
@@ -561,7 +575,8 @@ diff-end-of-hunk
         (goto-char (or end (point-max)))
         (while (eq ?\n (char-before (1- (point))))
           (forward-char -1)
-          (setq end (point)))))
+          (setq end (point))))
+      (setq end (diff-prev-line-if-patch-separator)))
     ;; The return value is used by easy-mmode-define-navigation.
     (goto-char (or end (point-max)))))
 
@@ -1426,6 +1441,8 @@ diff--font-lock-cleanup
 (defvar whitespace-style)
 (defvar whitespace-trailing-regexp)
 
+(setq-local diff-buffer-type nil)
+
 ;;;###autoload
 (define-derived-mode diff-mode fundamental-mode "Diff"
   "Major mode for viewing/editing context diffs.
@@ -1491,7 +1508,11 @@ diff-mode
   (add-function :filter-return (local 'filter-buffer-substring-function)
                 #'diff--filter-substring)
   (unless buffer-file-name
-    (hack-dir-local-variables-non-file-buffer)))
+    (hack-dir-local-variables-non-file-buffer))
+  (save-excursion
+    (if (re-search-forward "^diff --git" nil t)
+        (setq diff-buffer-type 'git)
+      (setq diff-buffer-type nil))))
 
 ;;;###autoload
 (define-minor-mode diff-minor-mode
-- 
2.23.0

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

From: Konstantin Kharlamov <hi-angel <at> yandex.ru>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 37395 <at> debbugs.gnu.org
Subject: Re: bug#37395: [PATCH] diff-mode.el: take into account patch
 separators
Date: Mon, 16 Sep 2019 23:31:10 +0300

On Пт, сен 13, 2019 at 09:58, Konstantin Kharlamov 
<hi-angel <at> yandex.ru> wrote:
> 
> 
> On Пт, сен 13, 2019 at 09:14, Eli Zaretskii <eliz <at> gnu.org> wrote:
>>>  From: Konstantin Kharlamov <Hi-Angel <at> yandex.ru>
>>>  Date: Fri, 13 Sep 2019 00:34:45 +0300
>>> 
>>>  * lisp/vc/diff-mode.el (diff-goto-line-before-patch-separator): an
>>>  inline function to check if prev. line was git-format-patch 
>>> separator,
>>>  in which case go there.
>>>  (diff-end-of-hunk): make use of 
>>> (diff-goto-line-before-patch-separator)
>> 
>> The descriptions of changes should start with a capital letter.  
>> Also,
>> your lines in the commit log message are too long, they should not
>> exceed 61 characters (because in the release tarball we create a
>> ChangeLog file from them, which prepends a TAB character to each
>> line).
>> 
>>>  +(defsubst diff-goto-line-before-patch-separator ()
>>>  +  "Go to prev. line, then if it has patch separator as produced
>>>  +by git-format-patch, stay there. Otherwise go back."
>> 
>> The first line of a doc string should be a complete sentence.  I
>> suggest to rephrase as follows:
>> 
>>   Return buffer position before patch separator produced by 
>> git-format-patch.
> 
> Thank you, I'll address the comments a bit later this day!
> 
>>>  +  (previous-line)
>>>  +  (when (not (looking-at "-- "))
>>>  +      (next-line))
>>>  +  (point))
>> 
>> Btw, Diff mode is more general than just Git-produced diffs.  Is 
>> there
>> any possibility that this change will misfire in diffs produced by
>> other tools?  If so, perhaps we should also verify the buffer is 
>> under
>> Git control.
> 
> Oh, while writing this, I figured I should change the regexp from `-- 
> ` to `^-- $`. Will do.
> 
> With that addressed: very unlikely. A miscalculation could happen if 
> all of the following holds true *simultaneously*:
> 
> 	* The diff removes a line with the exact text `- `, i.e. two 
> characters {dash,space}. That would result in {dash,dash,space} diff, 
> same as patch separator.
> 	* The line was the last line removed in the hunk.
> 	* The hunk has no context after the removed line
> 
> Note, these need to hold true simultaneously. Give the low 
> probability of this compared to git-format-patch that is used 
> literally everywhere (and that without this patch diff-mode can't 
> handle it), I think this is a reasonable trade-off.

While still on it: I figured I could detect whether diff has git-diff 
format by searching the `diff --git` text that seems to be always 
present when starting diff-mode, and only then try to find the 
patch-separator. Done. I'm not sure if the `setq-local foo` and then 
modification of the `foo` in `(define-derived-mode)` is the preferred 
way to do this, but otherwise it seems to be fine, and worked in my 
tests.

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

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Konstantin Kharlamov <Hi-Angel <at> yandex.ru>
Cc: 37395 <at> debbugs.gnu.org
Subject: Re: bug#37395: [PATCH v3] diff-mode.el: take into account patch
 separators
Date: Mon, 07 Oct 2019 06:39:40 +0200

(Some minor comments.)

Konstantin Kharlamov <Hi-Angel <at> yandex.ru> writes:

> +(defsubst diff-prev-line-if-patch-separator ()
> +  "Return previous line if it has patch separator produced by
> +git-format-patch."

I don't think this needs to be a defsubst -- a defun is fine here.  And
the first doc string line should be a complete sentence.

> +(setq-local diff-buffer-type nil)

This should probably be a defvar and then a setq-local in `diff-mode'.

> +  (save-excursion
> +    (if (re-search-forward "^diff --git" nil t)
> +        (setq diff-buffer-type 'git)
> +      (setq diff-buffer-type nil))))

Hm...  are we really guaranteed that all git diffs have that string in
it somewhere?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

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

From: Konstantin Kharlamov <hi-angel <at> yandex.ru>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 37395 <at> debbugs.gnu.org
Subject: Re: bug#37395: [PATCH v3] diff-mode.el: take into account patch
 separators
Date: Tue, 08 Oct 2019 02:04:22 +0300

On Пн, окт 7, 2019 at 06:39, Lars Ingebrigtsen <larsi <at> gnus.org> 
wrote:
> (Some minor comments.)

Thanks!

> Konstantin Kharlamov <Hi-Angel <at> yandex.ru> writes:
> 
>>  +(defsubst diff-prev-line-if-patch-separator ()
>>  +  "Return previous line if it has patch separator produced by
>>  +git-format-patch."
> 
> I don't think this needs to be a defsubst -- a defun is fine here.

Will do.

> And the first doc string line should be a complete sentence.

Sorry, I'm not sure I get it: do you want me to keep the 
"git-format-patch." text on the first line, with the rest of the 
sentence?

>>  +(setq-local diff-buffer-type nil)
> 
> This should probably be a defvar and then a setq-local in `diff-mode'.

Will do.

>>  +  (save-excursion
>>  +    (if (re-search-forward "^diff --git" nil t)
>>  +        (setq diff-buffer-type 'git)
>>  +      (setq diff-buffer-type nil))))
> 
> Hm...  are we really guaranteed that all git diffs have that string in
> it somewhere?

Well, according to #git channel on Freenode and this answer 
https://stackoverflow.com/questions/39834729/what-does-the-diff-git-output-in-git-diff-refer-to 
apparently, it's there unless someone explicitly changed config for it 
to not be there.

But any other ideas to detect git format are welcome. I personally 
would prefer to not do detection at all, because I'm sure the case 
where we could misdetect text and miscalculate the diff header is too 
statistically insignificant. Too many things need to happen at once — 
and it doesn't seem that diff-mode being used by a lot of people too, 
since pretty major problem that I fix here went unnoticed for so long.

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

From: Konstantin Kharlamov <hi-angel <at> yandex.ru>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 37395 <at> debbugs.gnu.org
Subject: Re: bug#37395: [PATCH v3] diff-mode.el: take into account patch
 separators
Date: Tue, 08 Oct 2019 02:12:43 +0300

On Вт, окт 8, 2019 at 02:04, Konstantin Kharlamov 
<hi-angel <at> yandex.ru> wrote:
> Well, according to #git channel on Freenode and this answer 
> https://stackoverflow.com/questions/39834729/what-does-the-diff-git-output-in-git-diff-refer-to 
> apparently, it's there unless someone explicitly changed config for 
> it to not be there.
> 
> But any other ideas to detect git format are welcome. I personally 
> would prefer to not do detection at all, because I'm sure the case 
> where we could misdetect text and miscalculate the diff header is too 
> statistically insignificant.

…as well as consequences insignificant too, in case it somehow have 
happened, I should add.

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

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Konstantin Kharlamov <hi-angel <at> yandex.ru>
Cc: 37395 <at> debbugs.gnu.org
Subject: Re: bug#37395: [PATCH v3] diff-mode.el: take into account patch
 separators
Date: Tue, 08 Oct 2019 17:59:42 +0200

Konstantin Kharlamov <hi-angel <at> yandex.ru> writes:

>> And the first doc string line should be a complete sentence.
>
> Sorry, I'm not sure I get it: do you want me to keep the
> "git-format-patch." text on the first line, with the rest of the 
> sentence?

If that doesn't make the line too long.  Otherwise you have to rewrite
the sentence.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

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

From: Konstantin Kharlamov <Hi-Angel <at> yandex.ru>
To: 37395 <at> debbugs.gnu.org
Subject: [PATCH v4] diff-mode.el: take into account patch separators
Date: Tue,  8 Oct 2019 22:34:20 +0300

* lisp/vc/diff-mode.el (diff-prev-line-if-patch-separator):
A function to return prev. line if it has git-format-patch
separator.
(diff-end-of-hunk): Make use of
diff-prev-line-if-patch-separator
diff-buffer-type: whether a buffer is a git-diff
(define-derived-mode): set diff-buffer-type to appropriate
value
---

v4:
    * replace defsubst with defun
    * define diff-buffer-type with defvar and set with setq-local
    * call `setq-local` just once

 lisp/vc/diff-mode.el | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index 19f9c802d4..1cc3cd3d4d 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -511,6 +511,19 @@ diff-hunk-style
     (goto-char (match-end 0)))
   style)
 
+(defun diff-prev-line-if-patch-separator ()
+  "Return previous line if it has patch separator as produced by git."
+  (pcase diff-buffer-type
+    ('git
+     (save-excursion
+       (let ((old-point (point)))
+         (previous-line)
+         (beginning-of-line)
+         (if (looking-at "^-- $")
+             (point)
+           old-point))))
+    (_ (point))))
+
 (defun diff-end-of-hunk (&optional style donttrustheader)
   "Advance to the end of the current hunk, and return its position."
   (let (end)
@@ -561,7 +574,8 @@ diff-end-of-hunk
         (goto-char (or end (point-max)))
         (while (eq ?\n (char-before (1- (point))))
           (forward-char -1)
-          (setq end (point)))))
+          (setq end (point))))
+      (setq end (diff-prev-line-if-patch-separator)))
     ;; The return value is used by easy-mmode-define-navigation.
     (goto-char (or end (point-max)))))
 
@@ -1425,6 +1439,7 @@ diff--font-lock-cleanup
 
 (defvar whitespace-style)
 (defvar whitespace-trailing-regexp)
+(defvar diff-buffer-type nil)
 
 ;;;###autoload
 (define-derived-mode diff-mode fundamental-mode "Diff"
@@ -1491,7 +1506,12 @@ diff-mode
   (add-function :filter-return (local 'filter-buffer-substring-function)
                 #'diff--filter-substring)
   (unless buffer-file-name
-    (hack-dir-local-variables-non-file-buffer)))
+    (hack-dir-local-variables-non-file-buffer))
+  (save-excursion
+    (setq-local diff-buffer-type
+                (if (re-search-forward "^diff --git" nil t)
+                    'git
+                  nil))))
 
 ;;;###autoload
 (define-minor-mode diff-minor-mode
-- 
2.23.0

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

From: Konstantin Kharlamov <hi-angel <at> yandex.ru>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 37395 <at> debbugs.gnu.org
Subject: Re: bug#37395: [PATCH v3] diff-mode.el: take into account patch
 separators
Date: Tue, 08 Oct 2019 23:08:32 +0300

On Вт, окт 8, 2019 at 17:59, Lars Ingebrigtsen <larsi <at> gnus.org> 
wrote:
> Konstantin Kharlamov <hi-angel <at> yandex.ru> writes:
> 
>>>  And the first doc string line should be a complete sentence.
>> 
>>  Sorry, I'm not sure I get it: do you want me to keep the
>>  "git-format-patch." text on the first line, with the rest of the
>>  sentence?
> 
> If that doesn't make the line too long.  Otherwise you have to rewrite
> the sentence.

Thanks, comments are addressed. That one too (I forgot to mention it in 
"v4" patch changes).

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

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Konstantin Kharlamov <Hi-Angel <at> yandex.ru>
Cc: 37395 <at> debbugs.gnu.org
Subject: Re: bug#37395: [PATCH v4] diff-mode.el: take into account patch
 separators
Date: Wed, 09 Oct 2019 21:36:57 +0200

Konstantin Kharlamov <Hi-Angel <at> yandex.ru> writes:

> * lisp/vc/diff-mode.el (diff-prev-line-if-patch-separator):
> A function to return prev. line if it has git-format-patch
> separator.
> (diff-end-of-hunk): Make use of
> diff-prev-line-if-patch-separator
> diff-buffer-type: whether a buffer is a git-diff
> (define-derived-mode): set diff-buffer-type to appropriate
> value

Byte-compiling gives:

In diff-prev-line-if-patch-separator:
vc/diff-mode.el:516:10:Warning: reference to free variable `diff-buffer-type'
vc/diff-mode.el:517:7:Warning: `previous-line' is for interactive use only;
    use `forward-line' with negative argument instead.

So you have to move the defvar earlier and adjust the previous-line, but
otherwise I think it looks OK...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

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

From: Konstantin Kharlamov <Hi-Angel <at> yandex.ru>
To: 37395 <at> debbugs.gnu.org
Subject: [PATCH v5] diff-mode.el: take into account patch separators
Date: Wed,  9 Oct 2019 23:07:32 +0300

* lisp/vc/diff-mode.el (diff-prev-line-if-patch-separator):
A function to return prev. line if it has git-format-patch
separator.
(diff-end-of-hunk): Make use of
diff-prev-line-if-patch-separator
diff-buffer-type: whether a buffer is a git-diff
(define-derived-mode): set diff-buffer-type to appropriate
value
---

v5: * move defvar diff-buffer-type before the 1st use
    * replace (previous-line) with (forward-line -1) and remove
      (line-beginning) call, since forward-line sets point to
      line-beginning.

 lisp/vc/diff-mode.el | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index 19f9c802d4..c86f15cee0 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -504,6 +504,7 @@ diff-file-header-re
 (defconst diff-separator-re "^--+ ?$")
 
 (defvar diff-narrowed-to nil)
+(defvar diff-buffer-type nil)
 
 (defun diff-hunk-style (&optional style)
   (when (looking-at diff-hunk-header-re)
@@ -511,6 +512,18 @@ diff-hunk-style
     (goto-char (match-end 0)))
   style)
 
+(defun diff-prev-line-if-patch-separator ()
+  "Return previous line if it has patch separator as produced by git."
+  (pcase diff-buffer-type
+    ('git
+     (save-excursion
+       (let ((old-point (point)))
+         (forward-line -1)
+         (if (looking-at "^-- $")
+             (point)
+           old-point))))
+    (_ (point))))
+
 (defun diff-end-of-hunk (&optional style donttrustheader)
   "Advance to the end of the current hunk, and return its position."
   (let (end)
@@ -561,7 +574,8 @@ diff-end-of-hunk
         (goto-char (or end (point-max)))
         (while (eq ?\n (char-before (1- (point))))
           (forward-char -1)
-          (setq end (point)))))
+          (setq end (point))))
+      (setq end (diff-prev-line-if-patch-separator)))
     ;; The return value is used by easy-mmode-define-navigation.
     (goto-char (or end (point-max)))))
 
@@ -1491,7 +1505,12 @@ diff-mode
   (add-function :filter-return (local 'filter-buffer-substring-function)
                 #'diff--filter-substring)
   (unless buffer-file-name
-    (hack-dir-local-variables-non-file-buffer)))
+    (hack-dir-local-variables-non-file-buffer))
+  (save-excursion
+    (setq-local diff-buffer-type
+                (if (re-search-forward "^diff --git" nil t)
+                    'git
+                  nil))))
 
 ;;;###autoload
 (define-minor-mode diff-minor-mode
-- 
2.23.0

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

From: Konstantin Kharlamov <hi-angel <at> yandex.ru>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 37395 <at> debbugs.gnu.org
Subject: Re: bug#37395: [PATCH v4] diff-mode.el: take into account patch
 separators
Date: Wed, 09 Oct 2019 23:08:16 +0300

Ah, sorry, I missed it. v5 sent, now should be no new warnings.

On Ср, окт 9, 2019 at 21:36, Lars Ingebrigtsen <larsi <at> gnus.org> 
wrote:
> Konstantin Kharlamov <Hi-Angel <at> yandex.ru> writes:
> 
>>  * lisp/vc/diff-mode.el (diff-prev-line-if-patch-separator):
>>  A function to return prev. line if it has git-format-patch
>>  separator.
>>  (diff-end-of-hunk): Make use of
>>  diff-prev-line-if-patch-separator
>>  diff-buffer-type: whether a buffer is a git-diff
>>  (define-derived-mode): set diff-buffer-type to appropriate
>>  value
> 
> Byte-compiling gives:
> 
> In diff-prev-line-if-patch-separator:
> vc/diff-mode.el:516:10:Warning: reference to free variable 
> `diff-buffer-type'
> vc/diff-mode.el:517:7:Warning: `previous-line' is for interactive use 
> only;
>     use `forward-line' with negative argument instead.
> 
> So you have to move the defvar earlier and adjust the previous-line, 
> but
> otherwise I think it looks OK...
> 
> --
> (domestic pets only, the antidote for overdose, milk.)
>    bloggy blog: http://lars.ingebrigtsen.no

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

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Konstantin Kharlamov <Hi-Angel <at> yandex.ru>
Cc: 37395 <at> debbugs.gnu.org
Subject: Re: bug#37395: [PATCH v5] diff-mode.el: take into account patch
 separators
Date: Sun, 13 Oct 2019 05:53:11 +0200

Konstantin Kharlamov <Hi-Angel <at> yandex.ru> writes:

> v5: * move defvar diff-buffer-type before the 1st use
>     * replace (previous-line) with (forward-line -1) and remove
>       (line-beginning) call, since forward-line sets point to
>       line-beginning.

Thanks; applied to the trunk.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Previous Next

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