Is M-j broken for you in Org on Emacs 27 and 28?

Is M-j broken for you in Org on Emacs 27 and 28?

[Richard Lawrence]

Hi Org community,

Some questions for those of you on Emacs 27 and 28:

Does M-j in an org-mode buffer do what you expect?
Does it throw an error?
What function is M-j bound to in Org?

Backstory:

I have long been on Emacs 26.3 (in Debian stable) but recently decided
to try a newer Emacs from GNU Guix. I immediately ran into an issue,
and now I'm trying to figure out if it's a bug, and if so where to file
it: in both Emacs 27.2 and 28.0.50, typing M-j in an Org buffer throws
(wrong-type-argument char-or-string-p nil)

The source of this problem appears to be that the keybinding for M-j
changed between Emacs 26 and 27. In Emacs 26 it calls
indent-new-comment-line. In Emacs 27 and 28 it calls
default-indent-new-line, and the call stack look like:

  insert-before-markers-and-inherit(nil)
  org-comment-line-break-function(nil)
  default-indent-new-line()
  funcall-interactively(default-indent-new-line)
  call-interactively(default-indent-new-line nil nil)
  command-execute(default-indent-new-line)

The error arises because insert-before-markers-and-inherit cannot accept
nil (the value of fill-prefix in this context).

I see this error in emacs -q with both Emacs 27 and 28 from Guix.

After some investigation, the functions involved here don't appear to
have changed at all recently (though see [1]); just the binding. This
leads me to ask: why hasn't this been discovered already? Which leads
me to wonder if I am using M-j in some non-standard way.

Some time in the distant past, I internalized the idea that M-j is a
better way to type a newline because (a) it doesn't involve a pinky
reach and (b) in most contexts in Emacs, it is more likely to "do what I
mean" than RET is. In particular, it continues comments and indents
properly. (I am also an evil-mode user and there is probably some part
of my brain that thinks "M-j is like j for insert mode".) But maybe that
was always wrong, and the recently changed binding is just an indication
that I was not using M-j as intended.

So which is it? Is this a bug in Emacs, in Org, or in my ingrained
typing habits? Many thanks for your advice.

-- 
Best,
Richard

[1] There is a commit which changed default-indent-new-line in August of
this year, but the changes don't seem relevant to the error I'm seeing,
since I also see it in Emacs 27.2:

https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=b41f31d2b60269bd0e7addd1081f3738f91e76bc

Re: Is M-j broken for you in Org on Emacs 27 and 28?

[Greg Minshall]

Richard,

i also see an error message when entering M-j running:
: emacs -Q foo.org

cheers, Greg

----
Org mode version 9.5 (9.5-gced2b3 @ /home/minshall/.emacs.d/straight/build/org/)

GNU Emacs 27.2 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.27,
cairo version 1.17.4) of 2021-03-26

Re: Is M-j broken for you in Org on Emacs 27 and 28?

[Colin Baxter 😺]

>>>>> Richard Lawrence <richard.lawrence@uni-tuebingen.de> writes:

    > Hi Org community, Some questions for those of you on Emacs 27 and
    > 28:

    > Does M-j in an org-mode buffer do what you expect?  Does it throw
    > an error?  What function is M-j bound to in Org?

    > Backstory:

    > I have long been on Emacs 26.3 (in Debian stable) but recently
    > decided to try a newer Emacs from GNU Guix. I immediately ran into
    > an issue, and now I'm trying to figure out if it's a bug, and if
    > so where to file it: in both Emacs 27.2 and 28.0.50, typing M-j in
    > an Org buffer throws (wrong-type-argument char-or-string-p nil)

    > The source of this problem appears to be that the keybinding for
    > M-j changed between Emacs 26 and 27. In Emacs 26 it calls
    > indent-new-comment-line. In Emacs 27 and 28 it calls
    > default-indent-new-line, and the call stack look like:

    >   insert-before-markers-and-inherit(nil)
    > org-comment-line-break-function(nil) default-indent-new-line()
    > funcall-interactively(default-indent-new-line)
    > call-interactively(default-indent-new-line nil nil)
    > command-execute(default-indent-new-line)

    > The error arises because insert-before-markers-and-inherit cannot
    > accept nil (the value of fill-prefix in this context).

    > I see this error in emacs -q with both Emacs 27 and 28 from Guix.

    > After some investigation, the functions involved here don't appear
    > to have changed at all recently (though see [1]); just the
    > binding. This leads me to ask: why hasn't this been discovered
    > already? Which leads me to wonder if I am using M-j in some
    > non-standard way.

    > Some time in the distant past, I internalized the idea that M-j is
    > a better way to type a newline because (a) it doesn't involve a
    > pinky reach and (b) in most contexts in Emacs, it is more likely
    > to "do what I mean" than RET is. In particular, it continues
    > comments and indents properly. (I am also an evil-mode user and
    > there is probably some part of my brain that thinks "M-j is like j
    > for insert mode".) But maybe that was always wrong, and the
    > recently changed binding is just an indication that I was not
    > using M-j as intended.

    > So which is it? Is this a bug in Emacs, in Org, or in my ingrained
    > typing habits? Many thanks for your advice.

    > -- Best, Richard

    > [1] There is a commit which changed default-indent-new-line in
    > August of this year, but the changes don't seem relevant to the
    > error I'm seeing, since I also see it in Emacs 27.2:

    > https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=b41f31d2b60269bd0e7addd1081f3738f91e76bc

I confirm that it also appears broken to me in emacs-27.2, with the same
error as you found. I have never noticed it before, possibly because I
use C-j rather than M-j.

Best wishes,

Re: Is M-j broken for you in Org on Emacs 27 and 28?

[Richard Lawrence]

Colin Baxter 😺 <m43cap@yandex.com> writes:

> I confirm that it also appears broken to me in emacs-27.2, with the same
> error as you found. I have never noticed it before, possibly because I
> use C-j rather than M-j.

Thanks for confirming. Do you know what the difference between C-j and
M-j is "supposed" to be? They both do very mode-dependent things. I
guess M-j is more explicitly aimed at continuing comments (which is
probably why I started using it), but it has always worked great for me
as a newline-and-indent-like-I-want command outside of comments too.

-- 
Best,
Richard

Re: Is M-j broken for you in Org on Emacs 27 and 28?

[Tim Cross]

Richard Lawrence <richard.lawrence@uni-tuebingen.de> writes:

> Colin Baxter 😺 <m43cap@yandex.com> writes:
>
>> I confirm that it also appears broken to me in emacs-27.2, with the same
>> error as you found. I have never noticed it before, possibly because I
>> use C-j rather than M-j.
>
> Thanks for confirming. Do you know what the difference between C-j and
> M-j is "supposed" to be? They both do very mode-dependent things. I
> guess M-j is more explicitly aimed at continuing comments (which is
> probably why I started using it), but it has always worked great for me
> as a newline-and-indent-like-I-want command outside of comments too.

I'm running Emacs 28 and cannot reproduce the issue you observe. Running
emacs -Q I find M-j is bound to

M-j runs the command default-indent-new-line (found in global-map),
which is an interactive compiled Lisp function in ‘simple.el’.

It is bound to C-M-j, M-j.

(default-indent-new-line &optional SOFT FORCE)

Break line at point and indent.
If a comment syntax is defined, call ‘comment-line-break-function’.

The inserted newline is marked hard if variable ‘use-hard-newlines’ is true,
unless optional argument SOFT is non-nil.

This binding is the same inside and outside of org mode. This is with
org version

Org mode version 9.5 (release_9.5-72-gc5d6656 @ /usr/local/share/emacs/28.0.60/lisp/org/)

and Emacs version

GNU Emacs 28.0.60 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.30,
cairo version 1.16.0) of 2021-11-26

