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

[Konstantin Kharlamov <hi-angel@yandex.ru>]

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 
>>> \x7f\x7fseparator,
>>>  in which case go there.
>>>  (diff-end-of-hunk): make use of 
>>> \x7f\x7f(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 
>> \x7fgit-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.