Re: ob-shell intentions and paperwork (was Bash results broken?)

[Ihor Radchenko]

Max Nikulin  writes:

> And changes made by this commit are included into diff shown for the 
> merge commit 4f319088ba by cgit. E.g. gitk for local repository does not 
> show any changes for the merge commit.
>
> So Matt did not squashed commits before committing to the main branch 
> and detailed commit messages are preserved. That is why I do not 
> consider cgit render as a strong enough reason for reverting.

Should we then report a bug to cgit mailing list?

> However I would prefer linear commit history when possible, so I suggest 
> to do the following in similar cases (not verified, may have typos)
>
> git fetch  # get latest commits
> git checkout ob-shell-cleanup-tests
> git rebase origin/main
> git checkout main
> git pull
> git merge --ff-only ob-shell-cleanup-tests
>
> (Omit --ff-only if it is real merge because the fix is committed to the 
> bugfix branch.)
>
> It should help to avoid confusion and to make git archeology easier.

Sure. Using rebase for merging and pulling is generally a good idea.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at .
Support Org development at ,
or support my work at 

Re: ob-shell intentions and paperwork (was Bash results broken?)

[Max Nikulin]

On 13/01/2023 22:23, Ihor Radchenko wrote:

Matt writes:


Would you like me to correct how I've incorporated my changes?


No. I was referring to the initial situation with a single commit being
displayed.

I am not sure what Max was trying to point out.


Look at the commit message for
https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=4f319088ba5f11d4b6adf808f39f11dfa52c08e4


Merge branch 'ob-shell-cleanup-tests'


It suggests that this is a merge commit for a local branch. Notice that 
the commit has 2 parents:



committer   Matthew Trzcinski 2023-01-11 11:16:53 -0500
parent  07c63df9c7f40b17acb5c517baa0d321098f31da (diff)
parent  9ed2821053cf7b0c2f02fe941a54e1d3a9837aeb (diff)


Second one:

https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=9ed2821053cf7b0c2f02fe941a54e1d3a9837aeb

authorMatt Trzcinski 2022-12-30 13:03:59 -0500
committer Matt Trzcinski 2022-12-31 13:56:27 -0500

test-ob-shell.el: Organize tests

And changes made by this commit are included into diff shown for the 
merge commit 4f319088ba by cgit. E.g. gitk for local repository does not 
show any changes for the merge commit.


So Matt did not squashed commits before committing to the main branch 
and detailed commit messages are preserved. That is why I do not 
consider cgit render as a strong enough reason for reverting.


However I would prefer linear commit history when possible, so I suggest 
to do the following in similar cases (not verified, may have typos)


git fetch  # get latest commits
git checkout ob-shell-cleanup-tests
git rebase origin/main
git checkout main
git pull
git merge --ff-only ob-shell-cleanup-tests

(Omit --ff-only if it is real merge because the fix is committed to the 
bugfix branch.)


It should help to avoid confusion and to make git archeology easier.

Re: ob-shell intentions and paperwork (was Bash results broken?)

[Ihor Radchenko]

Matt  writes:

>   On Fri, 13 Jan 2023 04:36:40 -0500  Ihor Radchenko  wrote --- 
>  > Cgit displays our bugfix merges with all the required commits.
>  > So, what happened what not ideal either way.
>  
> Would you like me to correct how I've incorporated my changes?

No. I was referring to the initial situation with a single commit being
displayed.

I am not sure what Max was trying to point out.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at .
Support Org development at ,
or support my work at 

Re: ob-shell intentions and paperwork (was Bash results broken?)

[Matt]

  On Fri, 13 Jan 2023 04:36:40 -0500  Ihor Radchenko  wrote --- 
 > Cgit displays our bugfix merges with all the required commits.
 > So, what happened what not ideal either way.
 
Would you like me to correct how I've incorporated my changes?

Re: ob-shell intentions and paperwork (was Bash results broken?)

[Ihor Radchenko]

Max Nikulin  writes:

>> It looks like you lost all the individual commits and commit messages in
>> the process.
>
> Ihor, it is usual merge commit of a branch with multiple commits. Cgit 
> shows combined changes, but commits was not squashed. The branch started at
> dff9565c2c8bb7fe100c5278136d3de22ce41051 Bastien 2022-12-30 09:17:38 +0100

Cgit displays our bugfix merges with all the required commits.
So, what happened what not ideal either way.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at .
Support Org development at ,
or support my work at 

Re: ob-shell intentions and paperwork (was Bash results broken?)

[Max Nikulin]

On 12/01/2023 00:02, Ihor Radchenko wrote:

I was not, thank you.  I've since pushed.


This:
https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=4f319088ba5f11d4b6adf808f39f11dfa52c08e4
?

It looks like you lost all the individual commits and commit messages in
the process.


Ihor, it is usual merge commit of a branch with multiple commits. Cgit 
shows combined changes, but commits was not squashed. The branch started at

dff9565c2c8bb7fe100c5278136d3de22ce41051 Bastien 2022-12-30 09:17:38 +0100

It would be better to do
git fetch
git rebase origin/main
before merging the branch to make git history more linear. To ensure 
fast forward merge it is possible to add --ff-only option to git merge.

Re: ob-shell intentions and paperwork (was Bash results broken?)

[Ihor Radchenko]

Matt  writes:

>   On Wed, 11 Jan 2023 12:02:42 -0500  Ihor Radchenko  wrote --- 
>  > It looks like you lost all the individual commits and commit messages in
>  > the process.
>  > 
>  > Could you please revert 4f319088ba5 and re-push in such a way that
>  > individual commits do appear on main?
>
> Goodness!  Sorry!  Thanks for catching that.  I reverted the merge and then 
> applied the patches.

