bug#28998: 27.0.50; Man--sections is in reverse order

bug#28998: 27.0.50; Man--sections is in reverse order

[Basil L. Contovounesios]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: 0001-lisp-man.el-Man-build-section-alist-Reverse-sections.patch --]
[-- Type: text/x-diff, Size: 820 bytes --]

From 824ff332efebc69b436e4893626a68125af20cd5 Mon Sep 17 00:00:00 2001
From: "Basil L. Contovounesios" <contovob@tcd.ie>
Date: Wed, 25 Oct 2017 16:57:43 +0100
Subject: [PATCH 1/2] * lisp/man.el (Man-build-section-alist): Reverse sections

---
 lisp/man.el | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lisp/man.el b/lisp/man.el
index 7a892c6e88..f7b1609c92 100644
--- a/lisp/man.el
+++ b/lisp/man.el
@@ -1522,7 +1522,8 @@ Man-build-section-alist
       (let ((section (match-string 1)))
         (unless (member section Man--sections)
           (push section Man--sections)))
-      (forward-line 1))))
+      (forward-line 1)))
+  (setq Man--sections (nreverse Man--sections)))
 
 (defsubst Man-build-references-alist ()
   "Build the list of references (in the SEE ALSO section)."
-- 
2.14.2


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0002-Rename-Man-build-section-alist.patch --]
[-- Type: text/x-diff, Size: 2113 bytes --]

From ecbe8f2d076cbfb61d447ab0faca8e581864d459 Mon Sep 17 00:00:00 2001
From: "Basil L. Contovounesios" <contovob@tcd.ie>
Date: Wed, 25 Oct 2017 17:03:12 +0100
Subject: [PATCH 2/2] Rename Man-build-section-alist

Man--sections is a list of strings, so rename inline function to
Man-build-section-list.

* lisp/man.el (Man-build-section-alist): Do it and reduce syntax.
(Man-goto-page):
* lisp/woman.el (woman-find-file): Use it.
---
 lisp/man.el   | 12 ++++++------
 lisp/woman.el |  2 +-
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/lisp/man.el b/lisp/man.el
index f7b1609c92..2ff25344fe 100644
--- a/lisp/man.el
+++ b/lisp/man.el
@@ -1513,16 +1513,16 @@ Man-mode
   (set (make-local-variable 'bookmark-make-record-function)
        'Man-bookmark-make-record))
 
-(defsubst Man-build-section-alist ()
+(defsubst Man-build-section-list ()
   "Build the list of manpage sections."
-  (setq Man--sections nil)
+  (setq Man--sections ())
   (goto-char (point-min))
-  (let ((case-fold-search nil))
-    (while (re-search-forward Man-heading-regexp (point-max) t)
+  (let (case-fold-search)
+    (while (re-search-forward Man-heading-regexp nil t)
       (let ((section (match-string 1)))
         (unless (member section Man--sections)
           (push section Man--sections)))
-      (forward-line 1)))
+      (forward-line)))
   (setq Man--sections (nreverse Man--sections)))
 
 (defsubst Man-build-references-alist ()
@@ -1803,7 +1803,7 @@ Man-goto-page
       (widen)
       (goto-char page-start)
       (narrow-to-region page-start page-end)
-      (Man-build-section-alist)
+      (Man-build-section-list)
       (Man-build-references-alist)
       (goto-char (point-min)))))
 
diff --git a/lisp/woman.el b/lisp/woman.el
index 111086e362..1edf6e34c3 100644
--- a/lisp/woman.el
+++ b/lisp/woman.el
@@ -1619,7 +1619,7 @@ woman-find-file
 	      (setq woman-buffer-alist
 		    (cons (cons file-name bufname) woman-buffer-alist)
 		    woman-buffer-number 0)))))
-  (Man-build-section-alist)
+  (Man-build-section-list)
   (Man-build-references-alist)
   (goto-char (point-min)))
 
-- 
2.14.2


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


# Current behaviour

The inline function Man-build-section-alist in lisp/man.el
populates the buffer-local list Man--sections with the
section names of the current manpage in reverse natural
order (i.e. the common section headers NAME and SEE ALSO end
up somewhere near the end and beginning of Man--sections,
respectively).

# Discussion

In the function Man-goto-section, Man--sections is passed as
a collection to completing-read.  The order of candidates is
irrelevant in a default Emacs configuration, as the
minibuffer help buffer sorts candidates lexicographically by
default.  There is, however, at least one completion
frontend (in my case, ivy.el[1] from ELPA) which displays
candidates in the received order.

AFAICT there is no purpose or benefit to keeping
Man--sections reversed, other than avoiding a call to
nreverse, which AFAIK is quite an efficient operation.

