bug#46364: regression in lm-commentary

bug#46364: regression in lm-commentary

[Boruch Baum]

In emacs 26, lm-commentary functions fine.
In the emacs-snapshot that I'm using from ~2020-09, it does not
correctly indent sub headings.

This was observed for the lisp file[1], as processed by MELPA[2], and
reproduced by me locally. I don't know what version MELPA is using.

My correspondence with MELPA support can be found at the github issue
here[3].

refs:
=====
[1] https://github.com/Boruch-Baum/emacs-diredc
[2] https://melpa.org/#/diredc
[3] https://github.com/melpa/melpa/issues/7398

--
hkp://keys.gnupg.net
CA45 09B5 5351 7C11 A9D1  7286 0036 9E45 1595 8BC0

bug#46364: regression in lm-commentary

[Lars Ingebrigtsen]

Boruch Baum <boruch_baum@gmx.com> writes:

> In emacs 26, lm-commentary functions fine.
> In the emacs-snapshot that I'm using from ~2020-09, it does not
> correctly indent sub headings.
>
> This was observed for the lisp file[1], as processed by MELPA[2], and
> reproduced by me locally. I don't know what version MELPA is using.

I'm not sure how Melpa is involved here?  Is the problem that
(lm-commentary "/that/file") doesn't return the correct result on some
particular file?  If so, can you include the file here in the bug
tracker, as well as instructions for how to reproduce the bug?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#46364: regression in lm-commentary

[Boruch Baum]

On 2021-02-07 19:38, Lars Ingebrigtsen wrote:
> Boruch Baum <boruch_baum@gmx.com> writes:
>
> > In emacs 26, lm-commentary functions fine.
> > In the emacs-snapshot that I'm using from ~2020-09, it does not
> > correctly indent sub headings.
> >
> > This was observed for the lisp file[1], as processed by MELPA[2], and
> > reproduced by me locally. I don't know what version MELPA is using.
>
> I'm not sure how Melpa is involved here?

I thought I was clear about that. They produced the problem. I gave you
links to the file, and the MELPA URL of the erroneous output, and the
URL of the github discussion.

> Is the problem that (lm-commentary "/that/file") doesn't return the
> correct result on some particular file?

Isn't that what I wrote? What was't clear about what I wrote? I'm
looking at it now and I don't see any ambiguity.

> If so, can you include the file here in the bug tracker, as well as
> instructions for how to reproduce the bug?

You know what, Lars, just forget it. Just forget I ever reported this
bug. Go ahead and close it. I'm not in the mood today for this. Let
someone else report it sometime in the future and deal with

--
hkp://keys.gnupg.net
CA45 09B5 5351 7C11 A9D1  7286 0036 9E45 1595 8BC0

bug#46364: regression in lm-commentary

[Basil L. Contovounesios]

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

reopen 46364 !
tags 46364 - moreinfo
tags 46364 + confirmed
found 46364 28.0.50
quit

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Boruch Baum <boruch_baum@gmx.com> writes:
>
>> In emacs 26, lm-commentary functions fine.
>> In the emacs-snapshot that I'm using from ~2020-09, it does not
>> correctly indent sub headings.
>>
>> This was observed for the lisp file[1], as processed by MELPA[2], and
>> reproduced by me locally. I don't know what version MELPA is using.
>
> I'm not sure how Melpa is involved here?  Is the problem that
> (lm-commentary "/that/file") doesn't return the correct result on some
> particular file?  If so, can you include the file here in the bug
> tracker, as well as instructions for how to reproduce the bug?

I attach the file in question, diredc.el.  Here's the recipe:

