bug#37548: Implement sanitation of single-file package long description

bug#37548: Implement sanitation of single-file package long description

[Bruno Félix Rezende Ribeiro]

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

Hello Emacs developers,

The inlined patch implements sanitation of single-file package’s long
description which is derived from the package’s commentary header
section.  It removes the commentary header, the double semicolon prefix
of each line, trailing new-lines and trailing white-space.  I think this
is the usual practice for packages in GNU ELPA and MELPA repositories.
Furthermore it’s aligned with the intended behavior for multi-file
packages which is to read the long description from a README file[1] ---
which presumably does not have commentary sections nor double semicolon
prefixes.


Please, let me know of any changes required.
Thanks!


PS: For some reason I was not able to use a single regexp within a
single invocation of ‘replace-regexp-in-string’, as would be natural.
It simply didn’t work as expected.  It’s working fine now with nested
calls.



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: package-x-single-file-package-long-description-sanitization.patch --]
[-- Type: text/x-diff, Size: 1609 bytes --]

diff --git a/lisp/emacs-lisp/package-x.el b/lisp/emacs-lisp/package-x.el
index 2815be3..7fe6f6d 100644
--- a/lisp/emacs-lisp/package-x.el
+++ b/lisp/emacs-lisp/package-x.el
@@ -159,6 +159,7 @@ DESCRIPTION is the text of the news item."
 
 (declare-function lm-commentary "lisp-mnt" (&optional file))
 (defvar tar-data-buffer)