Thanks! Looks fine now.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at .
Support Org development at ,
or support my work at 

Re: ob-shell intentions and paperwork (was Bash results broken?)

[Matt]

  On Wed, 11 Jan 2023 12:02:42 -0500  Ihor Radchenko  wrote --- 
 > It looks like you lost all the individual commits and commit messages in
 > the process.
 > 
 > Could you please revert 4f319088ba5 and re-push in such a way that
 > individual commits do appear on main?

Goodness!  Sorry!  Thanks for catching that.  I reverted the merge and then 
applied the patches.

Re: ob-shell intentions and paperwork (was Bash results broken?)

[Ihor Radchenko]

Matt  writes:

> I was not, thank you.  I've since pushed.

This:
https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=4f319088ba5f11d4b6adf808f39f11dfa52c08e4
?

It looks like you lost all the individual commits and commit messages in
the process.

Could you please revert 4f319088ba5 and re-push in such a way that
individual commits do appear on main?

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at .
Support Org development at ,
or support my work at 

Re: ob-shell intentions and paperwork (was Bash results broken?)

[Matt]

  On Wed, 11 Jan 2023 06:53:41 -0500  Ihor Radchenko  wrote --- 
 > I have the following Git configuration:
 > 
 >  u remote.origin.url 
 > yanta...@git.savannah.gnu.org:/srv/git/emacs/org-mode.git
 > 
 > Are you using the same?

I was not, thank you.  I've since pushed.

Re: ob-shell intentions and paperwork (was Bash results broken?)

[Ihor Radchenko]

Matt  writes:

>   On Thu, 05 Jan 2023 06:21:16 -0500  Bastien Guerry  wrote --- 
>  > My bad: I did not warn Emacs maintainers in time.  Now it is done,
>  > I will let you know when they grand you access to the Emacs project.
>
> I got an email from Eli on Thursday saying I was added.  I've still been 
> getting an error when trying to push:
>
> fatal: remote error: access denied or repository not exported: 
> /emacs/org-mode.git

I have the following Git configuration:

 u remote.origin.url yanta...@git.savannah.gnu.org:/srv/git/emacs/org-mode.git

Are you using the same?

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at .
Support Org development at ,
or support my work at 

Re: ob-shell intentions and paperwork (was Bash results broken?)

[Matt]

  On Thu, 05 Jan 2023 06:21:16 -0500  Bastien Guerry  wrote --- 
 > My bad: I did not warn Emacs maintainers in time.  Now it is done,
 > I will let you know when they grand you access to the Emacs project.

I got an email from Eli on Thursday saying I was added.  I've still been 
getting an error when trying to push:

fatal: remote error: access denied or repository not exported: 
/emacs/org-mode.git

Re: ob-shell intentions and paperwork (was Bash results broken?)

[Bastien Guerry]

Ihor Radchenko  writes:

>> I'm not able to push to git://git.sv.gnu.org/emacs/org-mode.git.

My bad: I did not warn Emacs maintainers in time.  Now it is done,
I will let you know when they grand you access to the Emacs project.

-- 
 Bastien

Re: ob-shell intentions and paperwork (was Bash results broken?)

[Ihor Radchenko]

Matt  writes:

>  > Feel free to push upstream.
>
> I'm not able to push to git://git.sv.gnu.org/emacs/org-mode.git.
>
> I've read through the following and, as far as I can tell, I've followed the 
> directions.
>
> - https://orgmode.org/worg/org-contribute.html
> - https://orgmode.org/worg/org-maintenance.html
> - https://www.emacswiki.org/emacs/GitForEmacsDevs

Adding Bastien to the loop.

Meanwhile, can you check if you are listed as a member of "emacs" group
in ?

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at .
Support Org development at ,
or support my work at 

Re: Refactor org-babel-shell-initialize? (was Re: ob-shell intentions and paperwork (was Bash results broken?))

[Ihor Radchenko]

Ihor Radchenko  writes:

>> Is there a way to see the definition of`org-babel-execute:csh' using the 
>> current `org-babel-shell-initialize', that is, when generated by a function?
>
> https://github.com/Wilfred/helpful displays the function code in such
> scenario. Probably, I need to raise this problem on emacs-devel.

https://debbugs.gnu.org/cgi/bugreport.cgi?bug=60568

