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

[Tim Cross <theophilusx@gmail.com>]

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.