Re: [bug, patch, ox] INCLUDE and footnotes

[Rasmus <rasmus@gmx.us>]

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

Hi,

Thanks again!

Nicolas Goaziou <mail@nicolasgoaziou.fr> writes:

>> I did not know markers but they seem perfect in this case.  The manual
>> mentions setting markers to nil after use.  I guess it's not necessary
>> here since they are in a (let ⋯)?
>
> It is. Binding between the symbol and the marker disappears with the
> `let', but the marker still exists. It cannot be GC'ed unless it is set
> to nil.
>
> It is not terribly important here as we're working in a temporary buffer
> anyway, so the marker will ultimately disappear at the end of the export
> process. However, it's a good habit to have.

Thanks for the explanation.

>> I check if I'm at a footnote reference 'cause I never want to deal with a
>> footnote-*definition* directly.  AFAIK org-footnote-re matches both.  It
>> seems a footnote-reference can never be at the beginning of line, but I
>> would still prefer a more explicit test through org-element.
>
> I wasn't clear. The check is important. The minor issue is that you bind
> LABEL and FOOTNOTE-TYPE too early, before making sure you are at
> a footnote reference. It would be more logical to do
>
>   (let ((object (org-element-context)))
>     (when (eq (org-element-type object) 'footnote-reference)
>       (let ((footnote-type (org-element-property :type object))
>             (label (org-element-property :label object)))
>         ...)))

I see.  I disagree that it's more since it's directly inside a loop over
org-footnote-re.  So if we are not at a footnote-{reference,definition}
it's probably a bug in the regexp. 

>> The only "bug" *I'm aware of* is that it will pick up the wrong new-label
>> for footnote for something like [fn:ref with space].  But this is anyway
>> not a proper label, I think.  Is that OK?
>
> [fn: ref with space]

While your comment excels in preciseness the terseness makes it hard to
appreciate its depth.  In my org-installation "[fn: ref with space]" is
not a valid footnote.

>> +	      (when (and (not included) (> (hash-table-count footnotes) 0))
>> +		(org-with-wide-buffer
>> +		 (goto-char (point-max))
>> +		 (unless (bolp) (insert "\n"))
>> +		 (maphash (lambda (ref def) (insert (format "[%s] %s\n" ref def)))
>> +			  footnotes)))))))))))
>
>   (unless included ...)
>
> is sufficient. No need to check for table emptiness (although it doesn't
> cost much): maphash will simply do nothing.

Thanks.  It's not necessary anymore.

>> +      (let ((marker-min (point-min-marker))
>
> This marker is not necessary since you're not going to add contents
> before (point-min) anyway. A plain number is enough.

I might if I include *Bar here:

* Foo
[1] foo

* Bar
Baz[1]



>> +	(goto-char (point-min))
>> +	(while (re-search-forward org-footnote-re nil t)
>> +	  (let* ((reference (org-element-context))
>> +		 (label (org-element-property :label reference))
>> +		 (digit-label (and label (org-string-match-p "\\`[0-9]+\\'" label))))
>> +	    (when (eq (org-element-type reference) 'footnote-reference)
>
> See above about order of bindings.

Adopted and commented on above.

>> +	      (goto-char (1+ (org-element-property :begin reference)))
>> +	      (when label
>> +		(let ((new-label
>> +		       (buffer-substring-no-properties
>> +			(point)
>> +			(progn (if digit-label (insert (format "fn:%d-" id))
>> +				 (forward-char 3)
>> +				 (insert (format "%d-" id)))
>> +			       (1- (search-forward "]"))))))
>
>> +		  (unless (eq (org-element-property :type reference) 'inline)
>
> A comment about what we're going to do would be nice.

Comments are added.

—Rasmus

-- 
I feel emotional landscapes they puzzle me

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-ox.el-Fix-footnote-bug-in-INCLUDE-keyword.patch --]
[-- Type: text/x-diff, Size: 8816 bytes --]

From d1e02f19e20341a650164f7bfb9aea97465225e1 Mon Sep 17 00:00:00 2001
From: rasmus <rasmus@gmx.us>
Date: Tue, 9 Dec 2014 12:40:52 +0100
Subject: [PATCH 1/2] ox.el: Fix footnote-bug in #+INCLUDE-keyword

* ox.el (org-export--prepare-file-contents): Preserve footnotes
when using the LINES argument.  New optional argument FOOTNOTES.
 (org-export-expand-include-keyword): New optional argument
 FOOTNOTES.
* test-ox.el (test-org-export/expand-include): Add test for INCLUDE
  with :lines and footnotes.
---
 lisp/ox.el              | 94 +++++++++++++++++++++++++++++++++++--------------
 testing/lisp/test-ox.el | 32 +++++++++++++++--
 2 files changed, 98 insertions(+), 28 deletions(-)

diff --git a/lisp/ox.el b/lisp/ox.el
index 9d9e794..6f3888b 100644
--- a/lisp/ox.el
+++ b/lisp/ox.el
@@ -3052,17 +3052,20 @@ locally for the subtree through node properties."
 		   (car key)
 		   (if (org-string-nw-p val) (format " %s" val) ""))))))))
 