+(defvar lm-commentary-header)
 
 (defun package-upload-buffer-internal (pkg-desc extension &optional archive-url)
   "Upload a package whose contents are in the current buffer.
@@ -204,7 +205,17 @@ if it exists."
 	       (split-version (package-desc-version pkg-desc))
 	       (commentary
                 (pcase file-type
-                  ('single (lm-commentary))
+                  ('single (replace-regexp-in-string ; Get rid of...
+                            "[[:blank:]]*$" "" ; trailing white-space
+                            (replace-regexp-in-string
+                             (format "%s\\|%s\\|%s"
+                                     ;; commentary header
+                                     (concat "^;;;[[:blank:]]*\\("
+                                             lm-commentary-header
+                                             "\\):[[:blank:]\n]*")
+                                     "^;;[[:blank:]]*" ; double semicolon prefix
+                                     "[[:blank:]\n]*\\'") ; trailing new-lines
+                             "" (lm-commentary))))
                   ('tar nil))) ;; FIXME: Get it from the README file.
                (extras (package-desc-extras pkg-desc))
 	       (pkg-version (package-version-join split-version))

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



Footnotes: 
[1]  I’ve implemented that in bug#37546.

-- 
 88888  FFFFF Bruno Félix Rezende Ribeiro (oitofelix) [0x28D618AF]
 8   8  F     http://oitofelix.freeshell.org/
 88888  FFFF  mailto:oitofelix@gnu.org
 8   8  F     irc://chat.freenode.org/oitofelix
 88888  F     xmpp://oitofelix@riseup.net

bug#37548: Implement sanitation of single-file package long description

[Stefan Kangas]

Bruno Félix Rezende Ribeiro <oitofelix@gnu.org> writes:

> Hello Emacs developers,

Hi Bruno,

And thanks for your patch.

> The inlined patch implements sanitation of single-file package’s long
> description which is derived from the package’s commentary header
> section.  It removes the commentary header, the double semicolon prefix
> of each line, trailing new-lines and trailing white-space.  I think this
> is the usual practice for packages in GNU ELPA and MELPA repositories.
> Furthermore it’s aligned with the intended behavior for multi-file
> packages which is to read the long description from a README file[1] ---
> which presumably does not have commentary sections nor double semicolon
> prefixes.

I agree with the change.  However, there seems to be code duplication
here, since the same is done in package.el:

          ;; For built-in packages, get the description from the
          ;; Commentary header.
          (let ((fn (locate-file (format "%s.el" name) load-path
                                 load-file-rep-suffixes))
                (opoint (point)))
            (insert (or (lm-commentary fn) ""))
            (save-excursion
              (goto-char opoint)
              (when (re-search-forward "^;;; Commentary:\n" nil t)
                (replace-match ""))
              (while (re-search-forward "^\\(;+ ?\\)" nil t)
                (replace-match ""))))

Maybe it would make more sense to create a new function in package.el
that takes care of this?  That way we don't have the same
functionality in two places.

FWIW, I would probably prefer to base it on the code already in
package.el, since I find it a bit easier to read when the regular
expressions are split up.

Best regards,
Stefan Kangas

bug#37548: Implement sanitation of single-file package long description

[Stefan Kangas]

Stefan Kangas <stefan@marxist.se> writes:

> Maybe it would make more sense to create a new function in package.el
> that takes care of this?  That way we don't have the same
> functionality in two places.

I noticed something else:  There is actually already code duplication
in package.el -- there is code to strip the commentary section in both
package--get-description and describe-package-1.

Perhaps it would make sense to look this all over and see how we can do better?

I also have two general questions, which are applicable to both your
recent patches:

1. It looks likely that this together with your other patch and your
previous contributions will together amount to more than 15 lines of
code.  That means that you would have to sign Copyright Assignment
papers for GNU Emacs.  I see you're emailing from gnu.org, so I assume
there are no surprises for you here; I guess Eli can help you sort
that out if it's not already.

2. Could you please provide a commit message formatted as a changelog
entry?  Details on this are in the CONTRIBUTE file in the repository.

Best regards,
Stefan Kangas

bug#37548: Implement sanitation of single-file package long description

[Bruno Félix Rezende Ribeiro]

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

> Stefan Kangas <stefan@marxist.se> writes:
>
> I noticed something else:  There is actually already code duplication
> in package.el -- there is code to strip the commentary section in both
> package--get-description and describe-package-1.
>
> Perhaps it would make sense to look this all over and see how we can do better?

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 also have two general questions, which are applicable to both your
> recent patches:
>
> 1. It looks likely that this together with your other patch and your
> previous contributions will together amount to more than 15 lines of
> code.  That means that you would have to sign Copyright Assignment
> papers for GNU Emacs.  I see you're emailing from gnu.org, so I assume
> there are no surprises for you here; I guess Eli can help you sort
> that out if it's not already.

I’ve assigned my copyright for work on Emacs to the FSF already.


> 2. Could you please provide a commit message formatted as a changelog
> entry?  Details on this are in the CONTRIBUTE file in the repository.

Please, find it in the patch attached.



[-- Attachment #2: 0001-Globally-sanitize-single-file-package-long-descripti.patch --]
[-- Type: text/x-diff, Size: 6755 bytes --]

From d3e3983fc6cb74900bfa99f0bfcf2497ab396d67 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bruno=20F=C3=A9lix=20Rezende=20Ribeiro?= <oitofelix@gnu.org>
Date: Tue, 8 Oct 2019 04:32:18 -0300
Subject: [PATCH] Globally sanitize single-file package long descriptions
 (Bug#37548)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Consistent with multi-file package descriptions which don’t have
commentary sections nor double semicolon prefixes.
* lisp/emacs-lisp/lisp-mnt.el (lm-commentary): Remove commentary
header, double semicolon prefixes of each line, trailing new-lines and
trailing white-space from commentary.
* lisp/emacs-lisp/package.el (package--get-description)
(describe-package-1):
* lisp/finder.el (finder-commentary):
* lisp/info.el (Info-finder-find-node): remove ad-hoc sanitation.
---
 lisp/emacs-lisp/lisp-mnt.el | 14 +++++++++++++-
 lisp/emacs-lisp/package.el  | 30 ++++++++----------------------
 lisp/finder.el              |  8 +-------
 lisp/info.el                | 16 ++--------------
 4 files changed, 24 insertions(+), 44 deletions(-)

diff --git a/lisp/emacs-lisp/lisp-mnt.el b/lisp/emacs-lisp/lisp-mnt.el
index 91c7615..dda7895 100644
--- a/lisp/emacs-lisp/lisp-mnt.el
+++ b/lisp/emacs-lisp/lisp-mnt.el
@@ -4,6 +4,7 @@
 ;; Inc.
 
 ;; Author: Eric S. Raymond <esr@snark.thyrsus.com>
+;;         Bruno Félix Rezende Ribeiro <oitofelix@gnu.org>
 ;; Maintainer: emacs-devel@gnu.org
 ;; Created: 14 Jul 1992
 ;; Keywords: docs
@@ -485,7 +486,18 @@ absent, return nil."
   (lm-with-file file
     (let ((start (lm-commentary-start)))
       (when start
-        (buffer-substring-no-properties start (lm-commentary-end))))))
+        (replace-regexp-in-string       ; Get rid of...
+         "[[:blank:]]*$" ""             ; trailing white-space
+         (replace-regexp-in-string
+          (format "%s\\|%s\\|%s"
+                  ;; commentary header
+                  (concat "^;;;[[:blank:]]*\\("
+                          lm-commentary-header
+                          "\\):[[:blank:]\n]*")
+                  "^;;[[:blank:]]*"     ; double semicolon prefix
+                  "[[:blank:]\n]*\\'")  ; trailing new-lines
+          "" (buffer-substring-no-properties
+              start (lm-commentary-end))))))))
 
 (defun lm-homepage (&optional file)
   "Return the homepage in file FILE, or current buffer if FILE is nil."
diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
index ab1fb8b..f65559d 100644
--- a/lisp/emacs-lisp/package.el
+++ b/lisp/emacs-lisp/package.el
@@ -4,6 +4,7 @@
 
 ;; Author: Tom Tromey <tromey@redhat.com>
 ;;         Daniel Hackney <dan@haxney.org>
+;;         Bruno Félix Rezende Ribeiro <oitofelix@gnu.org>
 ;; Created: 10 Mar 2007
 ;; Version: 1.1.0
 ;; Keywords: tools
@@ -2349,18 +2350,9 @@ The description is read from the installed package files."
      result
 
      ;; Look for Commentary header.
-     (let ((mainsrcfile (expand-file-name (format "%s.el" (package-desc-name desc))
-                                          srcdir)))
-       (when (file-readable-p mainsrcfile)
-         (with-temp-buffer
-           (insert (or (lm-commentary mainsrcfile) ""))
-           (goto-char (point-min))
-           (when (re-search-forward "^;;; Commentary:\n" nil t)
-             (replace-match ""))
-           (while (re-search-forward "^\\(;+ ?\\)" nil t)
-             (replace-match ""))
-           (buffer-string))))
-     )))
+     (or (lm-commentary (expand-file-name
+                         (format "%s.el" (package-desc-name desc)) srcdir))
+         ""))))
 
 (defun describe-package-1 (pkg)
   "Insert the package description for PKG.
@@ -2555,16 +2547,10 @@ Helper function for `describe-package'."
       (if built-in
           ;; For built-in packages, get the description from the
           ;; Commentary header.
-          (let ((fn (locate-file (format "%s.el" name) load-path
-                                 load-file-rep-suffixes))
-                (opoint (point)))
-            (insert (or (lm-commentary fn) ""))
-            (save-excursion
-              (goto-char opoint)
-              (when (re-search-forward "^;;; Commentary:\n" nil t)
-                (replace-match ""))
-              (while (re-search-forward "^\\(;+ ?\\)" nil t)
-                (replace-match ""))))
+          (insert (or (lm-commentary (locate-file (format "%s.el" name)
+                                                  load-path
+                                                  load-file-rep-suffixes))
+                      ""))
 
         (if (package-installed-p desc)
             ;; For installed packages, get the description from the
diff --git a/lisp/finder.el b/lisp/finder.el
index 89706cf..02d25ec 100644
--- a/lisp/finder.el
+++ b/lisp/finder.el
@@ -4,6 +4,7 @@
 ;; Inc.
 
 ;; Author: Eric S. Raymond <esr@snark.thyrsus.com>
+;;         Bruno Félix Rezende Ribeiro <oitofelix@gnu.org>
 ;; Created: 16 Jun 1992
 ;; Version: 1.0
 ;; Keywords: help
@@ -394,13 +395,6 @@ FILE should be in a form suitable for passing to `locate-library'."
     (erase-buffer)
     (insert str)
     (goto-char (point-min))
-    (delete-blank-lines)
-    (goto-char (point-max))
-    (delete-blank-lines)
-    (goto-char (point-min))
-    (while (re-search-forward "^;+ ?" nil t)
-      (replace-match "" nil nil))
-    (goto-char (point-min))
     (while (re-search-forward "\\<\\([-[:alnum:]]+\\.el\\)\\>" nil t)
       (if (locate-library (match-string 1))
           (make-text-button (match-beginning 1) (match-end 1)
diff --git a/lisp/info.el b/lisp/info.el
index 02f3ea5..4262219 100644
--- a/lisp/info.el
+++ b/lisp/info.el
@@ -3780,20 +3780,8 @@ Build a menu of the possible matches."
     ;; there is no "nxml.el" (it's nxml-mode.el).
     ;; But package.el makes the same assumption.
     ;; I think nxml is the only exception - maybe it should be just be renamed.
-    (let ((str (ignore-errors (lm-commentary (find-library-name nodename)))))
-      (if (null str)
-	  (insert "Can’t find package description.\n\n")
-	(insert
-	 (with-temp-buffer
-	   (insert str)
-	   (goto-char (point-min))
-	   (delete-blank-lines)
-	   (goto-char (point-max))
-	   (delete-blank-lines)
-	   (goto-char (point-min))
-	   (while (re-search-forward "^;+ ?" nil t)
-	     (replace-match "" nil nil))
-	   (buffer-string))))))))
+    (insert (or (ignore-errors (lm-commentary (find-library-name nodename)))
+                (insert "Can’t find package description.\n\n"))))))
 
 ;;;###autoload
 (defun info-finder (&optional keywords)
-- 
2.7.4


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


-- 
Bruno Félix Rezende Ribeiro (oitofelix) [0x28D618AF]
<http://oitofelix.freeshell.org/>

bug#37548: Implement sanitation of single-file package long description

[Eli Zaretskii]

> From: Bruno Félix Rezende Ribeiro <oitofelix@gnu.org>
> Date: Tue, 08 Oct 2019 05:36:40 -0300
> Cc: Bruno Félix Rezende Ribeiro <oitofelix@gnu.org>,
>  37548@debbugs.gnu.org
> 
> > 1. It looks likely that this together with your other patch and your
> > previous contributions will together amount to more than 15 lines of
> > code.  That means that you would have to sign Copyright Assignment
> > papers for GNU Emacs.  I see you're emailing from gnu.org, so I assume
> > there are no surprises for you here; I guess Eli can help you sort
> > that out if it's not already.
> 
> I’ve assigned my copyright for work on Emacs to the FSF already.

Right, Bruno's copyright assignment is on file.

bug#37548: Implement sanitation of single-file package long description

[Stefan Kangas]

Bruno Félix Rezende Ribeiro <oitofelix@gnu.org> writes:

>> Stefan Kangas <stefan@marxist.se> writes:
>>
>> I noticed something else:  There is actually already code duplication
>> in package.el -- there is code to strip the commentary section in both
>> package--get-description and describe-package-1.
>>
>> Perhaps it would make sense to look this all over and see how we can do better?
>
> 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.

Sorry for the late reply here.  I think your approach makes sense.

> I’ve assigned my copyright for work on Emacs to the FSF already.

Great, thanks.

> Please, find it in the patch attached.

I think the patch looks good, but I didn't test it yet.

By the way, it would be very good if you would like to add tests.  I
don't think a lack of tests should stop us from applying your patch.
But it would be a big plus to have them.

> --- a/lisp/emacs-lisp/lisp-mnt.el
> +++ b/lisp/emacs-lisp/lisp-mnt.el
> @@ -4,6 +4,7 @@
>  ;; Inc.
>  
>  ;; Author: Eric S. Raymond <esr@snark.thyrsus.com>
> +;;         Bruno Félix Rezende Ribeiro <oitofelix@gnu.org>
>  ;; Maintainer: emacs-devel@gnu.org
>  ;; Created: 14 Jul 1992
>  ;; Keywords: docs

> --- a/lisp/emacs-lisp/package.el
> +++ b/lisp/emacs-lisp/package.el
> @@ -4,6 +4,7 @@
>  
>  ;; Author: Tom Tromey <tromey@redhat.com>
>  ;;         Daniel Hackney <dan@haxney.org>
> +;;         Bruno Félix Rezende Ribeiro <oitofelix@gnu.org>
>  ;; Created: 10 Mar 2007
>  ;; Version: 1.1.0
>  ;; Keywords: tools

I think we don't usually add our names as authors in every file we
change.  We have other ways to track that, such as the AUTHORS file.

For this file, for instance, I see only one person in the author
field, but AFAICT there are 16 contributors with 99 commits in total.

Does anyone know if there is a general guideline for when to add your
name to the "Author" line at the top of the file?

Best regards,
Stefan Kangas

bug#37548: Implement sanitation of single-file package long description

[Eli Zaretskii]

> From: Stefan Kangas <stefan@marxist.se>
> Date: Mon, 11 Nov 2019 20:02:04 +0100
> Cc: 37548@debbugs.gnu.org
> 
> Does anyone know if there is a general guideline for when to add your
> name to the "Author" line at the top of the file?

Only when the file is first written, AFAIK.

bug#37548: Implement sanitation of single-file package long description

[Stefan Kangas]

close 37548 28.1
thanks

Stefan Kangas <stefan@marxist.se> writes:

>> Please, find it in the patch attached.
>
> I think the patch looks good, but I didn't test it yet.

Sorry for the long delay here.

I have now reviewed and tested your patch again, and also tested all
relevant functionality AFAICT.  I've pushed it to the master branch
with one or two minor stylistic changes, modulo this:

> I think we don't usually add our names as authors in every file we
> change.  We have other ways to track that, such as the AUTHORS file.

I'm consequently closing this bug.  Thank you again for your
contribution to Emacs.

Best regards,
Stefan Kangas