Re: file-mode source code block header argument

[Kyle Meyer <kyle@kyleam.com>]

John Herrlin writes:

> I am looking for a way to set permission on a file created from source
> code block result when :file header argument is used. I was looking for
> something like :tangle-mode but could not find anything. I wrote a patch
> that does just that and it works for my small use case.

Thanks for the patch.

> It's a header argument called :file-mode and can be used in the same
> way as :tangle-mode.
>
> Example usage:
>
> #+BEGIN_SRC shell :results file :file script.sh :file-mode (identity #o755)
>   echo "#!/bin/bash"
>   echo "echo Hello World"
> #+END_SRC
>
> Is this a suitable way of doing it?

Looks sensible to me.  Hopefully others with more knowledge and interest
in Babel will chime in if they disagree.  In the meantime, I mostly just
have convention nit-picks...

> Subject: [PATCH] ob-core: file-mode option in source code block arguments
>
> * ob-core.el (org-babel-execute-src-block): Source code block header
> argument :file-mode can set file permission if :file argument is provided

Please end this sentence with a period.

Also, assuming the listing at
https://orgmode.org/worg/org-contribute.html is up to date, this should
come with a TINYCHANGE cookie.

> ---
>  doc/org-manual.org      | 12 ++++++++++++
>  lisp/ob-core.el         |  5 ++++-
>  testing/lisp/test-ob.el | 17 +++++++++++++++++
>  3 files changed, 33 insertions(+), 1 deletion(-)

Please also add an ORG-NEWS entry.

> diff --git a/doc/org-manual.org b/doc/org-manual.org
> index b616446..2919139 100644
> --- a/doc/org-manual.org
> +++ b/doc/org-manual.org
> @@ -17440,6 +17440,18 @@ default behavior is to automatically determine the result type.
>    uses the generated file name for both the "link" and
>    "description" parts of the link.
>  
> +  #+cindex: @samp{file-mode}, header argument
> +  The =file-mode= header argument defines the file permission. For

s/permission/permissions/

Also, the convention for this project is two spaces following a
sentence.

> +  example, to make a read-only file, use ‘:file-mode (identity
> +  #o444)’. To make it executable, use ‘:file-mode (identity #o755)’

These ‘...’ should be =...=, I think.

I know this text is just following the text for :tangle-mode, but from
my point of view, "For example ..." through to the example block could
be dropped.  If the reader knows about file permissions, then it's
unnecessary; if they don't, I don't see how it'd be enough to help them
figure out what's going on.

> +  #+begin_example
> +  ,#+BEGIN_SRC shell :results file :file script.sh :file-mode (identity #o755)
> +    echo "#!/bin/bash"
> +    echo "echo Hello World"
> +  ,#+END_SRC
> +  #+end_example

> diff --git a/lisp/ob-core.el b/lisp/ob-core.el
> index e798595..cc3e002 100644
> --- a/lisp/ob-core.el
> +++ b/lisp/ob-core.el
> @@ -731,7 +731,10 @@ block."
>  		    (with-temp-file file
>  		      (insert (org-babel-format-result
>  			       result
> -			       (cdr (assq :sep params))))))
> +			       (cdr (assq :sep params)))))
> +		    ;; Set permissions if header argument `:file-mode' is provided

To continue my nit-picking: Please follow the style of the surrounding
comments, ending with a period and filling the paragraph.

> +		    (when (assq :file-mode params)
> +		      (set-file-modes file (cdr (assq :file-mode params)))))

Looks good.

>  		  (setq result file))
>  		;; Possibly perform post process provided its
>  		;; appropriate.  Dynamically bind "*this*" to the
> diff --git a/testing/lisp/test-ob.el b/testing/lisp/test-ob.el
> index 7c44622..c4aaad1 100644
> --- a/testing/lisp/test-ob.el
> +++ b/testing/lisp/test-ob.el
> @@ -1746,6 +1746,23 @@ line 1
>  		   (cdr (assq :file (nth 2 (org-babel-get-src-block-info t))))))
>      ))
>  
> +(ert-deftest test-ob/file-mode ()
> +  "Ensure that file have correct permissions."
> +  (let* ((file       (org-babel-temp-file "file-mode-" ".sh"))
> +	 (filename   (file-name-nondirectory file))
> +	 (path       (file-name-directory file)))
> +    (org-test-with-temp-text
> +	(concat
> +	 "#+BEGIN_SRC emacs-lisp :results file "
> +	 ":file " filename " "
> +	 ":output-dir " path " "
> +	 ":file-mode (identity #o755)
> +     nil
> +     #+END_SRC")

Rather than using org-babel-temp-file to generate a temporary file, I
think the preferred way to do this would be to use
org-test-with-temp-text-in-file.

> +      (org-babel-execute-src-block))
> +    (should (equal (file-modes file)
> +		   493))))

To my eyes, spelling this as #o755 would be clearer.

So, perhaps something like this:

(ert-deftest test-ob/file-mode ()
  "Ensure that file have correct permissions."
  (should
   (equal #o755
          (org-test-with-temp-text-in-file "
#+begin_src emacs-lisp :results file :file t.sh :file-mode (identity #o755)
nil
#+end_src"
            (org-babel-next-src-block)
            (org-babel-execute-src-block)
            (unwind-protect
                (file-modes "t.sh")
              (delete-file "t.sh"))))))