One broken property drawer prevents setting of any property

One broken property drawer prevents setting of any property

[Eric Abrahamsen]

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

Looks like gmane's down for a bit, but presumably this will
eventually go through.

As I mentioned in the last message, if any property drawer in an org
file is malformed, it makes it impossible to set properties on any other
heading in the file. This is because, before the property is set, the
file is scanned for other valid property keys, and when the scanning
process hits the malformed drawer, it errors out.

The attached file, opened from emacs -Q, is enough to cause the error.
Trying to set a property on the second heading gives:

Debugger entered--Lisp error: (wrong-type-argument integer-or-marker-p nil)
  org-buffer-property-keys(nil t t)
  org-read-property-name()
  org-set-property(nil nil)
  call-interactively(org-set-property nil nil)

`org-buffer-property-keys' contains a call to `org-get-property-block',
which returns nil on a broken property drawer, and leads to the type
argument above.

By passing the FORCE argument to `org-get-property-block', the broken
block ends up getting silently repaired, and everything works as normal.
I'm not sure, however, that silently repairing things without the user's
knowledge is the right thing to do...

Eric


[-- Attachment #2: test.org --]
[-- Type: application/vnd.lotus-organizer, Size: 59 bytes --]

Re: One broken property drawer prevents setting of any property

[Bastien]

Hi Eric,

Eric Abrahamsen <eric@ericabrahamsen.net> writes:

> By passing the FORCE argument to `org-get-property-block', the broken
> block ends up getting silently repaired, and everything works as normal.

Can you show this as a patch?

> I'm not sure, however, that silently repairing things without the user's
> knowledge is the right thing to do...

We can warn the user with a temporary message, or ask him for
confirmation.  Or provide a helper command to repair drawers and
advertize it instead of throwing an error.

I'll look into this after you send me the patch.

Thanks,

-- 
 Bastien

Re: One broken property drawer prevents setting of any property

[Eric Abrahamsen]

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

Bastien <bzg@gnu.org> writes:

> Hi Eric,
>
> Eric Abrahamsen <eric@ericabrahamsen.net> writes:
>
>> By passing the FORCE argument to `org-get-property-block', the broken
>> block ends up getting silently repaired, and everything works as normal.
>
> Can you show this as a patch?
>
>> I'm not sure, however, that silently repairing things without the user's
>> knowledge is the right thing to do...
>
> We can warn the user with a temporary message, or ask him for
> confirmation.  Or provide a helper command to repair drawers and
> advertize it instead of throwing an error.
>
> I'll look into this after you send me the patch.

The more I thought about it, the more it seemed the silent repair is a
bad idea: it's not really repair, it's just sticking a new :END: in, and
will very likely result in cruft in the buffer. This patch wraps the
scan in catch/throw and asks users if they want to repair broken
drawers. Since the y-or-no-p stops at the broken drawer, it should be
useful for manual fixing.

E


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Warn-users-of-malformed-property-drawers.patch --]
[-- Type: text/x-diff, Size: 1443 bytes --]

From 3995f4816ba400f5c7645f84b5dcf7e84698d604 Mon Sep 17 00:00:00 2001
From: Eric Abrahamsen <eric@ericabrahamsen.net>
Date: Fri, 23 May 2014 16:52:58 +0800
Subject: [PATCH] Warn users of malformed property drawers

org.el (org-buffer-property-keys): When scanning the buffer for valid
property keys, give users a chance to repair any malformed property
drawers.
---
 lisp/org.el | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/lisp/org.el b/lisp/org.el
index b16515e..a3f0cba 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -15712,12 +15712,17 @@ formats in the current buffer."
 	(widen)
 	(goto-char (point-min))
 	(while (re-search-forward org-property-start-re nil t)
-	  (setq range (org-get-property-block))
-	  (goto-char (car range))
-	  (while (re-search-forward org-property-re
-		  (cdr range) t)
-	    (add-to-list 'rtn (org-match-string-no-properties 2)))
-	  (outline-next-heading))))
+	  (catch 'cont
+	    (setq range (or (org-get-property-block)
+			    (if (y-or-n-p
+				 (format "Malformed drawer at %d, repair?" (point)))
+				(org-get-property-block nil nil t)
+				(throw 'cont nil))))
+	    (goto-char (car range))
+	    (while (re-search-forward org-property-re
+				      (cdr range) t)
+	      (add-to-list 'rtn (org-match-string-no-properties 2)))
+	    (outline-next-heading)))))
 
     (when include-specials
       (setq rtn (append org-special-properties rtn)))
-- 
1.9.3

Re: One broken property drawer prevents setting of any property

[Bastien]

Hi Eric,

Eric Abrahamsen <eric@ericabrahamsen.net> writes:

> The more I thought about it, the more it seemed the silent repair is a
> bad idea: it's not really repair, it's just sticking a new :END: in, and
> will very likely result in cruft in the buffer. This patch wraps the
> scan in catch/throw and asks users if they want to repair broken
> drawers. Since the y-or-no-p stops at the broken drawer, it should be
> useful for manual fixing.

Agreed and applied on master, thanks a lot!

-- 
 Bastien