Re: [ANN] lisp/ob-tangle-sync.el

[Mehmet Tekman <mtekman89@gmail.com>]

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


Hello João and org!

Mehmet Tekman <mtekman89@gmail.com> on 29/04/2024 wrote:

>> Can you wait until Sunday for me to resolve this, and then we can
>> discuss?

Okay, I've cleaned up my branches and rebased where I could, and now I
think my code is at a place where you can jump in.

This will be a long email summarizing what's been done so far, where the
pitfalls were, where we are now, and what needs to be done:

So, my code is hosted at =https://gitlab.com/mtekman/org-mode= and with
the exception of the =header-reform= branch, is about 1 year behind
upstream.

* Ancient History

Here we detail what some of ideas and pitfalls were. The main takeaway
message is that we want to keep all sync actions everything within
":tangle", and we want to unify mutual exclusivity and hierarchical
merging with ":results".

You're probably already familiar with all this, but if not, here goes:


*** Initial PR: `:tangle-sync' adds a sync parameter

This was the initial PR that implemented two-way syncing via a new
:tangle-sync header parameter. The work was performed in the
=ob-tangle-sync-2023= branch, specifically the commit range:
=34727abb6...2f0f54d68=, and specifically localized mainly in the file
=ob-tangle-sync.el=.

This :tangle-sync parameter described in what direction a block should
be synced: either "export", "import", "skip", or "sync".

e.g.1 :tangle filename :tangle-sync action

#+begin_src bash :tangle export_this_file.bash :tangle-sync export
  echo file and sync-action as seperate header args
#+end_src

That git branch also reworked the detangle framework in the
=ob-tangle.el= file, allowing it to detangle on a more block basis than
it was doing before.

*** New effort: Rework the existing `:tangle' to take filename and sync parameter

As the code developed and started impacting more of =ob-tangle.el= code
base, it was suggested that instead of creating an entirely new header
argument (which would add to the complexity of the org code base), why
shouldn't we simply compound the sync command to the existing :tangle
header?

e.g.2 :tangle filename sync-action

#+begin_src bash :tangle export_this_file.bash export
  echo file and sync-action under the same banner
#+end_src

with the implication that the last word in the :tangle string would be
a sync keyword.

This now meant that instead of working on the =ob-tangle-sync.el= and
=ob.tangle.el= files, that we would also need to start affecting the
=ob-core.el= code.

This is what commits =4b9d05d8c...HEAD= in the =ob-tangle-sync-2023=
branch attempted to do, by making use of the mutual-exclusive keyword
framework already present for :results

#+begin_src elisp
  ;; see: org-babel-common-header-args-w-values
  '(results	. ((file list vector table scalar verbatim)
    	       (raw html latex org code pp drawer link graphics)
    	       (replace silent none discard append prepend)
    	       (output value)))
  ;;
  (tangle . ((tangle yes no :any)
             (import export skip sync))) ;; added
#+end_src

This way someone can specify mutually exclusive values in headers such
as:

e.g.3 
#+begin_src bash :results file raw replace output
  echo multiple compounding results values which are all valid
#+end_src

or, hopefully, in the case for :tangle, something like:

e.g.4:
#+begin_example
:tangle yes sync   ;; filename inherited 
:tangle no export  ;; nothing should happen
:tangle my_custom_file.sh import   ;; imports specifically from that file
#+end_example

*** New problem: `:results' and `:tangle' mutually exclusive groups are not alike

You may notice that the `:results' mutually exclusive values and the
`:tangle' mutually exclusive values differ on one problematic value:
":any"

#+begin_src elisp
  (tangle . ((tangle yes no :any)
             (import export skip sync)))
#+end_src

