[bug#44663] [PATCH] ui: Launch $PAGER through the shell.

[bug#44663] [PATCH] ui: Launch $PAGER through the shell.

[Tobias Geerinckx-Rice via Guix-patches via]

This is the convention elsewhere and sounds like the right thing to do.

* guix/ui.scm (call-with-paginated-output-port): Substitute OPEN-PIPE
for OPEN-PIPE*.

Reported by Daniel Brooks <db48x@db48x.net>.
---
 guix/ui.scm | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/guix/ui.scm b/guix/ui.scm
index 4e686297e8..2b7d9dd64b 100644
--- a/guix/ui.scm
+++ b/guix/ui.scm
@@ -72,7 +72,7 @@
   #:use-module (ice-9 match)
   #:use-module (ice-9 format)
   #:use-module (ice-9 regex)
-  #:autoload   (ice-9 popen) (open-pipe* close-pipe)
+  #:autoload   (ice-9 popen) (open-pipe close-pipe)
   #:autoload   (system base compile) (compile-file)
   #:autoload   (system repl repl)  (start-repl)
   #:autoload   (system repl debug) (make-debug stack->vector)
@@ -1673,9 +1673,9 @@ zero means that PACKAGE does not match any of REGEXPS."
       ;; instead of 'r': this strips hyperlinks but allows 'less' to make a
       ;; good estimate of the line length.
       (let ((pager (with-environment-variables `(("LESS" ,less-options))
-                     (open-pipe* OPEN_WRITE
-                                 (or (getenv "GUIX_PAGER") (getenv "PAGER")
-                                     "less")))))
+                     (open-pipe (or (getenv "GUIX_PAGER") (getenv "PAGER")
+                                    "less")
+                                OPEN_WRITE))))
         (dynamic-wind
           (const #t)
           (lambda () (proc pager))
-- 
2.29.2

[bug#44663] [PATCH] ui: Launch $PAGER through the shell.

[Tobias Geerinckx-Rice via Guix-patches via]

[-- Attachment #1.1: Type: text/plain, Size: 702 bytes --]

Guix,

Tobias Geerinckx-Rice via Guix-patches via 写道：
> This is the convention elsewhere and sounds like the right thing 
> to do.

Before this patch, using PAGER= failed:

  $ PAGER= guix search e
  In execvp of : No such file or directory
  $

With it, it fails in a slightly worse way:

  $ PAGER= guix search e
  $ # nothing, because we spawn the shell that swallows all

Attached are two possible solutions.  One falls back to ‘less’, 
the other to no paging.

I think I prefer the former (‘Ignore empty $PAGER variables’) 
because the concept of ‘unset but empty’ could confuse ‘users’. 
Is that too patronising?  Do tell.

Kind regards,

T G-R


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: 0001-ui-Ignore-empty-PAGER-variables.patch --]
[-- Type: text/x-patch, Size: 1336 bytes --]

From dc64aadd9b124df37fcdf2f6dc057b61cf05a473 Mon Sep 17 00:00:00 2001
From: Tobias Geerinckx-Rice <me@tobias.gr>
Date: Sun, 15 Nov 2020 20:36:32 +0100
Subject: [PATCH] ui: Ignore empty $PAGER variables.

* guix/ui.scm (call-with-paginated-output-port): Treat "" as unset.
---
 guix/ui.scm | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/guix/ui.scm b/guix/ui.scm
index 66614eef7c..584afc65dd 100644
--- a/guix/ui.scm
+++ b/guix/ui.scm
@@ -1673,8 +1673,11 @@ zero means that PACKAGE does not match any of REGEXPS."
       ;; instead of 'r': this strips hyperlinks but allows 'less' to make a
       ;; good estimate of the line length.
       (let ((pager (with-environment-variables `(("LESS" ,less-options))
-                     (open-pipe (or (getenv "GUIX_PAGER") (getenv "PAGER")
-                                    "less")
+                     ;; Ignore environment variables set to "" as if unset.
+                     (open-pipe (find (lambda (s) (and s (not (string=? "" s))))
+                                      (list (getenv "GUIX_PAGER")
+                                            (getenv "PAGER")
+                                            "less"))
                                 OPEN_WRITE))))
         (dynamic-wind
           (const #t)
-- 
2.29.2


[-- Attachment #1.3: 0001-ui-Disable-paging-if-PAGER-is-set-to-the-empty-strin.patch --]
[-- Type: text/x-patch, Size: 2961 bytes --]

From e1cf7e852c4a4c0cfce8c0de5625d026229dd71b Mon Sep 17 00:00:00 2001
From: Tobias Geerinckx-Rice <me@tobias.gr>
Date: Sun, 15 Nov 2020 20:26:54 +0100
Subject: [PATCH] ui: Disable paging if $PAGER is set to the empty string.

* guix/ui.scm (call-with-paginated-output-port): Don't open a pipe if $PAGER is "".
---
 guix/ui.scm | 39 +++++++++++++++++++++------------------
 1 file changed, 21 insertions(+), 18 deletions(-)

diff --git a/guix/ui.scm b/guix/ui.scm
index 66614eef7c..bb03e06759 100644
--- a/guix/ui.scm
+++ b/guix/ui.scm
@@ -1663,24 +1663,27 @@ zero means that PACKAGE does not match any of REGEXPS."
 
 (define* (call-with-paginated-output-port proc
                                           #:key (less-options "FrX"))
-  (if (isatty?* (current-output-port))
-      ;; Set 'LESS' so that 'less' exits if everything fits on the screen (F),
-      ;; lets ANSI escapes through (r), does not send the termcap
-      ;; initialization string (X).  Set it unconditionally because some
-      ;; distros set it to something that doesn't work here.
-      ;;
-      ;; For things that produce long lines, such as 'guix processes', use 'R'
-      ;; instead of 'r': this strips hyperlinks but allows 'less' to make a
-      ;; good estimate of the line length.
-      (let ((pager (with-environment-variables `(("LESS" ,less-options))
-                     (open-pipe (or (getenv "GUIX_PAGER") (getenv "PAGER")
-                                    "less")
-                                OPEN_WRITE))))
-        (dynamic-wind
-          (const #t)
-          (lambda () (proc pager))
-          (lambda () (close-pipe pager))))
-      (proc (current-output-port))))
+  (let ((command (or (getenv "GUIX_PAGER") (getenv "PAGER")
+                     "less")))
+    ;; If a user types ‘PAGER= guix foo’ their intention is probably to disable
+    ;; paging entirely, not to use Guix's default pager.
+    (if (and (not (string=? "" command))
+             (isatty?* (current-output-port)))
+        ;; Set 'LESS' so that 'less' exits if everything fits on the screen (F),
+        ;; lets ANSI escapes through (r), does not send the termcap
+        ;; initialization string (X).  Set it unconditionally because some
+        ;; distros set it to something that doesn't work here.
+        ;;
+        ;; For things that produce long lines, such as 'guix processes', use 'R'
+        ;; instead of 'r': this strips hyperlinks but allows 'less' to make a
+        ;; good estimate of the line length.
+        (let ((pager (with-environment-variables `(("LESS" ,less-options))
+                       (open-pipe command OPEN_WRITE))))
+          (dynamic-wind
+            (const #t)
+            (lambda () (proc pager))
+            (lambda () (close-pipe pager))))
+        (proc (current-output-port)))))
 
 (define-syntax with-paginated-output-port
   (syntax-rules ()
-- 
2.29.2


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 247 bytes --]

[bug#44663] [PATCH] ui: Launch $PAGER through the shell.

[Ludovic Courtès]

Hi,

Tobias Geerinckx-Rice <me@tobias.gr> skribis:

> This is the convention elsewhere and sounds like the right thing to do.
>
> * guix/ui.scm (call-with-paginated-output-port): Substitute OPEN-PIPE
> for OPEN-PIPE*.
>
> Reported by Daniel Brooks <db48x@db48x.net>.

What’s the rationale though?  Are there cases where this makes a
practical difference?

I’m all for avoiding the shell if there’s no need for it.

Thanks,
Ludo’.

[bug#44663] [PATCH] ui: Launch $PAGER through the shell.

[Daniel Brooks]

Ludovic Courtès <ludo@gnu.org> writes:

> What’s the rationale though?  Are there cases where this makes a
> practical difference?

The error I hit was effectively running PAGER=less -FXRS guix search
foo. Everything else that uses PAGER handles this case just fine.

db48x

[bug#44663] [PATCH] ui: Launch $PAGER through the shell.

[Ludovic Courtès]

Daniel Brooks <db48x@db48x.net> skribis:

> Ludovic Courtès <ludo@gnu.org> writes:
>
>> What’s the rationale though?  Are there cases where this makes a
>> practical difference?
>
> The error I hit was effectively running PAGER=less -FXRS guix search
> foo. Everything else that uses PAGER handles this case just fine.

Oh, I see, hmm.  I feel that going through the shell makes things more
brittle, but you describe a valid use case, so maybe we should just go
ahead and apply the patch Tobias posted.

Ludo’.

[bug#44663] [PATCH v2] ui: Handle multiword and empty $PAGER values.

[Tobias Geerinckx-Rice via Guix-patches via]

* guix/ui.scm (call-with-paginated-output-port): Empty PAGER values
disable paging.  Non-empty ones are split into command arguments.

Reported by Daniel Brooks <db48x@db48x.net>.
---
 guix/ui.scm | 47 ++++++++++++++++++++++++++++-------------------
 1 file changed, 28 insertions(+), 19 deletions(-)

diff --git a/guix/ui.scm b/guix/ui.scm
index a59be74ecd..37099eac7b 100644
--- a/guix/ui.scm
+++ b/guix/ui.scm
@@ -12,7 +12,7 @@
 ;;; Copyright © 2018 Kyle Meyer <kyle@kyleam.com>
 ;;; Copyright © 2018 Ricardo Wurmus <rekado@elephly.net>
 ;;; Copyright © 2019 Chris Marusich <cmmarusich@gmail.com>
-;;; Copyright © 2019 Tobias Geerinckx-Rice <me@tobias.gr>
+;;; Copyright © 2019, 2020 Tobias Geerinckx-Rice <me@tobias.gr>
 ;;; Copyright © 2019 Simon Tournier <zimon.toutoune@gmail.com>
 ;;; Copyright © 2020 Arun Isaac <arunisaac@systemreboot.net>
 ;;; Copyright © 2020 Maxim Cournoyer <maxim.cournoyer@gmail.com>
@@ -1675,24 +1675,33 @@ zero means that PACKAGE does not match any of REGEXPS."
 
 (define* (call-with-paginated-output-port proc
                                           #:key (less-options "FrX"))
-  (if (isatty?* (current-output-port))
-      ;; Set 'LESS' so that 'less' exits if everything fits on the screen (F),
-      ;; lets ANSI escapes through (r), does not send the termcap
-      ;; initialization string (X).  Set it unconditionally because some
-      ;; distros set it to something that doesn't work here.
-      ;;
-      ;; For things that produce long lines, such as 'guix processes', use 'R'
-      ;; instead of 'r': this strips hyperlinks but allows 'less' to make a
-      ;; good estimate of the line length.
-      (let ((pager (with-environment-variables `(("LESS" ,less-options))
-                     (open-pipe* OPEN_WRITE
-                                 (or (getenv "GUIX_PAGER") (getenv "PAGER")
-                                     "less")))))
-        (dynamic-wind
-          (const #t)
-          (lambda () (proc pager))
-          (lambda () (close-pipe pager))))
-      (proc (current-output-port))))
+  (let ((pager-command-line (or (getenv "GUIX_PAGER")
+                                (getenv "PAGER")
+                                "less")))
+    ;; Setting PAGER to the empty string conventionally disables paging.
+    (if (and (not (string-null? pager-command-line))
+             (isatty?* (current-output-port)))
+        ;; Set 'LESS' so that 'less' exits if everything fits on the screen
+        ;; (F), lets ANSI escapes through (r), does not send the termcap
+        ;; initialization string (X).  Set it unconditionally because some
+        ;; distros set it to something that doesn't work here.
+        ;;
+        ;; For things that produce long lines, such as 'guix processes', use
+        ;; 'R' instead of 'r': this strips hyperlinks but allows 'less' to
+        ;; make a good estimate of the line length.
+        (let* ((pager (with-environment-variables `(("LESS" ,less-options))
+                        (apply open-pipe* OPEN_WRITE
+                               ;; Split into arguments.  Treat runs of multiple
+                               ;; whitespace characters as one.  libpipeline-
+                               ;; style "cmd one\ arg" escaping is unsupported.
+                               (remove ""
+                                       (string-split pager-command-line
+                                                     char-set:whitespace))))))
+          (dynamic-wind
+            (const #t)
+            (lambda () (proc pager))
+            (lambda () (close-pipe pager))))
+        (proc (current-output-port)))))
 
 (define-syntax with-paginated-output-port
   (syntax-rules ()
-- 
2.29.2

[bug#44663] ui: Launch $PAGER through the shell.

[Tobias Geerinckx-Rice via Guix-patches via]

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

> I feel that going through the shell makes things more brittle

I don't think it's brittle but it's icky.  Using OPEN-PIPE is just 
the easiest way to get the shell to do free parsing for us.  It 
also allows using pipes and for-loops in PAGER so I agree it's not 
ideal.

Here's a v2 that does more work for less result, but feels less 
dirty :-)

Kind regards,

T G-R

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 247 bytes --]

[bug#44663] [PATCH v2] ui: Handle multiword and empty $PAGER values.

[Tobias Geerinckx-Rice via Guix-patches via]

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

...it's also missing a

  #:use-module ((rnrs lists) #:select (remove))

that got dropped on the floor.

This module already imports SRFI-1, so that's some bonus ick for 
you.

Kind regards,

T G-R

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 247 bytes --]

bug#44663: [PATCH] ui: Launch $PAGER through the shell.

[Maxim Cournoyer]

Hey,

Tobias Geerinckx-Rice <me@tobias.gr> writes:

> * guix/ui.scm (call-with-paginated-output-port): Empty PAGER values
> disable paging.  Non-empty ones are split into command arguments.
>
> Reported by Daniel Brooks <db48x@db48x.net>.
> ---
>  guix/ui.scm | 47 ++++++++++++++++++++++++++++-------------------
>  1 file changed, 28 insertions(+), 19 deletions(-)

I see this got pushed with a81258c12415b9cee52c951b8b53cbe46f27654f
about a year ago.

Closing!

Maxim