-(defun org-export-expand-include-keyword (&optional included dir)
+(defun org-export-expand-include-keyword (&optional included dir footnotes)
   "Expand every include keyword in buffer.
 Optional argument INCLUDED is a list of included file names along
 with their line restriction, when appropriate.  It is used to
 avoid infinite recursion.  Optional argument DIR is the current
 working directory.  It is used to properly resolve relative
-paths."
+paths.  Optional argument FOOTNOTES is a hash-table used for
+storing and resolving footnotes.  It is created automatically."
   (let ((case-fold-search t)
 	(file-prefix (make-hash-table :test #'equal))
-	(current-prefix 0))
+	(current-prefix 0)
+	(footnotes (or footnotes (make-hash-table :test #'equal))))
     (goto-char (point-min))
+    ;; Expand INCLUDE keywords.
     (while (re-search-forward "^[ \t]*#\\+INCLUDE:" nil t)
       (let ((element (save-match-data (org-element-at-point))))
 	(when (eq (org-element-type element) 'keyword)
@@ -3155,15 +3158,24 @@ paths."
 			       file location only-contents lines)
 			    lines)))
 		     (org-mode)
-		     (insert
-		      (org-export--prepare-file-contents
-		       file lines ind minlevel
-		       (or (gethash file file-prefix)
-			   (puthash file (incf current-prefix) file-prefix)))))
+                     (insert (org-export--prepare-file-contents
+			      file lines ind minlevel
+			      (or (gethash file file-prefix)
+				  (puthash file (incf current-prefix) file-prefix))
+			      footnotes)))
 		   (org-export-expand-include-keyword
 		    (cons (list file lines) included)
-		    (file-name-directory file))
-		   (buffer-string)))))))))))))
+		    (file-name-directory file)
+		    footnotes)
+		   (buffer-string)))))
+	      ;; Expand footnotes after all files have been
+	      ;; included.  Footnotes are stored at end of buffer.
+	      (unless included
+		(org-with-wide-buffer
+		 (goto-char (point-max))
+		 (unless (bolp) (insert "\n"))
+		 (maphash (lambda (ref def) (insert (format "[%s] %s\n" ref def)))
+			  footnotes)))))))))))
 
 (defun org-export--inclusion-absolute-lines (file location only-contents lines)
   "Resolve absolute lines for an included file with file-link.
@@ -3227,8 +3239,8 @@ Return a string of lines to be included in the format expected by
 		       (while (< (point) end) (incf counter) (forward-line))
 		       counter))))))))
 
