[bug#55845] [PATCH 0/1] Improve pager selection logic when less is not installed

[Taiju HIGASHI <higashi@taiju.info>]

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