all messages for Guix-related lists mirrored at yhetil.org
 help / color / mirror / code / Atom feed
From: Tobias Geerinckx-Rice via Guix-patches via <guix-patches@gnu.org>
To: Taiju HIGASHI <higashi@taiju.info>
Cc: 55845@debbugs.gnu.org
Subject: [bug#55845] [PATCH 0/1] Improve pager selection logic when less is not installed
Date: Wed, 08 Jun 2022 14:14:34 +0200	[thread overview]
Message-ID: <87wndrwc5m@nckx> (raw)
In-Reply-To: <20220608102124.14865-1-higashi@taiju.info>

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

Hi!

Taiju HIGASHI 写道:
> The problem rarely occurs, but when we run guix commands in an 
> environment
> where "less" is not installed we get an error.

True.  Odd that it's gone unreported(?) for so long.

> I am concerned about performance degradation due to more 
> unnecessary
> processing.

Since you asked…  :-)

One way that this is ‘expensive’ is that it always calls WHICH at 
least once, no matter what Guix was invoked to do.

If you're familiar with Haskell or Nix: Scheme is not that, it's 
not ‘lazy’ and will evaluate the (if (which "less") …) even when 
the value is never used.  Turning AVAILABLE-PAGER into a procedure 
would avoid that.

Also, you're looking up the final pager in $PATH twice: you call 
WHICH, but then discard its work by returning the relative string 
"less".

The final OPEN-PIPE* invokes a shell which will search $PATH 
again.  We could save it the trouble by returning an absolute file 
name: the result of WHICH.

And since WHICH returns #f on failure, you can replace the nested 
IFs with a single OR:

  (define (available-pager)
    (or (which "less")
        (which "more")))

And well, as you probably noticed by now, it's actually more clear 
and concise if we just in-line what little is left:

  (let ((pager-command-line (or (getenv "GUIX_PAGER")
                                (getenv "PAGER")
                                (which "less")
                                (which "more")
                                "")))
    …

Your original patch returns #f if no pages could be found.  I 
don't think that is handled, but "" is, so return that instead.

Now I think that's 100% equivalent to your original; let me know 
if I missed a spot.

> Also, if you feel that this is a minor issue and not worth 
> addressing, please
> feel free to dismiss it. (Still, a fix to make the error message 
> more friendly
> might be a good idea.)

It *is* minor, but then so is the fix, and as written above it 
doesn't add ‘overhead’.  I think it's a good idea to check for 
"more" (but no more) and silently disable paging otherwise.

Thanks!

T G-R

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

  parent reply	other threads:[~2022-06-08 13:19 UTC|newest]

Thread overview: 29+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-06-08 10:21 [bug#55845] [PATCH 0/1] Improve pager selection logic when less is not installed Taiju HIGASHI
2022-06-08 10:22 ` [bug#55845] [PATCH 1/1] ui: " Taiju HIGASHI
2022-06-08 13:18   ` Maxime Devos
     [not found]     ` <87leu72sbo.fsf@taiju.info>
2022-06-08 15:08       ` Maxime Devos
2022-06-08 15:17         ` Taiju HIGASHI
2022-06-08 16:46           ` Maxime Devos
2022-06-09  9:52         ` Taiju HIGASHI
2022-06-09 10:23           ` Taiju HIGASHI
2022-06-09 19:43             ` Maxime Devos
2022-06-10  0:39               ` Taiju HIGASHI
2022-06-10  7:47                 ` Maxime Devos
2022-06-10  8:40                   ` Taiju HIGASHI
2022-06-10 15:08                     ` Maxime Devos
2022-06-11 11:26                       ` Taiju HIGASHI
2022-06-14 23:58                         ` Taiju HIGASHI
2022-06-15  8:02                           ` Maxime Devos
2022-06-16 21:43                         ` [bug#55845] [PATCH 0/1] " Ludovic Courtès
2022-06-17  0:38                           ` Taiju HIGASHI
2022-06-17 12:39                             ` Ludovic Courtès
2022-06-17 13:36                               ` Taiju HIGASHI
2022-06-17 15:12                                 ` Ludovic Courtès
2022-06-18 14:11                                   ` Taiju HIGASHI
2022-06-10  0:55               ` [bug#55845] [PATCH 1/1] ui: " Taiju HIGASHI
2022-06-10  7:37                 ` Maxime Devos
2022-06-10  8:52                   ` Taiju HIGASHI
2022-06-08 12:14 ` Tobias Geerinckx-Rice via Guix-patches via [this message]
2022-06-08 13:12   ` [bug#55845] [PATCH 0/1] " Taiju HIGASHI
2022-06-08 14:22     ` Tobias Geerinckx-Rice via Guix-patches via
2022-06-08 15:09       ` Taiju HIGASHI

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87wndrwc5m@nckx \
    --to=guix-patches@gnu.org \
    --cc=55845@debbugs.gnu.org \
    --cc=higashi@taiju.info \
    --cc=me@tobias.gr \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this external index

	https://git.savannah.gnu.org/cgit/guix.git

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.