Conversely, keeping the sections in their natural order
(i.e. from (point-min) to (point-max)) both seems more
natural and is more convenient for users of completion
frontends like ivy.el.

Footnotes: 
[1]  https://github.com/abo-abo/swiper

# Patch

I attach two patches.

The first patch implements my suggested reversing of the
reversed Man--sections at the end of
Man-build-section-alist.

The second patch is purely aesthetic in removing some
redundant syntax from Man-build-section-alist and renaming
it to the more apt Man-build-section-list, seeing as
Man--sections is never a list of cons cells.

# Environment

In GNU Emacs 27.0.50 (build 1, x86_64-pc-linux-gnu, X toolkit, Xaw3d scroll bars)
 of 2017-10-25 built on thunk
Repository revision: 090f4f157eea6f0d0d13963520f5e05706de142f
Windowing system distributor 'The X.Org Foundation', version 11.0.11905000
System Description:	Debian GNU/Linux testing (buster)

Configured using:
 'configure --prefix=/home/blc/.local
 --enable-locallisppath= --with-mailutils --with-sound=yes
 --with-x-toolkit=lucid --with-xpm --with-jpeg --with-tiff
 --with-gif --with-png --with-rsvg --with-libsystemd
 --with-xml2 --with-imagemagick --with-xft --with-libotf
 --with-m17n-flt --with-toolkit-scroll-bars --with-xaw3d
 --with-xim --with-gpm --with-dbus --with-gsettings
 --with-selinux --with-gnutls --with-zlib --with-modules
 --with-threads --with-file-notification=yes --with-x
 --without-gconf --with-lcms2 'CFLAGS=-flto
 -fomit-frame-pointer -march=native -maes -mavx -mcrc32
 -mf16c -mfpmath=sse -mfsgsbase -mfxsr
 -minline-all-stringops -mmmx -mpclmul -mpopcnt -msahf
 -msse4.2 -mxsave -mxsaveopt -mvzeroupper -O3 -pipe'
 LDFLAGS=-flto'

Configured features:
XAW3D XPM JPEG TIFF GIF PNG RSVG IMAGEMAGICK SOUND GPM DBUS
GSETTINGS NOTIFY ACL LIBSELINUX GNUTLS LIBXML2 FREETYPE
M17N_FLT LIBOTF XFT ZLIB TOOLKIT_SCROLL_BARS LUCID X11
MODULES LIBSYSTEMD LCMS2

Important settings:
  value of $LANG: en_IE.UTF-8
  locale-coding-system: utf-8-unix

Major mode: IBuffer

Minor modes in effect:
  projectile-mode: t
  ibuffer-auto-mode: t
  hl-line-mode: t
  magit-wip-before-change-mode: t
  magit-wip-after-apply-mode: t
  magit-wip-after-save-mode: t
  magit-auto-revert-mode: t
  async-bytecomp-package-mode: t
  diff-auto-refine-mode: t
  recentf-mode: t
  ace-window-display-mode: t
  shell-dirtrack-mode: t
  counsel-mode: t
  global-paren-face-mode: t
  xterm-mouse-mode: t
  winner-mode: t
  global-whitespace-mode: t
  display-time-mode: t
  global-subword-mode: t
  subword-mode: t
  save-place-mode: t
  global-hi-lock-mode: t
  hi-lock-mode: t
  engine-mode: t
  delete-selection-mode: t
  display-battery-mode: t
  override-global-mode: t
  blc-dropbox-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  window-divider-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  buffer-read-only: t
  column-number-mode: t
  line-number-mode: t
  transient-mark-mode: t
  auto-save-visited-mode: t

