GNU bug report logs - #44663
[PATCH] ui: Launch $PAGER through the shell.

Previous Next

Package: guix-patches;

Reported by: Tobias Geerinckx-Rice <me <at> tobias.gr>

Date: Sun, 15 Nov 2020 18:48:02 UTC

Severity: normal

Tags: patch

Done: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>

Bug is archived. No further changes may be made.

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 44663 in the body.
You can then email your comments to 44663 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

View this report as an mbox folder, status mbox, maintainer mbox


Report forwarded to guix-patches <at> gnu.org:
bug#44663; Package guix-patches. (Sun, 15 Nov 2020 18:48:02 GMT) Full text and rfc822 format available.

Acknowledgement sent to Tobias Geerinckx-Rice <me <at> tobias.gr>:
New bug report received and forwarded. Copy sent to guix-patches <at> gnu.org. (Sun, 15 Nov 2020 18:48:02 GMT) Full text and rfc822 format available.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Tobias Geerinckx-Rice <me <at> tobias.gr>
To: guix-patches <at> gnu.org
Subject: [PATCH] ui: Launch $PAGER through the shell.
Date: Sun, 15 Nov 2020 19:47:26 +0100
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 <at> 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





Information forwarded to guix-patches <at> gnu.org:
bug#44663; Package guix-patches. (Sun, 15 Nov 2020 19:47:02 GMT) Full text and rfc822 format available.

Message #8 received at 44663 <at> debbugs.gnu.org (full text, mbox):