Note that C-j in org mode is different from 'normal' C-j in that it is
bound to org-return-and-maybe-indent. If you want M-j to act like C-j in
org mode, you would need to rebind M-j to org-return-and-maybe-indent in
an appropriate org mode startup hook. 

Re: Is M-j broken for you in Org on Emacs 27 and 28?

[Richard Lawrence]

Tim Cross <theophilusx@gmail.com> writes:

> I'm running Emacs 28 and cannot reproduce the issue you observe.

Hmm, the plot thickens!

> Running emacs -Q I find M-j is bound to
>
> M-j runs the command default-indent-new-line (found in global-map),
> which is an interactive compiled Lisp function in ‘simple.el’.

I definitely see the error in emacs -Q with

GNU Emacs 28.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.24, cairo version 1.16.0)

which only contains Org 9.3, in my installation. So the problem has been
around at least that long, but only surfaced for me because the binding
of M-j changed between Emacs 26 and 27.

> This binding is the same inside and outside of org mode.

Yes, but inside Org mode, default-indent-new-line calls
org-comment-line-break-function (because it is the value of
comment-line-break-function), which is where the error originates.

The last line of org-comment-line-break-function is:

  (insert-before-markers-and-inherit fill-prefix)

and fill-prefix is nil, at least in all the contexts where I've tried
this.

Since you're not seeing the error, could you please check in an Org
buffer whether:

  - comment-line-break-function is 'org-comment-line-break-function
  - org-comment-line-break-function contains the line above (it should;
    it's still there in the master branch)
  - fill-prefix is nil when you type M-j?
  
Thanks!

-- 
Best,
Richard

Re: Is M-j broken for you in Org on Emacs 27 and 28?

[Richard Lawrence]

Sorry, forgot to reply to this:

Tim Cross <theophilusx@gmail.com> writes:

> Note that C-j in org mode is different from 'normal' C-j in that it is
> bound to org-return-and-maybe-indent. If you want M-j to act like C-j in
> org mode, you would need to rebind M-j to org-return-and-maybe-indent in
> an appropriate org mode startup hook. 

This is a good suggestion, thanks; maybe that will be the best solution
for me, if the answer is that the current behavior is not a bug. 

It now seems to me that it must be a bug, though, since Org sometimes
calls a built-in C function with an argument it cannot accept, and
several people have confirmed this. The main question for me at this
point is: does this happen because org-comment-line-break-function is
being called when it shouldn't be, or because fill-prefix is nil when it
shouldn't be, or something else?

-- 
Best,
Richard

Re: Is M-j broken for you in Org on Emacs 27 and 28?

[Tim Cross]

Richard Lawrence <richard.lawrence@uni-tuebingen.de> writes:

> Tim Cross <theophilusx@gmail.com> writes:
>
>> I'm running Emacs 28 and cannot reproduce the issue you observe.
>
> Hmm, the plot thickens!
>
>> Running emacs -Q I find M-j is bound to
>>
>> M-j runs the command default-indent-new-line (found in global-map),
>> which is an interactive compiled Lisp function in ‘simple.el’.
>
> I definitely see the error in emacs -Q with
>
> GNU Emacs 28.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.24, cairo version 1.16.0)
>
> which only contains Org 9.3, in my installation. So the problem has been
> around at least that long, but only surfaced for me because the binding
> of M-j changed between Emacs 26 and 27.
>

I think something is very wrong if your Emacs 28 has org 9.3. I'm pretty
sure the earliest version which was bundled with Emacs 28 was 9.4 - it
is certainly 9.5 now and that is the version that will be bundled with
it when it is released.

My suggestion would be to update Emacs to current version on the
emacs-28 branch of the git repo. This is at 'rc1' level and close to
what will be released. There is not much oint in trying to diagnose an
issue with both an old development version of Emacs and a version of org
which is two releases old. Update Emacs and ensure org is at least
version 9.5 (verison 9.5.1 was just released).

Re: Is M-j broken for you in Org on Emacs 27 and 28?

[Colin Baxter 😺]

>>>>> Richard Lawrence <richard.lawrence@uni-tuebingen.de> writes:

    > Tim Cross <theophilusx@gmail.com> writes:
    >> I'm running Emacs 28 and cannot reproduce the issue you observe.

    > Hmm, the plot thickens!

    >> Running emacs -Q I find M-j is bound to
    >> 
    >> M-j runs the command default-indent-new-line (found in
    >> global-map), which is an interactive compiled Lisp function in
    >> ‘simple.el’.

    > I definitely see the error in emacs -Q with

Indeed, so do I with emacs -Q. I get the error message with emacs-27.2,
emacs-28.0.50 and 29.0.50. 

I'm afraid the nuances of line indenting in org-mode have always been a
black art to me. I stick to C-j because I've found it works for me in
most cases.

Best wishes,

Colin Baxter.

Re: Is M-j broken for you in Org on Emacs 27 and 28?

[Richard Lawrence]

Tim Cross <theophilusx@gmail.com> writes:

> I think something is very wrong if your Emacs 28 has org 9.3. I'm pretty
> sure the earliest version which was bundled with Emacs 28 was 9.4 - it
> is certainly 9.5 now and that is the version that will be bundled with
> it when it is released.

Fair enough -- I did think that was weird. I installed Emacs 28 as
emacs-next via Guix, which I am new to; maybe it installed a very old
build, or maybe something is screwed up in the load-path. 

Still, I think this is not relevant to the problem. I see the same
problem on Emacs 27.2 (also installed via Guix) with the built-in Org
9.4.4. I also see it on both Emacs 27.2 and 28 when I load Org
from the latest bugfix branch in git ("Org mode version 9.5.1
(release_9.5.1-13-gc91271 @ /home/rwl/src/org-mode/lisp/)").

Several other people have confirmed that they see it too -- two in this
thread, one privately to me off-list.

So I don't think it's a problem with my setup. But I'm still trying to
get clear on where the problem could be. The functions involved
(default-indent-new-line, org-comment-line-break-function, and
insert-before-markers-and-inherit) mostly haven't been modified in a
long time. What *has* changed is that M-j now calls this stack of
functions, when it used to call comment-indent-new-line (in Emacs 26).

In fact, if I run M-x default-indent-new-line in an Org buffer in Emacs
26 with the built-in Org 9.1.9, I get the same error. I think this issue
has been lurking for a while -- it's just that the new binding brought
it to the surface.

-- 
Best,
Richard

Re: Is M-j broken for you in Org on Emacs 27 and 28?

[Tim Cross]

Richard Lawrence <richard.lawrence@uni-tuebingen.de> writes:

> Tim Cross <theophilusx@gmail.com> writes:
>
>> I'm running Emacs 28 and cannot reproduce the issue you observe.
>
> Hmm, the plot thickens!
>
>> Running emacs -Q I find M-j is bound to
>>
>> M-j runs the command default-indent-new-line (found in global-map),
>> which is an interactive compiled Lisp function in ‘simple.el’.
>
> I definitely see the error in emacs -Q with
>
> GNU Emacs 28.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.24, cairo version 1.16.0)
 o>