>> (alist-get "csh" org-babel-shell-set-prompt-commands nil nil 'equal)
>>
>> Then, it will return the prompt associated with "csh".
>
> Good point. Would you mind making a patch?

Fixed on bugfix.
https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=93339de71

>>
>> I'm curious your thoughts about removing the `:set' function from 
>> `org-babel-shell-names' and using `add-variable-watcher' instead.  In my 
>> explorations, the watcher gets called when using customize, as well as when 
>> a new shell is added to `org-babel-shell-names' using `add-to-list'.
>
> I have never seen using variable watcher for this purpose.
> I suggest asking on emacs-devel first to hear what they think of it.

https://yhetil.org/emacs-devel/87a62xiewa.fsf@localhost/T/#u

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at .
Support Org development at ,
or support my work at 

Re: ob-shell intentions and paperwork (was Bash results broken?)

[Matt]

  On Tue, 03 Jan 2023 05:50:17 -0500  Ihor Radchenko  wrote --- 
 > I was mostly worried about session states affecting subsequent test
 > invocations. But I do agree that it may be better to keep them.

That makes sense.  I tend to run tests one at a time unless I'm about to submit 
patches or push...

 > Feel free to push upstream.

I'm not able to push to git://git.sv.gnu.org/emacs/org-mode.git.

I've read through the following and, as far as I can tell, I've followed the 
directions.

- https://orgmode.org/worg/org-contribute.html
- https://orgmode.org/worg/org-maintenance.html
- https://www.emacswiki.org/emacs/GitForEmacsDevs

Re: ob-shell intentions and paperwork (was Bash results broken?)

[Ihor Radchenko]

Matt  writes:

>   On Mon, 02 Jan 2023 04:47:10 -0500  Ihor Radchenko  wrote --- 
>  > They will not be reliable when tests are executed interactively.
>  > If the `should' condition fails, `kill-buffer' will never be executed
>  > leaving dirty state, especially for sessions.
>
> From my perspective, that's the point and a good thing.  That "dirty state" 
> information may be crucial to understanding why the failure happened.

Fair point.

I was mostly worried about session states affecting subsequent test
invocations. But I do agree that it may be better to keep them.
Feel free to push upstream.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at .
Support Org development at ,
or support my work at 

Re: Refactor org-babel-shell-initialize? (was Re: ob-shell intentions and paperwork (was Bash results broken?))

[Ihor Radchenko]

Matt  writes:
>   On Sat, 31 Dec 2022 07:56:10 -0500  Ihor Radchenko  wrote --- 
>  > As for being a macro, there will be not much gain - the convention is
>  > mostly designed for things like `cl-defun' aimed to be used in the code.
>  > `org-babel-shell-initialize' is only used by `org-babel-shell-names'.
>
> I'm not sure what you mean by "to be used in code".  Do you mean called 
> within the global scope?

There are certain conventions about indentation of macros with "defun" in
them. They are automatically applied in emacs-lisp-mode.

Also, some heuristics for imenu looks for "defun" top-level forms, AFAIR.

In all these scenarios, it is assumed that "defun" macros are used in
the source code to define functions during compile time. Not on runtime.

> 1. The source is not explicit for a given `org-babel-execute:some-shell', 
> making it difficult to debug
>
> The benefit of using a macro is clarity of the defined functions.  Here's the 
> core `org-babel-shell-initialize' functionality as a macro.  Note that it 
> doesn't loop through `org-babel-shell-names'.
> ...
> You can expand to see the definitions:
>
> (pp-macroexpand-expression '(define-babel-shell-1 "csh"))
>
> Is there a way to see the definition of`org-babel-execute:csh' using the 
> current `org-babel-shell-initialize', that is, when generated by a function?

https://github.com/Wilfred/helpful displays the function code in such
scenario. Probably, I need to raise this problem on emacs-devel.

> Looking at the expansion, I see what appears to be an error:
>
> (alist-get "csh" org-babel-shell-set-prompt-commands)
>
> `org-babel-shell-set-prompt-commands' is an alist keyed by string shell names 
> and whose values are shell commands to set the prompt.  `alist-get' uses `eq' 
> by default which always returns nil for string comparisons.  That is, 
> (alist-get "csh" org-babel-shell-set-prompt-commands) returns nil, not 
> because the corresponding alist value is nil.  Rather, because the (eq "csh" 
> "csh") comparison evaluates as nil.  The TESTFN probably should be `equal' or 
> `string=':
>
> (alist-get "csh" org-babel-shell-set-prompt-commands nil nil 'equal)
>
> Then, it will return the prompt associated with "csh".

Good point. Would you mind making a patch?

> 2. Source navigation gets confused when looking up help for a generated 
> function.  For example, `C-h f org-babel-execute:sh' goes to the top of 
> ob-shell.el

This is indeed to be expected.

> Generating the execute functions with a macro also has this problem.  I'm not 
> sure there's a way around it other than defining each function with `defun'.  
> Doing that would be a bad idea because of the massive code 
> duplication/maintenance it would create.

Yup.

> 3. It does not adhere to the coding convention.
>
> I'll requote the convention here for convenience.
>
>> (elisp) Coding Conventions says,
>>
>> "• Constructs that define a function or variable should be macros, not
>> functions, and their names should start with ‘define-’. The macro
>> should receive the name to be defined as the first argument. That
>> will help various tools find the definition automatically. Avoid
>> constructing the names in the macro itself, since that would
>> confuse these tools."
>
> It's not clear to me why the convention exists, beyond consistency (a valid 
> reason).  I suspected it was to make the code clearer (points 1) and to "help 
> various tools find the definition automatically" (point 2).  
>
> I'm biased by my experience into thinking a macro actually addresses point 1. 
>  I could be wrong about it, though. It could just have been luck and personal 
> preference, and I may be overlooking some hidden complexity, a common problem 
> with macros.  Is there anything you see that I might be overlooking?

Nothing substantial, AFAIK.

> AFAICT, a macro doesn't help with finding the definition of the generated 
> function.  Any idea what tools it's talking about?

See above.

> Also, the way I defined `define-babel-shell-1' doesn't fit the convention.  
> Something like this would:
>
> (defmacro define-babel-execute-shell-2 (name)
>   "Define functions and variables needed by Org Babel to execute a shell.
>
> NAME is a symbol of the form `org-babel-execute:my-shell'."
>   (declare (indent nil) (debug t))
>   (let ((shell-program (cadr (split-string (symbol-name name) ":"

Symbol argument will create awkward back-and-forth conversion between
string and a symbol here.

> 4. Except for using the customize interface, changing `org-babel-shell-names' 
> requires reevaluating the function generator (`org-babel-shell-initialize' or 
> some variant of `define-babel-shell-1' )
>
> A macro would not solve the need to re-evaluate the function generation when 
> a change is made to `org-babel-shell-names'.  Either way, we need to loop/map 
> over the list of shells.
>
> I'm curious your thoughts about removing the `:set' function from 
> `org-babel-shell-names' and using 

Re: ob-shell intentions and paperwork (was Bash results broken?)

[Matt]

  On Mon, 02 Jan 2023 04:47:10 -0500  Ihor Radchenko  wrote --- 
 > They will not be reliable when tests are executed interactively.
 > If the `should' condition fails, `kill-buffer' will never be executed
 > leaving dirty state, especially for sessions.

>From my perspective, that's the point and a good thing.  That "dirty state" 
>information may be crucial to understanding why the failure happened.

Can you explain why you would want a failed test to remove failure related 
information?

Re: ob-shell intentions and paperwork (was Bash results broken?)

[Ihor Radchenko]

Matt  writes:

> If this set of patches look good, I can push them to main.

Just one more comment.
You are using constructs like

(if (should ...)
 (kill-buffer ...))

They will not be reliable when tests are executed interactively.
If the `should' condition fails, `kill-buffer' will never be executed
leaving dirty state, especially for sessions.

You can instead use

(unwind-protect
 (should ...)
 (kill-buffer ...))

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at .
Support Org development at ,
or support my work at 

Refactor org-babel-shell-initialize? (was Re: ob-shell intentions and paperwork (was Bash results broken?))

[Matt]

  On Sat, 31 Dec 2022 07:56:10 -0500  Ihor Radchenko  wrote --- 
 > As for being a macro, there will be not much gain - the convention is
 > mostly designed for things like `cl-defun' aimed to be used in the code.
 > `org-babel-shell-initialize' is only used by `org-babel-shell-names'.

I'm not sure what you mean by "to be used in code".  Do you mean called within 
the global scope?

 > I do not have objections if it were a macro though. (But I do not see
 > how it would help debugging).
 >
 > > Because `org-babel-shell-initialize' is a function factory, you can't 
 > > easily examine or modify their definitions.  `C-h f org-babel-execute:sh' 
 > > jumps to the top of lisp/ob-shell.el.  Changing the definition requires 
 > > reevaluating the definition for all the execute functions (or first 
 > > changing `org-babel-shell-names').
 > 
 > This is indeed a downside. Any better ideas?
 > ob-core dictates that we must have org-babel-execute:lang functions to
 > make things work.

My apologies, I feel there are too many separate issues I've brought up.  Since 
I've already brought them up, let me try to be more clear about them.  

I observe four issues with the current form of `org-babel-shell-initialize':

1. The source is not explicit for a given `org-babel-execute:some-shell', 
making it difficult to debug
2. Source navigation gets confused when looking up help for a generated 
function.  For example, `C-h f org-babel-execute:sh' goes to the top of 
ob-shell.el
3. It does not adhere to the coding convention
4. Except for using the customize interface, changing `org-babel-shell-names' 
requires reevaluating the function generator, currently 
`org-babel-shell-initialize'

Here's my perspective on each point.

1. The source is not explicit for a given `org-babel-execute:some-shell', 
making it difficult to debug

The benefit of using a macro is clarity of the defined functions.  Here's the 
core `org-babel-shell-initialize' functionality as a macro.  Note that it 
doesn't loop through `org-babel-shell-names'.

(defmacro define-babel-shell-1 (shell-name)
  (declare (indent nil) (debug t))
  (let ((function-name (intern (concat "my-org-babel-execute:" shell-name
`(progn
   (defun ,function-name (body params)
 ,(format "Execute a block of %s commands with Babel." shell-name)
 (let ((shell-file-name ,shell-name)
   (org-babel-prompt-command
(or (alist-get ,shell-name org-babel-shell-set-prompt-commands)
(alist-get t org-babel-shell-set-prompt-commands
   (org-babel-execute:shell body params)))
   (defalias
 ',(intern (concat "org-babel-variable-assignments:" shell-name))
 'org-babel-variable-assignments:shell
 ,(format "Return list of %s statements assigning to the block's 
variables." shell-name))
   (defvar ,(intern (concat "org-babel-default-header-args:" shell-name)) 
nil

You can expand to see the definitions:

(pp-macroexpand-expression '(define-babel-shell-1 "csh"))

Is there a way to see the definition of`org-babel-execute:csh' using the 
current `org-babel-shell-initialize', that is, when generated by a function?

Looking at the expansion, I see what appears to be an error:

(alist-get "csh" org-babel-shell-set-prompt-commands)

`org-babel-shell-set-prompt-commands' is an alist keyed by string shell names 
and whose values are shell commands to set the prompt.  `alist-get' uses `eq' 
by default which always returns nil for string comparisons.  That is, 
(alist-get "csh" org-babel-shell-set-prompt-commands) returns nil, not because 
the corresponding alist value is nil.  Rather, because the (eq "csh" "csh") 
comparison evaluates as nil.  The TESTFN probably should be `equal' or 
`string=':

(alist-get "csh" org-babel-shell-set-prompt-commands nil nil 'equal)

Then, it will return the prompt associated with "csh".

All this is visible from the function version of `org-babel-shell-initialize', 
of course.  However, it requires mentally tracking that ,name resolves to a 
string.  Using a macro along with `pp-macroexpand-expression' makes the defined 
function explicit.  The forms generated by the macro expansion are readily 
available to eval whereas the function version makes the definitions 
inaccessible (AFAICT).  

2. Source navigation gets confused when looking up help for a generated 
function.  For example, `C-h f org-babel-execute:sh' goes to the top of 
ob-shell.el

Generating the execute functions with a macro also has this problem.  I'm not 
sure there's a way around it other than defining each function with `defun'.  
Doing that would be a bad idea because of the massive code 
duplication/maintenance it would create.

3. It does not adhere to the coding convention.

I'll requote the convention here for convenience.

> (elisp) Coding Conventions says,
>
> "• Constructs that define a function or variable should be macros, not
> functions, and their names should start 

Re: ob-shell intentions and paperwork (was Bash results broken?)

[Matt]

  On Sat, 31 Dec 2022 09:31:16 -0500  Ihor Radchenko  wrote --- 
 > Matt m...@excalamus.com> writes:
 > 
 > > I've backed out the `require' change and adjusted everything else based on 
 > > your feedback.  There is a separate patch for each refactor that created a 
 > > new test.   The remaining refactors are in a single patch.  I was also 
 > > able to resolve the issue I had with inserting the test name for the 
 > > session (the "yes" string in `test-ob-shell/session').
 > 
 > Thanks!
 > 
 > I can see that you still prefer to use `string-join' for Org body.
 > Why not a bare string?

Because I often program in Python where double quoted strings typically don't 
span lines.  One of the original tests had the string "#+BEGIN_SRC sh :results 
list\necho 1\necho 2\necho 3\n#+END_SRC" in it (that is, used newline 
characters instead of inserting new lines), so my brain assumed strings Emacs 
Lisp behaved similarly to Python.  Silly brain :)  Thanks for continuing to 
follow up on that.

I've updated the patches to use bare strings.

If this set of patches look good, I can push them to main.

0001-test-ob-shell.el-Split-test-ob-shell-dont-error-on-e.patch
Description: Binary data


0002-test-ob-shell.el-Refactor-test-ob-shell-session.patch
Description: Binary data


0003-test-ob-shell.el-Refactor-ob-shell-generic-uses-no-a.patch
Description: Binary data


0004-Refactor-ob-shell-bash-uses-assoc-arrays.patch
Description: Binary data


0005-test-ob-shell.el-Refactor-test-names-and-kill-test-b.patch
Description: Binary data


0006-ob-shell-test.org-Remove-ob-shell-test.org.patch
Description: Binary data


0007-test-ob-shell.el-Organize-tests.patch
Description: Binary data

Re: ob-shell intentions and paperwork (was Bash results broken?)

[Ihor Radchenko]

Matt  writes:

> I've backed out the `require' change and adjusted everything else based on 
> your feedback.  There is a separate patch for each refactor that created a 
> new test.   The remaining refactors are in a single patch.  I was also able 
> to resolve the issue I had with inserting the test name for the session (the 
> "yes" string in `test-ob-shell/session').  The issue with 
> `org-babel-shell-initialize' still stands but is outside the scope of these 
> patches and isn't dealt with here.

Thanks!

I can see that you still prefer to use `string-join' for Org body.
Why not a bare string? There is no particular problem with
`string-join', but the rest of the tests are all using direct string
when possible - your patch makes ob-shell stand out of common style.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at .
Support Org development at ,
or support my work at 

Re: ob-shell intentions and paperwork (was Bash results broken?)

[Ihor Radchenko]

Matt  writes:

>  > > +;; TODO refactor into macro.  Currently violates (elisp) Coding
>  > > +;; Conventions and is hard to debug.
>  > >  (defun org-babel-shell-initialize ()
>  > >"Define execution functions associated to shell names.
>  > 
>  > Could you please elaborate? Which particular convention does it violate?
>  > What is hard to debug?
>
> (elisp) Coding Conventions says,
>
> "• Constructs that define a function or variable should be macros, not
>  functions, and their names should start with ‘define-’.  The macro
>  should receive the name to be defined as the first argument.  That
>  will help various tools find the definition automatically.  Avoid
>  constructing the names in the macro itself, since that would
>  confuse these tools."
>
> The `org-babel-shell-initialize' function defines *all* the 
> `org-babel-execute:XXX' functions given by `org-babel-shell-names' (sh, bash, 
> zsh, etc.).

I agree that `org-babel-shell-initialize' could use a better name.

As for being a macro, there will be not much gain - the convention is
mostly designed for things like `cl-defun' aimed to be used in the code.
`org-babel-shell-initialize' is only used by `org-babel-shell-names'.

I do not have objections if it were a macro though. (But I do not see
how it would help debugging).

> Because `org-babel-shell-initialize' is a function factory, you can't easily 
> examine or modify their definitions.  `C-h f org-babel-execute:sh' jumps to 
> the top of lisp/ob-shell.el.  Changing the definition requires reevaluating 
> the definition for all the execute functions (or first changing 
> `org-babel-shell-names').

This is indeed a downside. Any better ideas?
ob-core dictates that we must have org-babel-execute:lang functions to
make things work.

> This was a problem for me when I wanted to make the session name string for 
> `test-ob-shell/session' the test name (mentioned above).  In the test, when I 
> replaced the session name string with a variable containing the string, 
> `org-babel-execute:sh' failed with a type error.  I couldn't get the variable 
> to evaluate (with backquote and comma or otherwise).  Without an explicit 
> function definition or a macro to expand, I found it hard to debug/experiment 
> with (and so left the test name as a hard coded string).

Could you please explain a bit more about the problem? I do not see how
macro would help in this situation.

> I probably don't need it and am happy to remove it.An older version of 
> the function was more complex and made sense as a separate function (or so I 
> thought).  My aim was to make the test strings easy to read so that it's 
> clearer what's being tested (i.e. not write multi-line strings on a single 
> line).  I could use concat and add "\n" to the end of each line.  Or, simply 
> write out the string-join.  Maybe there's another way to write multi-line 
> strings that I'm not aware of in Elisp, maybe something like Python's 
> triple-quote?

We write multi-line docstrings all the time without extra macros. I
recommend using paredit or similar packages to auto-escape things that
need to be escaped.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at .
Support Org development at ,
or support my work at 

Re: ob-shell intentions and paperwork (was Bash results broken?)

[Matt]

  On Fri, 30 Dec 2022 00:34:38 -0500  Matt  wrote --- 
 > 
 >   On Thu, 29 Dec 2022 06:08:59 -0500  Ihor Radchenko  wrote --- 

 >  > > From: Matt Trzcinski m...@excalamus.com>
 >  > > +(require 'org-test (expand-file-name "../org-test.el"))
 >  > 
 >  > I am unsure here. What will happen if you run this file from
 >  > default-directory not the same with file location?

I was mistaken.  Loading a new Emacs instance and running `(load 
"/path/to/test-ob-shell.el")' fails with file-missing error.  I'm content to 
leave that alone for now.

I've backed out the `require' change and adjusted everything else based on your 
feedback.  There is a separate patch for each refactor that created a new test. 
  The remaining refactors are in a single patch.  I was also able to resolve 
the issue I had with inserting the test name for the session (the "yes" string 
in `test-ob-shell/session').  The issue with `org-babel-shell-initialize' still 
stands but is outside the scope of these patches and isn't dealt with here.



0001-test-ob-shell.el-Split-test-ob-shell-dont-error-on-e.patch
Description: Binary data


0002-test-ob-shell.el-Refactor-test-ob-shell-session.patch
Description: Binary data


0003-test-ob-shell.el-Refactor-ob-shell-generic-uses-no-a.patch
Description: Binary data


0004-Refactor-ob-shell-bash-uses-assoc-arrays.patch
Description: Binary data


0005-test-ob-shell.el-Refactor-test-names-and-kill-test-b.patch
Description: Binary data


0006-ob-shell-test.org-Remove-ob-shell-test.org.patch
Description: Binary data


0007-test-ob-shell.el-Organize-tests.patch
Description: Binary data

Re: ob-shell intentions and paperwork (was Bash results broken?)

[Bastien Guerry]

Hi Matthew,

Matt  writes:

>  You're correct, I've not contributed to core.  I would love to
> maintain lisp/ob-shell.el.

Your wish has been granted:
https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=e8ceb4a2

> I'm expecting life changes in the coming
> months and can't anticipate how that will affect my time.  Would it be
> a problem if I need to step down as maintainer for a period?

It is no problem at all.

You don't even need to step down as maintainer: if requests against
ob-shell.el are not *that* pressing during the time you're not fully
available, users and maintainers can just be patient.

I've warned Emacs maintainers and they'll give you write access to
the Org repository.

Thanks!

-- 
 Bastien

Re: ob-shell intentions and paperwork (was Bash results broken?)

[Matt]

  On Thu, 29 Dec 2022 06:08:59 -0500  Ihor Radchenko  wrote --- 
 
 > Does it mean that you are willing to maintain lisp/ob-shell.el?
 > We usually give write access to the maintainers and regular
 > contributors. AFAIR, you previously contributed to WORG but not to Org
 > core.

 You're correct, I've not contributed to core.  I would love to maintain 
lisp/ob-shell.el.  I'm expecting life changes in the coming months and can't 
anticipate how that will affect my time.  Would it be a problem if I need to 
step down as maintainer for a period?

 > > From: Matt Trzcinski m...@excalamus.com>
 > > 
 > > [fn:1] Session name is a string and not a variable (such as
 > > `session-name').  This is because `org-babel-execute:sh' produces a
 > > type error when the session name is defined with a variable.  The
 > > source of the error appears to be the `params' symbol in
 > > `org-babel-execute:sh'.  `params' does not evaluate as a variable as
 > > expected–it evaluates as a symbol.  However, `org-babel-execute:sh' is
 > > defined within a function factory which makes it difficult to debug.
 > > Hard code the test name for now and refactor it later once
 > > `org-babel-execute:sh` is refactored.
 > 
 > There is nothing wrong here. `org-babel-execute-src-block' takes care
 > about parameter processing making sure that :session value is always a
 > string.
 
Originally the test used "yes" for a comparison string and the shell it ran in. 
  I changed "yes" to be the test name because when it was "yes", I didn't know 
what test had produced it.  However, the problem with the test name being hard 
coded as a string is that if the function name changes, the test string may get 
out of sync.  More on this below.

 > > +;; TODO refactor into macro.  Currently violates (elisp) Coding
 > > +;; Conventions and is hard to debug.
 > >  (defun org-babel-shell-initialize ()
 > >"Define execution functions associated to shell names.
 > 
 > Could you please elaborate? Which particular convention does it violate?
 > What is hard to debug?

(elisp) Coding Conventions says,

"• Constructs that define a function or variable should be macros, not
 functions, and their names should start with ‘define-’.  The macro
 should receive the name to be defined as the first argument.  That
 will help various tools find the definition automatically.  Avoid
 constructing the names in the macro itself, since that would
 confuse these tools."

The `org-babel-shell-initialize' function defines *all* the 
`org-babel-execute:XXX' functions given by `org-babel-shell-names' (sh, bash, 
zsh, etc.).

Because `org-babel-shell-initialize' is a function factory, you can't easily 
examine or modify their definitions.  `C-h f org-babel-execute:sh' jumps to the 
top of lisp/ob-shell.el.  Changing the definition requires reevaluating the 
definition for all the execute functions (or first changing 
`org-babel-shell-names').

This was a problem for me when I wanted to make the session name string for 
`test-ob-shell/session' the test name (mentioned above).  In the test, when I 
replaced the session name string with a variable containing the string, 
`org-babel-execute:sh' failed with a type error.  I couldn't get the variable 
to evaluate (with backquote and comma or otherwise).  Without an explicit 
function definition or a macro to expand, I found it hard to debug/experiment 
with (and so left the test name as a hard coded string).

 > >  (ert-deftest test-ob-shell/dont-insert-spaces-on-expanded-bodies ()
 > >"Expanded shell bodies should not start with a blank line unless
 > > -the body of the tangled block does."
 > > +the strings of the tangled block does."
 > 
 > What does "strings of the tangled block" refer to? The previous variant
 > is a lot more clear for me.

I believe this is a query-replace error. Good catch!

 > > +(ert-deftest test-ob-shell/generic-uses-no-arrays ()
 > > +  "Test generic serialization of array into a single string."
 > > +  (org-test-with-temp-text
 > > +  (test-ob-shell-multiline-string
 > > +   "#+NAME: sample_array"
 > > +   "| one   |"
 > > +   "| two   |"
 > > +   "| three |"
 > 
 > Why do you need `test-ob-shell-multiline-string' here?
 > Can simply type-in the string directly, as the rest of tests do.

I probably don't need it and am happy to remove it.An older version of the 
function was more complex and made sense as a separate function (or so I 
thought).  My aim was to make the test strings easy to read so that it's 
clearer what's being tested (i.e. not write multi-line strings on a single 
line).  I could use concat and add "\n" to the end of each line.  Or, simply 
write out the string-join.  Maybe there's another way to write multi-line 
strings that I'm not aware of in Elisp, maybe something like Python's 
triple-quote?

 > > +(require 'org-test (expand-file-name "../org-test.el"))
 > 
 > I am unsure here. What will happen if you run this file from
 > 

Re: ob-shell intentions and paperwork (was Bash results broken?)

[Bastien Guerry]

Ihor Radchenko  writes:

> Bastien, could you please check Matt's copyright paperwork record in
> FSF?

Matt's copyright paperwork are OK, I added him as FSF-copyrighted
contributor on Worg.

> Does it mean that you are willing to maintain lisp/ob-shell.el?

Until Matt wants to be the maintainer for ob-shell.el, I suggest Ihor
applies the patches.

> In order for you to get admitted to Emacs group, we will first need to
> reach out to Emacs maintainers authorizing your write access.

Matt, you reached out to the Emacs maintainers and they asked me
whether I should give you write access: I told them to wait until you
confirm you want to the be the ob-shell.el maintainer.  If you don't,
I think sharing patches that another Org maintainer applies is enough.

Thanks!

-- 
 Bastien

Re: ob-shell intentions and paperwork (was Bash results broken?)

[Ihor Radchenko]

Bastien, could you please check Matt's copyright paperwork record in
FSF?

Matt  writes:

>   On Wed, 21 Dec 2022 01:17:50 -0500  Matt  wrote --- 
>
>  > Currently, though, I'm refactoring the ob-shell tests to remove dependency 
> on ob-shell-test.org and to stop the suite from littering. 
>
> Done.  Branched off bugfix, 12e10eb0d, and refactored test-ob-shell.el.  See 
> attached patches.

Better use main for development.
bugfix is for... bugfixes :)

>  >  After that, I intend to incorporate the worg examples as tests.
>
> This is probably a good place for me to pause and get feedback before writing 
> more code. 

Sure. See below.

> I just got the signed paperwork back from Craig and am waiting to be admitted 
> to the Emacs group on Savannah.

Does it mean that you are willing to maintain lisp/ob-shell.el?
We usually give write access to the maintainers and regular
contributors. AFAIR, you previously contributed to WORG but not to Org
core.

In order for you to get admitted to Emacs group, we will first need to
reach out to Emacs maintainers authorizing your write access.

Meanwhile, once Bastien confirms your copyright status, I can apply
patches on your behalf.

> From ecdb71afa00ca137b4faa737393cb027907a7f9f Mon Sep 17 00:00:00 2001
> From: Matt Trzcinski 
> Date: Tue, 20 Dec 2022 13:51:26 -0500
> Subject: [PATCH 01/20] test-ob-shell.el: Remove mixed tabs and spaces
>
> * test-ob-shell.el: Convert tabs to spaces.
>
> Change made under the premise that one or the other is better than
> both.

We generally avoid whitespace-only changes. They tend to generate merge
conflicts. We do whitespace changes alongside with other changes in the
code only - this minimizes merge conflicts.
See https://orgmode.org/list/874jvkz5k8@bzg.fr

>  (ert-deftest test-ob-shell/dont-error-on-empty-results ()
> -  "Was throwing an elisp error when shell blocks threw errors and
> -returned empty results."
> -  (should (null (org-babel-execute:sh "ls NoSuchFileOrDirectory.txt" nil
> +  (should (null (org-babel-execute:sh nil nil

nil BODY argument for `org-babel-execute:sh' is not supported.
Better use explicit empty body: "".

It is also a good idea to add docstrings to the tests for completeness.

> [fn:1] Session name is a string and not a variable (such as
> `session-name').  This is because `org-babel-execute:sh' produces a
> type error when the session name is defined with a variable.  The
> source of the error appears to be the `params' symbol in
> `org-babel-execute:sh'.  `params' does not evaluate as a variable as
> expected–it evaluates as a symbol.  However, `org-babel-execute:sh' is
> defined within a function factory which makes it difficult to debug.
> Hard code the test name for now and refactor it later once
> `org-babel-execute:sh` is refactored.

There is nothing wrong here. `org-babel-execute-src-block' takes care
about parameter processing making sure that :session value is always a
string.

> +;; TODO refactor into macro.  Currently violates (elisp) Coding
> +;; Conventions and is hard to debug.
>  (defun org-babel-shell-initialize ()
>"Define execution functions associated to shell names.

Could you please elaborate? Which particular convention does it violate?
What is hard to debug?

>  (ert-deftest test-ob-shell/dont-insert-spaces-on-expanded-bodies ()
>"Expanded shell bodies should not start with a blank line unless
> -the body of the tangled block does."
> +the strings of the tangled block does."

What does "strings of the tangled block" refer to? The previous variant
is a lot more clear for me.

> +(ert-deftest test-ob-shell/generic-uses-no-arrays ()
> +  "Test generic serialization of array into a single string."
> +  (org-test-with-temp-text
> +  (test-ob-shell-multiline-string
> +   "#+NAME: sample_array"
> +   "| one   |"
> +   "| two   |"
> +   "| three |"

Why do you need `test-ob-shell-multiline-string' here?
Can simply type-in the string directly, as the rest of tests do.

> +(require 'org-test (expand-file-name "../org-test.el"))

I am unsure here. What will happen if you run this file from
default-directory not the same with file location?

Also, the repetitive patches changing names + killing error buffer for
individual tests can be merged into a single patch.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at .
Support Org development at ,
or support my work at 

Re: ob-shell intentions and paperwork (was Bash results broken?)

[Matt]

  On Wed, 21 Dec 2022 01:17:50 -0500  Matt  wrote --- 

 > Currently, though, I'm refactoring the ob-shell tests to remove dependency 
 > on ob-shell-test.org and to stop the suite from littering. 

Done.  Branched off bugfix, 12e10eb0d, and refactored test-ob-shell.el.  See 
attached patches.

The main changes spanning the patches are:

- tests are now standalone; they don't rely on ob-shell-test.org
- tests now share a common prefix, "test-ob-shell".  The entire suite runs with 
(ert "test-ob-shell")
- buffers and processes created during a test are killed when the test passes

 >  After that, I intend to incorporate the worg examples as tests.

This is probably a good place for me to pause and get feedback before writing 
more code. 

I just got the signed paperwork back from Craig and am waiting to be admitted 
to the Emacs group on Savannah.

0001-test-ob-shell.el-Remove-mixed-tabs-and-spaces.patch
Description: Binary data


0002-test-ob-shell.el-Split-cases-into-two-tests.patch
Description: Binary data


0003-test-ob-shell.el-Refactor-test-ob-shell-session.patch
Description: Binary data


0004-test-ob-shell.el-Refactor-ob-shell-generic-uses-no-a.patch
Description: Binary data


0005-test-ob-shell.el-Refactor-ob-shell-bash-uses-arrays.patch
Description: Binary data


0006-test-ob-shell.el-Refactor-ob-shell-generic-uses-no-a.patch
Description: Binary data


0007-test-ob-shell.el-Refactor-ob-shell-bash-uses-assoc-a.patch
Description: Binary data


0008-test-ob-shell.el-Refactor-ob-shell-simple-list.patch
Description: Binary data


0009-test-ob-shell.el-Refactor-ob-shell-remote-with-stdin.patch
Description: Binary data


0010-test-ob-shell.el-Refactor-ob-shell-results-table.patch
Description: Binary data


0011-test-ob-shell.el-Refactor-ob-shell-results-list.patch
Description: Binary data


0012-test-ob-shell.el-Refactor-ob-shell-standard-output-a.patch
Description: Binary data


0013-test-ob-shell.el-Refactor-ob-shell-standard-output-a.patch
Description: Binary data


0014-test-ob-shell.el-Refactor-ob-shell-error-output-afte.patch
Description: Binary data


0015-test-ob-shell.el-Refactor-ob-shell-error-output-afte.patch
Description: Binary data


0016-test-ob-shell.el-Refactor-ob-shell-error-output-afte.patch
Description: Binary data


0017-test-ob-shell.el-Refactor-ob-shell-exit-code.patch
Description: Binary data


0018-test-ob-shell.el-Refactor-ob-shell-exit-code-multipl.patch
Description: Binary data


0019-ob-shell-test.org-Remove-ob-shell-test.org.patch
Description: Binary data


0020-test-ob-shell.el-Organize-tests.patch
Description: Binary data

ob-shell intentions and paperwork (was Bash results broken?)

[Matt]

  On Fri, 16 Dec 2022 12:41:45 -0500  Ihor Radchenko  wrote --- 
 > We really need more tests.

I'm working on giving ob-shell a little bit of love.  I wrote the worg 
documentation for it earlier this year.  I tried to include examples of all 
coded functionality, including previously undocumented features.  I think the 
examples would make for good tests since they document the present and expected 
behavior.  Currently, though, I'm refactoring the ob-shell tests to remove 
dependency on ob-shell-test.org and to stop the suite from littering.  After 
that, I intend to incorporate the worg examples as tests.  I'd like to adopt 
ob-shell, life permitting.  Sadly, I'm not confident I'm able to commit to that 
right now.  So, if I can get the FSF paperwork cleared, hopefully, I can make 
little contributions here and there.  

I've included patches of what I've been doing.   They're not ready for actual 
submission yet, both in quality and legality.  However, I'm including them for 
feedback, to make sure I'm not getting way off base.  Overall, I'd *really* 
like to implement async for ob-shell similar to ob-python.  However, I feel 
like it'd be more responsible and respectful to clean up the tests and code a 
little first before proposing such a big change (looking at you, 
org-babel-shell-initialize).

Can someone please assist me with the FSF paperwork off list?

0001-Remove-mixed-tabs-and-spaces.patch
Description: Binary data


0002-Split-test-ob-shell-dont-error-on-empty-results.patch
Description: Binary data


0003-Refactor-test-ob-shell-session.patch
Description: Binary data


0004-Refactor-ob-shell-generic-uses-no-arrays.patch
Description: Binary data


0005-Refactor-ob-shell-bash-uses-arrays.patch
Description: Binary data


0006-Refactor-ob-shell-generic-uses-no-assoc-arrays.patch
Description: Binary data


0007-Refactor-ob-shell-bash-uses-assoc-arrays.patch
Description: Binary data