* [PATCH] ob-tangle.el: fix ‘:comments noweb’ double linking
@ 2022-07-29 4:54 Hraban Luyat
2022-07-30 4:56 ` Ihor Radchenko
0 siblings, 1 reply; 15+ messages in thread
From: Hraban Luyat @ 2022-07-29 4:54 UTC (permalink / raw)
To: emacs-orgmode; +Cc: hraban
* lisp/ob-tangle.el: Refactor the double implementation to a single
helper function. This avoids the double link wrapping.
* testing/lisp/test-ob-tangle.el: Add unit tests.
Babel tangle allows inserting comments at the tangled site which link
back to the source in the org file. This linking was implemented
twice, to handle separate cases, but when using ‘:comments noweb’ it
ended up going through both codepaths. This resulted in doubly wrapped
links.
By refactoring all link generation into a single function, this double
wrapping is avoided.
Example file, /tmp/test.org:
* Inner
#+name: inner
#+begin_src emacs-lisp
2
#+end_src
* Main
#+header: :tangle test.el :comments noweb :noweb yes
#+begin_src emacs-lisp
1
<<inner>>
#+end_src
Before:
;; [[file:test.org::*Main][Main:1]]
1
;; [[[[file:/tmp/test.org::inner][inner]]][inner]]
2
;; inner ends here
;; Main:1 ends here
After:
;; [[file:test.org::*Main][Main:1]]
1
;; [[file:test.org::inner][inner]]
2
;; inner ends here
;; Main:1 ends here
This is my first org-mode patch, all comments welcome :). I signed a
copyright assignment to the FSF 2021-07-12.
---
lisp/ob-tangle.el | 54 ++++++++++++++++----------------
testing/lisp/test-ob-tangle.el | 56 ++++++++++++++++++++++++++++++++++
2 files changed, 83 insertions(+), 27 deletions(-)
diff --git a/lisp/ob-tangle.el b/lisp/ob-tangle.el
index fdba72278..078b1c77a 100644
--- a/lisp/ob-tangle.el
+++ b/lisp/ob-tangle.el
@@ -469,6 +469,29 @@ code blocks by target file."
(mapcar (lambda (b) (cons (car b) (nreverse (cdr b))))
(nreverse blocks))))
+(defun org-babel-tangle--unbracketed-link (params)
+ "Get a raw link to the src block at point, without brackets.
+
+The PARAMS are the 3rd element of the info for the same src block.
+"
+ (let* (;; The created link is transient. Using ID is not necessary,
+ ;; but could have side-effects if used. An ID property may
+ ;; be added to existing entries thus creatin unexpected file
+ ;; modifications.
+ (org-id-link-to-org-use-id nil)
+ (l (org-no-properties (org-store-link nil)))
+ (bare (and (string-match org-link-bracket-re l)
+ (match-string 1 l))))
+ (when bare
+ (if (and org-babel-tangle-use-relative-file-links
+ (string-match org-link-types-re bare)
+ (string= (match-string 1 bare) "file"))
+ (concat "file:"
+ (file-relative-name (substring bare (match-end 0))
+ (file-name-directory
+ (cdr (assq :tangle params)))))
+ bare))))
+
(defun org-babel-tangle-single-block (block-counter &optional only-this-block)
"Collect the tangled source for current block.
Return the list of block attributes needed by
@@ -485,16 +508,7 @@ non-nil, return the full association list to be used by
(extra (nth 3 info))
(coderef (nth 6 info))
(cref-regexp (org-src-coderef-regexp coderef))
- (link (let* (
- ;; The created link is transient. Using ID is
- ;; not necessary, but could have side-effects if
- ;; used. An ID property may be added to
- ;; existing entries thus creatin unexpected file
- ;; modifications.
- (org-id-link-to-org-use-id nil)
- (l (org-no-properties (org-store-link nil))))
- (and (string-match org-link-bracket-re l)
- (match-string 1 l))))
+ (link (org-babel-tangle--unbracketed-link params))
(source-name
(or (nth 4 info)
(format "%s:%d"
@@ -548,15 +562,7 @@ non-nil, return the full association list to be used by
(if org-babel-tangle-use-relative-file-links
(file-relative-name file)
file)
- (if (and org-babel-tangle-use-relative-file-links
- (string-match org-link-types-re link)
- (string= (match-string 1 link) "file")
- (stringp src-tfile))
- (concat "file:"
- (file-relative-name (substring link (match-end 0))
- (file-name-directory
- src-tfile)))
- link)
+ link
source-name
params
(if org-src-preserve-indentation
@@ -574,18 +580,12 @@ non-nil, return the full association list to be used by
INFO, when non nil, is the source block information, as returned
by `org-babel-get-src-block-info'."
(let ((link-data (pcase (or info (org-babel-get-src-block-info 'light))
- (`(,_ ,_ ,_ ,_ ,name ,start ,_)
+ (`(,_ ,_ ,params ,_ ,name ,start ,_)
`(("start-line" . ,(org-with-point-at start
(number-to-string
(line-number-at-pos))))
("file" . ,(buffer-file-name))
- ("link" . ,(let (;; The created link is transient. Using ID is
- ;; not necessary, but could have side-effects if
- ;; used. An ID property may be added to
- ;; existing entries thus creatin unexpected file
- ;; modifications.
- (org-id-link-to-org-use-id nil))
- (org-no-properties (org-store-link nil))))
+ ("link" . ,(org-babel-tangle--unbracketed-link params))
("source-name" . ,name))))))
(list (org-fill-template org-babel-tangle-comment-format-beg link-data)
(org-fill-template org-babel-tangle-comment-format-end link-data))))
diff --git a/testing/lisp/test-ob-tangle.el b/testing/lisp/test-ob-tangle.el
index 2ed4ba0da..fecb105ba 100644
--- a/testing/lisp/test-ob-tangle.el
+++ b/testing/lisp/test-ob-tangle.el
@@ -159,6 +159,62 @@ echo 1
(search-forward (concat "[file:" file) nil t)))
(delete-file "test-ob-tangle.el")))))
+(ert-deftest ob-tangle/comment-noweb-relative ()
+ "Test :comments noweb tangling with relative file paths"
+ (should
+ (org-test-with-temp-text-in-file
+ "* Inner
+#+name: inner
+#+begin_src emacs-lisp
+2
+#+end_src
+
+* Main
+#+header: :tangle \"test-ob-tangle.el\" :comments noweb :noweb yes
+#+begin_src emacs-lisp
+1
+<<inner>>
+#+end_src"
+ (unwind-protect
+ (let ((org-babel-tangle-use-relative-file-links t))
+ (org-babel-tangle)
+ (with-temp-buffer
+ (insert-file-contents "test-ob-tangle.el")
+ (buffer-string)
+ (goto-char (point-min))
+ (and
+ (search-forward (concat ";; [[file:" (file-name-nondirectory file) "::inner") nil t)
+ (search-forward ";; inner ends here" nil t))))
+ (delete-file "test-ob-tangle.el")))))
+
+(ert-deftest ob-tangle/comment-noweb-absolute ()
+ "Test :comments noweb tangling with absolute file path"
+ (should
+ (org-test-with-temp-text-in-file
+ "* Inner
+#+name: inner
+#+begin_src emacs-lisp
+2
+#+end_src
+
+* Main
+#+header: :tangle \"test-ob-tangle.el\" :comments noweb :noweb yes
+#+begin_src emacs-lisp
+1
+<<inner>>
+#+end_src"
+ (unwind-protect
+ (let ((org-babel-tangle-use-relative-file-links nil))
+ (org-babel-tangle)
+ (with-temp-buffer
+ (insert-file-contents "test-ob-tangle.el")
+ (buffer-string)
+ (goto-char (point-min))
+ (and
+ (search-forward (concat ";; [[file:" file "::inner") nil t)
+ (search-forward ";; inner ends here" nil t))))
+ (delete-file "test-ob-tangle.el")))))
+
(ert-deftest ob-tangle/jump-to-org ()
"Test `org-babel-tangle-jump-to-org' specifications."
;; Standard test.
--
2.36.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] ob-tangle.el: fix ‘:comments noweb’ double linking
2022-07-29 4:54 [PATCH] ob-tangle.el: fix ‘:comments noweb’ double linking Hraban Luyat
@ 2022-07-30 4:56 ` Ihor Radchenko
2022-07-30 23:42 ` Hraban Luyat
2022-08-03 11:31 ` Bastien Guerry
0 siblings, 2 replies; 15+ messages in thread
From: Ihor Radchenko @ 2022-07-30 4:56 UTC (permalink / raw)
To: Hraban Luyat, Bastien; +Cc: emacs-orgmode
Hraban Luyat <hraban@0brg.net> writes:
> This is my first org-mode patch, all comments welcome :). I signed a
> copyright assignment to the FSF 2021-07-12.
Thanks for the contribution!
Bastien, could you please check the FSF records?
The patch looks good in general. Few minor comments below.
> * lisp/ob-tangle.el: Refactor the double implementation to a single
> helper function. This avoids the double link wrapping.
> * testing/lisp/test-ob-tangle.el: Add unit tests.
>
> Babel tangle allows inserting comments at the tangled site which link
> back to the source in the org file. This linking was implemented
> twice, to handle separate cases, but when using ‘:comments noweb’ it
> ended up going through both codepaths. This resulted in doubly wrapped
> links.
Please use double space " " between sentences. See
https://orgmode.org/worg/org-contribute.html#commit-messages
> +(defun org-babel-tangle--unbracketed-link (params)
> + "Get a raw link to the src block at point, without brackets.
> +
> +The PARAMS are the 3rd element of the info for the same src block.
> +"
No newline at the end of the docstring, please.
See D.6 Tips for Documentation Strings in Elisp manual:
• Do not start or end a documentation string with whitespace.
> + (let* (;; The created link is transient. Using ID is not necessary,
> + ;; but could have side-effects if used. An ID property may
> + ;; be added to existing entries thus creatin unexpected file
> + ;; modifications.
Can as well fix the "creatin" typo here.
> + (org-id-link-to-org-use-id nil)
> + (l (org-no-properties (org-store-link nil)))
> + (bare (and (string-match org-link-bracket-re l)
> + (match-string 1 l))))
For safety, please wrap the matching into save-match-data.
This is often annoying when calling some random function unexpectedly
changes match-data.
> +(ert-deftest ob-tangle/comment-noweb-relative ()
> + "Test :comments noweb tangling with relative file paths"
> +(ert-deftest ob-tangle/comment-noweb-absolute ()
> + "Test :comments noweb tangling with absolute file path"
Full stop at the end of sentence, please.
Best,
Ihor
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] ob-tangle.el: fix ‘:comments noweb’ double linking
2022-07-30 4:56 ` Ihor Radchenko
@ 2022-07-30 23:42 ` Hraban Luyat
2022-08-03 11:40 ` Ihor Radchenko
2022-08-03 15:55 ` Max Nikulin
2022-08-03 11:31 ` Bastien Guerry
1 sibling, 2 replies; 15+ messages in thread
From: Hraban Luyat @ 2022-07-30 23:42 UTC (permalink / raw)
To: Ihor Radchenko; +Cc: bzg, emacs-orgmode
[-- Attachment #1: Type: text/plain, Size: 420 bytes --]
On 7/30/22 12:56 AM, Ihor Radchenko wrote:
> Hraban Luyat <hraban@0brg.net> writes:
>
>> This is my first org-mode patch, all comments welcome :). I signed a
>> copyright assignment to the FSF 2021-07-12.
>
> Thanks for the contribution!
>
> Bastien, could you please check the FSF records?
>
> The patch looks good in general. Few minor comments below.
>
Done, new patch attached. Thanks for the feedback.
[-- Attachment #2: 0001-ob-tangle.el-fix-comments-noweb-double-linking.patch --]
[-- Type: text/plain, Size: 8021 bytes --]
From 778558a5b0d38ee79d47b0068f68c761326e5e61 Mon Sep 17 00:00:00 2001
From: Hraban Luyat <hraban@0brg.net>
Date: Thu, 28 Jul 2022 22:32:08 -0400
Subject: [PATCH] =?UTF-8?q?ob-tangle.el:=20fix=20=E2=80=98:comments=20nowe?=
=?UTF-8?q?b=E2=80=99=20double=20linking?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
* lisp/ob-tangle.el: Refactor the double implementation to a single
helper function. This avoids the double link wrapping.
* testing/lisp/test-ob-tangle.el: Add unit tests.
Babel tangle allows inserting comments at the tangled site which link
back to the source in the org file. This linking was implemented
twice, to handle separate cases, but when using ‘:comments noweb’ it
ended up going through both codepaths. This resulted in doubly
wrapped links.
By refactoring all link generation into a single function, this double
wrapping is avoided.
Example file, /tmp/test.org:
* Inner
#+name: inner
#+begin_src emacs-lisp
2
#+end_src
* Main
#+header: :tangle test.el :comments noweb :noweb yes
#+begin_src emacs-lisp
1
<<inner>>
#+end_src
Before:
;; [[file:test.org::*Main][Main:1]]
1
;; [[[[file:/tmp/test.org::inner][inner]]][inner]]
2
;; inner ends here
;; Main:1 ends here
After:
;; [[file:test.org::*Main][Main:1]]
1
;; [[file:test.org::inner][inner]]
2
;; inner ends here
;; Main:1 ends here
---
lisp/ob-tangle.el | 54 ++++++++++++++++----------------
testing/lisp/test-ob-tangle.el | 56 ++++++++++++++++++++++++++++++++++
2 files changed, 83 insertions(+), 27 deletions(-)
diff --git a/lisp/ob-tangle.el b/lisp/ob-tangle.el
index fdba72278..f85f07e70 100644
--- a/lisp/ob-tangle.el
+++ b/lisp/ob-tangle.el
@@ -469,6 +469,29 @@ code blocks by target file."
(mapcar (lambda (b) (cons (car b) (nreverse (cdr b))))
(nreverse blocks))))
+(defun org-babel-tangle--unbracketed-link (params)
+ "Get a raw link to the src block at point, without brackets.
+
+The PARAMS are the 3rd element of the info for the same src block."
+ (save-match-data
+ (let* (;; The created link is transient. Using ID is not necessary,
+ ;; but could have side-effects if used. An ID property may
+ ;; be added to existing entries thus creating unexpected file
+ ;; modifications.
+ (org-id-link-to-org-use-id nil)
+ (l (org-no-properties (org-store-link nil)))
+ (bare (and (string-match org-link-bracket-re l)
+ (match-string 1 l))))
+ (when bare
+ (if (and org-babel-tangle-use-relative-file-links
+ (string-match org-link-types-re bare)
+ (string= (match-string 1 bare) "file"))
+ (concat "file:"
+ (file-relative-name (substring bare (match-end 0))
+ (file-name-directory
+ (cdr (assq :tangle params)))))
+ bare)))))
+
(defun org-babel-tangle-single-block (block-counter &optional only-this-block)
"Collect the tangled source for current block.
Return the list of block attributes needed by
@@ -485,16 +508,7 @@ non-nil, return the full association list to be used by
(extra (nth 3 info))
(coderef (nth 6 info))
(cref-regexp (org-src-coderef-regexp coderef))
- (link (let* (
- ;; The created link is transient. Using ID is
- ;; not necessary, but could have side-effects if
- ;; used. An ID property may be added to
- ;; existing entries thus creatin unexpected file
- ;; modifications.
- (org-id-link-to-org-use-id nil)
- (l (org-no-properties (org-store-link nil))))
- (and (string-match org-link-bracket-re l)
- (match-string 1 l))))
+ (link (org-babel-tangle--unbracketed-link params))
(source-name
(or (nth 4 info)
(format "%s:%d"
@@ -548,15 +562,7 @@ non-nil, return the full association list to be used by
(if org-babel-tangle-use-relative-file-links
(file-relative-name file)
file)
- (if (and org-babel-tangle-use-relative-file-links
- (string-match org-link-types-re link)
- (string= (match-string 1 link) "file")
- (stringp src-tfile))
- (concat "file:"
- (file-relative-name (substring link (match-end 0))
- (file-name-directory
- src-tfile)))
- link)
+ link
source-name
params
(if org-src-preserve-indentation
@@ -574,18 +580,12 @@ non-nil, return the full association list to be used by
INFO, when non nil, is the source block information, as returned
by `org-babel-get-src-block-info'."
(let ((link-data (pcase (or info (org-babel-get-src-block-info 'light))
- (`(,_ ,_ ,_ ,_ ,name ,start ,_)
+ (`(,_ ,_ ,params ,_ ,name ,start ,_)
`(("start-line" . ,(org-with-point-at start
(number-to-string
(line-number-at-pos))))
("file" . ,(buffer-file-name))
- ("link" . ,(let (;; The created link is transient. Using ID is
- ;; not necessary, but could have side-effects if
- ;; used. An ID property may be added to
- ;; existing entries thus creatin unexpected file
- ;; modifications.
- (org-id-link-to-org-use-id nil))
- (org-no-properties (org-store-link nil))))
+ ("link" . ,(org-babel-tangle--unbracketed-link params))
("source-name" . ,name))))))
(list (org-fill-template org-babel-tangle-comment-format-beg link-data)
(org-fill-template org-babel-tangle-comment-format-end link-data))))
diff --git a/testing/lisp/test-ob-tangle.el b/testing/lisp/test-ob-tangle.el
index 2ed4ba0da..618e118e0 100644
--- a/testing/lisp/test-ob-tangle.el
+++ b/testing/lisp/test-ob-tangle.el
@@ -159,6 +159,62 @@ echo 1
(search-forward (concat "[file:" file) nil t)))
(delete-file "test-ob-tangle.el")))))
+(ert-deftest ob-tangle/comment-noweb-relative ()
+ "Test :comments noweb tangling with relative file paths."
+ (should
+ (org-test-with-temp-text-in-file
+ "* Inner
+#+name: inner
+#+begin_src emacs-lisp
+2
+#+end_src
+
+* Main
+#+header: :tangle \"test-ob-tangle.el\" :comments noweb :noweb yes
+#+begin_src emacs-lisp
+1
+<<inner>>
+#+end_src"
+ (unwind-protect
+ (let ((org-babel-tangle-use-relative-file-links t))
+ (org-babel-tangle)
+ (with-temp-buffer
+ (insert-file-contents "test-ob-tangle.el")
+ (buffer-string)
+ (goto-char (point-min))
+ (and
+ (search-forward (concat ";; [[file:" (file-name-nondirectory file) "::inner") nil t)
+ (search-forward ";; inner ends here" nil t))))
+ (delete-file "test-ob-tangle.el")))))
+
+(ert-deftest ob-tangle/comment-noweb-absolute ()
+ "Test :comments noweb tangling with absolute file path."
+ (should
+ (org-test-with-temp-text-in-file
+ "* Inner
+#+name: inner
+#+begin_src emacs-lisp
+2
+#+end_src
+
+* Main
+#+header: :tangle \"test-ob-tangle.el\" :comments noweb :noweb yes
+#+begin_src emacs-lisp
+1
+<<inner>>
+#+end_src"
+ (unwind-protect
+ (let ((org-babel-tangle-use-relative-file-links nil))
+ (org-babel-tangle)
+ (with-temp-buffer
+ (insert-file-contents "test-ob-tangle.el")
+ (buffer-string)
+ (goto-char (point-min))
+ (and
+ (search-forward (concat ";; [[file:" file "::inner") nil t)
+ (search-forward ";; inner ends here" nil t))))
+ (delete-file "test-ob-tangle.el")))))
+
(ert-deftest ob-tangle/jump-to-org ()
"Test `org-babel-tangle-jump-to-org' specifications."
;; Standard test.
--
2.36.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] ob-tangle.el: fix ‘:comments noweb’ double linking
2022-07-30 4:56 ` Ihor Radchenko
2022-07-30 23:42 ` Hraban Luyat
@ 2022-08-03 11:31 ` Bastien Guerry
1 sibling, 0 replies; 15+ messages in thread
From: Bastien Guerry @ 2022-08-03 11:31 UTC (permalink / raw)
To: Ihor Radchenko; +Cc: Hraban Luyat, emacs-orgmode
Hi Ihor and Hraban,
Ihor Radchenko <yantar92@gmail.com> writes:
> Bastien, could you please check the FSF records?
Yes, I confirm Hraban FSF assignment is all good. I added him on
https://orgmode.org/worg/contributors.html in the list of FSF-signed
contributors.
Thanks!
--
Bastien
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] ob-tangle.el: fix ‘:comments noweb’ double linking
2022-07-30 23:42 ` Hraban Luyat
@ 2022-08-03 11:40 ` Ihor Radchenko
2022-08-03 15:55 ` Max Nikulin
1 sibling, 0 replies; 15+ messages in thread
From: Ihor Radchenko @ 2022-08-03 11:40 UTC (permalink / raw)
To: Hraban Luyat; +Cc: emacs-orgmode
Hraban Luyat <hraban@0brg.net> writes:
> Done, new patch attached. Thanks for the feedback.
Thanks!
I have applied an earlier conflicting patch recently. Now, your patch
does not apply any more. Could you kindly update it?
Best,
Ihor
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] ob-tangle.el: fix ‘:comments noweb’ double linking
2022-07-30 23:42 ` Hraban Luyat
2022-08-03 11:40 ` Ihor Radchenko
@ 2022-08-03 15:55 ` Max Nikulin
2022-08-10 20:54 ` Hraban Luyat
1 sibling, 1 reply; 15+ messages in thread
From: Max Nikulin @ 2022-08-03 15:55 UTC (permalink / raw)
To: emacs-orgmode
On 31/07/2022 06:42, Hraban Luyat wrote:
> Before:
>
> ;; [[file:test.org::*Main][Main:1]]
> 1
> ;; [[[[file:/tmp/test.org::inner][inner]]][inner]]
>
> After:
>
> ;; [[file:test.org::*Main][Main:1]]
> 1
> ;; [[file:test.org::inner][inner]]
Certainly the fix will be an improvement.
> + (when bare
> + (if (and org-babel-tangle-use-relative-file-links
> + (string-match org-link-types-re bare)
> + (string= (match-string 1 bare) "file"))
> + (concat "file:"
> + (file-relative-name (substring bare (match-end 0))
> + (file-name-directory
> + (cdr (assq :tangle params)))))
Is there any problem with the following?
(alist-get :tangle params)
> + bare)))))
I have not read the patch care carefully, so I may miss something. It
seems that (when bare (if (and other...) (action) bare)) may be
simplified to
(and bare other... (action))
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] ob-tangle.el: fix ‘:comments noweb’ double linking
2022-08-03 15:55 ` Max Nikulin
@ 2022-08-10 20:54 ` Hraban Luyat
2022-08-11 4:26 ` Ihor Radchenko
2022-08-11 15:00 ` Max Nikulin
0 siblings, 2 replies; 15+ messages in thread
From: Hraban Luyat @ 2022-08-10 20:54 UTC (permalink / raw)
To: emacs-orgmode
[-- Attachment #1: Type: text/plain, Size: 1729 bytes --]
On 8/3/22 11:55 AM, Max Nikulin wrote:
>> + (when bare
>> + (if (and org-babel-tangle-use-relative-file-links
>> + (string-match org-link-types-re bare)
>> + (string= (match-string 1 bare) "file"))
>> + (concat "file:"
>> + (file-relative-name (substring bare (match-end 0))
>> + (file-name-directory
>> + (cdr (assq :tangle params)))))
>
> Is there any problem with the following?
>
> (alist-get :tangle params)
This bit of code was moved, I didn't write it. The original code uses a
variable `src-tfile' which isn't available here, so I reused the
definition of that variable (which is (cdr (assq yada yada))). When
creating this patch, I tried to change as little as possible, to keep
everything the same as much as I can. Don't write new code, just move
existing code around.
The (cdr (assq ..)) is used in some other places, too; maybe it's worth
a separate refactor if we want to change that? I'd rather keep this
patch as isolated as possible.
>> + bare)))))
>
> I have not read the patch care carefully, so I may miss something. It
> seems that (when bare (if (and other...) (action) bare)) may be
> simplified to
>
> (and bare other... (action))
>
>
Do you mean to rewrite
(when bare (if x y bare))
to this?
(and bare x y)
If that's what you meant, I think it would evaluate differently if bare
= truthy and x = falsy, right? Form 1 evaluates to `bare', form 2
evaluates to x (i.e. NIL). Or did I misunderstand the suggestion?
@Ihor: I have rebased the patch and attached it.
[-- Attachment #2: 0001-ob-tangle.el-fix-comments-noweb-double-linking.patch --]
[-- Type: text/plain, Size: 8021 bytes --]
From 778558a5b0d38ee79d47b0068f68c761326e5e61 Mon Sep 17 00:00:00 2001
From: Hraban Luyat <hraban@0brg.net>
Date: Thu, 28 Jul 2022 22:32:08 -0400
Subject: [PATCH] =?UTF-8?q?ob-tangle.el:=20fix=20=E2=80=98:comments=20nowe?=
=?UTF-8?q?b=E2=80=99=20double=20linking?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
* lisp/ob-tangle.el: Refactor the double implementation to a single
helper function. This avoids the double link wrapping.
* testing/lisp/test-ob-tangle.el: Add unit tests.
Babel tangle allows inserting comments at the tangled site which link
back to the source in the org file. This linking was implemented
twice, to handle separate cases, but when using ‘:comments noweb’ it
ended up going through both codepaths. This resulted in doubly
wrapped links.
By refactoring all link generation into a single function, this double
wrapping is avoided.
Example file, /tmp/test.org:
* Inner
#+name: inner
#+begin_src emacs-lisp
2
#+end_src
* Main
#+header: :tangle test.el :comments noweb :noweb yes
#+begin_src emacs-lisp
1
<<inner>>
#+end_src
Before:
;; [[file:test.org::*Main][Main:1]]
1
;; [[[[file:/tmp/test.org::inner][inner]]][inner]]
2
;; inner ends here
;; Main:1 ends here
After:
;; [[file:test.org::*Main][Main:1]]
1
;; [[file:test.org::inner][inner]]
2
;; inner ends here
;; Main:1 ends here
---
lisp/ob-tangle.el | 54 ++++++++++++++++----------------
testing/lisp/test-ob-tangle.el | 56 ++++++++++++++++++++++++++++++++++
2 files changed, 83 insertions(+), 27 deletions(-)
diff --git a/lisp/ob-tangle.el b/lisp/ob-tangle.el
index fdba72278..f85f07e70 100644
--- a/lisp/ob-tangle.el
+++ b/lisp/ob-tangle.el
@@ -469,6 +469,29 @@ code blocks by target file."
(mapcar (lambda (b) (cons (car b) (nreverse (cdr b))))
(nreverse blocks))))
+(defun org-babel-tangle--unbracketed-link (params)
+ "Get a raw link to the src block at point, without brackets.
+
+The PARAMS are the 3rd element of the info for the same src block."
+ (save-match-data
+ (let* (;; The created link is transient. Using ID is not necessary,
+ ;; but could have side-effects if used. An ID property may
+ ;; be added to existing entries thus creating unexpected file
+ ;; modifications.
+ (org-id-link-to-org-use-id nil)
+ (l (org-no-properties (org-store-link nil)))
+ (bare (and (string-match org-link-bracket-re l)
+ (match-string 1 l))))
+ (when bare
+ (if (and org-babel-tangle-use-relative-file-links
+ (string-match org-link-types-re bare)
+ (string= (match-string 1 bare) "file"))
+ (concat "file:"
+ (file-relative-name (substring bare (match-end 0))
+ (file-name-directory
+ (cdr (assq :tangle params)))))
+ bare)))))
+
(defun org-babel-tangle-single-block (block-counter &optional only-this-block)
"Collect the tangled source for current block.
Return the list of block attributes needed by
@@ -485,16 +508,7 @@ non-nil, return the full association list to be used by
(extra (nth 3 info))
(coderef (nth 6 info))
(cref-regexp (org-src-coderef-regexp coderef))
- (link (let* (
- ;; The created link is transient. Using ID is
- ;; not necessary, but could have side-effects if
- ;; used. An ID property may be added to
- ;; existing entries thus creatin unexpected file
- ;; modifications.
- (org-id-link-to-org-use-id nil)
- (l (org-no-properties (org-store-link nil))))
- (and (string-match org-link-bracket-re l)
- (match-string 1 l))))
+ (link (org-babel-tangle--unbracketed-link params))
(source-name
(or (nth 4 info)
(format "%s:%d"
@@ -548,15 +562,7 @@ non-nil, return the full association list to be used by
(if org-babel-tangle-use-relative-file-links
(file-relative-name file)
file)
- (if (and org-babel-tangle-use-relative-file-links
- (string-match org-link-types-re link)
- (string= (match-string 1 link) "file")
- (stringp src-tfile))
- (concat "file:"
- (file-relative-name (substring link (match-end 0))
- (file-name-directory
- src-tfile)))
- link)
+ link
source-name
params
(if org-src-preserve-indentation
@@ -574,18 +580,12 @@ non-nil, return the full association list to be used by
INFO, when non nil, is the source block information, as returned
by `org-babel-get-src-block-info'."
(let ((link-data (pcase (or info (org-babel-get-src-block-info 'light))
- (`(,_ ,_ ,_ ,_ ,name ,start ,_)
+ (`(,_ ,_ ,params ,_ ,name ,start ,_)
`(("start-line" . ,(org-with-point-at start
(number-to-string
(line-number-at-pos))))
("file" . ,(buffer-file-name))
- ("link" . ,(let (;; The created link is transient. Using ID is
- ;; not necessary, but could have side-effects if
- ;; used. An ID property may be added to
- ;; existing entries thus creatin unexpected file
- ;; modifications.
- (org-id-link-to-org-use-id nil))
- (org-no-properties (org-store-link nil))))
+ ("link" . ,(org-babel-tangle--unbracketed-link params))
("source-name" . ,name))))))
(list (org-fill-template org-babel-tangle-comment-format-beg link-data)
(org-fill-template org-babel-tangle-comment-format-end link-data))))
diff --git a/testing/lisp/test-ob-tangle.el b/testing/lisp/test-ob-tangle.el
index 2ed4ba0da..618e118e0 100644
--- a/testing/lisp/test-ob-tangle.el
+++ b/testing/lisp/test-ob-tangle.el
@@ -159,6 +159,62 @@ echo 1
(search-forward (concat "[file:" file) nil t)))
(delete-file "test-ob-tangle.el")))))
+(ert-deftest ob-tangle/comment-noweb-relative ()
+ "Test :comments noweb tangling with relative file paths."
+ (should
+ (org-test-with-temp-text-in-file
+ "* Inner
+#+name: inner
+#+begin_src emacs-lisp
+2
+#+end_src
+
+* Main
+#+header: :tangle \"test-ob-tangle.el\" :comments noweb :noweb yes
+#+begin_src emacs-lisp
+1
+<<inner>>
+#+end_src"
+ (unwind-protect
+ (let ((org-babel-tangle-use-relative-file-links t))
+ (org-babel-tangle)
+ (with-temp-buffer
+ (insert-file-contents "test-ob-tangle.el")
+ (buffer-string)
+ (goto-char (point-min))
+ (and
+ (search-forward (concat ";; [[file:" (file-name-nondirectory file) "::inner") nil t)
+ (search-forward ";; inner ends here" nil t))))
+ (delete-file "test-ob-tangle.el")))))
+
+(ert-deftest ob-tangle/comment-noweb-absolute ()
+ "Test :comments noweb tangling with absolute file path."
+ (should
+ (org-test-with-temp-text-in-file
+ "* Inner
+#+name: inner
+#+begin_src emacs-lisp
+2
+#+end_src
+
+* Main
+#+header: :tangle \"test-ob-tangle.el\" :comments noweb :noweb yes
+#+begin_src emacs-lisp
+1
+<<inner>>
+#+end_src"
+ (unwind-protect
+ (let ((org-babel-tangle-use-relative-file-links nil))
+ (org-babel-tangle)
+ (with-temp-buffer
+ (insert-file-contents "test-ob-tangle.el")
+ (buffer-string)
+ (goto-char (point-min))
+ (and
+ (search-forward (concat ";; [[file:" file "::inner") nil t)
+ (search-forward ";; inner ends here" nil t))))
+ (delete-file "test-ob-tangle.el")))))
+
(ert-deftest ob-tangle/jump-to-org ()
"Test `org-babel-tangle-jump-to-org' specifications."
;; Standard test.
--
2.36.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] ob-tangle.el: fix ‘:comments noweb’ double linking
2022-08-10 20:54 ` Hraban Luyat
@ 2022-08-11 4:26 ` Ihor Radchenko
2022-08-12 2:21 ` Hraban Luyat
2022-08-11 15:00 ` Max Nikulin
1 sibling, 1 reply; 15+ messages in thread
From: Ihor Radchenko @ 2022-08-11 4:26 UTC (permalink / raw)
To: Hraban Luyat; +Cc: emacs-orgmode
Hraban Luyat <hraban@0brg.net> writes:
>> Is there any problem with the following?
>>
>> (alist-get :tangle params)
>
> This bit of code was moved, I didn't write it. The original code uses a
> variable `src-tfile' which isn't available here, so I reused the
> definition of that variable (which is (cdr (assq yada yada))). When
> creating this patch, I tried to change as little as possible, to keep
> everything the same as much as I can. Don't write new code, just move
> existing code around.
>
> The (cdr (assq ..)) is used in some other places, too; maybe it's worth
> a separate refactor if we want to change that? I'd rather keep this
> patch as isolated as possible.
I suspect that alist-get was not there in Emacs 24.
Otherwise, alist-get with no optional parameters is just a wrapper for
(cdr (assq...))
We can change it, though I do not see this as a big problem.
> @Ihor: I have rebased the patch and attached it.
Sorry, but the patch still does not apply on my side onto the current
main branch.
--
Ihor Radchenko,
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] ob-tangle.el: fix ‘:comments noweb’ double linking
2022-08-10 20:54 ` Hraban Luyat
2022-08-11 4:26 ` Ihor Radchenko
@ 2022-08-11 15:00 ` Max Nikulin
1 sibling, 0 replies; 15+ messages in thread
From: Max Nikulin @ 2022-08-11 15:00 UTC (permalink / raw)
To: emacs-orgmode
On 11/08/2022 03:54, Hraban Luyat wrote:
> On 8/3/22 11:55 AM, Max Nikulin wrote:
>
> Do you mean to rewrite
>
> (when bare (if x y bare))
>
> to this?
>
> (and bare x y)
>
> If that's what you meant, I think it would evaluate differently if bare
> = truthy and x = falsy, right? Form 1 evaluates to `bare', form 2
> evaluates to x (i.e. NIL). Or did I misunderstand the suggestion?
You are right. I tried to suggest an expression that was wrong.
I am going to make another attempt:
(if (and bare
org-babel-tangle-use-relative-file-links
(string-match org-link-types-re bare)
(string= (match-string 1 bare) "file"))
(concat "file:"
(file-relative-name (substring bare (match-end 0))
(file-name-directory
(cdr (assq :tangle params)))))
bare)
I do not think that such code is dramatically clearer, but at least it
has 1 conditional form less.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] ob-tangle.el: fix ‘:comments noweb’ double linking
2022-08-11 4:26 ` Ihor Radchenko
@ 2022-08-12 2:21 ` Hraban Luyat
2022-08-12 13:16 ` Max Nikulin
2022-08-13 6:40 ` Ihor Radchenko
0 siblings, 2 replies; 15+ messages in thread
From: Hraban Luyat @ 2022-08-12 2:21 UTC (permalink / raw)
To: emacs-orgmode
[-- Attachment #1: Type: text/plain, Size: 1543 bytes --]
On 8/11/22 12:26 AM, Ihor Radchenko wrote:
> Hraban Luyat <hraban@0brg.net> writes:
>
>>> Is there any problem with the following?
>>>
>>> (alist-get :tangle params)
>>
>> This bit of code was moved, I didn't write it. The original code uses a
>> variable `src-tfile' which isn't available here, so I reused the
>> definition of that variable (which is (cdr (assq yada yada))). When
>> creating this patch, I tried to change as little as possible, to keep
>> everything the same as much as I can. Don't write new code, just move
>> existing code around.
>>
>> The (cdr (assq ..)) is used in some other places, too; maybe it's worth
>> a separate refactor if we want to change that? I'd rather keep this
>> patch as isolated as possible.
>
> I suspect that alist-get was not there in Emacs 24.
> Otherwise, alist-get with no optional parameters is just a wrapper for
> (cdr (assq...))
>
> We can change it, though I do not see this as a big problem.
>
>> @Ihor: I have rebased the patch and attached it.
>
> Sorry, but the patch still does not apply on my side onto the current
> main branch.
Just rebased and recreated it. Based off
6acc58c9c6bcfd45dcc5964cac7e3df8347121cc.
@Max: what do you think of when-let? That seems more appropriate for
this situation. Thoughts?
>
> --
> Ihor Radchenko,
> 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
[-- Attachment #2: 0001-ob-tangle.el-fix-comments-noweb-double-linking.patch --]
[-- Type: text/plain, Size: 8516 bytes --]
From 0c89c48a80b0095c40a1c4c478fdfd581e0110fd Mon Sep 17 00:00:00 2001
From: Hraban Luyat <hraban@0brg.net>
Date: Mon, 8 Aug 2022 16:58:05 -0400
Subject: [PATCH] =?UTF-8?q?ob-tangle.el:=20fix=20=E2=80=98:comments=20nowe?=
=?UTF-8?q?b=E2=80=99=20double=20linking?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
* lisp/ob-tangle.el: Refactor the double implementation to a single
helper function. This avoids the double link wrapping.
* testing/lisp/test-ob-tangle.el: Add unit tests.
Babel tangle allows inserting comments at the tangled site which link
back to the source in the org file. This linking was implemented
twice, to handle separate cases, but when using ‘:comments noweb’ it
ended up going through both codepaths. This resulted in doubly
wrapped links.
By refactoring all link generation into a single function, this double
wrapping is avoided.
Example file, /tmp/test.org:
* Inner
#+name: inner
#+begin_src emacs-lisp
2
#+end_src
* Main
#+header: :tangle test.el :comments noweb :noweb yes
#+begin_src emacs-lisp
1
<<inner>>
#+end_src
Before:
;; [[file:test.org::*Main][Main:1]]
1
;; [[[[file:/tmp/test.org::inner][inner]]][inner]]
2
;; inner ends here
;; Main:1 ends here
After:
;; [[file:test.org::*Main][Main:1]]
1
;; [[file:test.org::inner][inner]]
2
;; inner ends here
;; Main:1 ends here
---
lisp/ob-tangle.el | 62 +++++++++++++++++-----------------
testing/lisp/test-ob-tangle.el | 56 ++++++++++++++++++++++++++++++
2 files changed, 87 insertions(+), 31 deletions(-)
diff --git a/lisp/ob-tangle.el b/lisp/ob-tangle.el
index 4b8fad6ce..4db0adda7 100644
--- a/lisp/ob-tangle.el
+++ b/lisp/ob-tangle.el
@@ -469,6 +469,33 @@ code blocks by target file."
(mapcar (lambda (b) (cons (car b) (nreverse (cdr b))))
(nreverse blocks))))
+(defun org-babel-tangle--unbracketed-link (params)
+ "Get a raw link to the src block at point, without brackets.
+
+The PARAMS are the 3rd element of the info for the same src block."
+ (unless (string= "no" (cdr (assq :comments params)))
+ (save-match-data
+ (let* (;; The created link is transient. Using ID is not necessary,
+ ;; but could have side-effects if used. An ID property may
+ ;; be added to existing entries thus creating unexpected file
+ ;; modifications.
+ (org-id-link-to-org-use-id nil)
+ (l (org-no-properties
+ (cl-letf (((symbol-function 'org-store-link-functions)
+ (lambda () nil)))
+ (org-store-link nil))))
+ (bare (and (string-match org-link-bracket-re l)
+ (match-string 1 l))))
+ (when bare
+ (if (and org-babel-tangle-use-relative-file-links
+ (string-match org-link-types-re bare)
+ (string= (match-string 1 bare) "file"))
+ (concat "file:"
+ (file-relative-name (substring bare (match-end 0))
+ (file-name-directory
+ (cdr (assq :tangle params)))))
+ bare))))))
+
(defun org-babel-tangle-single-block (block-counter &optional only-this-block)
"Collect the tangled source for current block.
Return the list of block attributes needed by
@@ -485,20 +512,7 @@ non-nil, return the full association list to be used by
(extra (nth 3 info))
(coderef (nth 6 info))
(cref-regexp (org-src-coderef-regexp coderef))
- (link (if (string= "no" (cdr (assq :comments params))) ""
- (let* (
- ;; The created link is transient. Using ID is
- ;; not necessary, but could have side-effects if
- ;; used. An ID property may be added to
- ;; existing entries thus creating unexpected
- ;; file modifications.
- (org-id-link-to-org-use-id nil)
- (l (org-no-properties
- (cl-letf (((symbol-function 'org-store-link-functions)
- (lambda () nil)))
- (org-store-link nil)))))
- (and (string-match org-link-bracket-re l)
- (match-string 1 l)))))
+ (link (org-babel-tangle--unbracketed-link params))
(source-name
(or (nth 4 info)
(format "%s:%d"
@@ -552,15 +566,7 @@ non-nil, return the full association list to be used by
(if org-babel-tangle-use-relative-file-links
(file-relative-name file)
file)
- (if (and org-babel-tangle-use-relative-file-links
- (string-match org-link-types-re link)
- (string= (match-string 1 link) "file")
- (stringp src-tfile))
- (concat "file:"
- (file-relative-name (substring link (match-end 0))
- (file-name-directory
- src-tfile)))
- link)
+ link
source-name
params
(if org-src-preserve-indentation
@@ -578,18 +584,12 @@ non-nil, return the full association list to be used by
INFO, when non nil, is the source block information, as returned
by `org-babel-get-src-block-info'."
(let ((link-data (pcase (or info (org-babel-get-src-block-info 'light))
- (`(,_ ,_ ,_ ,_ ,name ,start ,_)
+ (`(,_ ,_ ,params ,_ ,name ,start ,_)
`(("start-line" . ,(org-with-point-at start
(number-to-string
(line-number-at-pos))))
("file" . ,(buffer-file-name))
- ("link" . ,(let (;; The created link is transient. Using ID is
- ;; not necessary, but could have side-effects if
- ;; used. An ID property may be added to
- ;; existing entries thus creatin unexpected file
- ;; modifications.
- (org-id-link-to-org-use-id nil))
- (org-no-properties (org-store-link nil))))
+ ("link" . ,(org-babel-tangle--unbracketed-link params))
("source-name" . ,name))))))
(list (org-fill-template org-babel-tangle-comment-format-beg link-data)
(org-fill-template org-babel-tangle-comment-format-end link-data))))
diff --git a/testing/lisp/test-ob-tangle.el b/testing/lisp/test-ob-tangle.el
index 2ed4ba0da..618e118e0 100644
--- a/testing/lisp/test-ob-tangle.el
+++ b/testing/lisp/test-ob-tangle.el
@@ -159,6 +159,62 @@ echo 1
(search-forward (concat "[file:" file) nil t)))
(delete-file "test-ob-tangle.el")))))
+(ert-deftest ob-tangle/comment-noweb-relative ()
+ "Test :comments noweb tangling with relative file paths."
+ (should
+ (org-test-with-temp-text-in-file
+ "* Inner
+#+name: inner
+#+begin_src emacs-lisp
+2
+#+end_src
+
+* Main
+#+header: :tangle \"test-ob-tangle.el\" :comments noweb :noweb yes
+#+begin_src emacs-lisp
+1
+<<inner>>
+#+end_src"
+ (unwind-protect
+ (let ((org-babel-tangle-use-relative-file-links t))
+ (org-babel-tangle)
+ (with-temp-buffer
+ (insert-file-contents "test-ob-tangle.el")
+ (buffer-string)
+ (goto-char (point-min))
+ (and
+ (search-forward (concat ";; [[file:" (file-name-nondirectory file) "::inner") nil t)
+ (search-forward ";; inner ends here" nil t))))
+ (delete-file "test-ob-tangle.el")))))
+
+(ert-deftest ob-tangle/comment-noweb-absolute ()
+ "Test :comments noweb tangling with absolute file path."
+ (should
+ (org-test-with-temp-text-in-file
+ "* Inner
+#+name: inner
+#+begin_src emacs-lisp
+2
+#+end_src
+
+* Main
+#+header: :tangle \"test-ob-tangle.el\" :comments noweb :noweb yes
+#+begin_src emacs-lisp
+1
+<<inner>>
+#+end_src"
+ (unwind-protect
+ (let ((org-babel-tangle-use-relative-file-links nil))
+ (org-babel-tangle)
+ (with-temp-buffer
+ (insert-file-contents "test-ob-tangle.el")
+ (buffer-string)
+ (goto-char (point-min))
+ (and
+ (search-forward (concat ";; [[file:" file "::inner") nil t)
+ (search-forward ";; inner ends here" nil t))))
+ (delete-file "test-ob-tangle.el")))))
+
(ert-deftest ob-tangle/jump-to-org ()
"Test `org-babel-tangle-jump-to-org' specifications."
;; Standard test.
--
2.36.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] ob-tangle.el: fix ‘:comments noweb’ double linking
2022-08-12 2:21 ` Hraban Luyat
@ 2022-08-12 13:16 ` Max Nikulin
2022-08-13 6:42 ` Ihor Radchenko
2022-08-13 6:40 ` Ihor Radchenko
1 sibling, 1 reply; 15+ messages in thread
From: Max Nikulin @ 2022-08-12 13:16 UTC (permalink / raw)
To: emacs-orgmode
On 12/08/2022 09:21, Hraban Luyat wrote:
>
> @Max: what do you think of when-let? That seems more appropriate for
> this situation. Thoughts?
At first I thought about compatibility, but this form is available since
Emacs-25, so it is not a problem.
Maybe I missed something, but I do not see clear advantage. Anyway it
leads to `if' nested inside `when-let'.
> + (bare (and (string-match org-link-bracket-re l)
> + (match-string 1 l))))
> + (when bare
> + (if (and org-babel-tangle-use-relative-file-links
> + (string-match org-link-types-re bare)
> + (string= (match-string 1 bare) "file"))
It looks like (string-prefix-p "file:" bare) but without the complex
regexp. I see, such code was used before. By the way, why it is "bare",
not e.g. "target"?
> + (concat "file:"
> + (file-relative-name (substring bare (match-end 0))
> + (file-name-directory
> + (cdr (assq :tangle params)))))
I do not insist on changes since possible improvements are not really
significant.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] ob-tangle.el: fix ‘:comments noweb’ double linking
2022-08-12 2:21 ` Hraban Luyat
2022-08-12 13:16 ` Max Nikulin
@ 2022-08-13 6:40 ` Ihor Radchenko
1 sibling, 0 replies; 15+ messages in thread
From: Ihor Radchenko @ 2022-08-13 6:40 UTC (permalink / raw)
To: Hraban Luyat; +Cc: emacs-orgmode
Hraban Luyat <hraban@0brg.net> writes:
>>> @Ihor: I have rebased the patch and attached it.
Applied onto main via 8a781d35d.
https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=8a781d35dc68f20fa2a5546c98ba3d9b77ee3cda
Thanks again for your contribution!
--
Ihor Radchenko,
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] ob-tangle.el: fix ‘:comments noweb’ double linking
2022-08-12 13:16 ` Max Nikulin
@ 2022-08-13 6:42 ` Ihor Radchenko
2022-08-13 8:06 ` Max Nikulin
0 siblings, 1 reply; 15+ messages in thread
From: Ihor Radchenko @ 2022-08-13 6:42 UTC (permalink / raw)
To: Max Nikulin; +Cc: emacs-orgmode
Max Nikulin <manikulin@gmail.com> writes:
>> + (bare (and (string-match org-link-bracket-re l)
>> + (match-string 1 l))))
>> + (when bare
>> + (if (and org-babel-tangle-use-relative-file-links
>> + (string-match org-link-types-re bare)
>> + (string= (match-string 1 bare) "file"))
>
> It looks like (string-prefix-p "file:" bare) but without the complex
> regexp. I see, such code was used before. By the way, why it is "bare",
> not e.g. "target"?
match-end is used later in the code. Thus, string-match is justified.
--
Ihor Radchenko,
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] ob-tangle.el: fix ‘:comments noweb’ double linking
2022-08-13 6:42 ` Ihor Radchenko
@ 2022-08-13 8:06 ` Max Nikulin
2022-08-14 3:20 ` Ihor Radchenko
0 siblings, 1 reply; 15+ messages in thread
From: Max Nikulin @ 2022-08-13 8:06 UTC (permalink / raw)
To: emacs-orgmode
On 13/08/2022 13:42, Ihor Radchenko wrote:
> Max Nikulin writes:
>
>>> + (bare (and (string-match org-link-bracket-re l)
>>> + (match-string 1 l))))
>>> + (when bare
>>> + (if (and org-babel-tangle-use-relative-file-links
>>> + (string-match org-link-types-re bare)
>>> + (string= (match-string 1 bare) "file"))
>>
>> It looks like (string-prefix-p "file:" bare) but without the complex
>> regexp. I see, such code was used before. By the way, why it is "bare",
>> not e.g. "target"?
>
> match-end is used later in the code. Thus, string-match is justified.
The matched string is always "file:", so there is no actual point in
`string-match'. It does not really matter however.
(let ((params '((:tangle . "/home/user/file.el")))
(type "file:")
(target "file:///dev/null"))
(if (and target
org-babel-tangle-use-relative-file-links
(string-prefix-p type target))
(concat type
(file-relative-name
(substring target (length type))
(file-name-directory (alist-get :tangle params))))
target))
In the code existed before and moved by the committed patch I do not
like that "then" branch of the `if' form uses side effect of evaluation
of the condition.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] ob-tangle.el: fix ‘:comments noweb’ double linking
2022-08-13 8:06 ` Max Nikulin
@ 2022-08-14 3:20 ` Ihor Radchenko
0 siblings, 0 replies; 15+ messages in thread
From: Ihor Radchenko @ 2022-08-14 3:20 UTC (permalink / raw)
To: Max Nikulin; +Cc: emacs-orgmode
Max Nikulin <manikulin@gmail.com> writes:
>>> It looks like (string-prefix-p "file:" bare) but without the complex
>>> regexp. I see, such code was used before. By the way, why it is "bare",
>>> not e.g. "target"?
>>
>> match-end is used later in the code. Thus, string-match is justified.
>
> The matched string is always "file:", so there is no actual point in
> `string-match'. It does not really matter however.
>
> (let ((params '((:tangle . "/home/user/file.el")))
> (type "file:")
> (target "file:///dev/null"))
> (if (and target
> org-babel-tangle-use-relative-file-links
> (string-prefix-p type target))
> (concat type
> (file-relative-name
> (substring target (length type))
> (file-name-directory (alist-get :tangle params))))
> target))
>
> In the code existed before and moved by the committed patch I do not
> like that "then" branch of the `if' form uses side effect of evaluation
> of the condition.
Sounds reasonable.
Could you make a patch?
--
Ihor Radchenko,
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
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2022-08-14 3:20 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-07-29 4:54 [PATCH] ob-tangle.el: fix ‘:comments noweb’ double linking Hraban Luyat
2022-07-30 4:56 ` Ihor Radchenko
2022-07-30 23:42 ` Hraban Luyat
2022-08-03 11:40 ` Ihor Radchenko
2022-08-03 15:55 ` Max Nikulin
2022-08-10 20:54 ` Hraban Luyat
2022-08-11 4:26 ` Ihor Radchenko
2022-08-12 2:21 ` Hraban Luyat
2022-08-12 13:16 ` Max Nikulin
2022-08-13 6:42 ` Ihor Radchenko
2022-08-13 8:06 ` Max Nikulin
2022-08-14 3:20 ` Ihor Radchenko
2022-08-13 6:40 ` Ihor Radchenko
2022-08-11 15:00 ` Max Nikulin
2022-08-03 11:31 ` Bastien Guerry
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs/org-mode.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).