This `:any' parameter means that the first argument of the tangle header
could be of any arbitrary length and take on any value… including those
specific values that we need as the sync-action in the second argument.
Oh dear!

e.g.5:
#+begin_example
:tangle filename.sh export   ;; easily resolvable
:tangle import import        ;; uh... we import? and assume the filename comes up the hirarchy
:tangle import export        ;; uh... we assume they are both sync-actions but only last is valid?
#+end_example

And this gets even worse if we have to consider filenames with spaces:

e.g.6: 
#+begin_example
:tangle filename with spaces.sh     ;; filename only, inherit sync action from elsewhere
:tangle filename with spaces sync   ;; Um. We sync, and assume the filename is "filename with spaces"?
#+end_example

*** Solution: Custom splitting tangle function

Okay, so the problems shown in examples 5 and 6 can be fixed quite
easily - you just split the whole tangle header by spaces, and test if
the last word is a valid sync-action or not.

This was implemented in the splitting function
=org-babel--handle-tangle-args= which specifically handles that header.

But once again, we see the code base splitting into custom
header-keyword-specific solutions instead of making one coherent unified
approach for handling mutually exclusive arguments.

That is, ideally, :results and :tangle should be handled by the same
methods, especially in regards to:

1. Processing parameters

   `org-babel-process-params' evaluates header values (I think?), and in
   the case of :results, also splits it into parameters, namely a somewhat
   hidden parameter named `:result-params'.

   This has the benefit that the `:results' header arg is correctly
   split into its component mutually exclusive groups (stored in
   `:result-params'), but that the initial `:results' raw string is
   still kept and widely processed for the majority of cases where the
   value of this parameter is not complex.

2. Merging parameters

   `org-babel-merge-params' merges parameters with the same header
   keyword across hierarchies, so that the final block knows what its
   final parameters are.

   e.g.7:

   #+begin_src org
     ,#+PROPERTY: header-args :tangle /tmp/default_tangle.txt

     ,#+begin_src conf :tangle import
       Import text
     ,#+end_src
   #+end_src

   The tangle parameters at the top-level PROPERTY are merged down
   into the source code block, giving a final :tangle result of:

   #+begin_src elisp
     (merge ":tangle /tmp/default_tangle.txt export" ;; the export sync-action is implied
            ":tangle nil import")                    ;; the nil filename is somehow inferred
   #+end_src

   which should eventually result in final parameters
   ":tangle /tmp/default_tangle.txt import"  for that block.


*** Unified Merge function

I have several (unimaginatively named) branches like merge-params-first,
merge-params-second, ..., merge-params-fif that attempted to implement a
unified merge strategy for the `:tangle filename sync-action' idea.

The latest (read: cleanest branch) is the `merge-params-fif' branch with
diff =1866c0825= implementing a new =org-babel--merge-headers= function
which tries to act as a unified approach to merging :tangle :results and
:export headers.

It worked (to some degree), but it tripped up on issue of having
"filenames with spaces", so I think this might be dead code at this
point, especially since we pivoted towards `:tangle-params' being a
solution for resolving sync actions.

(Essentially, you can ignore any branch prefixed with "merge-params-*")


* Where we are now

The branch I have most recently worked on (I rebased it earlier this
week) is called =header-reform=. It splits a :tangle header such as

this =:tangle filename with space.txt export=
into =:tangle-params ("filename with space.txt" "export")=

such that it does not interfere with the rest of org headers.

This is where I would greatly need you help, João.

The work still to be done from my perspective:

1. Implement tests  <<- we are here

   We need to know what the desired behaviour of :tangle and
   :tangle-params will be in different contexts.

   I have written 12 test cases as a start, but some contexts are
   still unclear to me and I cannot do this alone.

   Please see the second and third patches attached to this email.
   

2. Unified merge function

   Something that handles tangle-params and result-params, as well
   something that can handle mutually exclusive groups such as tangle
   and results, especially when the :any keyword is involved.

3. Rebase/Edit the tangle-sync changes from the =ob-tangle-sync-2023= branch

   Finally add the two way syncing functionality for tangled blocks.


I've attached the two working patches from the =header-reform= branch to
this mail, and one floating WIP inline patch which should get things
started.

Apologies for the length, I just wanted to summarize the efforts and
show where you could come in.

@Ihor please feel free to clarify anything I've written that sounds not
quite right. (And thanks to all for the infinite patience!)


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-lisp-ob-core.el-org-babel-split-str-quoted-org-babel.patch --]
[-- Type: text/x-patch, Size: 6037 bytes --]