> which only contains Org 9.3, in my installation. So the problem has been
> around at least that long, but only surfaced for me because the binding
> of M-j changed between Emacs 26 and 27.
>
>> This binding is the same inside and outside of org mode.
>
> Yes, but inside Org mode, default-indent-new-line calls
> org-comment-line-break-function (because it is the value of
> comment-line-break-function), which is where the error originates.
>
> The last line of org-comment-line-break-function is:
>
>   (insert-before-markers-and-inherit fill-prefix)
>
> and fill-prefix is nil, at least in all the contexts where I've tried
> this.
>
> Since you're not seeing the error, could you please check in an Org
> buffer whether:
>
>   - comment-line-break-function is 'org-comment-line-break-function
>   - org-comment-line-break-function contains the line above (it should;
>     it's still there in the master branch)
>   - fill-prefix is nil when you type M-j?
>   

I just checked this when running emacs -Q and get the following

comment-line-break-function is a variable defined in ‘simple.el’.

Its value is ‘org-comment-line-break-function’
Local in buffer test.org; global value is 
comment-indent-new-line

Mode-specific function that line breaks and continues a comment.
This function is called during auto-filling when a comment syntax
is defined.
The function should take a single optional argument, which is a flag
indicating whether it should use soft newlines.

  This variable may be risky if used as a file-local variable.

and fill-prefix is

fill-prefix is a variable defined in ‘simple.el’.

Its value is nil

String for filling to insert at front of new line, or nil for none.

  Automatically becomes buffer-local when set.
  This variable is safe as a file local variable if its value
  satisfies the predicate ‘string-or-null-p’.
  You can customize this variable.
  Probably introduced at or before Emacs version 1.7.

and I don't get any error with M-j and cannot reproduce the issue you
are encountering. .

Emacs is

GNU Emacs 28.0.60 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.30,
cairo version 1.16.0) of 2021-11-30

and org version is

Org mode version 9.5.1 (release_9.5.1-11-g96d91b @
/usr/local/share/emacs/28.0.60/lisp/org/)

Looking at the git log, I can only find these messages relating to
default-indent-new-line

commit b41f31d2b60269bd0e7addd1081f3738f91e76bc
Author: Lars Ingebrigtsen <larsi@gnus.org>
Date:   Wed Aug 4 10:03:12 2021 +0200

    Make `M-j' work reliably if `comment-auto-fill-only-comments' is set
    
    * lisp/simple.el (default-indent-new-line): Force breaking the
    line when called interactively (bug#49849).  (Perhaps the
    interactive command should be rebound and call this function
    instead...)

