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

[Mehmet Tekman]

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!)

0001-lisp-ob-core.el-org-babel-split-str-quoted-org-babel.patch
Description: Text Data

0002-testing-lisp-test-ob.el-New-tests-for-merge-params.patch
Description: Text Data

>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


Best,
Mehmet