From 7a0b71fc519ec20b68ae75985085530750ad6e17 Mon Sep 17 00:00:00 2001
From: Mehmet Tekman <mtekman89@gmail.com>
Date: Wed, 20 Sep 2023 11:35:00 +0200
Subject: [PATCH 1/3] * lisp/ob-core.el (org-babel--split-str-quoted
 org-babel--tangle-split org-babel-process-params): functions to assist
 splitting a :tangle header containing a filename with spaces as well as a
 sync-action into two parameter components of strictly filename and
 sync-action for use with :tangle-params.

* lisp/ob-exp.el (org-babel-exp-code): fix to function to not include
:tangle-params as as real header

* testing/lisp/test-ob.el (test-ob/process-params-no-duplicates): fix
to include :tangle-params as part of process-params test
---
 lisp/ob-core.el         | 65 +++++++++++++++++++++++++++++++++++++----
 lisp/ob-exp.el          |  2 +-
 testing/lisp/test-ob.el |  2 ++
 3 files changed, 63 insertions(+), 6 deletions(-)

diff --git a/lisp/ob-core.el b/lisp/ob-core.el
index 73fb70c26..019f3d88d 100644
--- a/lisp/ob-core.el
+++ b/lisp/ob-core.el
@@ -1785,6 +1785,52 @@ HEADER-ARGUMENTS is alist of all the arguments."
 	  header-arguments)
     (nreverse results)))
 
+
+(defun org-babel--split-str-quoted (str)
+  "Splits a string that may or may not contain quotes."
+  (let (result pos)
+    (while (string-match (rx (or (seq "\"" (group (* (not "\""))) "\"")
+                                 (group (+ (not space)))))
+                         str pos)
+      (let ((match-str1 (match-string 1 str))
+            (match-str2 (match-string 2 str)))
+        (if match-str1
+            (progn
+              (push match-str1 result)
+              (setq pos (1+ (match-end 1))))
+          (push match-str2 result)
+          (setq pos (match-end 2)))))
+    (nreverse result)))
+
+(defun org-babel--tangle-split (raw-tangle)
+  "Split a :tangle header into two: filename of multiple words, and
+sync action. The result does not take into account any merged properties.
+
+If the filename is actually a tangle keyword such as 'no', then specify no sync action.
+
+Actions can be
+  :any (specific filename) import/export/sync
+  yes (inherited filename) import/export/sync
+  no (will not export) (no sync action)"
+  (let* ((valid-sync-actions '("import" "export" "sync"))
+         (file-action (org-babel--split-str-quoted raw-tangle))
+         (file (car file-action))
+         (sync-action (nth (1- (length file-action)) file-action)))
+    (if (member sync-action valid-sync-actions)
+        ;; If last word matches, assume the previous are all part of
+        ;; the filename
+        (setq file (mapconcat #'identity (nreverse (cdr (nreverse file-action))) " "))
+
+      ;; Otherwise a sync action was not given, and we default to normal tangle keywords
+      ;; such as a filename (assumes export), yes (assumes export), no (no sync-action)
+      (if (string-equal sync-action "no")
+          (setq sync-action nil)
+        ;; No sync action at all given? Assume the default: export
+        (setq sync-action "export"
+              file raw-tangle)))
+    (list file sync-action)))
+
+
 (defun org-babel-process-params (params)
   "Expand variables in PARAMS and add summary parameters."
   (let* ((processed-vars (mapcar (lambda (el)
@@ -1807,7 +1853,13 @@ HEADER-ARGUMENTS is alist of all the arguments."
 					    raw-result
                                           ;; FIXME: Arbitrary code evaluation.
 					  (eval raw-result t)))
-			  (cdr (assq :result-params params))))))
+			  (cdr (assq :result-params params)))))
+         (raw-tangle (cdr (assq :tangle params)))
+         (tangle-params (if raw-tangle
+                            (delete-dups
+	                     (append
+	                      (org-babel--tangle-split raw-tangle)
+                              (cdr (assq :tangle-params params)))))))
     (append
      (mapcar (lambda (var) (cons :var var)) (car vars-and-names))
      (list
@@ -1815,14 +1867,17 @@ HEADER-ARGUMENTS is alist of all the arguments."
 			       (cadr  vars-and-names)))
       (cons :rowname-names (or (cdr (assq :rowname-names params))
 			       (cl-caddr vars-and-names)))