-(defun org-export--prepare-file-contents (file &optional lines ind minlevel id)
-  "Prepare the contents of FILE for inclusion and return them as a string.
+(defun org-export--prepare-file-contents (file &optional lines ind minlevel id footnotes)
+  "Prepare contents of FILE for inclusion and return it as a string.
 
 When optional argument LINES is a string specifying a range of
 lines, include only those lines.
@@ -3242,11 +3254,14 @@ headline encountered.
 Optional argument MINLEVEL, when non-nil, is an integer
 specifying the level that any top-level headline in the included
 file should have.
-
 Optional argument ID is an integer that will be inserted before
 each footnote definition and reference if FILE is an Org file.
 This is useful to avoid conflicts when more than one Org file
-with footnotes is included in a document."
+with footnotes is included in a document.
+
+Optional argument FOOTNOTES is a hash-table to store footnotes in
+the included document.
+"
   (with-temp-buffer
     (insert-file-contents file)
     (when lines
@@ -3308,19 +3323,46 @@ with footnotes is included in a document."
     ;; Append ID to all footnote references and definitions, so they
     ;; become file specific and cannot collide with footnotes in other
     ;; included files.
+    ;; Further, collect relevant footnotes outside of LINES.
     (when id
-      (goto-char (point-min))
-      (while (re-search-forward org-footnote-re nil t)
-	(let ((reference (org-element-context)))
-	  (when (memq (org-element-type reference)
-		      '(footnote-reference footnote-definition))
-	    (goto-char (org-element-property :begin reference))
-	    (forward-char)
-	    (let ((label (org-element-property :label reference)))
-	      (cond ((not label))
-		    ((org-string-match-p "\\`[0-9]+\\'" label)
-		     (insert (format "fn:%d-" id)))
-		    (t (forward-char 3) (insert (format "%d-" id)))))))))
+      (let ((marker-min (point-min-marker))
+	    (marker-max (point-max-marker)))
+	(goto-char (point-min))
+	(while (re-search-forward org-footnote-re nil t)
+	  (let ((reference (org-element-context)))
+	    (when (eq (org-element-type reference) 'footnote-reference)
+	      (let* ((label (org-element-property :label reference))
+		     (digit-label (and label (org-string-match-p "\\`[0-9]+\\'" label))))
+		;; Update the footnote-reference at point and collect
+		;; the new label, which is only used for footnotes
+		;; outsides LINES.
+		(goto-char (1+ (org-element-property :begin reference)))
+		(when label
+		  ;; If label is akin to [1] convert it to [fn:ID-1].
+		  ;; Otherwise add "ID-" after "fn:".
+		  (if digit-label (insert (format "fn:%d-" id))
+		    (forward-char 3)
+		    (insert (format "%d-" id)))
+		  (unless (eq (org-element-property :type reference) 'inline)
+		    (org-with-wide-buffer
+		     (let* ((definition (org-footnote-get-definition label))
+			    (beginning (copy-marker (nth 1 definition))))
+		       (goto-char (1+ beginning))
+		       ;; Update label cf. above.  Necessary when
+		       ;; including whole files.
+		       (if digit-label (insert (format "fn:%d-" id))
+			 (forward-char 3)
+			 (insert (format "%d-" id)))
+		       ;; Store footnotes outside of LINES.
+		       (when (or (< beginning marker-min) (> beginning marker-max))
+			 (puthash
+			  (buffer-substring (goto-char (1+ beginning))
+					    (1- (search-forward "]")))
+			  (org-element-normalize-string (nth 3 definition))
+			  footnotes))
+		       (set-marker beginning nil)))))))))
+	(set-marker marker-min nil)
+	(set-marker marker-max nil)))
     (org-element-normalize-string (buffer-string))))
 
 (defun org-export-execute-babel-code ()
diff --git a/testing/lisp/test-ox.el b/testing/lisp/test-ox.el
index 9a0e787..37e2e23 100644
--- a/testing/lisp/test-ox.el
+++ b/testing/lisp/test-ox.el
@@ -904,12 +904,13 @@ Footnotes[fn:1], [fn:test] and [fn:inline:anonymous footnote].
 			(org-element-property :label ref)))))))))))))
   ;; Footnotes labels are not local to each include keyword.
   (should
-   (= 3
+   (= 4
       (length
        (delete-dups
 	(let ((contents "
-Footnotes[fn:1], [fn:test] and [fn:inline:anonymous footnote].
+Footnotes[fn:1], [fn:test], [2] and [fn:inline:anonymous footnote].
 \[fn:1] Footnote 1
+\[2] Footnote 2
 \[fn:test] Footnote \"test\""))
 	  (org-test-with-temp-text-in-file contents
 	    (let ((file (buffer-file-name)))
@@ -919,6 +920,33 @@ Footnotes[fn:1], [fn:test] and [fn:inline:anonymous footnote].
 		(org-element-map (org-element-parse-buffer)
 		    'footnote-reference
 		  (lambda (ref) (org-element-property :label ref)))))))))))
+  ;; Footnotes are supported by :lines-like elements and unnecessary
+  ;; footnotes are dropped.
+  (should
+   (= 4
+      (length
+       (delete-dups
+	(let ((contents "
+* foo
+Footnotes[fn:1]
+* bar
+Footnotes[fn:2], foot[fn:test], digit only[3], and [fn:inline:anonymous footnote]
+
+\[fn:1] Footnote 1
+\[fn:2] Footnote 1
+* Footnotes
+\[fn:test] Footnote \"test\"
+\[3] Footnote 3
+"))
+	  (org-test-with-temp-text-in-file contents
+	    (let ((file (buffer-file-name)))
+	      (org-test-with-temp-text
+		  (format "#+INCLUDE: \"%s::*bar\"
+" file)
+		(org-export-expand-include-keyword)
+		(org-element-map (org-element-parse-buffer)
+		    'footnote-definition
+		  (lambda (ref) (org-element-property :label ref)))))))))))
   ;; If only-contents is non-nil only include contents of element.
   (should
    (equal
-- 
2.2.1