diff-sanity-check-hunk fails on "unterminated" hunks

diff-sanity-check-hunk fails on "unterminated" hunks

[Bob Rogers]

[-- Attachment #1: message body text --]
[-- Type: text/plain, Size: 2564 bytes --]

   To reproduce the problem (in Emacs 22 or trunk):

   1.  Put the first attachment (unterminated-hunk-test.patch) in the
top directory of your Emacs working copy.

   2.  Do "emacs -Q unterminated-hunk-test.patch" to bring it up in
diff-mode.

   3.  Move point into the first hunk and type C-c C-c; it should move
to that hunk in the source, but instead it gives the error "Hunk
seriously messed up".  Doing the same thing in the second hunk should
find the source (with the message "Hunk already applied").

   Note that this patch was produced in a slightly unusual way.  Instead
of doing 

	rogers@rgrjr> diff -ur lisp/url.old lisp/url

to get all differing files at once (as any reasonable person would), I
produced the diffs separately and concatenated them:

	rogers@rgrjr> diff -u lisp/url.old/url-dav.el lisp/url/url-dav.el > unterminated-hunk-test.patch
	rogers@rgrjr> diff -u lisp/url.old/url-ftp.el lisp/url/url-ftp.el >> unterminated-hunk-test.patch

For this reason, the usual "diff -ur file1 file2" line that precedes
each file in a normal multifile diff is missing.  The start of the
second file therefore comes right after the last hunk of the first file,
and because it's a unidiff, it looks like this:

	--- lisp/url.old/url-ftp.el	2007-09-11 20:42:21.000000000 -0400
	+++ lisp/url/url-ftp.el	2007-08-13 19:47:02.000000000 -0400
	@@ -24,12 +24,12 @@

So diff-sanity-check-hunk sees one more "-" line and one more "+" line
than it should, and barfs when it comes to the "@@" for the first hunk
of the second file.

   The second attachment
(fix-diff-sanity-check-hunk-for-unterminated-hunk.patch) fixes the
problem by doing the normal exit test (exit when both line counters get
to zero) before checking the BOL character.  (It's actually just a small
change but with a lot of whitespace adjustment.)

   One could argue that this is an artificial problem caused by an
eccentric application of "diff".  However, I would argue that unidiff
hunks *are* unterminated, except by counting lines as the original code
does, so the revision strikes me as cleaner and more robust.  Also, the
concatenation of two syntactically valid patch files should also be
considered a syntactically valid patch file (modulo issues with relative
file names), so Emacs ought to deal with such files gracefully.
Finally, the issue came up naturally enough for me, so it may not be all
that rare; I ran vc-diff repeatedly on a list of changed file names,
concatenating the output into a diff-mode buffer.

   WDOT?

					-- Bob Rogers
					   http://rgrjr.dyndns.org/


[-- Attachment #2: Type: text/plain, Size: 1062 bytes --]

--- lisp/url.old/url-dav.el	2007-09-11 20:42:46.000000000 -0400
+++ lisp/url/url-dav.el	2007-08-13 19:47:02.000000000 -0400
@@ -23,7 +23,7 @@
 ;; Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
 ;; Boston, MA 02110-1301, USA.
 
-    ;; DAV is in RFC 2518.
+;; DAV is in RFC 2518.
 
 ;;; Commentary:
 
--- lisp/url.old/url-ftp.el	2007-09-11 20:42:21.000000000 -0400
+++ lisp/url/url-ftp.el	2007-08-13 19:47:02.000000000 -0400
@@ -24,12 +24,12 @@
 
 ;;; Commentary:
 
-    ;; We knew not what we did when we overloaded 'file' to mean 'file'
-    ;; and 'ftp' back in the dark ages of the web.
-    ;;
-    ;; This stub file is just here to please the auto-scheme-loading code
-    ;; in url-methods.el and just maps everything onto the code in
-    ;; url-file.
+;; We knew not what we did when we overloaded 'file' to mean 'file'
+;; and 'ftp' back in the dark ages of the web.
+;;
+;; This stub file is just here to please the auto-scheme-loading code
+;; in url-methods.el and just maps everything onto the code in
+;; url-file.
 
 ;;; Code:
 

[-- Attachment #3: Type: text/plain, Size: 2209 bytes --]

Index: lisp/diff-mode.el
===================================================================
RCS file: /sources/emacs/emacs/lisp/diff-mode.el,v
retrieving revision 1.113
diff -c -r1.113 diff-mode.el
*** lisp/diff-mode.el	11 Sep 2007 06:59:13 -0000	1.113
--- lisp/diff-mode.el	12 Sep 2007 01:32:24 -0000
***************
*** 1243,1264 ****
                  (after (if (match-string 2) (string-to-number (match-string 2)) 1)))
              (forward-line)
              (while
!                 (case (char-after)
!                   (?\s (decf before) (decf after) t)
!                   (?- (decf before) t)
!                   (?+ (decf after) t)
!                   (t
!                    (cond
!                     ((and (zerop before) (zerop after)) nil)
!                     ((or (< before 0) (< after 0))
!                      (error (if (or (zerop before) (zerop after))
!                                 "End of hunk ambiguously marked"
!                               "Hunk seriously messed up")))
!                     ((not (y-or-n-p "Try to auto-fix whitespace loss and word-wrap damage? "))
!                      (error "Abort!"))
!                     ((eolp) (insert " ") (forward-line -1) t)
!                     (t (insert " ")
!                        (delete-region (- (point) 2) (- (point) 1)) t))))
                (forward-line)))))
  
         ;; A plain diff.