+      (cons :tangle-params tangle-params)  ;; always a list of two: tangle-keyword sync-action
       (cons :result-params result-params)
       (cons :result-type  (cond ((member "output" result-params) 'output)
 				((member "value" result-params) 'value)
-				(t 'value))))
+				(t 'value)))
+      )
      (cl-remove-if
-      (lambda (x) (memq (car x) '(:colname-names :rowname-names :result-params
-					         :result-type :var)))
-      params))))
+      (lambda (x) (memq (car x) '(:colname-names :rowname-names :tangle-params
+                             :result-params :result-type
+                             :var)))
+       params))))
 
 ;; row and column names
 (defun org-babel-del-hlines (table)
diff --git a/lisp/ob-exp.el b/lisp/ob-exp.el
index 80eaeeb27..05ac01c39 100644
--- a/lisp/ob-exp.el
+++ b/lisp/ob-exp.el
@@ -448,7 +448,7 @@ replaced with its value."
                            ;; Special parameters that are not real header
                            ;; arguments.
                            (memq (car pair)
-                                 '( :result-params :result-type
+                                 '( :result-params :result-type :tangle-params
                                     ;; This is an obsolete parameter still
                                     ;; used in some tests.
                                     :flags))
diff --git a/testing/lisp/test-ob.el b/testing/lisp/test-ob.el
index c088af7c8..e0de5a3ad 100644
--- a/testing/lisp/test-ob.el
+++ b/testing/lisp/test-ob.el
@@ -2131,10 +2131,12 @@ default-directory
 				      (:rowname-names)
 				      (:result-params)
 				      (:result-type)
+                                      (:tangle-params)
 				      (:var . "\"foo\"")))
 	  '((:var)
 	    (:colname-names)
 	    (:rowname-names)
+            (:tangle-params)
 	    (:result-params)
 	    (:result-type . value)))))
 
-- 
2.41.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-testing-lisp-test-ob.el-New-tests-for-merge-params.patch --]
[-- Type: text/x-patch, Size: 6263 bytes --]

From 22cf61fcaf75d4fa3b65c71f05d2ea8d71adaca5 Mon Sep 17 00:00:00 2001
From: Mehmet Tekman <mtekman89@gmail.com>
Date: Fri, 3 May 2024 15:11:38 +0200
Subject: [PATCH 2/3] * testing/lisp/test-ob.el: New tests for merge-params

(test-ob/get-src-block-property):
(test-ob/merge-params): add new tests for the merge-params source
block header handling function.
---
 testing/lisp/test-ob.el | 143 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 143 insertions(+)

diff --git a/testing/lisp/test-ob.el b/testing/lisp/test-ob.el
index e0de5a3ad..604f5ac92 100644
--- a/testing/lisp/test-ob.el
+++ b/testing/lisp/test-ob.el
@@ -314,6 +314,149 @@ this is simple"
     (org-babel-next-src-block)
     (should (= 14 (org-babel-execute-src-block)))))
 