From: Tobias Geerinckx-Rice <me <at> tobias.gr>
To: 44663 <at> debbugs.gnu.org
Subject: Re: [bug#44663] [PATCH] ui: Launch $PAGER through the shell.
Date: Sun, 15 Nov 2020 20:46:47 +0100
[Message part 1 (text/plain, inline)]
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

[0001-ui-Ignore-empty-PAGER-variables.patch (text/x-patch, inline)]
From dc64aadd9b124df37fcdf2f6dc057b61cf05a473 Mon Sep 17 00:00:00 2001
From: Tobias Geerinckx-Rice <me <at> 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

[0001-ui-Disable-paging-if-PAGER-is-set-to-the-empty-strin.patch (text/x-patch, inline)]
From e1cf7e852c4a4c0cfce8c0de5625d026229dd71b Mon Sep 17 00:00:00 2001
From: Tobias Geerinckx-Rice <me <at> 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

[signature.asc (application/pgp-signature, inline)]

Information forwarded to guix-patches <at> gnu.org:
bug#44663; Package guix-patches. (Sun, 15 Nov 2020 20:47:02 GMT) Full text and rfc822 format available.

Message #11 received at 44663 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Tobias Geerinckx-Rice <me <at> tobias.gr>
Cc: 44663 <at> debbugs.gnu.org
Subject: Re: [bug#44663] [PATCH] ui: Launch $PAGER through the shell.
Date: Sun, 15 Nov 2020 21:46:27 +0100
Hi,

Tobias Geerinckx-Rice <me <at> 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 <at> 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’.




Information forwarded to guix-patches <at> gnu.org:
bug#44663; Package guix-patches. (Sun, 15 Nov 2020 21:39:01 GMT) Full text and rfc822 format available.

Message #14 received at 44663 <at> debbugs.gnu.org (full text, mbox):

From: Daniel Brooks <db48x <at> db48x.net>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 44663 <at> debbugs.gnu.org, Tobias Geerinckx-Rice <me <at> tobias.gr>
Subject: Re: [bug#44663] [PATCH] ui: Launch $PAGER through the shell.
Date: Sun, 15 Nov 2020 13:38:35 -0800
Ludovic Courtès <ludo <at> 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




Information forwarded to guix-patches <at> gnu.org:
bug#44663; Package guix-patches. (Mon, 16 Nov 2020 08:18:02 GMT) Full text and rfc822 format available.

Message #17 received at 44663 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Daniel Brooks <db48x <at> db48x.net>
Cc: 44663 <at> debbugs.gnu.org, Tobias Geerinckx-Rice <me <at> tobias.gr>
Subject: Re: [bug#44663] [PATCH] ui: Launch $PAGER through the shell.
Date: Mon, 16 Nov 2020 09:16:57 +0100
Daniel Brooks <db48x <at> db48x.net> skribis:

> Ludovic Courtès <ludo <at> 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’.




Information forwarded to guix-patches <at> gnu.org:
bug#44663; Package guix-patches. (Sun, 29 Nov 2020 16:44:01 GMT) Full text and rfc822 format available.

Message #20 received at 44663 <at> debbugs.gnu.org (full text, mbox):

From: Tobias Geerinckx-Rice <me <at> tobias.gr>
To: 44663 <at> debbugs.gnu.org
Subject: [PATCH v2] ui: Handle multiword and empty $PAGER values.
Date: Sun, 29 Nov 2020 17:43:28 +0100
* 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 <at> 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 <at> kyleam.com>
 ;;; Copyright © 2018 Ricardo Wurmus <rekado <at> elephly.net>
 ;;; Copyright © 2019 Chris Marusich <cmmarusich <at> gmail.com>
-;;; Copyright © 2019 Tobias Geerinckx-Rice <me <at> tobias.gr>
+;;; Copyright © 2019, 2020 Tobias Geerinckx-Rice <me <at> tobias.gr>
 ;;; Copyright © 2019 Simon Tournier <zimon.toutoune <at> gmail.com>
 ;;; Copyright © 2020 Arun Isaac <arunisaac <at> systemreboot.net>
 ;;; Copyright © 2020 Maxim Cournoyer <maxim.cournoyer <at> 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





Information forwarded to guix-patches <at> gnu.org:
bug#44663; Package guix-patches. (Sun, 29 Nov 2020 16:53:01 GMT) Full text and rfc822 format available.

Message #23 received at 44663 <at> debbugs.gnu.org (full text, mbox):

From: Tobias Geerinckx-Rice <me <at> tobias.gr>
To: 44663 <at> debbugs.gnu.org
Subject: ui: Launch $PAGER through the shell.
Date: Sun, 29 Nov 2020 17:52:05 +0100
[Message part 1 (text/plain, inline)]
> 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
[signature.asc (application/pgp-signature, inline)]

Information forwarded to guix-patches <at> gnu.org:
bug#44663; Package guix-patches. (Sun, 29 Nov 2020 16:58:02 GMT) Full text and rfc822 format available.

Message #26 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Tobias Geerinckx-Rice <me <at> tobias.gr>
To: Tobias Geerinckx-Rice via Guix-patches <guix-patches <at> gnu.org>
Cc: 44663 <at> debbugs.gnu.org
Subject: Re: [bug#44663] [PATCH v2] ui: Handle multiword and empty $PAGER
 values.
Date: Sun, 29 Nov 2020 17:56:59 +0100
[Message part 1 (text/plain, inline)]
...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
[signature.asc (application/pgp-signature, inline)]

Information forwarded to guix-patches <at> gnu.org:
bug#44663; Package guix-patches. (Sun, 29 Nov 2020 16:58:02 GMT) Full text and rfc822 format available.

Reply sent to Maxim Cournoyer <maxim.cournoyer <at> gmail.com>:
You have taken responsibility. (Tue, 03 Aug 2021 20:04:01 GMT) Full text and rfc822 format available.

Notification sent to Tobias Geerinckx-Rice <me <at> tobias.gr>:
bug acknowledged by developer. (Tue, 03 Aug 2021 20:04:02 GMT) Full text and rfc822 format available.

Message #34 received at 44663-done <at> debbugs.gnu.org (full text, mbox):

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: Tobias Geerinckx-Rice <me <at> tobias.gr>
Cc: 44663-done <at> debbugs.gnu.org
Subject: Re: bug#44663: [PATCH] ui: Launch $PAGER through the shell.
Date: Tue, 03 Aug 2021 16:02:56 -0400
Hey,

Tobias Geerinckx-Rice <me <at> 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 <at> 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




bug archived. Request was from Debbugs Internal Request <help-debbugs <at> gnu.org> to internal_control <at> debbugs.gnu.org. (Wed, 01 Sep 2021 11:24:11 GMT) Full text and rfc822 format available.

This bug report was last modified 2 years and 209 days ago.

Previous Next


GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.