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

  • Done
  • quality assurance status badge
Details
4 participants
  • Daniel Brooks
  • Ludovic Courtès
  • Maxim Cournoyer
  • Tobias Geerinckx-Rice
Owner
unassigned
Submitted by
Tobias Geerinckx-Rice
Severity
normal
T
T
Tobias Geerinckx-Rice wrote on 15 Nov 2020 19:47
(address . guix-patches@gnu.org)
20201115184726.29944-1-me@tobias.gr
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(-)

Toggle diff (28 lines)
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
T
T
Tobias Geerinckx-Rice wrote on 15 Nov 2020 20:46
(address . 44663@debbugs.gnu.org)
878sb2xv14.fsf@nckx
Guix,

Tobias Geerinckx-Rice via Guix-patches via ???
Toggle quote (3 lines)
> 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
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(-)

Toggle diff (20 lines)
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
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(-)

Toggle diff (52 lines)
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
-----BEGIN PGP SIGNATURE-----

iIMEARYKACsWIQT12iAyS4c9C3o4dnINsP+IT1VteQUCX7GFpw0cbWVAdG9iaWFz
LmdyAAoJEA2w/4hPVW15JZgBALaG1REpJTNylqtJt31+NVRwVhOBIXMJp1jmbq4G
qgHZAP0fbctqYofXfp5Sg2ctS7kJmuPmRyY90XfHpxXotDY6Dw==
=6BJg
-----END PGP SIGNATURE-----

L
L
Ludovic Courtès wrote on 15 Nov 2020 21:46
(name . Tobias Geerinckx-Rice)(address . me@tobias.gr)(address . 44663@debbugs.gnu.org)
875z6673h8.fsf@gnu.org
Hi,

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

Toggle quote (7 lines)
> 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’.
D
D
Daniel Brooks wrote on 15 Nov 2020 22:38
(name . Ludovic Courtès)(address . ludo@gnu.org)
877dqm47xg.fsf@db48x.net
Ludovic Courtès <ludo@gnu.org> writes:

Toggle quote (3 lines)
> 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
L
L
Ludovic Courtès wrote on 16 Nov 2020 09:16
(name . Daniel Brooks)(address . db48x@db48x.net)
87h7pp4sxy.fsf@gnu.org
Daniel Brooks <db48x@db48x.net> skribis:

Toggle quote (8 lines)
> 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’.
T
T
Tobias Geerinckx-Rice wrote on 29 Nov 2020 17:43
[PATCH v2] ui: Handle multiword and empty $PAGER values.
(address . 44663@debbugs.gnu.org)
20201129164328.18776-1-me@tobias.gr
* 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(-)

Toggle diff (67 lines)
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
T
T
Tobias Geerinckx-Rice wrote on 29 Nov 2020 17:52
ui: Launch $PAGER through the shell.
(address . 44663@debbugs.gnu.org)
87blfgyukq.fsf@nckx
Toggle quote (2 lines)
> 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
-----BEGIN PGP SIGNATURE-----

iIMEARYKACsWIQT12iAyS4c9C3o4dnINsP+IT1VteQUCX8PRtQ0cbWVAdG9iaWFz
LmdyAAoJEA2w/4hPVW15SecBAKRIDfX3so6MXkoMVFoR+3sodq0sQIYnCzeMkRpK
H6kXAQDyvNmSSyqd/irgHIL9fpvr7hJtJ3Fc7uwnQOGz3J4lBQ==
=YkI8
-----END PGP SIGNATURE-----

T
T
Tobias Geerinckx-Rice wrote on 29 Nov 2020 17:56
Re: [bug#44663] [PATCH v2] ui: Handle multiword and empty $PAGER values.
(name . Tobias Geerinckx-Rice via Guix-patches)(address . guix-patches@gnu.org)(address . 44663@debbugs.gnu.org)
878sakyuck.fsf@nckx
...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
-----BEGIN PGP SIGNATURE-----

iIMEARYKACsWIQT12iAyS4c9C3o4dnINsP+IT1VteQUCX8PS2w0cbWVAdG9iaWFz
LmdyAAoJEA2w/4hPVW15LFoA/2J7gPLgeGRyD59SdWG2mgXjMg5XWcUBVSl6Yswr
ssQuAQDvw9+bXFV7YuezZF//T2eQOrmDj1H1UrRxz7Ew2BcKAQ==
=D6H4
-----END PGP SIGNATURE-----

M
M
Maxim Cournoyer wrote on 3 Aug 2021 22:02
Re: bug#44663: [PATCH] ui: Launch $PAGER through the shell.
(name . Tobias Geerinckx-Rice)(address . me@tobias.gr)(address . 44663-done@debbugs.gnu.org)
87lf5i8ff3.fsf_-_@gmail.com
Hey,

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

Toggle quote (8 lines)
> * 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
Closed
?