bug-gnu-emacs
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

bug#37395: [PATCH] diff-mode.el: take into account patch separators


From: Konstantin Kharlamov
Subject: 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@yandex.ru> wrote:


On Пт, сен 13, 2019 at 09:14, Eli Zaretskii <eliz@gnu.org> wrote:
 From: Konstantin Kharlamov <Hi-Angel@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.







reply via email to

[Prev in Thread] Current Thread [Next in Thread]