commit 8a11e430ec261c08cc928a7a5b05ee1027f50368
Author: Dmitry Gutov <dgutov@yandex.ru>
Date:   Thu Jun 27 16:57:47 2019 +0200

    Use `default-indent-new-line' instead of `indent-new-comment-line'
    
    * lisp/simple.el (default-indent-new-line): Doc string fix.
    
    * lisp/textmodes/refill.el (refill-post-command-function): Make
    default-indent-new-line work as indent-new-comment-line.
    
    * lisp/textmodes/refill.el (refill-post-command-function): Bind
    `M-C-j' and `M-j' to default-indent-new-line instead of
    indent-new-comment-line to allow overriding via
    `comment-line-break-function' (bug#12413).

commit 0b727f9d087d950c0d6614c9ec743a935d4e5fc7
Author: Richard M. Stallman <rms@gnu.org>
Date:   Tue Aug 7 03:04:23 2007 +0000

    (default-indent-new-line): New function.
    It calls comment-line-break-function if there are comments.
    (do-auto-fill): Use that.

which indicates the function was added in 2007 by RMS and made the
default for M-j in 2019. 

Based on your report of having org 9.3, my suspicion is that your org
version is too old for the current Emacs versions (since the change in
2019 to use default-indent-new-line for C-M-j and M-j. I don't think
this is a bug in current Org or Emacs.

The other possibility is that you have a broken "mixed" installation of
org. This can easily occur if org is upgraded when it has already been
loaded into the Emacs instance when the upgrade is performed. It is
critically important to ensure Org has not been loaded before attempting
to do an upgrade. Unfortunately, it is very easy to not realise that
something in your init file is loading org. One of the advantages of
using use-package is that you can configure things so that org is not
loaded until you actually open an org file, which makes it easier to
start a new Emacs instance and perform the upgrade in a clean
environment. Where people often come undone is when some other package
they are using loads org because of an internal dependency. 

[PATCH] Fix org-comment-line-break-function (was: Is M-j broken for you in Org on Emacs 27 and 28?)

[Richard Lawrence]

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

Hi Tim and all,

Thanks for sticking with me here...

Tim Cross <theophilusx@gmail.com> writes:

> I just checked this when running emacs -Q and get the following
>
> comment-line-break-function is a variable defined in ‘simple.el’.
>
> Its value is ‘org-comment-line-break-function’
>
> and fill-prefix is
>
> fill-prefix is a variable defined in ‘simple.el’.
>
> Its value is nil

OK, those are the values I have too...

> and I don't get any error with M-j and cannot reproduce the issue you
> are encountering. .

Do you see an error if you explicitly call

(insert-before-markers-and-inherit nil)

?

Because that is what org-comment-line-break-function does when
fill-prefix is nil. That is the source of the error on all the versions
where I have reproduced it. If you don't see the error then, it would
indicate to me that something in your setup is suppressing it.

> Looking at the git log, I can only find these messages relating to
> default-indent-new-line
> ...
> which indicates the function was added in 2007 by RMS and made the
> default for M-j in 2019. 

Right, which is why I don't see it with M-j in Emacs 26, presumably.
Emacs 26.1, the version installed on my Debian system, was released May
28, 2018.

> my suspicion is that your org version is too old for the current Emacs
> versions... The other possibility is that you have a broken "mixed"
> installation of org.

I'm aware of the difficulties of a mixed installation and have been
careful to avoid them. I run Org from git, usually a recent pull of the
bugfix branch, which I load via use-package from my init file. But I
have also tested it with the built-in Org for various versions of Emacs
with emacs -Q. I do not have Org installed via ELPA.

Just to be extra, super sure, I built Emacs this afternoon from a
checkout of the repo, and the error is *still* there, with the same
cause. In that build, with emacs -Q, I have:

(org-version)
"9.5"

(emacs-version)
"GNU Emacs 29.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.5, cairo version 1.16.0)
 of 2021-11-30"

At this point I've replicated the bug on my machine in four different
builds of Emacs (version 26.1 from Debian, 27.2 and "emacs-next" from
Guix, and version 29.0.50 I built myself from source) with several
versions of Org (the built-in ones in these Emacsen and a recent build
of the bugfix branch). It is robustly reproducible for me, and the cause
is clear: default-indent-new-line calls org-comment-line-break-function,
which calls

(insert-before-markers-and-inherit nil)

which is a type error. I'm looking for help figuring out what the right
fix is. I attach a patch for the simplest fix I can think of; please let
me know if something else would be better.

-- 
Best,
Richard


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: org.el.diff --]
[-- Type: text/x-diff, Size: 418 bytes --]

diff --git a/lisp/org.el b/lisp/org.el
index 1a1375461..fdeec0d67 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -19695,7 +19695,8 @@ non-nil."
   (save-excursion (forward-char -1) (delete-horizontal-space))
   (delete-horizontal-space)
   (indent-to-left-margin)
-  (insert-before-markers-and-inherit fill-prefix))
+  (when fill-prefix
+    (insert-before-markers-and-inherit fill-prefix)))
 
 
 ;;; Fixed Width Areas

Re: Is M-j broken for you in Org on Emacs 27 and 28?

[Morgan Willcock]

Tim Cross <theophilusx@gmail.com> writes:

> Based on your report of having org 9.3, my suspicion is that your org
> version is too old for the current Emacs versions (since the change in
> 2019 to use default-indent-new-line for C-M-j and M-j. I don't think
> this is a bug in current Org or Emacs.

Just for reference, I see the same problem on Emacs 27.2 using the
default and latest versions of Org-mode.

i.e.

 - using `emacs -Q` the Org version is listed as 9.4.4 and I see error

 - running emacs normally the Org version is listed as 9.5.1 and I see
   the error

> The other possibility is that you have a broken "mixed" installation of
> org. This can easily occur if org is upgraded when it has already been
> loaded into the Emacs instance when the upgrade is performed.

I upgraded the package using `emacs -Q` and I only use GNU ELPA as a
package source. I think that in my case the package installation was
clean.

Thanks,
Morgan

Re: [PATCH] Fix org-comment-line-break-function

[Marco Wahl]

Hi Richard and all,

[...]

> Just to be extra, super sure, I built Emacs this afternoon from a
> checkout of the repo, and the error is *still* there, with the same
> cause. In that build, with emacs -Q, I have:
>
> (org-version)
> "9.5"
>
> (emacs-version)
> "GNU Emacs 29.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.5, cairo version 1.16.0)
>  of 2021-11-30"
>
> At this point I've replicated the bug on my machine in four different
> builds of Emacs (version 26.1 from Debian, 27.2 and "emacs-next" from
> Guix, and version 29.0.50 I built myself from source) with several
> versions of Org (the built-in ones in these Emacsen and a recent build
> of the bugfix branch). It is robustly reproducible for me, and the cause
> is clear: default-indent-new-line calls org-comment-line-break-function,
> which calls
>
> (insert-before-markers-and-inherit nil)
>
> which is a type error. I'm looking for help figuring out what the right
> fix is. I attach a patch for the simplest fix I can think of; please let
> me know if something else would be better.

    diff --git a/lisp/org.el b/lisp/org.el
    index 1a1375461..fdeec0d67 100644
    --- a/lisp/org.el
    +++ b/lisp/org.el
    @@ -19695,7 +19695,8 @@ non-nil."
       (save-excursion (forward-char -1) (delete-horizontal-space))
       (delete-horizontal-space)
       (indent-to-left-margin)
    -  (insert-before-markers-and-inherit fill-prefix))
    +  (when fill-prefix
    +    (insert-before-markers-and-inherit fill-prefix)))
 
I don't have anything better.  I think this is a good patch.  It makes
M-j work again.

Possible refinements and improvements can follow.

+1 for applying of your patch.


Ciao,
-- 
Marco

Re: [PATCH] Fix org-comment-line-break-function

[Tim Cross]

Marco Wahl <marcowahlsoft@gmail.com> writes:

> Hi Richard and all,
>
> [...]
>
>> Just to be extra, super sure, I built Emacs this afternoon from a
>> checkout of the repo, and the error is *still* there, with the same
>> cause. In that build, with emacs -Q, I have:
>>
>> (org-version)
>> "9.5"
>>
>> (emacs-version)
>> "GNU Emacs 29.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.5, cairo version 1.16.0)
>>  of 2021-11-30"
>>
>> At this point I've replicated the bug on my machine in four different
>> builds of Emacs (version 26.1 from Debian, 27.2 and "emacs-next" from
>> Guix, and version 29.0.50 I built myself from source) with several
>> versions of Org (the built-in ones in these Emacsen and a recent build
>> of the bugfix branch). It is robustly reproducible for me, and the cause
>> is clear: default-indent-new-line calls org-comment-line-break-function,
>> which calls
>>
>> (insert-before-markers-and-inherit nil)
>>
>> which is a type error. I'm looking for help figuring out what the right
>> fix is. I attach a patch for the simplest fix I can think of; please let
>> me know if something else would be better.
>
>     diff --git a/lisp/org.el b/lisp/org.el
>     index 1a1375461..fdeec0d67 100644
>     --- a/lisp/org.el
>     +++ b/lisp/org.el
>     @@ -19695,7 +19695,8 @@ non-nil."
>        (save-excursion (forward-char -1) (delete-horizontal-space))
>        (delete-horizontal-space)
>        (indent-to-left-margin)
>     -  (insert-before-markers-and-inherit fill-prefix))
>     +  (when fill-prefix
>     +    (insert-before-markers-and-inherit fill-prefix)))
>  
> I don't have anything better.  I think this is a good patch.  It makes
> M-j work again.
>
> Possible refinements and improvements can follow.
>
> +1 for applying of your patch.
>
>

I was finally able to reproduce the error. It depends to some degree on
the text in the buffer and where the cursor is when you hit M-j. Adding
some additional text and moving the cursor to different locations
enabled me to reproduce the error and I now agree it is a bug in
org-comment-line-break-function.

I don't know if your patch is the right fix or not because I don't
understand what the purpose of insert-before-marks-and-inherit is - in
fact, the doc string for that function doesn't even state what the @rest
args argument is for, so I don't understand why it is passing in
fill-prefix. For example, is it safe to assume
insert-before-merks-and-inherit does not need to be called if
fill-prefix is nil? Why is that function even called with the
fill-prefix as an argument? 

Re: [PATCH] Fix org-comment-line-break-function

[Kaushal Modi]

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

On Tue, Nov 30, 2021 at 3:20 PM Marco Wahl <marcowahlsoft@gmail.com> wrote:

>
>
>     diff --git a/lisp/org.el b/lisp/org.el
>     index 1a1375461..fdeec0d67 100644
>     --- a/lisp/org.el
>     +++ b/lisp/org.el
>     @@ -19695,7 +19695,8 @@ non-nil."
>        (save-excursion (forward-char -1) (delete-horizontal-space))
>        (delete-horizontal-space)
>        (indent-to-left-margin)
>     -  (insert-before-markers-and-inherit fill-prefix))
>     +  (when fill-prefix
>     +    (insert-before-markers-and-inherit fill-prefix)))
>
> I don't have anything better.  I think this is a good patch.  It makes
> M-j work again.
>
> Possible refinements and improvements can follow.
>
> +1 for applying of your patch.
>

I am able to reproduce that M-j issue (using Emacs version: GNU Emacs
28.0.60 (build 9, x86_64-pc-linux-gnu, GTK+ Version 3.22.30, cairo version
1.15.12)
 of 2021-11-29, built using commit
c4daff9cf844ec85930bdcd2064787c92c260861, and Org mode version 9.5
(release_9.5-292-g5e35de)).

And this patch fixes that for me as well.

+1 for applying this patch.

=====

Before this patch, M-j gave this backtrace with debug enabled:

Debugger entered--Lisp error: (wrong-type-argument char-or-string-p nil)
  insert-before-markers-and-inherit(nil)
  org-comment-line-break-function(nil)
  default-indent-new-line(nil t)
  funcall-interactively(default-indent-new-line nil t)
  call-interactively(default-indent-new-line nil nil)
  command-execute(default-indent-new-line)


Output of C-h k M-j:

M-j runs the command default-indent-new-line (found in global-map),
which is an interactive compiled Lisp function in ‘simple.el’.

It is bound to C-M-j, M-j.

(default-indent-new-line &optional SOFT FORCE)

Break line at point and indent.
If a comment syntax is defined, call ‘comment-line-break-function’.

The inserted newline is marked hard if variable ‘use-hard-newlines’ is true,
unless optional argument SOFT is non-nil.

[-- Attachment #2: Type: text/html, Size: 2649 bytes --]

Re: [PATCH] Fix org-comment-line-break-function

[Tim Cross]

Kaushal Modi <kaushal.modi@gmail.com> writes:

> On Tue, Nov 30, 2021 at 3:20 PM Marco Wahl <marcowahlsoft@gmail.com> wrote:
>
>      diff --git a/lisp/org.el b/lisp/org.el
>      index 1a1375461..fdeec0d67 100644
>      --- a/lisp/org.el
>      +++ b/lisp/org.el
>      @@ -19695,7 +19695,8 @@ non-nil."
>         (save-excursion (forward-char -1) (delete-horizontal-space))
>         (delete-horizontal-space)
>         (indent-to-left-margin)
>      -  (insert-before-markers-and-inherit fill-prefix))
>      +  (when fill-prefix
>      +    (insert-before-markers-and-inherit fill-prefix)))
>
>  I don't have anything better.  I think this is a good patch.  It makes
>  M-j work again.
>
>  Possible refinements and improvements can follow.
>
>  +1 for applying of your patch.
>
> I am able to reproduce that M-j issue (using Emacs version: GNU Emacs 28.0.60 (build 9, x86_64-pc-linux-gnu, GTK+ Version 3.22.30,
> cairo version 1.15.12)
>  of 2021-11-29, built using commit c4daff9cf844ec85930bdcd2064787c92c260861, and Org mode version 9.5
> (release_9.5-292-g5e35de)).
>
> And this patch fixes that for me as well.
>
> +1 for applying this patch.
>
> =====
>
> Before this patch, M-j gave this backtrace with debug enabled:
>
> Debugger entered--Lisp error: (wrong-type-argument char-or-string-p nil)
>   insert-before-markers-and-inherit(nil)
>   org-comment-line-break-function(nil)
>   default-indent-new-line(nil t)
>   funcall-interactively(default-indent-new-line nil t)
>   call-interactively(default-indent-new-line nil nil)
>   command-execute(default-indent-new-line)
>
> Output of C-h k M-j:
>
> M-j runs the command default-indent-new-line (found in global-map),
> which is an interactive compiled Lisp function in ‘simple.el’.
>
> It is bound to C-M-j, M-j.
>
> (default-indent-new-line &optional SOFT FORCE)
>
> Break line at point and indent.
> If a comment syntax is defined, call ‘comment-line-break-function’.
>
> The inserted newline is marked hard if variable ‘use-hard-newlines’ is true,
> unless optional argument SOFT is non-nil.

I'm not sure this is the right patch to apply. While it does fix the
immediate error, it really does so by just avoiding the call to
insert-before-markers-and-inherit when fill-prefix is nil. It does not
address the question of what that function is supposed to do or whether
the correct fix is either to call the function without the fill-prefix
argument (which also works in that it avoids the error) or if instead,
the patch should be

(if fill-prefix
  (insert-before-markers-and-inherit fill-prefix)
 (insert-before-markers-and-inherit))

I note also that with or without the patch, the function does not appear
to work correctly anyway. If you hit M-j while in a comment, the new
line should be indented appropriately and have the comment character
prefix i.e. start a new comment line. It does not do that. This is
supposed to be the key difference between C-j and M-j.

Regardless, I think that unless we understand the purpose of
insert-before-markers-and-inherit, we should make the patch such that it
still calls that function. Even if fill-prefix is nil there is
probably a good reason why the markers and properties need to be
modified for some situations. 

It would be good to get Nicholas' input here as I think he wrote the
original function back in 2012. 

Re: [PATCH] Fix org-comment-line-break-function

[Kaushal Modi]

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

On Tue, Nov 30, 2021, 6:29 PM Tim Cross <theophilusx@gmail.com> wrote:

>
> Regardless, I think that unless we understand the purpose of
> insert-before-markers-and-inherit, we should make the patch such that it
> still calls that function. Even if fill-prefix is nil there is
> probably a good reason why the markers and properties need to be
> modified for some situations.
>

Oops! You're right; I did not verify that binding within a comment. I just
checked that it prevented the error.

It would be good to understand the purpose of that function call and have a
proper fix.

>

[-- Attachment #2: Type: text/html, Size: 1137 bytes --]

Re: [PATCH] Fix org-comment-line-break-function

[Kaushal Modi]

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

On Tue, Nov 30, 2021 at 6:29 PM Tim Cross <theophilusx@gmail.com> wrote:

>
> It would be good to get Nicholas' input here as I think he wrote the
> original function back in 2012.
>

Just to see what happens, I tried this:

M-: (setq-local comment-line-break-function #'comment-indent-new-line)

.. and then M-j started working perfectly! It worked fine both: in Org
comment lines and out of comment lines.

I see that comment-indent-new-line was added to emacs in newcomment.el back
in 2000. So I don't know why org-comment-line-break-function was added. May
be Nicolas can comment more on that.

So would this patch work?

=====

From 1a9187b82ed8d4e8d54ddd369a44d34295281fc3 Mon Sep 17 00:00:00 2001
From: Kaushal Modi <kaushal.modi@gmail.com>
Date: Tue, 30 Nov 2021 20:37:10 -0500
Subject: [PATCH] org: Remove `org-comment-line-break-function'

