[PATCH] org-macs: Fix incorrect use of relative paths in org-compile-file

[PATCH] org-macs: Fix incorrect use of relative paths in org-compile-file

[Roshan Shariff]

* org-macs.el (org-compile-file, org-compile-file-commands): Avoid
converting the source path to be relative to the default-directory,
which breaks for absolute source paths when the current directory is a
symlink.

Commit 5a8a1d4ff [1] changed org-compile-file to use
`file-relative-name` for the source argument. This was intended to fix
bug [2] by expanding ~ directories in the source path, like a shell.

Unfortunately, this forced use of relative paths breaks when
default-directory is a symlink, and the source to be compiled has an
absolute path.

For example, on macOS Ventura, ~/Dropbox is a symlink to
~/Library/CloudStorage/Dropbox. Suppose `default-directory` is
/Users/username/Dropbox and you try to compile /var/tmp/test.org. Its
relative path is ../../../var/tmp/test.org. But the working directory
of a compilation process is actually ~/Library/CloudStorage/Dropbox,
relative to which the source path resolves to
"/Users/username/var/tmp/test.org". The process thus cannot find the
source file.

This commit changes `org-compile-file` and its helper function
`org-compile-file-commands` to avoid the use of relative
paths. Instead, to address bug [2], `expand-file-name` is used (only
on absolute paths) for ~ expansion. Otherwise, the source path is
passed unchanged to the compilation command. The `file-truename` of
the source directory is used to construct absolute source and output
paths if needed by the command.

[1] https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=5a8a1d4ff
[2] https://orgmode.org/list/25528.42190.53674.62381@gargle.gargle.HOWL
---
 lisp/org-macs.el | 39 +++++++++++++++++++++++++++------------
 1 file changed, 27 insertions(+), 12 deletions(-)

diff --git a/lisp/org-macs.el b/lisp/org-macs.el
index e102f01c3..5d8f65193 100644
--- a/lisp/org-macs.el
+++ b/lisp/org-macs.el
@@ -1606,16 +1606,20 @@ filename.  Otherwise, it raises an error.
 When PROCESS is a list of commands, optional argument LOG-BUF can
 be set to a buffer or a buffer name.  `shell-command' then uses
 it for output."
+  (when (file-name-absolute-p source)
+    ;; Expand "~" and "~user" .  Shell expansion will be disabled
+    ;; in the shell command call.
+    (setq source (expand-file-name source)))
   (let* ((commands (org-compile-file-commands source process ext spec err-msg))
-         (output (expand-file-name (concat (file-name-base source) "." ext)
-                                   (file-name-directory source)))
+         (output (file-name-concat (file-name-directory source)
+                                   (concat (file-name-base source) "." ext)))
          (log-buf (and log-buf (get-buffer-create log-buf)))
          (time (file-attribute-modification-time (file-attributes output))))
     (save-window-excursion
       (dolist (command commands)
         (cond
          ((functionp command)
-          (funcall command (shell-quote-argument (file-relative-name source))))
+          (funcall command (shell-quote-argument source)))
          ((stringp command) (shell-command command log-buf)))))
     ;; Check for process failure.  Output file is expected to be
     ;; located in the same directory as SOURCE.
@@ -1658,22 +1662,33 @@ argument SPEC, as an alist following the pattern
 
 Throw an error if PROCESS does not satisfy the described patterns.
 The error string will be appended with ERR-MSG, when it is a string."
+  (when (file-name-absolute-p source)
+    ;; Expand "~" and "~user" .  Shell expansion will be disabled
+    ;; in the shell command call.
+    (setq source (expand-file-name source)))
   (let* ((base-name (file-name-base source))
-	 (full-name (file-truename source))
-         (relative-name (file-relative-name source))
-	 (out-dir (if (file-name-directory source)
-                      ;; Expand "~".  Shell expansion will be disabled
-                      ;; in the shell command call.
-                      (file-name-directory full-name)
-                    "./"))
-	 (output (expand-file-name (concat (file-name-base source) "." ext) out-dir))
+	 (out-dir (file-name-directory source))
+         ;; Don't use (expand-file-name SOURCE) for the absolute path,
+         ;; in case SOURCE starts with ../ and default-directory is a
+         ;; symlink.  Instead, resolve symlinks in the directory
+         ;; component of SOURCE...
+         (true-out-dir (file-truename out-dir))
+         ;; but use the original file name component of SOURCE in case
+         ;; it is a symlink; we want %f and %F to have the same file
+         ;; name component:
+	 (full-name (file-name-concat true-out-dir
+                                      (file-name-nondirectory source)))
+         ;; The absolute path OUTPUT is the same as FULL-NAME, except
+         ;; with extension EXT:
+	 (output (file-name-concat true-out-dir
+                                   (concat base-name "." ext)))
 	 (err-msg (if (stringp err-msg) (concat ".  " err-msg) "")))
     (pcase process
       ((pred functionp) (list process))
       ((pred consp)
        (let ((spec (append spec
 			   `((?b . ,(shell-quote-argument base-name))
-			     (?f . ,(shell-quote-argument relative-name))
+			     (?f . ,(shell-quote-argument source))
 			     (?F . ,(shell-quote-argument full-name))
 			     (?o . ,(shell-quote-argument out-dir))
 			     (?O . ,(shell-quote-argument output))))))

