[PATCH] lisp/ob-shell.el: Also override explicit-shell-file-name

[PATCH] lisp/ob-shell.el: Also override explicit-shell-file-name

[Aaron L. Zeng]

* lisp/ob-shell.el (org-babel-shell-initialize): Override
explicit-shell-file-name in addition to shell-file-name.

When a session with shell source blocks, execution calls `shell',
which checks `explicit-shell-file-name' variable before
`shell-file-name', to determine what shell to run.  If the user has
customized this variable to affect the behavior of M-x shell,
`org-babel-shell-initialize' should still run the shell specified by
the org source block's language name.

TINYCHANGE
---
 lisp/ob-shell.el | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lisp/ob-shell.el b/lisp/ob-shell.el
index 551c3785d..35d9e9376 100644
--- a/lisp/ob-shell.el
+++ b/lisp/ob-shell.el
@@ -81,7 +81,8 @@ is modified outside the Customize interface."
         (lambda (body params)
 	  (:documentation
            (format "Execute a block of %s commands with Babel." name))
-	  (let ((shell-file-name name))
+	  (let ((explicit-shell-file-name name)
+                (shell-file-name name))
 	    (org-babel-execute:shell body params))))
       (put fname 'definition-name 'org-babel-shell-initialize))
     (defalias (intern (concat "org-babel-variable-assignments:" name))
-- 
2.42.0

Re: [PATCH] lisp/ob-shell.el: Also override explicit-shell-file-name

[Matt]

 ---- On Mon, 11 Mar 2024 06:12:47 +0100  Aaron L. Zeng  wrote --- 
 > * lisp/ob-shell.el (org-babel-shell-initialize): Override
 > explicit-shell-file-name in addition to shell-file-name.
 > 
 > When a session with shell source blocks, execution calls `shell',
 > which checks `explicit-shell-file-name' variable before
 > `shell-file-name', to determine what shell to run.  If the user has
 > customized this variable to affect the behavior of M-x shell,
 > `org-babel-shell-initialize' should still run the shell specified by
 > the org source block's language name.
 > 
 > TINYCHANGE
 > ---
 >  lisp/ob-shell.el | 3 ++-
 >  1 file changed, 2 insertions(+), 1 deletion(-)
 > 
 > diff --git a/lisp/ob-shell.el b/lisp/ob-shell.el
 > index 551c3785d..35d9e9376 100644
 > --- a/lisp/ob-shell.el
 > +++ b/lisp/ob-shell.el
 > @@ -81,7 +81,8 @@ is modified outside the Customize interface."
 >          (lambda (body params)
 >        (:documentation
 >             (format "Execute a block of %s commands with Babel." name))
 > -      (let ((shell-file-name name))
 > +      (let ((explicit-shell-file-name name)
 > +                (shell-file-name name))
 >          (org-babel-execute:shell body params))))
 >        (put fname 'definition-name 'org-babel-shell-initialize))
 >      (defalias (intern (concat "org-babel-variable-assignments:" name))
 > -- 
 > 2.42.0

Thank you for your report and the patch. 

--
Matt Trzcinski
Emacs Org contributor (ob-shell)
Learn more about Org mode at https://orgmode.org
Support Org development at https://liberapay.com/org-mode

Re: [PATCH] lisp/ob-shell.el: Also override explicit-shell-file-name

[Ihor Radchenko]

Matt <matt@excalamus.com> writes:

> Thank you for your report and the patch. 

Matthew, you are free to apply patches relevant to ob-shell to savannah
repository, if you think that they are fine.

-- 
Ihor Radchenko // yantar92,
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>

Re: [PATCH] lisp/ob-shell.el: Also override explicit-shell-file-name

[Matt]

Finally had time to commit it. 

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

Nice catch and thanks for your contribution!

--
Matt Trzcinski
Emacs Org contributor (ob-shell)
Learn more about Org mode at https://orgmode.org
Support Org development at https://liberapay.com/org-mode

Re: [PATCH] lisp/ob-shell.el: Also override explicit-shell-file-name

[Aaron Zeng]

Thanks for the merge!

On Fri, Mar 15, 2024, at 17:38, Matt wrote:
> Finally had time to commit it. 
>
> https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=0e2a9524dc6da8b4d60672e85aba74076baac211
>
> Nice catch and thanks for your contribution!
>
> --
> Matt Trzcinski
> Emacs Org contributor (ob-shell)
> Learn more about Org mode at https://orgmode.org
> Support Org development at https://liberapay.com/org-mode

Re: [PATCH] lisp/ob-shell.el: Also override explicit-shell-file-name

[Ihor Radchenko]

Matt <matt@excalamus.com> writes:

> Finally had time to commit it. 
>
> https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=0e2a9524dc6da8b4d60672e85aba74076baac211

Thanks!

I notice that you changed the commit author and only referenced Aaron in
Submitted By: metadata.

For future, we prefer keeping the original commit author in the "author"
field of the commits - this is important to keep track of the number of
changed lines for contributors without FSF copyright assignment.

You may consider using https://git.kyleam.com/piem/about/ to apply
patches submitted as emails automatically. Or you can assign the commit
author manually, using -A git commit switch (magit also provides a
transient menu option for this).

-- 
Ihor Radchenko // yantar92,
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>

How to properly attribute authorship with Git (was Re: [PATCH] lisp/ob-shell.el: Also override explicit-shell-file-name)

[Matt]

 ---- On Sat, 16 Mar 2024 11:11:38 +0100  Ihor Radchenko  wrote --- 

 > I notice that you changed the commit author and only referenced Aaron in
 > Submitted By: metadata.
 > 
 > For future, we prefer keeping the original commit author in the "author"
 > field of the commits - this is important to keep track of the number of
 > changed lines for contributors without FSF copyright assignment.
 
Thank you for letting me know this is an issue.

First, would you like me to update the commit?  If so, I will need guidance.  The correct procedure to change the author after committing to remote is unclear to me.  I would think it's something like sync my local copy with the latest remote version, update the author locally, and force push the change.  I would then expect that the next time someone pulls, it would update their local with the author change.  It would, however, cause a conflict, I think, for someone in the middle of making a change who has not synced with the forced push version and is trying to push their change.

Second, I can update Worg with an explanation that it's important to credit authors using git's author field and how to do this.  Unless I missed it, worg/org-contribute makes no mention of the author field.  The version of git packaged by my distro is 2.41.0 and, AFAICT, has no -A flag for 'git' or 'git commit'.  However, the following works on my machine and, I guess, is the long option form:

    git commit --author "Arthur Override <arthur-override's-email>"

Third, this is at least the second time I've had issues working with a diff/patch.  The reason I submitted the change the way I did is that I could not get 'git apply <the-change>' to work.  I only got a useless error like "error: corrupt patch at line 10".  It's not clear to me if this is an error on my end or if the patch is indeed ill-formatted.  Can you confirm that the submitted patch is well-formatted?

--
Matt Trzcinski
Emacs Org contributor (ob-shell)
Learn more about Org mode at https://orgmode.org
Support Org development at https://liberapay.com/org-mode

Re: How to properly attribute authorship with Git (was Re: [PATCH] lisp/ob-shell.el: Also override explicit-shell-file-name)

[Ihor Radchenko]

Matt <matt@excalamus.com> writes:

>  > For future, we prefer keeping the original commit author in the "author"
>  > field of the commits - this is important to keep track of the number of
>  > changed lines for contributors without FSF copyright assignment.
>  
> Thank you for letting me know this is an issue.
>
> First, would you like me to update the commit?  If so, I will need guidance.  The correct procedure to change the author after committing to remote is unclear to me.  I would think it's something like sync my local copy with the latest remote version, update the author locally, and force push the change.  I would then expect that the next time someone pulls, it would update their local with the author change.  It would, however, cause a conflict, I think, for someone in the middle of making a change who has not synced with the forced push version and is trying to push their change.

We should avoid force pushing unless something is terribly broken.
What you may do instead is (1) revert the commit; (2) re-apply the
commit version with the correct author attribution.

> Second, I can update Worg with an explanation that it's important to credit authors using git's author field and how to do this.  Unless I missed it, worg/org-contribute makes no mention of the author field.  The version of git packaged by my distro is 2.41.0 and, AFAICT, has no -A flag for 'git' or 'git commit'.  However, the following works on my machine and, I guess, is the long option form:
>
>     git commit --author "Arthur Override <arthur-override's-email>"

You are right. Looks like -A is just Magit shortcut.

As for crediting authors, we may document it in https://orgmode.org/worg/org-maintenance.html#copyright
Although, it is under "core maintainer" section. Maybe we can make a
dedicated section for maintainers on how to deal with patch submissions.

> Third, this is at least the second time I've had issues working with a diff/patch.  The reason I submitted the change the way I did is that I could not get 'git apply <the-change>' to work.  I only got a useless error like "error: corrupt patch at line 10".  It's not clear to me if this is an error on my end or if the patch is indeed ill-formatted.  Can you confirm that the submitted patch is well-formatted?

There are several types of patches that may need to be applied
differently. Plain "diff" patches can be applied using git apply, while
maildir/.patch patches can be applied using git am.

-- 
Ihor Radchenko // yantar92,
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>

Re: How to properly attribute authorship with Git (was Re: [PATCH] lisp/ob-shell.el: Also override explicit-shell-file-name)

[Matt]

 ---- On Sun, 17 Mar 2024 11:31:00 +0100  Ihor Radchenko  wrote --- 
 > Matt matt@excalamus.com> writes:
 >
 > > First, would you like me to update the commit?  If so, I will need guidance.  The correct procedure to change the author after committing to remote is unclear to me.  I would think it's something like sync my local copy with the latest remote version, update the author locally, and force push the change.  I would then expect that the next time someone pulls, it would update their local with the author change.  It would, however, cause a conflict, I think, for someone in the middle of making a change who has not synced with the forced push version and is trying to push their change.
 > 
 > We should avoid force pushing unless something is terribly broken.
 > What you may do instead is (1) revert the commit; (2) re-apply the
 > commit version with the correct author attribution.

Done.

For the benefit of future me or anyone else who cares, I did:

1. git revert <hash-for-specific-commit-needing-modification>
2. make changes (e.g. emacs <file-needing-modification> followed by *type-type-type* or some incantation of 'git apply' or 'git am')
3. git commit --author "Arthur Author <arthur-author's-email>"
4. git push

'git revert', in this case, basically swaps the plus and minus signs in the diff for the specified commit and submits that as a new set of changes.  After applying those changes, it's possible, in this case, to proceed with "what you should have done in the first place".

 > > Second, I can update Worg with an explanation that it's important to credit authors using git's author field and how to do this.  Unless I missed it, worg/org-contribute makes no mention of the author field.  The version of git packaged by my distro is 2.41.0 and, AFAICT, has no -A flag for 'git' or 'git commit'.  However, the following works on my machine and, I guess, is the long option form:
 > >
 > >     git commit --author "Arthur Override "
 > 
 > You are right. Looks like -A is just Magit shortcut.
 > 
 > As for crediting authors, we may document it in https://orgmode.org/worg/org-maintenance.html#copyright
 > Although, it is under "core maintainer" section. Maybe we can make a
 > dedicated section for maintainers on how to deal with patch submissions.

I added a little section within copyright: https://git.sr.ht/~bzg/worg/commit/80152bee771b755aedfbe488497c5e4d0e7457c2

--
Matt Trzcinski
Emacs Org contributor (ob-shell)
Learn more about Org mode at https://orgmode.org
Support Org development at https://liberapay.com/org-mode

Re: How to properly attribute authorship with Git (was Re: [PATCH] lisp/ob-shell.el: Also override explicit-shell-file-name)

[Ihor Radchenko]

Matt <matt@excalamus.com> writes:

>  > We should avoid force pushing unless something is terribly broken.
>  > What you may do instead is (1) revert the commit; (2) re-apply the
>  > commit version with the correct author attribution.
>
> Done.
> ...
> I added a little section within copyright: https://git.sr.ht/~bzg/worg/commit/80152bee771b755aedfbe488497c5e4d0e7457c2

Thanks!

-- 
Ihor Radchenko // yantar92,
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>

Re: How to properly attribute authorship with Git (was Re: [PATCH] lisp/ob-shell.el: Also override explicit-shell-file-name)

[Max Nikulin]

On 17/03/2024 20:42, Matt wrote:
>   ---- On Sun, 17 Mar 2024 11:31:00 +0100  Ihor Radchenko  wrote ---
>   >
>   > (1) revert the commit; (2) re-apply the
>   > commit version with the correct author attribution.
> 
> Done.
> 
> 1. git revert <hash-for-specific-commit-needing-modification>
> 2. make changes (e.g. emacs <file-needing-modification> followed by *type-type-type* or some incantation of 'git apply' or 'git am')
> 3. git commit --author "Arthur Author <arthur-author's-email>"

In this particular case "git am" works fine and no explicit "git commit" 
is necessary.

Re: How to properly attribute authorship with Git (was Re: [PATCH] lisp/ob-shell.el: Also override explicit-shell-file-name)

[Matt]

 ---- On Sun, 17 Mar 2024 17:26:48 +0100  Max Nikulin  wrote --- 
 > On 17/03/2024 20:42, Matt wrote:
 > >   ---- On Sun, 17 Mar 2024 11:31:00 +0100  Ihor Radchenko  wrote ---
 > >   >
 > >   > (1) revert the commit; (2) re-apply the
 > >   > commit version with the correct author attribution.
 > > 
 > > Done.
 > > 
 > > 1. git revert 
 > > 2. make changes (e.g. emacs  followed by *type-type-type* or some incantation of 'git apply' or 'git am')
 > > 3. git commit --author "Arthur Author "
 > 
 > In this particular case "git am" works fine and no explicit "git commit" 
 > is necessary.

Thank you for taking the time to verify the patch formatting.  Sounds like a PEBKAC.  I'll look into fixing it :)

--
Matt Trzcinski
Emacs Org contributor (ob-shell)
Learn more about Org mode at https://orgmode.org
Support Org development at https://liberapay.com/org-mode