--- 1243,1262 ----
                  (after (if (match-string 2) (string-to-number (match-string 2)) 1)))
              (forward-line)
              (while
! 		(cond ((and (zerop before) (zerop after)) nil)
! 		      ((case (char-after)
! 			 (?\s (decf before) (decf after) t)
! 			 (?- (decf before) t)
! 			 (?+ (decf after) t)))
! 		      ((or (< before 0) (< after 0))
! 		       (error (if (or (zerop before) (zerop after))
! 				  "End of hunk ambiguously marked"
! 				  "Hunk seriously messed up")))
! 		      ((not (y-or-n-p "Try to auto-fix whitespace loss and word-wrap damage? "))
! 		       (error "Abort!"))
! 		      ((eolp) (insert " ") (forward-line -1) t)
! 		      (t (insert " ")
! 			 (delete-region (- (point) 2) (- (point) 1)) t))
                (forward-line)))))
  
         ;; A plain diff.

[-- Attachment #4: Type: text/plain, Size: 142 bytes --]

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/emacs-devel

Re: diff-sanity-check-hunk fails on "unterminated" hunks

[Stefan Monnier]

> to get all differing files at once (as any reasonable person would), I
> produced the diffs separately and concatenated them:

Yes, these diffs are problematic because you need to count the lines in
order to do the right thing and most of diff-mode.el doesn't count the lines
but just presumes that the hunk-end will be self-evident.

>    The second attachment
> (fix-diff-sanity-check-hunk-for-unterminated-hunk.patch) fixes the
> problem by doing the normal exit test (exit when both line counters get
> to zero) before checking the BOL character.  (It's actually just a small
> change but with a lot of whitespace adjustment.)

This might be right, but I think I prefer to also complain when the hunk is
formally correct but is followed by things that look like a continuation.
Especially since diff-mode will typically not handle them correctly.

Does the patch below address your particular problem (which I agree is
a case that should be handled, I've encountered it several times already,
tho that was before implementing diff-sanity-check-hunk, obviously).


        Stefan


--- diff-mode.el	27 aoû 2007 23:25:15 -0400	1.99.2.4
+++ diff-mode.el	12 sep 2007 01:03:59 -0400	
@@ -1181,7 +1181,16 @@
             (while
                 (case (char-after)
                   (?\s (decf before) (decf after) t)
-                  (?- (decf before) t)
+                  (?-
+                   (if (and (looking-at diff-file-header-re)
+                            (zerop before) (zerop after))
+                       ;; No need to query: this is a case where two patches
+                       ;; are concatenated and only counting the lines will
+                       ;; give the right result.  Let's just add an empty
+                       ;; line so that our code which doesn't count lines
+                       ;; will not get confused.
+                       (progn (save-excursion (insert "\n")) nil)
+                     (decf before) t))
                   (?+ (decf after) t)
                   (t
                    (cond

Re: diff-sanity-check-hunk fails on "unterminated" hunks

[Bob Rogers]

   From: Stefan Monnier <monnier@iro.umontreal.ca>
   Date: Wed, 12 Sep 2007 01:09:38 -0400

   > to get all differing files at once (as any reasonable person would), I
   > produced the diffs separately and concatenated them:

   Yes, these diffs are problematic because you need to count the lines in
   order to do the right thing and most of diff-mode.el doesn't count the lines
   but just presumes that the hunk-end will be self-evident.

But the hunk-extracting code (e.g. for C-c C-c) does seem to work with
such hunks, once diff-sanity-check-hunk is taught not to complain . . .

   . . .

   Does the patch below address your particular problem (which I agree is
   a case that should be handled, I've encountered it several times already,
   tho that was before implementing diff-sanity-check-hunk, obviously).

	   Stefan

This looks like it ought to work, and does seem more robust; I will test
it later today (no time now).  But I'm a bit concerned about the effect
of silently adding a newline; this will fail mysteriously if the diff is
read-only.

					-- Bob

Re: diff-sanity-check-hunk fails on "unterminated" hunks

[Bob Rogers]

   From: Bob Rogers <rogers-emacs@rgrjr.dyndns.org>
   Date: Wed, 12 Sep 2007 08:09:25 -0400

      From: Stefan Monnier <monnier@iro.umontreal.ca>
      Date: Wed, 12 Sep 2007 01:09:38 -0400

      . . .

      Does the patch below address your particular problem (which I agree is
      a case that should be handled, I've encountered it several times already,
      tho that was before implementing diff-sanity-check-hunk, obviously).

	      Stefan

   This looks like it ought to work, and does seem more robust; I will test
   it later today (no time now).  But I'm a bit concerned about the effect
   of silently adding a newline; this will fail mysteriously if the diff is
   read-only.

It works for me, but I still have reservations about modifying the
buffer.  It modifies the buffer successfully even if I have the
read-only bit set -- how does it do this?  But then I can't undo it
without doing C-x C-q first . . .

					-- Bob