base-commit: 73cb528c24322fef2d05142d36bc48bb9dac962e
-- 
2.41.0

Re: [PATCH] org-macs: Fix incorrect use of relative paths in org-compile-file

[Ihor Radchenko]

Roshan Shariff <roshan.shariff@gmail.com> writes:

> * org-macs.el (org-compile-file, org-compile-file-commands): Avoid
> converting the source path to be relative to the default-directory,
> which breaks for absolute source paths when the current directory is a
> symlink.
>
> Commit 5a8a1d4ff [1] changed org-compile-file to use
> `file-relative-name` for the source argument. This was intended to fix
> bug [2] by expanding ~ directories in the source path, like a shell.

Thanks for the report and for providing a detailed explanation with a
patch!

I have a few comments.

First, minor one: please put two spaces between sentences in the commit
message. It is our convention.

> -         (output (expand-file-name (concat (file-name-base source) "." ext)
> -                                   (file-name-directory source)))
> +         (output (file-name-concat (file-name-directory source)
> +                                   (concat (file-name-base source) "." ext)))

`file-name-concat' is only available since Emacs 28.
Please use `org-file-name-concat'.

> -			     (?f . ,(shell-quote-argument relative-name))
> +			     (?f . ,(shell-quote-argument source))

This will break user expectations.
The PROCESS argument can, for example, be `org-latex-pdf-process', which
promises that "%f in the command will be replaced by the relative file
name" (see the docstring).

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: [PATCH] org-macs: Fix incorrect use of relative paths in org-compile-file

[Roshan Shariff]

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

Hi Ihor,

On Sat, 5 Aug 2023 at 04:23, Ihor Radchenko <yantar92@posteo.net> wrote:
> First, minor one: please put two spaces between sentences in the commit
> message. It is our convention.
> `file-name-concat' is only available since Emacs 28.
> Please use `org-file-name-concat'.

Thanks for the feedback! I'm attaching a new version of the patch that
improves the commit message and avoids the use of file-name-concat.

> The PROCESS argument can, for example, be `org-latex-pdf-process', which
> promises that "%f in the command will be replaced by the relative file
> name" (see the docstring).

The patch now converts the source to a relative path if it is
absolute. Note that, according to the documentation of
file-relative-name, the path will still be absolute on Microsoft
Windows if the source and the current directory have different drive
letters; this is probably unavoidable. I have also updated the
docstring to clarify which paths are supposed to be relative and which
are absolute.

By the way, I forgot to mention that I ran into this bug while testing
tecosaur's latex-preview patch set on macOS. When running
org-latex-export-to-pdf on an Org file in ~/Dropbox, I got a warning
message about the preamble precompilation failing because it couldn't
find a temporary .tex file in /var. With this patch applied, the PDF
export completes without warnings.

Regards,
Roshan

