[PATCH] Remove unecesssary invocations of org-mode in ox-publish

[PATCH] Remove unecesssary invocations of org-mode in ox-publish

[Matt Lundin]

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

This patch fixes a bug in which org-publish makes the following call...

(let ((org-inhibit-startup t) (org-mode)))

...in buffers that are already open. This often happens, for instance,
when publishing a sitemap. But there is no need to call org-mode again
in these open buffers. Moreover calling org-mode with
org-inhibit-startup set to t destroys crucial information in open
buffers, such as dir-locals variables. This can interfere rather
seriously with web publishing, causing website files I am editing to
lose the following crucial settings after I call org-publish:

((org-mode .
	   ((org-link-file-path-type . relative)
	    (org-display-custom-times . t))))

Thanks,
Matt


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Remove-unnecessary-invocations-of-org-mode.patch --]
[-- Type: text/x-diff, Size: 2030 bytes --]

From 05410ec611fdf8483053e2905df9a52ea9c53551 Mon Sep 17 00:00:00 2001
From: Matt Lundin <mdl@imapmail.org>
Date: Mon, 28 Jul 2014 23:56:42 -0500
Subject: [PATCH] Remove unnecessary invocations of org-mode

* lisp/ox-publish.el: (org-publish-find-title) (org-publish-find-date)
  Fix unnecessary invocations of org-mode with
  org-inhibit-startup.

The functions above call org-mode with org-inhibit-startup in org
buffers that are already open. The result is that these open buffers
lose some crucial local variables during publishing. For instance,
an open buffer will lose its dir-local-variables-alist setting
during publishing. There is no need to call org-mode here, because
org-mode is a) either already initialized in the buffer or b)
initialized via earlier call to find-file-no-select.

This looks to me like a relic of some past time when these functions were
called in temporary buffers that contained copies of the file's contents.
---
 lisp/ox-publish.el | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/lisp/ox-publish.el b/lisp/ox-publish.el
