bug#43297: 27.1; corrupts patch when diff-update-on-the-fly is set to nil

bug#43297: 27.1; corrupts patch when diff-update-on-the-fly is set to nil

[Mark H Weaver]

I don't know if this behavior is new in 27.1, but that's the only
version I've tried this on.

When I set 'diff-update-on-the-fly' to nil, I find that some patches are
consistently corrupted when I save them while in diff-mode.  See below
for an example patch that demonstrates this behavior.

To reproduce:

* Save the example patch below to "test.patch", and make another copy
  in "test2.patch".

* Launch "emacs -nw -q"  (I used Emacs 27.1)

* Evaluate (setq diff-update-on-the-fly nil)

* Find file "test2.patch".

* Add a character anywhere in the buffer and then delete it, such that
  the buffer is now in a "modified" state.

* Save the file.

* Run "diff -u test.patch test2.patch" in the shell.

* Here's the result that I see:

--8<---------------cut here---------------start------------->8---
--- test.patch
+++ test2.patch
@@ -39,7 +39,7 @@
  
  	var elements = document.getElementsByTagName("style");
  	for(var i = 0; i < elements.length; i++){
-@@ -41,16 +48,20 @@ function reveal_css(){
+@@ -41,17 +48,20 @@ function reveal_css(){
  
  		}
  	}
--8<---------------cut here---------------end--------------->8---

Emacs corrupts the line count of the final hunk, making it one larger
than it should be (17 instead of 16).  This breaks the patch, such that
attempts to apply it will fail.

Note that the example patch includes the following footer immediately
after the last hunk, added by "git format-patch":

--8<---------------cut here---------------start------------->8---
-- 
2.26.2
--8<---------------cut here---------------end--------------->8---

My guess is that the line with "--" is being interpreted as a deleted
line, part of the hunk.  This bug only seems to happen when
'diff-update-on-the-fly' is set to nil.

The example patch that demonstrates this problem follows.

     Thanks,
       Mark


--8<---------------cut here---------------start------------->8---
From 2e8618d22568b6e00892a17303d437dd700eca98 Mon Sep 17 00:00:00 2001
From: Mark H Weaver <mhw@netris.org>
Date: Tue, 5 May 2020 21:27:41 -0400
Subject: [PATCH] Revert "Update Reveal hidden HTML."

I prefer the user interface of the old version.

This reverts commit f6e3adb6b2344ee2c7bb453a305fd2d6fb4c194c.
---
 .../passive_improve_css.js                    | 23 ++++++++++++++-----
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/data/extensions/SubmitMe@0xbeef.coffee/passive_improve_css.js b/data/extensions/SubmitMe@0xbeef.coffee/passive_improve_css.js
index 7692990..ca57982 100644
--- a/data/extensions/SubmitMe@0xbeef.coffee/passive_improve_css.js
+++ b/data/extensions/SubmitMe@0xbeef.coffee/passive_improve_css.js
@@ -10,6 +10,8 @@ THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLI
 */
 
 
+var bad = [];
+
 function escapeHTML (unsafe_str) {
     return unsafe_str
       .replace(/&/g, '&amp;')
@@ -20,9 +22,14 @@ function escapeHTML (unsafe_str) {
       .replace(/\//g, '&#x2F;')
 }
 
+function dounhide(){
+        for(var i = 0; i < bad.length; i++){
+                        bad[i].remove();
+                }
+}
+
 console.log("passive_improve_css.js");
 function reveal_css(){
-	var bad = [];
 
 	var elements = document.getElementsByTagName("style");
 	for(var i = 0; i < elements.length; i++){
@@ -41,16 +48,20 @@ function reveal_css(){
 
 		}
 	}
-	if(bad.length > 0 && window.confirm("Hidden HTML detected, would you like to reveal it?")){
-		for(var i = 0; i < bad.length; i++){
-			bad[i].remove();
-		}
+	if(bad.length > 0){
+		const insertedDiv = document.createElement('div');
+		insertedDiv.innerHTML= '<p id="unhide" class="button white" style="text-decoration:none!important; color:#000!important;  font-size:1em !important; font-family:\'sans-serif\'!important; font-weight:normal !important; background-color:transparent!important; margin:0!important; padding:0!important; font-size:10px!important; line-height:1!important"' +
+			'alt="Click to reveal hidden elements in this page">' +
+	                '<span>Reveal hidden elements</span>' +
+			'</a>';
+		insertedDiv.style="position:fixed; bottom:1em; right:1em; opacity:0.8; z-index: 2147483647 !important; border-radius: 3px !important; background-color: #fff !important; padding: 0.5em !important;   box-shadow: 0 0 3px grey !important; font-color:#bbb!important; cursor: pointer!important;";
+		insertedDiv.addEventListener("click", dounhide, false);
+		document.body.insertBefore(insertedDiv, document.body.firstChild);
 	}
 }
 
 reveal_css();
 
-
 /*
 var a = document.getElementsByTagName("style")[2];
 var btn = document.createElement("style");        // Create a <button> element
-- 
2.26.2

--8<---------------cut here---------------end--------------->8---

bug#43297: 27.1; corrupts patch when diff-update-on-the-fly is set to nil

[Lars Ingebrigtsen]

Mark H Weaver <mhw@netris.org> writes:

> When I set 'diff-update-on-the-fly' to nil, I find that some patches are
> consistently corrupted when I save them while in diff-mode.  See below
> for an example patch that demonstrates this behavior.

This bug is still present in Emacs 28.

An easier way to reproduce this bug is to just load the example patch
into a buffer and then eval-ing

  (diff-fixup-modifs (point-min) (point-max))

This shouldn't change the contents, but it does.

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

bug#43297: 27.1; corrupts patch when diff-update-on-the-fly is set to nil

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> An easier way to reproduce this bug is to just load the example patch
> into a buffer and then eval-ing
>
>   (diff-fixup-modifs (point-min) (point-max))
>
> This shouldn't change the contents, but it does.

Poking around in the code shows that it is indeed the signature that
triggers the misbehaviour.  It goes to the end of the buffer and then
goes backward, line by line, and this is triggered:

	      (pcase (char-after)
		(?\s (cl-incf space))
		(?+ (cl-incf plus))
		(?- (cl-incf minus))

Which makes it fix the line numbers in the hunk incorrectly.

I'm not familiar with this code at all -- it seems to be written with
the idea that there's just a patch in the current buffer, and nothing
else.  (At least at the end of the buffer.)  And here's it's a patch in
an email, so there's extra stuff.

I don't see any obvious ways of fixing this...  anybody got any ideas?

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

bug#43297: 27.1; corrupts patch when diff-update-on-the-fly is set to nil

[Robert Pluim]

>>>>> On Fri, 16 Oct 2020 09:01:54 +0200, Lars Ingebrigtsen <larsi@gnus.org> said:

    Lars> Lars Ingebrigtsen <larsi@gnus.org> writes:
    >> An easier way to reproduce this bug is to just load the example patch
    >> into a buffer and then eval-ing
    >> 
    >> (diff-fixup-modifs (point-min) (point-max))
    >> 
    >> This shouldn't change the contents, but it does.

    Lars> Poking around in the code shows that it is indeed the signature that
    Lars> triggers the misbehaviour.  It goes to the end of the buffer and then
    Lars> goes backward, line by line, and this is triggered:

    Lars> 	      (pcase (char-after)
    Lars> 		(?\s (cl-incf space))
    Lars> 		(?+ (cl-incf plus))
    Lars> 		(?- (cl-incf minus))

    Lars> Which makes it fix the line numbers in the hunk incorrectly.

    Lars> I'm not familiar with this code at all -- it seems to be written with
    Lars> the idea that there's just a patch in the current buffer, and nothing
    Lars> else.  (At least at the end of the buffer.)  And here's it's a patch in
    Lars> an email, so there's extra stuff.

    Lars> I don't see any obvious ways of fixing this...  anybody got any ideas?

Search backwards from end-of-buffer for "-- " and then narrow the
buffer from (point-min) to there? Kind of hacky I guess, but otherwise
you'll have to complicate the pcase.

Robert
-- 

bug#43297: 27.1; corrupts patch when diff-update-on-the-fly is set to nil

[Lars Ingebrigtsen]

Robert Pluim <rpluim@gmail.com> writes:

> Search backwards from end-of-buffer for "-- " and then narrow the
> buffer from (point-min) to there? Kind of hacky I guess, but otherwise
> you'll have to complicate the pcase.

I'm fine with complicating the pcase, but I don't really know how to
resolve this.  A line like "-- " may really be a legitimate diff line,
or it may be a signature.

If we parsed the file from the top, and we assumed that nobody had
edited the patch, then we could see whether the "-- " was outside the
patch hunk or not, but the point of the function is to fix up the hunk
headers, so that's not really an option, either.

So I don't quite know whether this can even be fixed...  and it's a real
problem, since git format-patch puts a "--" signature at the end, it
looks like.

But...  we could, as a heuristic, guess that if the very first line we
see that could belong to a patch looks like "-- ?", then we ignore it.

That's probably better than what we have today, where patch destruction
is assured.  With a hack like that, we'd destroy patches rarely, I
think.

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

bug#43297: 27.1; corrupts patch when diff-update-on-the-fly is set to nil

[Daniel Martín via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Mark H Weaver <mhw@netris.org> writes:

> I don't know if this behavior is new in 27.1, but that's the only
> version I've tried this on.
>
> When I set 'diff-update-on-the-fly' to nil, I find that some patches are
> consistently corrupted when I save them while in diff-mode.  See below
> for an example patch that demonstrates this behavior.
>
> To reproduce:
>
> * Save the example patch below to "test.patch", and make another copy
>   in "test2.patch".
>
> * Launch "emacs -nw -q"  (I used Emacs 27.1)
>
> * Evaluate (setq diff-update-on-the-fly nil)
>
> * Find file "test2.patch".
>
> * Add a character anywhere in the buffer and then delete it, such that
>   the buffer is now in a "modified" state.
>
> * Save the file.
>
> * Run "diff -u test.patch test2.patch" in the shell.
>
> * Here's the result that I see:
>
> --- test.patch
> +++ test2.patch
> @@ -39,7 +39,7 @@
>   
>   	var elements = document.getElementsByTagName("style");
>   	for(var i = 0; i < elements.length; i++){
> -@@ -41,16 +48,20 @@ function reveal_css(){
> +@@ -41,17 +48,20 @@ function reveal_css(){
>   
>   		}
>   	}
>
>
> Emacs corrupts the line count of the final hunk, making it one larger
> than it should be (17 instead of 16).  This breaks the patch, such that
> attempts to apply it will fail.

I've tried to reproduce this bug in an Emacs build from the current
master branch and I couldn't.  Could you try a recent build of Emacs and
confirm if it's also fixed for you?  Thanks.