[-- Attachment #2: v2-0001-org-macs-Fix-incorrect-use-of-relative-paths-in-o.patch --]
[-- Type: text/x-patch, Size: 5794 bytes --]

From 6e1f120818582695da9018f2111156b00c87104a Mon Sep 17 00:00:00 2001
From: Roshan Shariff <roshan.shariff@gmail.com>
Date: Fri, 4 Aug 2023 22:10:25 -0600
Subject: [PATCH v2] org-macs: Fix incorrect use of relative paths in
 org-compile-file

* org-macs.el (org-compile-file, org-compile-file-commands): Resolve
symlinks in default-directory before computing relative source path

Commit 5a8a1d4ff [1] changed org-compile-file to use
`file-relative-name` for the SOURCE argument.  This was intended to
fix bug [2] by expanding ~ directories, like a shell.  Unfortunately,
this breaks when DEFAULT-DIRECTORY is a symlink and SOURCE has an
absolute path.

For example, on macOS Ventura, ~/Dropbox is a symlink to
~/Library/CloudStorage/Dropbox.  Suppose DEFAULT-DIRECTORY is
/Users/username/Dropbox and SOURCE is /var/tmp/test.org, so its
relative path is ../../../var/tmp/test.org.  But the working directory
of a compilation process is actually ~/Library/CloudStorage/Dropbox,
relative to which the source path resolves to
/Users/username/var/tmp/test.org.  The process thus cannot find the
source file.

This commit changes `org-compile-file` and its helper function
`org-compile-file-commands` to resolve symlinks in DEFAULT-DIRECTORY
before computing the relative path of SOURCE.  If SOURCE is already
relative, it is used as-is.  The absolute path is processed by
`expand-file-name`, avoiding bug [1].

[1] https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=5a8a1d4ff
[2] https://orgmode.org/list/25528.42190.53674.62381@gargle.gargle.HOWL
---
 lisp/org-macs.el | 45 +++++++++++++++++++++++++--------------------
 1 file changed, 25 insertions(+), 20 deletions(-)

diff --git a/lisp/org-macs.el b/lisp/org-macs.el
index e102f01c3..dc5dbeab5 100644
--- a/lisp/org-macs.el
+++ b/lisp/org-macs.el
@@ -1607,15 +1607,18 @@ When PROCESS is a list of commands, optional argument LOG-BUF can
 be set to a buffer or a buffer name.  `shell-command' then uses
 it for output."
   (let* ((commands (org-compile-file-commands source process ext spec err-msg))
-         (output (expand-file-name (concat (file-name-base source) "." ext)
-                                   (file-name-directory source)))
+         (output (concat (file-name-sans-extension source) "." ext))
+         (relname (if (file-name-absolute-p source)
+                        (let ((pwd (file-truename default-directory)))
+                          (file-relative-name source pwd))
+                      source))
          (log-buf (and log-buf (get-buffer-create log-buf)))
          (time (file-attribute-modification-time (file-attributes output))))
     (save-window-excursion
       (dolist (command commands)
         (cond
          ((functionp command)
-          (funcall command (shell-quote-argument (file-relative-name source))))
+          (funcall command (shell-quote-argument relname)))
          ((stringp command) (shell-command command log-buf)))))
     ;; Check for process failure.  Output file is expected to be
     ;; located in the same directory as SOURCE.
@@ -1649,33 +1652,35 @@ the SOURCE file.
 
 If PROCESS is a list of commands, each of them is called using
 `shell-command'.  By default, in each command, %b, %f, %F, %o and
-%O are replaced with, respectively, SOURCE base name, name, full
-name, directory and absolute output file name.  It is possible,
-however, to use more place-holders by specifying them in optional
-argument SPEC, as an alist following the pattern
+%O are replaced with, respectively, SOURCE base name, relative
+file name, absolute file name, relative directory and absolute
+output file name.  It is possible, however, to use more
+place-holders by specifying them in optional argument SPEC, as an
+alist following the pattern
 
   (CHARACTER . REPLACEMENT-STRING).
 
 Throw an error if PROCESS does not satisfy the described patterns.
 The error string will be appended with ERR-MSG, when it is a string."
-  (let* ((base-name (file-name-base source))
-	 (full-name (file-truename source))
-         (relative-name (file-relative-name source))
-	 (out-dir (if (file-name-directory source)
-                      ;; Expand "~".  Shell expansion will be disabled
-                      ;; in the shell command call.
-                      (file-name-directory full-name)
-                    "./"))
-	 (output (expand-file-name (concat (file-name-base source) "." ext) out-dir))
+  (let* ((basename (file-name-base source))
+         ;; Resolve symlinks in default-directory to correctly handle
+         ;; absolute source paths or relative paths with ..
+         (pwd (file-truename default-directory))
+         (absname (expand-file-name source pwd))
+         (relname (if (file-name-absolute-p source)
+                        (file-relative-name source pwd)
+                      source))
+	 (relpath (or (file-name-directory relname) "./"))
+	 (output (concat (file-name-sans-extension absname) "." ext))
 	 (err-msg (if (stringp err-msg) (concat ".  " err-msg) "")))
     (pcase process
       ((pred functionp) (list process))
       ((pred consp)
        (let ((spec (append spec
-			   `((?b . ,(shell-quote-argument base-name))
-			     (?f . ,(shell-quote-argument relative-name))
-			     (?F . ,(shell-quote-argument full-name))
-			     (?o . ,(shell-quote-argument out-dir))
+			   `((?b . ,(shell-quote-argument basename))
+			     (?f . ,(shell-quote-argument relname))
+			     (?F . ,(shell-quote-argument absname))
+			     (?o . ,(shell-quote-argument relpath))
 			     (?O . ,(shell-quote-argument output))))))
          (mapcar (lambda (command) (format-spec command spec)) process)))
       (_ (error "No valid command to process %S%s" source err-msg)))))

base-commit: c7e1f78326581e8d994feaee69d725d3e073f89f
-- 
2.41.0

Re: [PATCH] org-macs: Fix incorrect use of relative paths in org-compile-file

[Ihor Radchenko]

Roshan Shariff <roshan.shariff@gmail.com> writes:

> Thanks for the feedback! I'm attaching a new version of the patch that
> improves the commit message and avoids the use of file-name-concat.

Thanks!
Applied, onto main, with minor amendments.
https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=4ea9a98f8

I added a TINYCHANGE cookie as you do not appear to have FSF copyright
assignment. I also duplicated your comment about "pwd" usage to
`org-compile-file' to avoid accidental edits in future.

Please note that your total contribution have now exceeded the allowed
limit we can accept without copyright paper work.
You may consider FSF copyright assignment in future. See
https://orgmode.org/worg/org-contribute.html#copyright

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: [PATCH] org-macs: Fix incorrect use of relative paths in org-compile-file

[Roshan Shariff]

Thanks again Ihor!

For the future, I wanted to clarify that I do have a copyright
assignment to the FSF "for the suite of programs known as GNU EMACS".
I have a signed copy of the assignment form dated 3 April, 2022. I
should be in the list as "Roshan Shariff", but if not I can follow up
with the FSF to see if they missed adding me.

On Sun, 6 Aug 2023 at 00:49, Ihor Radchenko <yantar92@posteo.net> wrote:
>
> Roshan Shariff <roshan.shariff@gmail.com> writes:
>
> > Thanks for the feedback! I'm attaching a new version of the patch that
> > improves the commit message and avoids the use of file-name-concat.
>
> Thanks!
> Applied, onto main, with minor amendments.
> https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=4ea9a98f8
>
> I added a TINYCHANGE cookie as you do not appear to have FSF copyright
> assignment. I also duplicated your comment about "pwd" usage to
> `org-compile-file' to avoid accidental edits in future.
>
> Please note that your total contribution have now exceeded the allowed
> limit we can accept without copyright paper work.
> You may consider FSF copyright assignment in future. See
> https://orgmode.org/worg/org-contribute.html#copyright
>
> --
> Ihor Radchenko // yantar92,
> Org mode contributor,
> Learn more about Org mode at <https://orgmode.org/>.
> Support Org development at <https://liberapay.com/org-mode>,
> or support my work at <https://liberapay.com/yantar92>

Re: [PATCH] org-macs: Fix incorrect use of relative paths in org-compile-file

[Ihor Radchenko]

Roshan Shariff <roshan.shariff@gmail.com> writes:

> For the future, I wanted to clarify that I do have a copyright
> assignment to the FSF "for the suite of programs known as GNU EMACS".
> I have a signed copy of the assignment form dated 3 April, 2022. I
> should be in the list as "Roshan Shariff", but if not I can follow up
> with the FSF to see if they missed adding me.

Thanks for the update.
I do not have access to the FSF records, and you are not yet listed on
https://orgmode.org/worg/contributors.html

Bastien, may you please confirm Roshan's copyright status?

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: [PATCH] org-macs: Fix incorrect use of relative paths in org-compile-file

[Bastien Guerry]

Ihor Radchenko <yantar92@posteo.net> writes:

> Bastien, may you please confirm Roshan's copyright status?

Yes, I do confirm Roshan is a FSF registered contributor for Emacs.

-- 
 Bastien Guerry

Re: [PATCH] org-macs: Fix incorrect use of relative paths in org-compile-file

[Ihor Radchenko]

Bastien Guerry <bzg@gnu.org> writes:

> Ihor Radchenko <yantar92@posteo.net> writes:
>
>> Bastien, may you please confirm Roshan's copyright status?
>
> Yes, I do confirm Roshan is a FSF registered contributor for Emacs.

Updated our records.
https://git.sr.ht/~bzg/worg/commit/9015a8ea

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>