bug#30908: woman2-roff-buffer fails to restore set-text-properties, etc. on error

bug#30908: woman2-roff-buffer fails to restore set-text-properties, etc. on error

[Ivan Shmakov]

[-- Attachment #1: Type: text/plain, Size: 735 bytes --]

Package: emacs
Version: 25.1.1
Tags: patch

	The woman2-roff-buffer function uses unwind-protect to restore
	the canonically-space-region, insert-and-inherit and
	set-text-properties functions it temporarily overrides.

	Unfortunately, the first thing (conditionally) called in the
	‘unwind’ branch is the woman2-format-paragraphs function, which
	may result in error, thus precluding said restoration, leading to:

set-text-properties is an alias for ‘ignore’.

(set-text-properties &rest IGNORE)

Do nothing and return nil.
This function accepts any number of arguments, but ignores them.

	Please thus consider the patch MIMEd.

-- 
FSF associate member #7257  np. Sacred Armor of Antiriad — Traxer

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/diff, Size: 1211 bytes --]

--- woman.el.~1~	2017-11-05 15:00:52.696647297 +0000
+++ woman.el	2018-03-22 17:47:44.014986089 +0000
@@ -3710,13 +3710,14 @@ defun woman2-roff-buffer ()
 	       (set-marker to (woman-find-next-control-line)))
              ;; Call the appropriate function:
              (funcall fn to)))