index be208df..94f12e9 100644
--- a/lisp/ox-publish.el
+++ b/lisp/ox-publish.el
@@ -806,11 +806,9 @@ Default for SITEMAP-FILENAME is 'sitemap.org'."
   "Find the title of FILE in project."
   (or
    (and (not reset) (org-publish-cache-get-file-property file :title nil t))
-   (let* ((org-inhibit-startup t)
-	  (visiting (find-buffer-visiting file))
+   (let* ((visiting (find-buffer-visiting file))
 	  (buffer (or visiting (find-file-noselect file))))
      (with-current-buffer buffer
-       (org-mode)
        (let ((title
 	      (let ((property (plist-get (org-export-get-environment) :title)))
 		(if property
@@ -831,7 +829,6 @@ time in `current-time' format."
 	   (file-buf (or visiting (find-file-noselect file nil)))
 	   (date (plist-get
 		  (with-current-buffer file-buf
-		    (let ((org-inhibit-startup t)) (org-mode))
 		    (org-export-get-environment))
 		  :date)))
       (unless visiting (kill-buffer file-buf))
-- 
2.0.2

Re: [PATCH] Remove unecesssary invocations of org-mode in ox-publish

[Bastien]

Hi Matt,

Matt Lundin <mdl@imapmail.org> writes:

> This patch fixes a bug in which org-publish makes the following
> call...

Applied, thanks!

-- 
 Bastien

Re: [PATCH] Remove unecesssary invocations of org-mode in ox-publish

[Matt Lundin]

Bastien <bzg@gnu.org> writes:

> Hi Matt,
>
> Matt Lundin <mdl@imapmail.org> writes:
>
>> This patch fixes a bug in which org-publish makes the following call...
>> (let ((org-inhibit-startup t) (org-mode)))
>
> Applied, thanks!

Oops... I see now that org-publish-find-date and org-publish-find-title
call org-export-get-environment. This in turn relies on org-set-local to
set #+BIND: variables, which requires the buffer to be writable. I have
an org-mode-hook that sets some of my web publishing files read-only (so
as to prevent accidental editing). Without org-inhibit-startup, these
buffers remain read-only, causing the following error message:

--8<---------------cut here---------------start------------->8---
org-export--get-global-options: Buffer is read-only: #<killed buffer>
--8<---------------cut here---------------end--------------->8---

This could be solved by wrapping org-export-get-environment withing
those functions with (let ((buffer-read-only nil)) ...).

However, I think the fundamental problem is that
org-export-get-environment should be called on a copy of the buffer.
That, at least, is how it is used in org-export-as (see lines 3084 -
3107 of ox.el). When org-publish-find-title and org-publish-call-date
calls it in a buffer that is already open, it does so on the original
copy of the buffer. This has the effect of setting the #+BIND variables
within the live buffer, which could (theoretically) be dangerous, since,
AFAICT, they should only be set in a temporary copy of the buffer.

So to be safe, we could do the following in org-publish-find-date and
org-publish-find-title... 

(org-export-with-buffer-copy (org-export-get-environment))

What do you think?

Best,
Matt

Re: [PATCH] Remove unecesssary invocations of org-mode in ox-publish

[Matt Lundin]

Matt Lundin <mdl@imapmail.org> writes:

> Bastien <bzg@gnu.org> writes:
>
>> Hi Matt,
>>
>> Matt Lundin <mdl@imapmail.org> writes:
>>
>>> This patch fixes a bug in which org-publish makes the following call...
>>> (let ((org-inhibit-startup t) (org-mode)))
>>
>> Applied, thanks!
>
> Oops... I see now that org-publish-find-date and org-publish-find-title
> call org-export-get-environment. This in turn relies on org-set-local to
> set #+BIND: variables, which requires the buffer to be writable. I have
> an org-mode-hook that sets some of my web publishing files read-only (so
> as to prevent accidental editing). Without org-inhibit-startup, these
> buffers remain read-only, causing the following error message:

[...continuing the previous post.]

I think I've found a more general problem here. When called in an org
buffer...

(let ((org-inhibit-startup t)) (org-mode))

...either wipes out dir-locals variables or prevents them from loading.
I have looked at the org-mode code and am not entirely sure why this
would be the case. 

However, this causes problems, for instance, when
org-agenda-inhibit-startup is t or when calling org-toggle-tags-groups.

And one more minor thing...

The documentation for org-agenda-inhibit-startup says that the default
is t. However, the default is nil.

,----
| Inhibit startup when preparing agenda buffers.
| When this variable is `t' (the default), the initialization of
| the Org agenda buffers is inhibited: e.g. the visibility state
| is not set, the tables are not re-aligned, etc.
`----

Thanks,
Matt

Re: [PATCH] Remove unecesssary invocations of org-mode in ox-publish

[Bastien]

Hi Matt,

Matt Lundin <mdl@imapmail.org> writes:

> The documentation for org-agenda-inhibit-startup says that the default
> is t. However, the default is nil.
>
> ,----
> | Inhibit startup when preparing agenda buffers.
> | When this variable is `t' (the default), the initialization of
> | the Org agenda buffers is inhibited: e.g. the visibility state
> | is not set, the tables are not re-aligned, etc.
> `----

Please go ahead for this too.

Thanks,

-- 
 Bastien

Re: [PATCH] Remove unecesssary invocations of org-mode in ox-publish

[Matt Lundin]

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

Bastien <bzg@gnu.org> writes:

> Hi Matt,
>
> Matt Lundin <mdl@imapmail.org> writes:
>
>> The documentation for org-agenda-inhibit-startup says that the default
>> is t. However, the default is nil.
>>
>> ,----
>> | Inhibit startup when preparing agenda buffers.
>> | When this variable is `t' (the default), the initialization of
>> | the Org agenda buffers is inhibited: e.g. the visibility state
>> | is not set, the tables are not re-aligned, etc.
>> `----
>
> Please go ahead for this too.

Gladly!


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-docstring-of-org-agenda-inhibit-startup.patch --]
[-- Type: text/x-diff, Size: 1070 bytes --]

From 5b2ef2c660d8163199f45d4dbf58d33f3ec1f918 Mon Sep 17 00:00:00 2001
From: Matt Lundin <mdl@imapmail.org>
Date: Tue, 29 Jul 2014 10:59:19 -0500
Subject: [PATCH] Fix docstring of org-agenda-inhibit-startup

* lisp/org.el: org-agenda-inhibit-startup: fix docstring to reflect
  default value
---
 lisp/org.el | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lisp/org.el b/lisp/org.el
index 5c4b211..df13fff 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -17961,9 +17961,9 @@ This requires Emacs >= 24.1, build with imagemagick support."
 
 (defcustom org-agenda-inhibit-startup nil
   "Inhibit startup when preparing agenda buffers.
-When this variable is `t' (the default), the initialization of
-the Org agenda buffers is inhibited: e.g. the visibility state
-is not set, the tables are not re-aligned, etc."
+When this variable is `t', the initialization of the Org agenda
+buffers is inhibited: e.g. the visibility state is not set, the
+tables are not re-aligned, etc."
   :type 'boolean
   :version "24.3"
   :group 'org-agenda)
-- 
2.0.3

Re: [PATCH] Remove unecesssary invocations of org-mode in ox-publish

[Bastien]

Matt Lundin <mdl@imapmail.org> writes:

>> Please go ahead for this too.
>
> Gladly!

Applied, thanks!

-- 
 Bastien

Re: [PATCH] Remove unecesssary invocations of org-mode in ox-publish

[Matt Lundin]

Matt Lundin <mdl@imapmail.org> writes:

> Bastien <bzg@gnu.org> writes:
>
>> Hi Matt,
>>
>> Matt Lundin <mdl@imapmail.org> writes:
>>
>>> This patch fixes a bug in which org-publish makes the following call...
>>> (let ((org-inhibit-startup t) (org-mode)))
>>
>> Applied, thanks!
>
> This could be solved by wrapping org-export-get-environment withing
> those functions with (let ((buffer-read-only nil)) ...).

Correction... this should be...

(let ((inhibit-read-only t)) ...)

Correction is for the record only, as this is moot given the rest of the
email.

Matt

Re: [PATCH] Remove unecesssary invocations of org-mode in ox-publish

[Bastien]

Hi Matt,

Matt Lundin <mdl@imapmail.org> writes:

> So to be safe, we could do the following in org-publish-find-date and
> org-publish-find-title... 
>
> (org-export-with-buffer-copy (org-export-get-environment))
>
> What do you think?

Yes, this sounds right, please go ahead.

I applied your fix in the maint branch, so if you need to revert it
please do the revert from there first.

Thanks in advance,

-- 
 Bastien

Re: [PATCH] Remove unecesssary invocations of org-mode in ox-publish

[Matt Lundin]

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

Bastien <bzg@gnu.org> writes:

> Hi Matt,
>
> Matt Lundin <mdl@imapmail.org> writes:
>
>> So to be safe, we could do the following in org-publish-find-date and
>> org-publish-find-title... 
>>
>> (org-export-with-buffer-copy (org-export-get-environment))
>>
>> What do you think?
>
> Yes, this sounds right, please go ahead.
>
> I applied your fix in the maint branch, so if you need to revert it
> please do the revert from there first.

Here's the patch. There's no need to revert anything, as this builds
upon my previous commit.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Call-org-export-get-environment-in-buffer-copy.patch --]
[-- Type: text/x-diff, Size: 1597 bytes --]

From 72217e53103a82e0397d1435331e3eed10f0fbd5 Mon Sep 17 00:00:00 2001
From: Matt Lundin <mdl@imapmail.org>
Date: Tue, 29 Jul 2014 10:39:57 -0500
Subject: [PATCH 1/2] Call org-export-get-environment in buffer copy

* lisp/ox-publish.el: (org-publish-find-title) (org-publish-find-date)
  Make sure to call org-export-get-environment in copy of
  buffer. Otherwise, #+bind variables meant for export can be set in
  live buffers.
---
 lisp/ox-publish.el | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/lisp/ox-publish.el b/lisp/ox-publish.el
index 94f12e9..26d4be7 100644
--- a/lisp/ox-publish.el
+++ b/lisp/ox-publish.el
@@ -810,7 +810,10 @@ Default for SITEMAP-FILENAME is 'sitemap.org'."
 	  (buffer (or visiting (find-file-noselect file))))
      (with-current-buffer buffer
        (let ((title
-	      (let ((property (plist-get (org-export-get-environment) :title)))
+	      (let ((property
+		     (plist-get
+		      (org-export-with-buffer-copy (org-export-get-environment))
+		      :title)))
 		(if property
 		    (org-no-properties (org-element-interpret-data property))
 		  (file-name-nondirectory (file-name-sans-extension file))))))
@@ -829,7 +832,7 @@ time in `current-time' format."
 	   (file-buf (or visiting (find-file-noselect file nil)))
 	   (date (plist-get
 		  (with-current-buffer file-buf
-		    (org-export-get-environment))
+		    (org-export-with-buffer-copy (org-export-get-environment)))
 		  :date)))
       (unless visiting (kill-buffer file-buf))
       ;; DATE is either a timestamp object or a secondary string.  If it
-- 
2.0.3

Re: [PATCH] Remove unecesssary invocations of org-mode in ox-publish

[Matt Lundin]

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

Matt Lundin <mdl@imapmail.org> writes:

> Bastien <bzg@gnu.org> writes:
>
>> Matt Lundin <mdl@imapmail.org> writes:
>>
>>> So to be safe, we could do the following in org-publish-find-date and
>>> org-publish-find-title... 
>>>
>>> (org-export-with-buffer-copy (org-export-get-environment))
>>>
>>> What do you think?
>>
>> Yes, this sounds right, please go ahead.
>>
>> I applied your fix in the maint branch, so if you need to revert it
>> please do the revert from there first.
>
> Here's the patch. There's no need to revert anything, as this builds
> upon my previous commit.
>
> From 72217e53103a82e0397d1435331e3eed10f0fbd5 Mon Sep 17 00:00:00 2001
> From: Matt Lundin <mdl@imapmail.org>
> Date: Tue, 29 Jul 2014 10:39:57 -0500
> Subject: [PATCH 1/2] Call org-export-get-environment in buffer copy
>
> * lisp/ox-publish.el: (org-publish-find-title) (org-publish-find-date)
>   Make sure to call org-export-get-environment in copy of
>   buffer. Otherwise, #+bind variables meant for export can be set in
>   live buffers.

Please *do not apply* the previous patch. Instead, I've attached an
optimized version.

All in all this patch + commit 507244d56b055e7595ba94fe89d45c7ddb2559df
modestly improves the performance of org-publish-org-sitemap. On a
directory of 104 files, it used to take this long...

 org-publish-org-sitemap      1           10.508871433  10.508871433

...to generate a sitemap. With this patch and commit 507244d, it takes
this long...

 org-publish-org-sitemap      1           7.700290794   7.700290794

Clearly, this is still very inadequate, but it is an improvement. I
would love to use the built in site-map functions, but they are simply
to slow for any larger projects.

Could we do something like this to speed it up?

--8<---------------cut here---------------start------------->8---
(with-temp-buffer 
  (insert-file-contents file)
  (goto-char (point-min))
  (when (re-search-forward "^#\\+TITLE:" nil t)
    (org-element-at-point)))
--8<---------------cut here---------------end--------------->8---

Until then, here's the patch.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Protect-open-buffers-when-publishing-sitemap.patch --]
[-- Type: text/x-diff, Size: 2482 bytes --]

From 9260e6d4c5537994ffb6df2f2cc48c62ad4ffb63 Mon Sep 17 00:00:00 2001
From: Matt Lundin <mdl@imapmail.org>
Date: Tue, 29 Jul 2014 13:35:41 -0500
Subject: [PATCH] Protect open buffers when publishing sitemap

* lisp/ox-publish.el: (org-publish-find-title) (org-publish-find-date)
  Make sure to call org-export-get-environment in copy of buffer if
  emacs is already visiting. Otherwise, #+bind variables meant for
  export can be set in live buffers.

This patch also optimizes the above functions by inhibiting
org-startup on buffers emacs visits temporarily.
---
 lisp/ox-publish.el | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/lisp/ox-publish.el b/lisp/ox-publish.el
index 94f12e9..df40572 100644
--- a/lisp/ox-publish.el
+++ b/lisp/ox-publish.el
@@ -806,11 +806,18 @@ Default for SITEMAP-FILENAME is 'sitemap.org'."
   "Find the title of FILE in project."
   (or
    (and (not reset) (org-publish-cache-get-file-property file :title nil t))
-   (let* ((visiting (find-buffer-visiting file))
+   (let* ((org-inhibit-startup t)
+	  (visiting (find-buffer-visiting file))
 	  (buffer (or visiting (find-file-noselect file))))
      (with-current-buffer buffer
        (let ((title
-	      (let ((property (plist-get (org-export-get-environment) :title)))
+	      (let ((property
+		     (plist-get
+		      ;; protect local variables in open buffers
+		      (if visiting
+			  (org-export-with-buffer-copy (org-export-get-environment))
+			(org-export-get-environment))
+		      :title)))
 		(if property
 		    (org-no-properties (org-element-interpret-data property))
 		  (file-name-nondirectory (file-name-sans-extension file))))))
@@ -825,11 +832,14 @@ If FILE is an Org file and provides a DATE keyword use it.  In
 any other case use the file system's modification time.  Return
 time in `current-time' format."
   (if (file-directory-p file) (nth 5 (file-attributes file))
-    (let* ((visiting (find-buffer-visiting file))
+    (let* ((org-inhibit-startup t)
+	   (visiting (find-buffer-visiting file))
 	   (file-buf (or visiting (find-file-noselect file nil)))
 	   (date (plist-get
 		  (with-current-buffer file-buf
-		    (org-export-get-environment))
+		    (if visiting
+			(org-export-with-buffer-copy (org-export-get-environment))
+		      (org-export-get-environment)))
 		  :date)))
       (unless visiting (kill-buffer file-buf))
       ;; DATE is either a timestamp object or a secondary string.  If it
-- 
2.0.3

Re: [PATCH] Remove unecesssary invocations of org-mode in ox-publish

[Bastien]

Hi Matt,

Matt Lundin <mdl@imapmail.org> writes:

> Please *do not apply* the previous patch. Instead, I've attached an
> optimized version.

Applied, thanks.

> All in all this patch + commit 507244d56b055e7595ba94fe89d45c7ddb2559df
> modestly improves the performance of org-publish-org-sitemap. On a
> directory of 104 files, it used to take this long...
>
>  org-publish-org-sitemap      1           10.508871433  10.508871433
>
> ...to generate a sitemap. With this patch and commit 507244d, it takes
> this long...
>
>  org-publish-org-sitemap      1           7.700290794   7.700290794
>
> Clearly, this is still very inadequate, but it is an improvement. I
> would love to use the built in site-map functions, but they are simply
> to slow for any larger projects.
>
> Could we do something like this to speed it up?
>
> (with-temp-buffer 
>   (insert-file-contents file)
>   (goto-char (point-min))
>   (when (re-search-forward "^#\\+TITLE:" nil t)
>     (org-element-at-point)))

I guess so.  But I'm in a rush and can't really check carefully.

In the meantime, thanks for the recent patches!

-- 
 Bastien

Re: [PATCH] Remove unecesssary invocations of org-mode in ox-publish

[Nicolas Goaziou]

Hello,

Matt Lundin <mdl@imapmail.org> writes:

> Clearly, this is still very inadequate, but it is an improvement. I
> would love to use the built in site-map functions, but they are simply
> to slow for any larger projects.
>
> Could we do something like this to speed it up?
>
> (with-temp-buffer 
>   (insert-file-contents file)
>   (goto-char (point-min))
>   (when (re-search-forward "^#\\+TITLE:" nil t)
>     (org-element-at-point)))

No, you also need to parse #+SETUPFILE: keywords. You could also get
false positive within a verbatim block:

 #+begin_example
 #+title: something
 #+end_example

However, it is possible to write a specialized function to extract
only #+TITLE.

Another option is to cache results. See `org-publish-cache-set' and
`org-publish-cache-get'.


Regards,

-- 
Nicolas Goaziou

Re: [PATCH] Remove unecesssary invocations of org-mode in ox-publish

[Matt Lundin]

Nicolas Goaziou <mail@nicolasgoaziou.fr> writes:

> Hello,
>
> Matt Lundin <mdl@imapmail.org> writes:
>
>> Clearly, this is still very inadequate, but it is an improvement. I
>> would love to use the built in site-map functions, but they are simply
>> to slow for any larger projects.
>>
>> Could we do something like this to speed it up?
>>
>> (with-temp-buffer 
>>   (insert-file-contents file)
>>   (goto-char (point-min))
>>   (when (re-search-forward "^#\\+TITLE:" nil t)
>>     (org-element-at-point)))
>
> No, you also need to parse #+SETUPFILE: keywords. You could also get
> false positive within a verbatim block:
>
>  #+begin_example
>  #+title: something
>  #+end_example

> However, it is possible to write a specialized function to extract
> only #+TITLE.
>
> Another option is to cache results. See `org-publish-cache-set' and
> `org-publish-cache-get'.

Thanks for the helpful information. I think the cache would be a nice
way to go, especially if it were combined a timestamp check. E.g., only
files that have been updated since the last publishing should be queried
for titles; otherwise, use the cached file.

I notice that org-publish-find-title does cache the title, but AFAICT
this is never used, since org-publish-format-file-entry calls
org-publish-find-title with the reset argument. Perhaps we could add a
variable to make this optional in org-publish-format-file-entry. E.g., 

--8<---------------cut here---------------start------------->8---
(defun org-publish-format-file-entry (fmt file project-plist)
  (format-spec
   fmt
   `((?t . ,(org-publish-find-title file org-publish-find-title-use-cache))
[...]
--8<---------------cut here---------------end--------------->8---

A similar option could be added to the date function date function.

Would it be O.K. if I went ahead and implemented this?
Thanks,
Matt

Re: [PATCH] Remove unecesssary invocations of org-mode in ox-publish

[Nicolas Goaziou]

Matt Lundin <mdl@imapmail.org> writes:

> Thanks for the helpful information. I think the cache would be a nice
> way to go, especially if it were combined a timestamp check. E.g., only
> files that have been updated since the last publishing should be queried
> for titles; otherwise, use the cached file.

Unfortunately, in the following (unexpected) setup

  == a.org ==
  #+SETUPFILE: b.org

  == b.org ==
  #+TITLE: Something

modifying "b.org" changes title for "a.org", even though the latter is
not modified. A dag would be needed to handle this case.

The same goes for #+INCLUDE keywords.

Overkill, IMO.

> I notice that org-publish-find-title does cache the title, but AFAICT
> this is never used, since org-publish-format-file-entry calls
> org-publish-find-title with the reset argument. Perhaps we could add a
> variable to make this optional in org-publish-format-file-entry. E.g., 
>
> (defun org-publish-format-file-entry (fmt file project-plist)
>   (format-spec
>    fmt
>    `((?t . ,(org-publish-find-title file org-publish-find-title-use-cache))
> [...]
>
> A similar option could be added to the date function date function.
>
> Would it be O.K. if I went ahead and implemented this?

Fine by me. I suggest to mention previous caveat in the docstring.


Regards,

-- 
Nicolas Goaziou