* lisp/org.el: Remove `org-comment-line-break-function' and let
`comment-line-break-function' be the default value.

This fixes the `M-j' binding issue reported by Richard Lawrence in
<https://lists.gnu.org/r/emacs-orgmode/2021-11/msg00639.html>.
---
 lisp/org.el | 17 ++---------------
 1 file changed, 2 insertions(+), 15 deletions(-)

diff --git a/lisp/org.el b/lisp/org.el
index ec59ddf44..ee8ca1f03 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -19624,8 +19624,7 @@ assumed to be significant there."

 ;; `org-auto-fill-function' takes care of auto-filling.  It calls
 ;; `do-auto-fill' only on valid areas with `fill-prefix' shadowed with
-;; `org-adaptive-fill-function' value.  Internally,
-;; `org-comment-line-break-function' breaks the line.
+;; `org-adaptive-fill-function' value.

 ;; `org-setup-filling' installs filling and auto-filling related
 ;; variables during `org-mode' initialization.
@@ -19647,8 +19646,7 @@ assumed to be significant there."
   (setq-local fill-paragraph-function 'org-fill-paragraph)
   (setq-local auto-fill-inhibit-regexp nil)
   (setq-local adaptive-fill-function 'org-adaptive-fill-function)
-  (setq-local normal-auto-fill-function 'org-auto-fill-function)
-  (setq-local comment-line-break-function
'org-comment-line-break-function))
+  (setq-local normal-auto-fill-function 'org-auto-fill-function))

 (defun org-fill-line-break-nobreak-p ()
   "Non-nil when a new line at point would create an Org line break."
@@ -19903,17 +19901,6 @@ filling the current element."
      (adaptive-fill-mode (not (equal fill-prefix ""))))
  (when fill-prefix (do-auto-fill))))))

-(defun org-comment-line-break-function (&optional soft)
-  "Break line at point and indent, continuing comment if within one.
-The inserted newline is marked hard if variable
-`use-hard-newlines' is true, unless optional argument SOFT is
-non-nil."
-  (if soft (insert-and-inherit ?\n) (newline 1))
-  (save-excursion (forward-char -1) (delete-horizontal-space))
-  (delete-horizontal-space)
-  (indent-to-left-margin)
-  (insert-before-markers-and-inherit fill-prefix))
-

 ;;; Fixed Width Areas

-- 
2.34.0

=====