Features:
(dabbrev counsel-projectile projectile grep compile ibuf-ext
ibuffer ibuffer-loaddefs view hl-line git-rebase
magit-bookmark magit-obsolete magit-blame magit-stash
magit-bisect magit-remote magit-commit magit-sequence
magit-notes magit-worktree magit-branch magit-files
magit-refs magit-status magit magit-repos magit-apply
magit-wip magit-log magit-diff smerge-mode magit-core
magit-autorevert autorevert filenotify magit-process
magit-margin magit-mode magit-git magit-section magit-popup
git-commit magit-utils log-edit pcvs-util add-log
with-editor async-bytecomp async dash unfill executable
tramp tramp-compat tramp-loaddefs trampver ucs-normalize
vc-git diff-mode recentf tree-widget bookmark pp shadow sort
footnote face-remap mail-extr gnus-msg gnus-art mm-uu
mml2015 mm-view mml-smime smime dig mailcap gnus-sum
gnus-group gnus-undo gnus-start gnus-cloud nnimap nnmail
mail-source tls gnutls utf7 netrc nnoo parse-time gnus-spec
gnus-int gnus-range gnus-win gnus nnheader emacsbug
eudcb-bbdb bbdb-com crm bbdb bbdb-site timezone eudc
eudc-options-file cus-edit eudc-vars wid-edit message rmc
puny git-annex dired-x dired dired-loaddefs format-spec
rfc822 mml mml-sec epa derived epg gnus-util rmail
rmail-loaddefs xdg mm-decode mm-bodies mm-encode mail-parse
rfc2231 mailabbrev gmm-utils mailheader sendmail rfc2047
rfc2045 ietf-drums mm-util mail-prsvr mail-utils ace-window
avy cl-print debug shell pcomplete comint ansi-color
cus-start cus-load colir color counsel jka-compr esh-util
etags xref project swiper ivy flx ivy-overlay ffap thingatpt
server bug-reference fic-mode fill-column-indicator
paren-face elec-pair xt-mouse winner ring disp-table
whitespace time cap-words superword subword saveplace paren
ibuf-macs highlight-escape-sequences hi-lock time-date
engine-mode delsel battery edmacro kmacro cl-extra help-mode
delight advice zenburn-theme browse-url use-package diminish
bind-key finder-inf tex-site info package easymenu
epg-config url-handlers url-parse auth-source cl-seq eieio
eieio-core cl-macs eieio-loaddefs password-cache url-vars
blc-macs blc-lib easy-mmode rx pcase thunk subr-x map seq
byte-opt gv bytecomp byte-compile cconv cl-loaddefs cl-lib
realpath mule-util tooltip eldoc electric uniquify
ediff-hook vc-hooks lisp-float-type mwheel term/x-win x-win
term/common-win x-dnd tool-bar dnd fontset image regexp-opt
fringe tabulated-list replace newcomment text-mode
elisp-mode lisp-mode prog-mode register page menu-bar
rfn-eshadow isearch timer select scroll-bar mouse jit-lock
font-lock syntax facemenu font-core term/tty-colors frame
cl-generic cham georgian utf-8-lang misc-lang vietnamese
tibetan thai tai-viet lao korean japanese eucjp-ms cp51932
hebrew greek romanian slovak czech european ethiopic indian
cyrillic chinese composite charscript charprop case-table
epa-hook jka-cmpr-hook help simple abbrev obarray minibuffer
cl-preloaded nadvice loaddefs button faces cus-face macroexp
files text-properties overlay sha1 md5 base64 format env
code-pages mule custom widget hashtable-print-readable
backquote dbusbind inotify lcms2 dynamic-setting
system-font-setting font-render-setting x-toolkit x
multi-tty make-network-process emacs)

Memory information:
((conses 16 545111 235884)
 (symbols 48 46611 7)
 (miscs 40 3990 14356)
 (strings 32 124523 28431)
 (string-bytes 1 3875163)
 (vectors 16 75843)
 (vector-slots 8 2064942 434084)
 (floats 8 566 2272)
 (intervals 56 2459 2469)
 (buffers 992 55))

Thanks,

-- 
Basil

bug#28998: 27.0.50; Man--sections is in reverse order

[Noam Postavsky]

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

> -(defsubst Man-build-section-alist ()
> +(defsubst Man-build-section-list ()

Perhaps we should switch to defun while we're at it?

> -  (let ((case-fold-search nil))
> +  (let (case-fold-search)

Personally, I like the more explicit version here.

> AFAICT there is no purpose or benefit to keeping
> Man--sections reversed, other than avoiding a call to
> nreverse, which AFAIK is quite an efficient operation.
>
> Conversely, keeping the sections in their natural order
> (i.e. from (point-min) to (point-max)) both seems more
> natural and is more convenient for users of completion
> frontends like ivy.el.

Makes sense to me.

bug#28998: 27.0.50; Man--sections is in reverse order

[Basil L. Contovounesios]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: 0001-Keep-Man-sections-in-natural-order-bug-28998.patch --]
[-- Type: text/x-diff, Size: 868 bytes --]

From 6bc6f316f4bd3dc80b96d2d894bce66487cb9127 Mon Sep 17 00:00:00 2001
From: "Basil L. Contovounesios" <contovob@tcd.ie>
Date: Wed, 25 Oct 2017 16:57:43 +0100
Subject: [PATCH 1/2] Keep Man sections in natural order (bug#28998)

* lisp/man.el (Man-build-section-alist): Reverse sections.
---
 lisp/man.el | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lisp/man.el b/lisp/man.el
index 7a892c6e88..f7b1609c92 100644
--- a/lisp/man.el
+++ b/lisp/man.el
@@ -1522,7 +1522,8 @@ Man-build-section-alist
       (let ((section (match-string 1)))
         (unless (member section Man--sections)
           (push section Man--sections)))
-      (forward-line 1))))
+      (forward-line 1)))
+  (setq Man--sections (nreverse Man--sections)))
 
 (defsubst Man-build-references-alist ()
   "Build the list of references (in the SEE ALSO section)."
-- 
2.14.2


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0002-Rename-Man-build-section-alist-bug-28998.patch --]
[-- Type: text/x-diff, Size: 2138 bytes --]

