Re: Allow #+SETUPFILE to point to an URL for the org file

[Nicolas Goaziou <mail@nicolasgoaziou.fr>]

Hello,

Kaushal Modi <kaushal.modi@gmail.com> writes:

> Here is my implementation.. it is still not baked into org.el, etc and
> provided as a complete patch; I have some questions..

Thank you.

> (defvar org-setupfile-ht (make-hash-table :test 'equal)

org-setupfile-ht -> org--setupfile-cache

if it is meant to be inserted in "org.el" proper, or
`org-setupfile--cache' if you want to create a new "org-setupfile.el"
library.

Nitpick : 'equal -> #'equal

>   "Hash table to store the org SETUPFILE.

Hash table to store SETUPFILE contents.

> This acts as a cache of setup files read using
> `org-insert-setupfile-contents'.")
>
> (defun org-setupfile-clear-cache ()

`org--setupfile-clear-cache' or `org-setupfile--clear-cache' depending
on the location of the function.

>   "Clear the SETUPFILE cache stored in `org-setupfile-ht'."
>   (interactive)
>   (clrhash org-setupfile-ht))
>
> (defun org-insert-setupfile-contents (setupfile &optional nocache noerror)
>   "Insert the contents of SETUPFILE.
> SETUPFILE can be a file path or URL.

file path -> file name

> If SETUPFILE is a file path, use `org-file-contents' to get the file
> contents.

Then, we might want to generalize `org-file-contents' instead (i.e., let
`org-file-contents' handle remote locations). WDYT?

> If it is a URL instead, download the contents. If the URL contents are
> already
> cached in the `org-setupfile-ht' hash table, the download step is
> skipped.

Mind the double spaces at the end of sentences.

> If NOCACHE is non-nil, do a fresh fetch of SETUPFILE even if cached version
> is
> available. This option applies only if SETUPFILE is a URL.
>
> If NOERROR is non-nil, ignore the error when unable to read the SETUPFILE
> from
> file or URL."
>   (require 'ffap)                       ;for `ffap-url-regexp'
>   (let* ((is-url (string-match-p ffap-url-regexp setupfile))

They are not equivalent, but could `org-file-remote-p', or
`file-remote-p' be used instead?

>          (cache (when (and is-url (not nocache))
>                   (gethash setupfile org-setupfile-ht)))

  (cache (and is-url (not nocache) (gethash setupfile org-setupfile-ht)))

>          (contents (when (and is-url cache) cache)))
>     (if is-url
>         (unless cache
>           (let (url-retrieve-header)
>             (with-current-buffer (url-retrieve-synchronously setupfile)
>               (goto-char (point-min))
>               ;; Take point to after the url-retrieve header
>               (re-search-forward "\n\n") ; 2 consecutive new-line chars

`re-search-forward' -> `search-forward'

>               (setq url-retrieve-header (buffer-substring-no-properties
>                                          (point-min) (point)))
>               (message url-retrieve-header) ;Dump the URL retrieve header
> to *Messages*
>               (if (string-match-p "HTTP.*\\s-+200\\s-OK"
> url-retrieve-header) ;URL retrieved correctly
>                   (progn
>                     (setq contents (buffer-substring-no-properties (point)
> (point-max)))
>                     ;; Update the cache `org-setupfile-ht'
>                     (puthash setupfile contents org-setupfile-ht))
>                 (funcall (if noerror 'message 'error)

  (if noerror #'message #'error)

>                          "Unable to fetch SETUPFILE from `%s'"

  `%s' -> %S

> setupfile)))))
>       (setq contents (org-file-contents setupfile noerror)))

I think it is clearer if wrapped like this:

  (contents
   (cond (cache)
         (is-url
          (let (url-retrieve-header)
            ...))
         (t (org-file-contents setupfile noerror))))

>     (when contents
>       (save-excursion
>         (insert contents)))))

This may not be necessary at this point if we merge `org-file-contents'
with the above.

> Question:
>
> - All the places where the content of SETUPFILE is inserted in a temp
> buffer, it is assumed that the file is retrieved from disk and not from URL.
>
> Example in ox.el:
>
>  ((equal key "SETUPFILE")
>   (let ((file
>  (expand-file-name
>   (org-unbracket-string "\"" "\"" (org-trim val)))))
>     ;; Avoid circular dependencies.
>     (unless (member file files)
>       (with-temp-buffer
> (setq default-directory
>   (file-name-directory file))
> (insert (org-file-contents file 'noerror))
> (let ((org-inhibit-startup t)) (org-mode))
> (funcall get-options (cons file files))))))
>
>
> Note the use of expand-file-name, (member file files), default-directory,
> (funcall get-options (cons file files)).

(member file files), (cons file files) and `expand-file-name' are used
to avoid reading over and over the same setup file. In particular, they
prevent circular dependencies.

You can ignore `expand-file-name' and replace `file' with `uri', i.e.,
it is straightforward to extend the code to remote file names.

`default-directory' is slightly more tricky, as it is used to properly
read recursively setup files with relative file names. I think our best
bet is to to check if current file name is local or remote, and ignore
`default-directory' setting in the latter case.


Regards,

-- 
Nicolas Goaziou