[-- Attachment #2: Type: text/html, Size: 4033 bytes --]

Re: [PATCH] Fix org-comment-line-break-function

[Tim Cross]

Kaushal Modi <kaushal.modi@gmail.com> writes:

> On Tue, Nov 30, 2021 at 6:29 PM Tim Cross <theophilusx@gmail.com> wrote:
>
>  It would be good to get Nicholas' input here as I think he wrote the
>  original function back in 2012. 
>
> Just to see what happens, I tried this:
>
> M-: (setq-local comment-line-break-function #'comment-indent-new-line)
>
> .. and then M-j started working perfectly! It worked fine both: in Org comment lines and out of comment lines.
>
> I see that comment-indent-new-line was added to emacs in newcomment.el back in 2000. So I don't know why
> org-comment-line-break-function was added. May be Nicolas can comment more on that.
>
> So would this patch work?
>

Well, that is the big question - why was org-comment-line-break-function
added instead of just using the default comment-indent-new-line? My
only thoughts are there must be some subtle difference in org which the
default function was not sophisticated enough to work with. Problem is,
not knowing what that might be makes it hard to test and verify the real
impact of making the change.

Until this is determined, I think the only 'safe' approach would be to
just advise those who are impacted by the M-j issue to set
comment-line-break-function to comment-indent-new-line and then wait to
see if someone who has more historical context to comment. If nothing or
nobody says anything after a couple of months, then maybe apply your
patch.

Re: [PATCH] Fix org-comment-line-break-function

[Richard Lawrence]

Tim Cross <theophilusx@gmail.com> writes:

> Well, that is the big question - why was
> org-comment-line-break-function added instead of just using the
> default comment-indent-new-line?

Looking back at commit d58d40f0c864ae3a6d7c66df34769619ad2486c1, I see
this comment was added by Nicolas (still in org.el):

;; `org-auto-fill-function' takes care of auto-filling. It calls
;; `do-auto-fill' only on valid areas with `fill-prefix' shadowed with
;; `org-adaptive-fill-function' value. Internally,
;; `org-comment-line-break-function' breaks the line.

which at least suggests ("Internally, ...") that
org-comment-line-break-function was never intended to be called to break
a comment via a user command like M-j, and was only intended to be
called during filling, in a context where fill-prefix would be set correctly.

In that case, the problem might be that org-setup-filling sets
comment-line-break-function to 'org-comment-line-break-function
(overriding the default value of 'comment-indent-new-line) "globally" in
Org buffers, causing it to be called even when fill-prefix is not
set correctly.

The documentation for (the variable) comment-line-break-function is a
bit confusing:

"Mode-specific function that line breaks and continues a comment.
This function is called during auto-filling when a comment syntax
is defined."

which also suggests that this function will only be called during
filling. But that is clearly not true (even if it once was), since
default-indent-new-line calls it directly.

So maybe org-comment-line-break-function was written under assumptions
that no longer hold?

It might also be worth pointing out here that
org-comment-line-break-function is *almost* a line-for-line copy of part of
default-indent-new-line, except the latter is more careful about
checking that fill-prefix is not nil before calling insert-* on it.

> Until this is determined, I think the only 'safe' approach would be to
> just advise those who are impacted by the M-j issue to set
> comment-line-break-function to comment-indent-new-line and then wait
> to see if someone who has more historical context to comment. 

This is a bit trickier than it sounds, because in Org buffers,
org-setup-filling calls

(setq-local comment-line-break-function 'org-comment-line-break-function)

which will override any global value for comment-line-break-function
(e.g. in your init file).  So you'd have to reset it in Org
buffers locally, via a hook that runs after org-setup-filling.

-- 
Best,
Richard

Re: [PATCH] Fix org-comment-line-break-function

[Marco Wahl]

Tim and all!

>>     diff --git a/lisp/org.el b/lisp/org.el
>>     index 1a1375461..fdeec0d67 100644
>>     --- a/lisp/org.el
>>     +++ b/lisp/org.el
>>     @@ -19695,7 +19695,8 @@ non-nil."
>>        (save-excursion (forward-char -1) (delete-horizontal-space))
>>        (delete-horizontal-space)
>>        (indent-to-left-margin)
>>     -  (insert-before-markers-and-inherit fill-prefix))
>>     +  (when fill-prefix
>>     +    (insert-before-markers-and-inherit fill-prefix)))
>>  
>> I don't have anything better.  I think this is a good patch.  It makes
>> M-j work again.
>>
>> Possible refinements and improvements can follow.
>>
>> +1 for applying of your patch.
>
> I was finally able to reproduce the error. It depends to some degree on
> the text in the buffer and where the cursor is when you hit M-j. Adding
> some additional text and moving the cursor to different locations
> enabled me to reproduce the error and I now agree it is a bug in
> org-comment-line-break-function.
>
> I don't know if your patch is the right fix or not because I don't
> understand what the purpose of insert-before-marks-and-inherit is - in
> fact, the doc string for that function doesn't even state what the @rest
> args argument is for, so I don't understand why it is passing in
> fill-prefix. For example, is it safe to assume
> insert-before-merks-and-inherit does not need to be called if
> fill-prefix is nil? Why is that function even called with the
> fill-prefix as an argument? 

Thanks for staying critical!

I can't answer your questions.  I agree that we should deepen the
understanding.  Your questions are a start.

Pragmatically I still vote for applying the patch immediately since it
is a step in the right direction AFAICS.


Ciao,
-- 
Marco

Re: [PATCH] Fix org-comment-line-break-function

[Nicolas Goaziou]

Hello,

Kaushal Modi <kaushal.modi@gmail.com> writes:

> I see that comment-indent-new-line was added to emacs in newcomment.el back
> in 2000. So I don't know why org-comment-line-break-function was added. May
> be Nicolas can comment more on that.

Sorry, I do not remember.

As pointed out in the thread, this function probably wasn't meant to be
public in the first place. Now it is.

> So would this patch work?

Such a patch must be accompanied with tests.

Regards,
-- 
Nicolas Goaziou

Re: [PATCH] Fix org-comment-line-break-function

[Tim Cross]

Given that Nicholas cannot remember the reason for the original function
and suspects it was meant to be an internal only function, I think this
patch is probably the best way forward and should be applied.

Kaushal Modi <kaushal.modi@gmail.com> writes:

> On Tue, Nov 30, 2021 at 6:29 PM Tim Cross <theophilusx@gmail.com> wrote:
>
>  It would be good to get Nicholas' input here as I think he wrote the
>  original function back in 2012. 
>
> Just to see what happens, I tried this:
>
> M-: (setq-local comment-line-break-function #'comment-indent-new-line)
>
> .. and then M-j started working perfectly! It worked fine both: in Org comment lines and out of comment lines.
>
> I see that comment-indent-new-line was added to emacs in newcomment.el back in 2000. So I don't know why
> org-comment-line-break-function was added. May be Nicolas can comment more on that.
>
> So would this patch work?
>
> =====
>
> From 1a9187b82ed8d4e8d54ddd369a44d34295281fc3 Mon Sep 17 00:00:00 2001
> From: Kaushal Modi <kaushal.modi@gmail.com>
> Date: Tue, 30 Nov 2021 20:37:10 -0500
> Subject: [PATCH] org: Remove `org-comment-line-break-function'
>
> * lisp/org.el: Remove `org-comment-line-break-function' and let
> `comment-line-break-function' be the default value.
>
> This fixes the `M-j' binding issue reported by Richard Lawrence in
> <https://lists.gnu.org/r/emacs-orgmode/2021-11/msg00639.html>.
> ---
>  lisp/org.el | 17 ++---------------
>  1 file changed, 2 insertions(+), 15 deletions(-)
>
> diff --git a/lisp/org.el b/lisp/org.el
> index ec59ddf44..ee8ca1f03 100644
> --- a/lisp/org.el
> +++ b/lisp/org.el
> @@ -19624,8 +19624,7 @@ assumed to be significant there."
>  
>  ;; `org-auto-fill-function' takes care of auto-filling.  It calls
>  ;; `do-auto-fill' only on valid areas with `fill-prefix' shadowed with
> -;; `org-adaptive-fill-function' value.  Internally,
> -;; `org-comment-line-break-function' breaks the line.
> +;; `org-adaptive-fill-function' value.  
>  
>  ;; `org-setup-filling' installs filling and auto-filling related
>  ;; variables during `org-mode' initialization.
> @@ -19647,8 +19646,7 @@ assumed to be significant there."
>    (setq-local fill-paragraph-function 'org-fill-paragraph)
>    (setq-local auto-fill-inhibit-regexp nil)
>    (setq-local adaptive-fill-function 'org-adaptive-fill-function)
> -  (setq-local normal-auto-fill-function 'org-auto-fill-function)
> -  (setq-local comment-line-break-function 'org-comment-line-break-function))
> +  (setq-local normal-auto-fill-function 'org-auto-fill-function))
>  
>  (defun org-fill-line-break-nobreak-p ()
>    "Non-nil when a new line at point would create an Org line break."
> @@ -19903,17 +19901,6 @@ filling the current element."
>       (adaptive-fill-mode (not (equal fill-prefix ""))))
>   (when fill-prefix (do-auto-fill))))))
>  
> -(defun org-comment-line-break-function (&optional soft)
> -  "Break line at point and indent, continuing comment if within one.
> -The inserted newline is marked hard if variable
> -`use-hard-newlines' is true, unless optional argument SOFT is
> -non-nil."
> -  (if soft (insert-and-inherit ?\n) (newline 1))
> -  (save-excursion (forward-char -1) (delete-horizontal-space))
> -  (delete-horizontal-space)
> -  (indent-to-left-margin)
> -  (insert-before-markers-and-inherit fill-prefix))
> -
>  
>  ;;; Fixed Width Areas

Re: [PATCH] Fix org-comment-line-break-function

[Kaushal Modi]

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

On Sat, Dec 4, 2021, 5:25 PM Tim Cross <theophilusx@gmail.com> wrote:

>
> Given that Nicholas cannot remember the reason for the original function
> and suspects it was meant to be an internal only function, I think this
> patch is probably the best way forward and should be applied.
>

Thanks. Nicolas asked me to add tests for this patch. But I need to look
into how to add tests for behavior of bindings. Need to add tests for M-j
binding behavior when cursor is within a comment or outside.

>

[-- Attachment #2: Type: text/html, Size: 996 bytes --]

Re: [PATCH] Fix org-comment-line-break-function

[Nicolas Goaziou]

Hello,

Kaushal Modi <kaushal.modi@gmail.com> writes:

> Thanks. Nicolas asked me to add tests for this patch. But I need to look
> into how to add tests for behavior of bindings. Need to add tests for M-j
> binding behavior when cursor is within a comment or outside.

I don't think you need to test the binding. You could test
(call-interactively o-c-l-b-f) instead. 

Besides, tests are not a blocker.

Regards,
-- 
Nicolas Goaziou

Re: [PATCH] Fix org-comment-line-break-function

[Richard Lawrence]

Kaushal Modi <kaushal.modi@gmail.com> writes:

> On Sat, Dec 4, 2021, 5:25 PM Tim Cross <theophilusx@gmail.com> wrote:
>
>> Given that Nicholas cannot remember the reason for the original function
>> and suspects it was meant to be an internal only function, I think this
>> patch is probably the best way forward and should be applied.
>
> Thanks. Nicolas asked me to add tests for this patch. But I need to look
> into how to add tests for behavior of bindings. Need to add tests for M-j
> binding behavior when cursor is within a comment or outside.

FWIW I have been running the equivalent of Kaushal's patch (I just
removed the local binding of comment-line-break-function to
'org-comment-line-break-function without deleting the function) and have
also set

(debug-on-entry 'org-comment-line-break-function)

I've been running this for at least a week with no issues, and have
never been dropped into the debugger. So at least the testing through my
normal work indicates there are no problems with the patch.

-- 
Best,
Richard

Re: [PATCH] Fix org-comment-line-break-function

[Kaushal Modi]

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

Hi Nicolas,

I have added few tests in the updated patch pasted in this email.
I have made the tests for (call-interactive #'default-indent-new-line)
because that the interactive function M-j is bound to by default.

Can you please review and commit it? The machine I am on right now does not
allow external ssh access.



From de607dff518eaa91149ff1aa8c255f67fb6ee887 Mon Sep 17 00:00:00 2001
From: Kaushal Modi <kaushal.modi@gmail.com>
Date: Tue, 30 Nov 2021 20:37:10 -0500
Subject: [PATCH] org: Remove `org-comment-line-break-function'

* lisp/org.el: Remove `org-comment-line-break-function' and let
`comment-line-break-function' be the default value.
* testing/lisp/test-org.el (test-org/default-indent-new-line): Add
tests for M-j behavior when point is inside or outside an Org comment.

This fixes the `M-j' binding issue reported by Richard Lawrence in
<https://lists.gnu.org/r/emacs-orgmode/2021-11/msg00639.html>.
---
 lisp/org.el              | 17 ++---------------
 testing/lisp/test-org.el | 19 +++++++++++++++++++
 2 files changed, 21 insertions(+), 15 deletions(-)

diff --git a/lisp/org.el b/lisp/org.el
index ec59ddf44..ee8ca1f03 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -19624,8 +19624,7 @@ assumed to be significant there."

 ;; `org-auto-fill-function' takes care of auto-filling.  It calls
 ;; `do-auto-fill' only on valid areas with `fill-prefix' shadowed with
-;; `org-adaptive-fill-function' value.  Internally,
-;; `org-comment-line-break-function' breaks the line.
+;; `org-adaptive-fill-function' value.

 ;; `org-setup-filling' installs filling and auto-filling related
 ;; variables during `org-mode' initialization.
@@ -19647,8 +19646,7 @@ assumed to be significant there."
   (setq-local fill-paragraph-function 'org-fill-paragraph)
   (setq-local auto-fill-inhibit-regexp nil)
   (setq-local adaptive-fill-function 'org-adaptive-fill-function)
-  (setq-local normal-auto-fill-function 'org-auto-fill-function)
-  (setq-local comment-line-break-function
'org-comment-line-break-function))
+  (setq-local normal-auto-fill-function 'org-auto-fill-function))

 (defun org-fill-line-break-nobreak-p ()
   "Non-nil when a new line at point would create an Org line break."
@@ -19903,17 +19901,6 @@ filling the current element."
      (adaptive-fill-mode (not (equal fill-prefix ""))))
  (when fill-prefix (do-auto-fill))))))

-(defun org-comment-line-break-function (&optional soft)
-  "Break line at point and indent, continuing comment if within one.
-The inserted newline is marked hard if variable
-`use-hard-newlines' is true, unless optional argument SOFT is
-non-nil."
-  (if soft (insert-and-inherit ?\n) (newline 1))
-  (save-excursion (forward-char -1) (delete-horizontal-space))
-  (delete-horizontal-space)
-  (indent-to-left-margin)
-  (insert-before-markers-and-inherit fill-prefix))
-

 ;;; Fixed Width Areas

diff --git a/testing/lisp/test-org.el b/testing/lisp/test-org.el
index 056ea7d87..de4ac7ea9 100644
--- a/testing/lisp/test-org.el
+++ b/testing/lisp/test-org.el
@@ -1200,6 +1200,25 @@
     (org-indent-region (point-min) (point-max))
     (buffer-string)))))