+(defun test-ob/get-src-block-property (properties)
+  "Get plist of PROPERTIES and values for the first src block in buffer.
+PROPERTIES is a list of property keywords or a single keyword."
+  (org-with-wide-buffer
+   (goto-char (point-min))
+   (org-babel-next-src-block)
+   (org-set-regexps-and-options)
+   (let ((all-props (nth 2 (org-babel-get-src-block-info))))
+     (if (listp properties)
+         (apply #'nconc (mapcar (lambda (p) (list p (cdr (assoc p all-props)))) properties))
+       (list properties (cdr (assoc properties all-props)))))))
+
+(ert-deftest test-ob/merge-params ()
+  "Test the output of merging multiple header parameters.  The
+expected output is given in the contents of the source code block
+in each test.  The desired test header parameters are given
+either as a symbol or a list in the `idtest-alist' variable.
+Multiple header parameters must be separated by a newline and
+exactly two spaces in the block contents."
+  (should ;; 1. inherit-document-header-args
+   (equal '(:tangle "/tmp/default_tangle.txt")
+          (org-test-with-temp-text
+              "\
+#+PROPERTY: header-args :tangle /tmp/default_tangle.txt
+* One
+#+begin_src conf
+#+end_src"
+            (test-ob/get-src-block-property :tangle))))
+  (should-not ;; 2. inherit-document-header-with-local-sync-action
+   ;; This should pass with newer merge function with multiple tangle parameters
+   (equal '(:tangle "/tmp/default_tangle.txt skip")
+          (org-test-with-temp-text
+              "\
+#+PROPERTY: header-args :tangle /tmp/default_tangle.txt
+* Two
+#+begin_src conf :tangle skip
+#+end_src"
+            (test-ob/get-src-block-property :tangle))))
+  (should ;; 3. override-document-header-with-local-tfile
+   (equal '(:tangle "randomfile sync")
+          (org-test-with-temp-text
+              "\
+#+PROPERTY: header-args :tangle /tmp/default_tangle.txt
+* Three
+#+begin_src conf :tangle randomfile sync
+#+end_src"
+            (test-ob/get-src-block-property :tangle))))
+  (should  ;; 4. override-document-and-parent-header-with-local-tfile-and-action
+   (equal '(:tangle "randomfile sync")
+          (org-test-with-temp-text
+              "\
+#+PROPERTY: header-args :tangle /tmp/default_tangle.txt
+* Four
+:PROPERTIES:
+:header-args: :tangle \"newfile.txt\" import
+:END:
+** A
+#+begin_src conf :tangle randomfile sync
+#+end_src"
+            (test-ob/get-src-block-property :tangle))))
+  (should ;; 5. test-tangle-and-default-results-param-together
+   (equal '(:tangle "randomfile" :results "replace")
+          (org-test-with-temp-text
+              "\
+* Five
+#+begin_src conf  :tangle randomfile
+#+end_src"
+            (test-ob/get-src-block-property '(:tangle :results)))))
+  (should-not  ;; 6. inherit-document-tfile-take-only-last-local-sync-action
+   ;; This should pass with newer merge function with multiple tangle parameters
+   (equal '(:tangle "/tmp/default_tangle.txt export")
+          (org-test-with-temp-text
+              "\
+#+PROPERTY: header-args :tangle /tmp/default_tangle.txt
+* Six
+#+begin_src conf  :tangle import export
+#+end_src"
+            (test-ob/get-src-block-property :tangle))))
+  (should-not  ;; 7. ignore-document-header-take-last-tfile-and-sync-action
+   ;; This should pass with newer merge function with multiple tangle parameters
+   (equal '(:tangle "fname2 export")
+          (org-test-with-temp-text
+              "\
+#+PROPERTY: header-args :tangle /tmp/default_tangle.txt
+* Seven
+#+begin_src conf  :tangle fname1 fname2 sync export
+#+end_src"
+            (test-ob/get-src-block-property :tangle))))
+  (should  ;; 8. test-results-and-exports
+   (equal '(:results "wrap file replace" :exports "code")
+          (org-test-with-temp-text
+              "\
+* Eight
+#+begin_src sh :results file wrap
+#+end_src"
+            (test-ob/get-src-block-property '(:results :exports)))))
+  (should  ;; 9. do-not-tangle-this-block --
+   (equal '(:tangle "no")
+          (org-test-with-temp-text
+              "\
+#+PROPERTY: header-args :tangle /tmp/default_tangle.txt
+* Nine
+#+begin_src conf :tangle no
+#+end_src"
+            (test-ob/get-src-block-property :tangle))))
+  (should  ;; 10. test-tangle-exports-and-comments
+   (equal '(:tangle "foo.txt" :exports "verbatim code" :comments "link")
+          (org-test-with-temp-text
+              "\
+#+PROPERTY: header-args :tangle /tmp/default_tangle.txt
+* Ten
+:PROPERTIES:
+:header-args: :tangle no :exports verbatim
+:END:
+#+begin_src conf :tangle \"foo.txt\" :comments link
+#+end_src"
+            (test-ob/get-src-block-property '(:tangle :exports :comments)))))
+  (should-not  ;; 11. override-document-and-heading-tfile-with-yes
+   ;; This should pass with newer merge function with multiple tangle parameters
+   (equal '(:tangle "foo.txt")
+          (org-test-with-temp-text
+              "\
+#+PROPERTY: header-args :tangle /tmp/default_tangle.txt
+* Eleven
+:PROPERTIES:
+:header-args: :tangle \"foo.txt\"
+:END:
+#+begin_src conf :tangle yes
+#+end_src"
+            (test-ob/get-src-block-property :tangle))))
+  (should  ;; 12. tangle-file-with-spaces
+   (equal '(:tangle "file with spaces.txt")
+          (org-test-with-temp-text
+              "\
+* Twelve
+:PROPERTIES:
+:header-args: :tangle \"foo.txt\"
+:END:
+** A
+#+begin_src conf :tangle \"file with spaces.txt\"
+#+end_src"
+            (test-ob/get-src-block-property :tangle)))))
+
 (ert-deftest test-ob/inline-src-blocks ()
   (should
    (= 1
-- 
2.41.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: floating patch, needs work --]
[-- Type: text/x-patch, Size: 2218 bytes --]

From 19d4689bb86ade87c0f6ef9338c33dee43a66cbe Mon Sep 17 00:00:00 2001
From: Mehmet Tekman <mtekman89@gmail.com>
Date: Sun, 5 May 2024 17:32:59 +0200
Subject: [PATCH 3/3] Floating commit, implementing tangle-params into tests.
 Needs work

---
 testing/lisp/test-ob.el | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/testing/lisp/test-ob.el b/testing/lisp/test-ob.el
index 604f5ac92..4ccb74d64 100644
--- a/testing/lisp/test-ob.el
+++ b/testing/lisp/test-ob.el
@@ -334,24 +334,29 @@ either as a symbol or a list in the `idtest-alist' variable.
 Multiple header parameters must be separated by a newline and
 exactly two spaces in the block contents."
   (should ;; 1. inherit-document-header-args
-   (equal '(:tangle "/tmp/default_tangle.txt")
+   (equal (cons '(:tangle "/tmp/default_tangle.txt")
+                ;;'(:tangle-params ("/tmp/default_tangle.txt" "export") ;; -- desired
+                '(:tangle-params ("" "export")))
           (org-test-with-temp-text
               "\
 #+PROPERTY: header-args :tangle /tmp/default_tangle.txt
 * One
-#+begin_src conf
+#+begin_src conf :tangle export
 #+end_src"
-            (test-ob/get-src-block-property :tangle))))
-  (should-not ;; 2. inherit-document-header-with-local-sync-action
+            (cons (test-ob/get-src-block-property :tangle)
+                  (test-ob/get-src-block-property :tangle-params)))))
+  (should ;; 2. inherit-document-header-with-local-sync-action
    ;; This should pass with newer merge function with multiple tangle parameters
-   (equal '(:tangle "/tmp/default_tangle.txt skip")
+   (equal (cons '(:tangle "no")
+                '(:tangle-params ("no" nil)))
           (org-test-with-temp-text
               "\
 #+PROPERTY: header-args :tangle /tmp/default_tangle.txt
 * Two
-#+begin_src conf :tangle skip
+#+begin_src conf :tangle no
 #+end_src"
-            (test-ob/get-src-block-property :tangle))))
+            (cons (test-ob/get-src-block-property :tangle)
+                  (test-ob/get-src-block-property :tangle-params)))))
   (should ;; 3. override-document-header-with-local-tfile
    (equal '(:tangle "randomfile sync")
           (org-test-with-temp-text
-- 
2.41.0


[-- Attachment #5: Type: text/plain, Size: 16 bytes --]



Best,
Mehmet