-      (if (not (eobp))			; This should not happen, but ...
-	  (woman2-format-paragraphs (copy-marker (point-max) t)
-                                    woman-left-margin))
-      (fset 'canonically-space-region canonically-space-region)
-      (fset 'set-text-properties set-text-properties)
-      (fset 'insert-and-inherit insert-and-inherit)
-      (set-marker to nil))))
+      (unwind-protect
+          (if (not (eobp))             ; This should not happen, but ...
+              (woman2-format-paragraphs (copy-marker (point-max) t)
+                                        woman-left-margin))
+        (fset 'canonically-space-region canonically-space-region)
+        (fset 'set-text-properties set-text-properties)
+        (fset 'insert-and-inherit insert-and-inherit)
+        (set-marker to nil)))))
 
 (defun woman-find-next-control-line (&optional pat)
   "Find and return start of next control line.

bug#30908: woman2-roff-buffer fails to restore set-text-properties, etc. on error

[Noam Postavsky]

Ivan Shmakov <ivan@siamics.net> writes:

> --- woman.el.~1~	2017-11-05 15:00:52.696647297 +0000
> +++ woman.el	2018-03-22 17:47:44.014986089 +0000
> @@ -3710,13 +3710,14 @@ defun woman2-roff-buffer ()

> +      (unwind-protect
> +          (if (not (eobp))             ; This should not happen, but ...
> +              (woman2-format-paragraphs (copy-marker (point-max) t)
> +                                        woman-left-margin))
> +        (fset 'canonically-space-region canonically-space-region)
> +        (fset 'set-text-properties set-text-properties)
> +        (fset 'insert-and-inherit insert-and-inherit)
> +        (set-marker to nil)))))

Shouldn't this rather be combined into the existing unwind-protect
around the while?

bug#30908: woman2-roff-buffer fails to restore set-text-properties, etc. on error

[Ivan Shmakov]

[-- Attachment #1: Type: text/plain, Size: 1098 bytes --]

>>>>> Noam Postavsky <npostavs@gmail.com> writes:
>>>>> Ivan Shmakov <ivan@siamics.net> writes:

 >> @@ -3710,13 +3710,14 @@ defun woman2-roff-buffer ()

 >> +      (unwind-protect
 >> +          (if (not (eobp))             ; This should not happen, but ...
 >> +              (woman2-format-paragraphs (copy-marker (point-max) t)
 >> +                                        woman-left-margin))
 >> +        (fset 'canonically-space-region canonically-space-region)
 >> +        (fset 'set-text-properties set-text-properties)
 >> +        (fset 'insert-and-inherit insert-and-inherit)
 >> +        (set-marker to nil)))))

 > Shouldn’t this rather be combined into the existing unwind-protect
 > around the while?

	Due to the (not (eobp)) guard, I’ve assumed that the
	woman2-format-paragraphs call in the ‘unwind’ branch is for some
	sort of fallback processing (in the case the while loop fails.)

	If the call ended up there by mistake, indeed a progn should be
	used, as per the updated patch MIMEd.

-- 
FSF associate member #7257  http://am-1.org/~ivan/

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/diff, Size: 4203 bytes --]

--- -	2018-03-23 02:20:53.907229515 +0000
+++ 81FOuhEi.el	2018-03-23 02:20:39.083966234 +0000
@@ -3673,46 +3673,47 @@
     (fset 'insert-and-inherit (symbol-function 'insert))
     (fset 'set-text-properties 'ignore)
     (unwind-protect
-	(while
-	    ;; Find next control line:
-            (re-search-forward woman-request-regexp nil t)
-          (cond
-           ;; Construct woman function to call:
-           ((setq fn (intern-soft
-                      (concat "woman2-"
-                              (setq woman-request (match-string 1)))))
-            ;; Delete request or macro name:
-            (woman-delete-match 0))
-           ;; Unrecognized request:
-           ((prog1 nil
-              ;; (WoMan-warn ".%s request ignored!" woman-request)
-              (WoMan-warn-ignored woman-request "ignored!")
-              ;; (setq fn 'woman2-LP)
-              ;; AVOID LEAVING A BLANK LINE!
-              ;; (setq fn 'woman2-format-paragraphs)
-              ))
-           ;; .LP assumes it is at eol and leaves a (blank) line,
-           ;; so leave point at end of line before paragraph:
-           ((or (looking-at "[ \t]*$") ; no argument
-                woman-ignore)          ; ignore all
-            ;; (beginning-of-line) (kill-line)
-            ;; AVOID LEAVING A BLANK LINE!
-            (beginning-of-line) (woman-delete-line 1))
-           (t (end-of-line) (insert ?\n))
-           )
-           (if (not (or fn
-                        (and (not (memq (following-char) '(?. ?')))
-                             (setq fn 'woman2-format-paragraphs))))
-               ()
-             ;; Find next control line:
-	     (if (equal woman-request "TS")
-		 (set-marker to (woman-find-next-control-line "TE"))
-	       (set-marker to (woman-find-next-control-line)))
-             ;; Call the appropriate function:
-             (funcall fn to)))
-      (if (not (eobp))			; This should not happen, but ...
-	  (woman2-format-paragraphs (copy-marker (point-max) t)
-                                    woman-left-margin))
+        (progn
+          (while
+              ;; Find next control line:
+              (re-search-forward woman-request-regexp nil t)
+            (cond
+             ;; Construct woman function to call:
+             ((setq fn (intern-soft
+                        (concat "woman2-"
+                                (setq woman-request (match-string 1)))))
+              ;; Delete request or macro name:
+              (woman-delete-match 0))
+             ;; Unrecognized request:
+             ((prog1 nil
+                ;; (WoMan-warn ".%s request ignored!" woman-request)
+                (WoMan-warn-ignored woman-request "ignored!")
+                ;; (setq fn 'woman2-LP)
+                ;; AVOID LEAVING A BLANK LINE!
+                ;; (setq fn 'woman2-format-paragraphs)
+                ))
+             ;; .LP assumes it is at eol and leaves a (blank) line,
+             ;; so leave point at end of line before paragraph:
+             ((or (looking-at "[ \t]*$") ; no argument
+                  woman-ignore)          ; ignore all
+              ;; (beginning-of-line) (kill-line)
+              ;; AVOID LEAVING A BLANK LINE!
+              (beginning-of-line) (woman-delete-line 1))
+             (t (end-of-line) (insert ?\n))
+             )
+            (if (not (or fn
+                         (and (not (memq (following-char) '(?. ?')))
+                              (setq fn 'woman2-format-paragraphs))))
+                ()
+              ;; Find next control line:
+              (if (equal woman-request "TS")
+                  (set-marker to (woman-find-next-control-line "TE"))
+                (set-marker to (woman-find-next-control-line)))
+              ;; Call the appropriate function:
+              (funcall fn to)))
+          (if (not (eobp))             ; This should not happen, but ...
+              (woman2-format-paragraphs (copy-marker (point-max) t)
+                                        woman-left-margin)))
       (fset 'canonically-space-region canonically-space-region)
       (fset 'set-text-properties set-text-properties)
       (fset 'insert-and-inherit insert-and-inherit)

bug#30908: woman2-roff-buffer fails to restore set-text-properties, etc. on error

[Noam Postavsky]

Ivan Shmakov <ivan@siamics.net> writes:

>  > Shouldn’t this rather be combined into the existing unwind-protect
>  > around the while?
>
> 	Due to the (not (eobp)) guard, I’ve assumed that the
> 	woman2-format-paragraphs call in the ‘unwind’ branch is for some
> 	sort of fallback processing (in the case the while loop fails.)

Hmm, looking at this again, I'm not sure.  That is, it's clearly
fallback processing in case the loop ends before going through the whole
buffer.  But does it also make sense as fallback processing when some
kind of error was signaled in the loop?  I don't know.

bug#30908: woman2-roff-buffer fails to restore set-text-properties, etc. on error

[Noam Postavsky]

tags 30908 fixed
close 30908 27.1
quit

Noam Postavsky <npostavs@gmail.com> writes:

> Ivan Shmakov <ivan@siamics.net> writes:
>
>>  > Shouldn’t this rather be combined into the existing unwind-protect
>>  > around the while?
>>
>> 	Due to the (not (eobp)) guard, I’ve assumed that the
>> 	woman2-format-paragraphs call in the ‘unwind’ branch is for some
>> 	sort of fallback processing (in the case the while loop fails.)
>
> Hmm, looking at this again, I'm not sure.  That is, it's clearly
> fallback processing in case the loop ends before going through the whole
> buffer.  But does it also make sense as fallback processing when some
> kind of error was signaled in the loop?  I don't know.

I've decided that it doesn't make sense to do any further processing
once we've hit an error.

[1: 66dbb787a2]: 2018-04-26 07:37:48 -0400
  Ensure woman2-roff-buffer restores functions on error (Bug#30908)
  https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=66dbb787a22d4ae1d513a3ee27e22eed395f5676