[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[bug#55845] [PATCH 0/1] Improve pager selection logic when less is not i
From: |
Taiju HIGASHI |
Subject: |
[bug#55845] [PATCH 0/1] Improve pager selection logic when less is not installed |
Date: |
Wed, 08 Jun 2022 22:12:34 +0900 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/28.1 (gnu/linux) |
Hi 写道,
Thank you for reviwing!
> 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.
I understand that I can delay the evaluation timing if I make it a
procedure, but is my understanding correct that the number of calls will
remain the same because it will be evaluated each time the
`call-with-paginated-output-port` procedure is called?
I agree with your point that it would be better to make it a procedure,
as it would be more eco-friendly to not have to evaluate when GUIX_PAGER
or PAGER is specified.
> 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.
I see, I did not understand that behavior. Thank you.
> 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")))
This one is also more readable. Thank you.
> 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")
> "")))
> …
You mean that the $PATH lookup in open-pipe can be suppressed?
Also, I misunderstood the string-null? spec; available-pager should have
returned an empty string.
> 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.
I thought what you said was completely correct.
>> 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.
I will just write what you have told me, but may I continue to modify
the patch?
Thank,
--
Taiju
- [bug#55845] [PATCH 0/1] Improve pager selection logic when less is not installed, (continued)
- [bug#55845] [PATCH 0/1] Improve pager selection logic when less is not installed, Ludovic Courtès, 2022/06/16
- [bug#55845] [PATCH 0/1] Improve pager selection logic when less is not installed, Taiju HIGASHI, 2022/06/16
- [bug#55845] [PATCH 0/1] Improve pager selection logic when less is not installed, Ludovic Courtès, 2022/06/17
- [bug#55845] [PATCH 0/1] Improve pager selection logic when less is not installed, Taiju HIGASHI, 2022/06/17
- [bug#55845] [PATCH 0/1] Improve pager selection logic when less is not installed, Ludovic Courtès, 2022/06/17
- [bug#55845] [PATCH 0/1] Improve pager selection logic when less is not installed, Taiju HIGASHI, 2022/06/18
- [bug#55845] [PATCH 1/1] ui: Improve pager selection logic when less is not installed., Taiju HIGASHI, 2022/06/09
- [bug#55845] [PATCH 1/1] ui: Improve pager selection logic when less is not installed., Maxime Devos, 2022/06/10
- [bug#55845] [PATCH 1/1] ui: Improve pager selection logic when less is not installed., Taiju HIGASHI, 2022/06/10
[bug#55845] [PATCH 0/1] Improve pager selection logic when less is not installed, Tobias Geerinckx-Rice, 2022/06/08
- [bug#55845] [PATCH 0/1] Improve pager selection logic when less is not installed,
Taiju HIGASHI <=