+(ert-deftest test-org/default-indent-new-line ()
+  "Test behavior of default binding `M-j'."
+  ;; Calling `M-j' when point is not in an Org comment:
+  (should
+   (equal "* Some heading\n"
+  (org-test-with-temp-text "* Some heading<point>"
+                           (call-interactively #'default-indent-new-line)
+                           (buffer-string))))
+  ;; Calling `M-j' when point is in an Org comment:
+  (should
+   (equal "# Some Org comment\n# "
+  (org-test-with-temp-text "# Some Org comment<point>"
+                           (call-interactively #'default-indent-new-line)
+                           (buffer-string))))
+  (should
+   (equal "# Some Org\n# comment"
+  (org-test-with-temp-text "# Some Org <point>comment"
+                           (call-interactively #'default-indent-new-line)
+                           (buffer-string)))))


 ;;; Editing
-- 
2.34.0





--
Kaushal Modi


On Mon, Dec 6, 2021 at 8:17 AM Richard Lawrence <
richard.lawrence@uni-tuebingen.de> wrote:

> Kaushal Modi <kaushal.modi@gmail.com> writes:
>
> > On Sat, Dec 4, 2021, 5:25 PM Tim Cross <theophilusx@gmail.com> wrote:
> >
> >> Given that Nicholas cannot remember the reason for the original function
> >> and suspects it was meant to be an internal only function, I think this
> >> patch is probably the best way forward and should be applied.
> >
> > Thanks. Nicolas asked me to add tests for this patch. But I need to look
> > into how to add tests for behavior of bindings. Need to add tests for M-j
> > binding behavior when cursor is within a comment or outside.
>
> FWIW I have been running the equivalent of Kaushal's patch (I just
> removed the local binding of comment-line-break-function to
> 'org-comment-line-break-function without deleting the function) and have
> also set
>
> (debug-on-entry 'org-comment-line-break-function)
>
> I've been running this for at least a week with no issues, and have
> never been dropped into the debugger. So at least the testing through my
> normal work indicates there are no problems with the patch.
>
> --
> Best,
> Richard
>

[-- Attachment #2: Type: text/html, Size: 6889 bytes --]

Re: [PATCH] Fix org-comment-line-break-function

[Nicolas Goaziou]

Hello,

Kaushal Modi <kaushal.modi@gmail.com> writes:

> I have added few tests in the updated patch pasted in this email.
> I have made the tests for (call-interactive #'default-indent-new-line)
> because that the interactive function M-j is bound to by default.
>
> Can you please review and commit it? The machine I am on right now does not
> allow external ssh access.

Unfortunately, this seems to break "test-org/auto-fill-function" test.
Could you have a look at it?

Regards,
-- 
Nicolas Goaziou

Re: [PATCH] Fix org-comment-line-break-function

[Ihor Radchenko]

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

Kaushal Modi <kaushal.modi@gmail.com> writes:

> Hi Nicolas,
>
> I have added few tests in the updated patch pasted in this email.
> I have made the tests for (call-interactive #'default-indent-new-line)
> because that the interactive function M-j is bound to by default.
>
> Can you please review and commit it? The machine I am on right now does not
> allow external ssh access.

Unfortunately, we cannot rely on the built-in `comment-indent-new-line'
to fill Org comments. This is because Emacs uses a complex entanglement
of regexp heuristics to determine comment at point and its boundaries.

I am attaching an alternative patch to fix the issue using Org element
parser. Note that I reused your tests.

Best,
Ihor


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-org-comment-line-break-function-Avoid-built-in-Emacs.patch --]
[-- Type: text/x-patch, Size: 3416 bytes --]

From ded35b55ca694e3eb831878160ac37ceec48b08e Mon Sep 17 00:00:00 2001
Message-Id: <ded35b55ca694e3eb831878160ac37ceec48b08e.1664428038.git.yantar92@gmail.com>
From: Ihor Radchenko <yantar92@gmail.com>
Date: Thu, 29 Sep 2022 13:02:46 +0800
Subject: [PATCH] org-comment-line-break-function: Avoid built-in Emacs comment
 machinery

* lisp/org.el (org-comment-line-break-function): Rely on Org
parser (`org-adaptive-fill-function') to determine comment filling.
Handle nil values of `fill-prefix' correctly.

* testing/lisp/test-org.el (test-org/default-indent-new-line): New
test.  Written by Kaushal Modi <kaushal.modi@gmail.com>
https://list.orgmode.org/CAFyQvY36DkBSNy2mPxDNZWeoTjUK8mAqgJM-zHxNamfReqGkuQ@mail.gmail.com/

Reported-by: Richard Lawrence <richard.lawrence@uni-tuebingen.de>
Link: https://list.orgmode.org/87lf18fue9.fsf@aquinas.i-did-not-set--mail-host-address--so-tickle-me/
---
 lisp/org.el              | 18 ++++++++++++------
 testing/lisp/test-org.el | 20 ++++++++++++++++++++
 2 files changed, 32 insertions(+), 6 deletions(-)

diff --git a/lisp/org.el b/lisp/org.el
index 036384a04..5ff60baf6 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -19295,12 +19295,18 @@ (defun org-comment-line-break-function (&optional soft)
   "Break line at point and indent, continuing comment if within one.
 The inserted newline is marked hard if variable
 `use-hard-newlines' is true, unless optional argument SOFT is
-non-nil."
-  (if soft (insert-and-inherit ?\n) (newline 1))
-  (save-excursion (forward-char -1) (delete-horizontal-space))
-  (delete-horizontal-space)
-  (indent-to-left-margin)
-  (insert-before-markers-and-inherit fill-prefix))
+non-nil.
+
+This function is a simplified version of `comment-indent-new-line'
+that bypasses the complex Emacs machinery dealing with comments.
+We instead rely on Org parser, utilizing `org-adaptive-fill-function'"
+  (let ((fill-prefix (org-adaptive-fill-function)))
+    (if soft (insert-and-inherit ?\n) (newline 1))
+    (save-excursion (forward-char -1) (delete-horizontal-space))
+    (delete-horizontal-space)
+    (indent-to-left-margin)
+    (when fill-prefix
+      (insert-before-markers-and-inherit fill-prefix))))
 
 
 ;;; Fixed Width Areas
diff --git a/testing/lisp/test-org.el b/testing/lisp/test-org.el
index 1b4157d0e..4a6a3a0b0 100644
--- a/testing/lisp/test-org.el
+++ b/testing/lisp/test-org.el
@@ -1405,6 +1405,26 @@ (ert-deftest test-org/indent-region ()
 	    (org-indent-region (point-min) (point-max))
 	    (buffer-string)))))
 
+(ert-deftest test-org/default-indent-new-line ()
+  "Test behavior of default binding `M-j'."
+  ;; Calling `M-j' when point is not in an Org comment:
+  (should
+   (equal "* Some heading\n"
+  (org-test-with-temp-text "* Some heading<point>"
+                           (call-interactively #'default-indent-new-line)
+                           (buffer-string))))
+  ;; Calling `M-j' when point is in an Org comment:
+  (should
+   (equal "# Some Org comment\n# "
+  (org-test-with-temp-text "# Some Org comment<point>"
+                           (call-interactively #'default-indent-new-line)
+                           (buffer-string))))
+  (should
+   (equal "# Some Org\n# comment"
+  (org-test-with-temp-text "# Some Org <point>comment"
+                           (call-interactively #'default-indent-new-line)
+                           (buffer-string)))))
+
 
 \f
 ;;; Editing
-- 
2.35.1


[-- Attachment #3: Type: text/plain, Size: 206 bytes --]


-- 
Ihor Radchenko,
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] Fix org-comment-line-break-function

[Ihor Radchenko]

Richard Lawrence <rwl@recursewithless.net> writes:

>> I am attaching an alternative patch to fix the issue using Org element
>> parser. Note that I reused your tests.
>
> FWIW I've tested this patch for a few days and it fixes the issue for
> me. I'd be happy if it were merged.

Thanks for testing.
Applied onto main.
https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=506d5b12ddbb319f0f70593cda54d8d9c580db7b

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

Fixed.
Applied.