0. emacs -Q
1. (require 'lisp-mnt) C-j
2. (lm-commentary "/path/to/diredc.el") C-j

In Emacs 27, the resulting string includes leading semicolons and
indentation.  In Emacs 28, it doesn't.  Version info follows my
signature.

Thanks,

-- 
Basil

In GNU Emacs 28.0.50 (build 1, x86_64-pc-linux-gnu, X toolkit, cairo version 1.16.0, Xaw3d scroll bars)
 of 2021-02-07 built on tia
Repository revision: 7c5938ad7d8884d03471e2395937e11611faadb9
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12010000
System Description: Debian GNU/Linux bullseye/sid

Configured using:
 'configure 'CC=ccache gcc' 'CFLAGS=-O2 -march=native' --config-cache
 --prefix=/home/blc/.local --enable-checking=structs
 --with-x-toolkit=lucid --with-file-notification=yes --with-x'

Configured features:
ACL CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GPM GSETTINGS HARFBUZZ JPEG
JSON LCMS2 LIBOTF LIBSELINUX LIBSYSTEMD LIBXML2 M17N_FLT MODULES NOTIFY
INOTIFY PDUMPER PNG RSVG SOUND THREADS TIFF TOOLKIT_SCROLL_BARS X11
XAW3D XDBE XIM XPM LUCID ZLIB

In GNU Emacs 27.1.91 (build 1, x86_64-pc-linux-gnu, X toolkit, cairo version 1.16.0, Xaw3d scroll bars)
 of 2021-02-06 built on tia
Repository revision: 8ad48a0bdd0806fe3bfbabf00c845381d9107cb0
Repository branch: emacs-27
Windowing system distributor 'The X.Org Foundation', version 11.0.12010000
System Description: Debian GNU/Linux bullseye/sid

Recent messages:
For information about GNU Emacs and the GNU system, type C-h C-a.
Mark set [2 times]

Configured using:
 'configure 'CC=ccache gcc' 'CFLAGS=-O0 -g3 -ggdb -gdwarf-4'
 --config-cache --prefix=/home/blc/.local --program-suffix=-27
 --enable-checking=yes,glyphs --enable-check-lisp-object-type
 --with-x-toolkit=lucid --with-file-notification=yes --with-x
 --with-cairo'

Configured features:
XAW3D XPM JPEG TIFF GIF PNG RSVG CAIRO SOUND GPM DBUS GSETTINGS GLIB
NOTIFY INOTIFY ACL LIBSELINUX GNUTLS LIBXML2 FREETYPE HARFBUZZ M17N_FLT
LIBOTF ZLIB TOOLKIT_SCROLL_BARS LUCID X11 XDBE XIM MODULES THREADS
LIBSYSTEMD JSON PDUMPER LCMS2 GMP

Important settings:
  value of $LANG: en_IE.UTF-8
  value of $XMODIFIERS: @im=ibus
  locale-coding-system: utf-8-unix


[-- Attachment #2: diredc.el --]
[-- Type: application/emacs-lisp, Size: 101988 bytes --]

bug#46364: regression in lm-commentary

[Matt Armstrong]

"Basil L. Contovounesios" <contovob@tcd.ie> writes:

> I attach the file in question, diredc.el.  Here's the recipe:
>
> 0. emacs -Q
> 1. (require 'lisp-mnt) C-j
> 2. (lm-commentary "/path/to/diredc.el") C-j
>
> In Emacs 27, the resulting string includes leading semicolons and
> indentation.  In Emacs 28, it doesn't.  Version info follows my
> signature.

Thanks Basil. I investigated this too and filed a new bug with the
information below (because I don't know debbugs well). My investigation
belongs here instead.

Lars asked about MELPA. Answer: MELPA is relevant only because in some
cases MELPA code calls `lm-commentary' to form user-facing description
of a package.

The issue is a behavior change in `lm-commentary' that probably deserves
some consideration, because it seems suboptimal in this example. The
behavior change is most likely caused by commit
963a9ffd66cb29f0370e9a4b854dddda242c54a6.

Prior to that commit, and in Emacs 27.1, the function returns the file's
commentary as an verbatim substring of the elisp source, including the
elisp comment characters, commentary headline, etc. E.g. a string like
this (I'll quote the string for email sanity):

> ";;; Commentary:
> ;;
> ;; This package extends and configures `dired' with features found in
> ;; almost all 'file managers', and also some unique features:
> ;;
> ;;   * Resilient dedicated dual-pane frame.
> ;;     * similar look to 'midnight commander'.
> ;;     * intelligent recovery of manually altered frame configuration
> ;;     * exit diredc/dired cleanly and totally
[...]

After that commit, the function returns a "sanitized" version of the
same content, such as:

> "
> This package extends and configures `dired' with features found in
> almost all 'file managers', and also some unique features:
> 
> * Resilient dedicated dual-pane frame.
> * similar look to 'midnight commander'.
> * intelligent recovery of manually altered frame configuration
> * exit diredc/dired cleanly and totally
[...]

It seems `lm-commentary' now strips all leading whitespace from every
line, as a "sanitization" step, and this has the unsatisfying side
effect of ruining any indentation formatting in the original commentary.

We need not go farther than Emacs' own lisp/align.el to see a similar
problem.

In Emacs 27, (lm-commentary "align.el") returns:

> ";;; Commentary:
> 
> ;; This mode allows you to align regions in a context-sensitive fashion.
> ;; The classic use is to align assignments:
> ;;
> ;;    int a = 1;
> ;;    short foo = 2;
> ;;    double blah = 4;
> ;;
> ;; becomes
> ;;
> ;;    int    a    = 1;
> ;;    short  foo  = 2;
> ;;    double blah = 4;
> 
> "

And in Emacs 28 (mainline):

> "This mode allows you to align regions in a context-sensitive fashion.
> The classic use is to align assignments:
> 
> int a = 1;
> short foo = 2;
> double blah = 4;
> 
> becomes
> 
> int    a    = 1;
> short  foo  = 2;
> double blah = 4;"

Perhaps this should be re-thought?

bug#46364: regression in lm-commentary

[Lars Ingebrigtsen]

Matt Armstrong <matt@rfc20.org> writes:

> The issue is a behavior change in `lm-commentary' that probably deserves
> some consideration, because it seems suboptimal in this example. The
> behavior change is most likely caused by commit
> 963a9ffd66cb29f0370e9a4b854dddda242c54a6.

Right.  The rationale was:

> I decided to tackle the problem’s root.  After figuring out that every
> function depending on ‘lm-commentary’ implemented their own ad-hoc
> sanitation for the same effect, I changed ‘lm-commentary’ to return a
> sanitized string and removed the code/functionality duplication from all
> callers.

(I've added Bruno to the CCs.)

> Prior to that commit, and in Emacs 27.1, the function returns the file's
> commentary as an verbatim substring of the elisp source, including the
> elisp comment characters, commentary headline, etc. E.g. a string like
> this (I'll quote the string for email sanity):

[...]

> It seems `lm-commentary' now strips all leading whitespace from every
> line, as a "sanitization" step, and this has the unsatisfying side
> effect of ruining any indentation formatting in the original commentary.

Is the removal of the leading white-space the only problem with the
sanitization?  Then perhaps that bit could be tweaked?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#46364: regression in lm-commentary

[Matt Armstrong]

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

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Matt Armstrong <matt@rfc20.org> writes:

[...]

>> It seems `lm-commentary' now strips all leading whitespace from every
>> line, as a "sanitization" step, and this has the unsatisfying side
>> effect of ruining any indentation formatting in the original commentary.
>
> Is the removal of the leading white-space the only problem with the
> sanitization?  Then perhaps that bit could be tweaked?

I noticed that Basil's commit 963a9ffd66cb29f0370e9a4b854dddda242c54a6
consolidated normalization logic but also changed the regex slightly
such that all leading whitespace was erased. I've attached a patch to go
back to the old ways. It seems to work.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: preserve leading whitespace --]
[-- Type: text/x-patch, Size: 1045 bytes --]

From eecfa79644edbda5830f77664379a0df59b76929 Mon Sep 17 00:00:00 2001
From: Matt Armstrong <matt@rfc20.org>
Date: Mon, 8 Feb 2021 10:44:34 -0800
Subject: [PATCH] Preserve leading whitespace in `lm-commentary'.

* lisp/emacs-lisp/lisp-mnt.el (lm-commentary): Preserve leading
whitespace. (Bug#46364)
---
 lisp/emacs-lisp/lisp-mnt.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lisp/emacs-lisp/lisp-mnt.el b/lisp/emacs-lisp/lisp-mnt.el
index adb9cb2372..6d9c8c3279 100644
--- a/lisp/emacs-lisp/lisp-mnt.el
+++ b/lisp/emacs-lisp/lisp-mnt.el
@@ -495,7 +495,7 @@ lm-commentary
                   (concat "^;;;[[:blank:]]*\\("
                           lm-commentary-header
                           "\\):[[:blank:]\n]*")
-                  "^;;[[:blank:]]*"     ; double semicolon prefix
+                  "^;;[[:blank:]]?"     ; double semicolon prefix
                   "[[:blank:]\n]*\\'")  ; trailing new-lines
           "" (buffer-substring-no-properties
               start (lm-commentary-end))))))))
-- 
2.30.0

bug#46364: regression in lm-commentary

[Basil L. Contovounesios]

Matt Armstrong <matt@rfc20.org> writes:

> Lars Ingebrigtsen <larsi@gnus.org> writes:
>
>> Matt Armstrong <matt@rfc20.org> writes:
>
> [...]
>
>>> It seems `lm-commentary' now strips all leading whitespace from every
>>> line, as a "sanitization" step, and this has the unsatisfying side
>>> effect of ruining any indentation formatting in the original commentary.
>>
>> Is the removal of the leading white-space the only problem with the
>> sanitization?  Then perhaps that bit could be tweaked?
>
> I noticed that Basil's commit 963a9ffd66cb29f0370e9a4b854dddda242c54a6
                 ^^^^^^^
                 Bruno's ;)

> consolidated normalization logic but also changed the regex slightly
> such that all leading whitespace was erased. I've attached a patch to go
> back to the old ways. It seems to work.

Given that lm-commentary is used outside of Emacs, I suggest its
behaviour be reverted to that in Emacs 27, and any sanitisation provided
as a separate function instead.

Thanks,

-- 
Basil

bug#46364: regression in lm-commentary

[Matt Armstrong]

"Basil L. Contovounesios" <contovob@tcd.ie> writes:

> Matt Armstrong <matt@rfc20.org> writes:
>> Lars Ingebrigtsen <larsi@gnus.org> writes:
>>> Matt Armstrong <matt@rfc20.org> writes:
>>
>> [...]
>>
>>>> It seems `lm-commentary' now strips all leading whitespace from every
>>>> line, as a "sanitization" step, and this has the unsatisfying side
>>>> effect of ruining any indentation formatting in the original commentary.
>>>
>>> Is the removal of the leading white-space the only problem with the
>>> sanitization?  Then perhaps that bit could be tweaked?
>>
>> I noticed that Basil's commit 963a9ffd66cb29f0370e9a4b854dddda242c54a6
>                  ^^^^^^^
>                  Bruno's ;)

Yes, thank you.  :-)


>> consolidated normalization logic but also changed the regex slightly
>> such that all leading whitespace was erased. I've attached a patch to go
>> back to the old ways. It seems to work.
>
> Given that lm-commentary is used outside of Emacs, I suggest its
> behaviour be reverted to that in Emacs 27, and any sanitisation provided
> as a separate function instead.

I leave the decision to Lars and other Emacs maintainers.

My opinion: while introducing a new function is surely safer, we should
submit the patch I proposed. Why?

I did a web search looking for callers of lm-commentary outside of Emacs
proper. In every case I found code that called lm-commentary and then
tried to do sanitization of the result (by removing the comment leaders
from each line, etc.).

For example, this is what MELPA does. See also :
https://www.emacswiki.org/emacs/finder+.el

It seems like the "sanitize" behavior is what callers want.

bug#46364: regression in lm-commentary

[Lars Ingebrigtsen]

Matt Armstrong <matt@rfc20.org> writes:

> I noticed that Basil's commit 963a9ffd66cb29f0370e9a4b854dddda242c54a6
> consolidated normalization logic but also changed the regex slightly
> such that all leading whitespace was erased. I've attached a patch to go
> back to the old ways. It seems to work.

Makes sense to me, so I've pushed this to Emacs 28 now.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no