Re: [O] orgalist-mode: wrong indentation in message mode after recent change in emacs

[Basil L. Contovounesios]

[CCing Stefan as the nadvice.el expert.]

Gregor Zattler <address@hidden> writes:

> Dear org-mode developers, hello Nicolas,
>      I use orgalist-mode while writing emails in
>      notmuch-message-mode.  Today I updated emacs (from git), and
>      since then, the second line of a paragraph get's indented
>      (and succeeding lines too).
>
> As demonstrated with this email.  While I write this email with
>    my configurations, I checked with emacs -Q as follows: visited
>    one org file (in order for org-mode to load), visited
>    orgalist.el, eval'ed this buffer and started writing an email:
>    bang! indented succeeding lines.
>
>
>
> I bisected emacs like so:
>
> make ; src/emacs -Q --eval '(org-mode)' -l
> ~/.emacs.d/elpa-27.0/orgalist-1.9/orgalist.el -f compose-mail --eval
> '(orgalist-mode)' -f end-of-buffer
>
> and this is the relevant commit:
>
> 2e3deb09bd42d22a9b354937ce63b151fb493d8a is the first bad commit
> commit 2e3deb09bd42d22a9b354937ce63b151fb493d8a
> Author: Basil L. Contovounesios <address@hidden>
> Date:   Sun Mar 31 19:39:54 2019 +0100
>
>     Do not set indent-line-function in text-mode
>
>     For discussion, see thread starting at:
>     https://lists.gnu.org/archive/html/emacs-devel/2019-03/msg01012.html
>     * lisp/textmodes/text-mode.el (text-mode): Do not reset
>     indent-line-function to its global default value of indent-relative.
>     * doc/lispref/modes.texi (Example Major Modes):
>     * etc/NEWS: Document change accordingly.

[...]

> I do not know what to do with this.

This seems to be a relapse(?) of bug#31361[1], which you have correctly
identified as being caused by my commit.  The following outlines what
happens now (you can safely skip the next four paragraphs if you don't
care about the technical details).

text-mode no longer makes indent-line-function buffer-local.
orgalist-mode advises indent-line-function buffer-locally using
add-function.  add-function uses advice--buffer-local to find which
place form to set.  advice--buffer-local sees that indent-line-function
is not a local variable, so it wraps it in a custom closure before
making it buffer-local.

orgalist-mode also advises indent-according-to-mode in order to work
around bug#31361 by temporarily unwrapping the advice added to
indent-line-function.  This is done because indent-according-to-mode
behaves differently if indent-line-function is set to indent-relative or
indent-relative-first-indent-point (née indent-relative-maybe).

Back when text-mode made indent-line-function buffer-local before
orgalist-mode advised it, (advice--cd*r indent-line-function) would
simply return indent-relative, and the advice on
indent-according-to-mode worked as intended.

Now that add-function sets indent-line-function to a custom closure,
however, (advice--cd*r indent-line-function) no longer returns
indent-relative and the workaround breaks.

The simplest fix would be to make indent-line-function buffer-local in
orgalist-mode before advising it with add-function, as the code already
expects and relies on this:

diff --git a/orgalist.el b/orgalist.el
index 3a3cfaa..cc14430 100644
--- a/orgalist.el
+++ b/orgalist.el
@@ -818,6 +818,11 @@ orgalist-mode
     (add-function :before-until
                   (local 'fill-paragraph-function)
                   #'orgalist--fill-item)
+    ;; Unless `indent-line-function' is buffer-local before it is
+    ;; advised with `add-function', the workaround for bug#31361 below
+    ;; will not work, as (advice--cd*r indent-line-function) will not
+    ;; compare `eq' to `indent-relative' in `indent-according-to-mode'.
+    (make-local-variable 'indent-line-function)
     (add-function :before-until
                   (local 'indent-line-function)
                   #'orgalist--indent-line)

Another option, of course, would be to revert my commit.  I'm not in the
slightest against reverting, but I'm not yet convinced it's necessary,
as orgalist-mode currently seems to rely on the alignment of several
implementation details.

I do wonder about three things, though.

The first is whether orgalist-mode couldn't use a custom
indent-line-function instead of advising what may or may not be set to
indent-relative by the user.

The second is why advice--buffer-local does what it does.  Stefan, why
does it behave differently depending on local-variable-p?  Why can't it
simply call make-local-variable before returning the symbol-value?

The third is why indent-according-to-mode hard-codes the check for
indent-relative and indent-relative-first-indent-point.  Wouldn't it be
nice if this check instead looked up some variable akin to
electric-indent-functions-without-reindent, that can be more easily
customised?

[1]: https://debbugs.gnu.org/31361

Thanks for reporting this, and sorry for the fallout.

-- 
Basil