From f96f95b830825681fb618f1727fcc3e0f488102f Mon Sep 17 00:00:00 2001
From: "Basil L. Contovounesios" <contovob@tcd.ie>
Date: Tue, 7 Nov 2017 14:51:37 +0000
Subject: [PATCH 2/2] Rename Man-build-section-alist (bug#28998)

The defsubst Man-build-section-alist builds and returns a list of
strings, so rename it to Man-build-section-list and make it a defun.

* lisp/man.el (Man-build-section-alist): Do it and reduce syntax.
(Man-goto-page):
* lisp/woman.el (woman-find-file): Use it.
---
 lisp/man.el   | 10 +++++-----
 lisp/woman.el |  2 +-
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/lisp/man.el b/lisp/man.el
index f7b1609c92..798e78bbe7 100644
--- a/lisp/man.el
+++ b/lisp/man.el
@@ -1513,16 +1513,16 @@ Man-mode
   (set (make-local-variable 'bookmark-make-record-function)
        'Man-bookmark-make-record))
 
-(defsubst Man-build-section-alist ()
+(defun Man-build-section-list ()
   "Build the list of manpage sections."
-  (setq Man--sections nil)
+  (setq Man--sections ())
   (goto-char (point-min))
   (let ((case-fold-search nil))
-    (while (re-search-forward Man-heading-regexp (point-max) t)
+    (while (re-search-forward Man-heading-regexp nil t)
       (let ((section (match-string 1)))
         (unless (member section Man--sections)
           (push section Man--sections)))
-      (forward-line 1)))
+      (forward-line)))
   (setq Man--sections (nreverse Man--sections)))
 
 (defsubst Man-build-references-alist ()
@@ -1803,7 +1803,7 @@ Man-goto-page
       (widen)
       (goto-char page-start)
       (narrow-to-region page-start page-end)
-      (Man-build-section-alist)
+      (Man-build-section-list)
       (Man-build-references-alist)
       (goto-char (point-min)))))
 
diff --git a/lisp/woman.el b/lisp/woman.el
index 111086e362..1edf6e34c3 100644
--- a/lisp/woman.el
+++ b/lisp/woman.el
@@ -1619,7 +1619,7 @@ woman-find-file
 	      (setq woman-buffer-alist
 		    (cons (cons file-name bufname) woman-buffer-alist)
 		    woman-buffer-number 0)))))
-  (Man-build-section-alist)
+  (Man-build-section-list)
   (Man-build-references-alist)
   (goto-char (point-min)))
 
-- 
2.14.2


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


Noam Postavsky <npostavs@users.sourceforge.net> writes:

>> -(defsubst Man-build-section-alist ()
>> +(defsubst Man-build-section-list ()
>
> Perhaps we should switch to defun while we're at it?
>
>> -  (let ((case-fold-search nil))
>> +  (let (case-fold-search)
>
> Personally, I like the more explicit version here.

I attach slightly reworded patches updated with your two
suggestions.

Thanks,

-- 
Basil

bug#28998: 27.0.50; Man--sections is in reverse order

[Noam Postavsky]

2017-11-07 10:07 GMT-05:00 Basil L. Contovounesios <contovob@tcd.ie>:

> I attach slightly reworded patches updated with your two
> suggestions.

Thanks, I plan to push patch#1 to emacs-26 and patch#2 to master in a
few days if there are no objections.

bug#28998: 27.0.50; Man--sections is in reverse order

[Noam Postavsky]

tags 28998 fixed
close 28998 26.1
quit

Noam Postavsky <npostavs@users.sourceforge.net> writes:

> Thanks, I plan to push patch#1 to emacs-26 and patch#2 to master in a
> few days if there are no objections.

Done.

[1: 9533d76b0b]: 2017-11-11 10:46:43 -0500
  Keep Man sections in natural order (bug#28998)
  https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=9533d76b0b5bfe2df1cccc55a92c2545b1de4e2b

[2: 000e729071]: 2017-11-11 11:12:28 -0500
  Rename Man-build-section-alist (bug#28998)
  https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=000e729071d6fb75eabe2bde8a5912440f1f3c44