The sound distributed version control system

#98 Use pager for log output

Closed on November 23, 2020
arijid79 on November 19, 2020

Running pijul log spits out a whole list of changes which can be hard to read (since scrolling via mouse is required in a GUI terminal) and pretty much impossible in a TTY. One option is to pipe it through a program like less or more. But people always forget to add it at the end. The other option is to alias the above command. The problem with this option is if the repo has just 2-3 changes, paging it will be useless

So it would be much to use a pager like pager or any other crate to facilitate this

loewenheim on November 19, 2020

I definitely agree with this. I tried to implement this using the pager crate and it works nicely, but for some reason I can’t then build pijul with nix:

❯ nix build
[…]
  error[E0432]: unresolved import `pager`
   --> src/commands/log.rs:8:5
    |
  8 | use pager::Pager;
    |     ^^^^^ use of undeclared type or module `pager`
arijid79 added a change on November 19, 2020
23LVKATNTT74YKHG7KJM6SBO2IVZEV24TQ46ZJIHQ2IXONWNVXJAC
main
arijid79 on November 19, 2020

Basically I have added these features

  • Use pager wrapper crate
  • Use less for the pager
  • Automatically turn off the pager if there is no stdout
  • Turn off pager if the log can be displayed in one page
pmeunier on November 19, 2020

Thanks for this change. However, I remember we’ve had problems with that crate in the past, hopefully it has improved.

Just one minor comment, why do you use with_pager rather than with_default_pager? This would allow users to set their own pager if they want. Additionally, it seems some changes were merged after you recorded yours, resulting in a conflict.

cole-h on November 19, 2020

FYI, @loewenheim – in order to build with Nix when adding new dependencies, you need to update Cargo.nix by running crate2nix generate (which is why it’s provided in the devShell).

loewenheim on November 19, 2020

Thanks, @cole-h, I didn’t know about that!

arijid79 on November 19, 2020

There seems to be a issue with the run() function in log.rs due to which the q button does not close less when it is at the very end of the output. This is not a issue with pager crate since I did a test where it didn’t seem to happen

arijid79 added a change on November 20, 2020
Fix conflicts by arijit79,
72OS4BBUCWUXERRLCV64MDWMG5PSHFKEJRJRW3FCWOWG453HYITQC
arijid79 on November 20, 2020

I found the moins crate to be perfectly working with the log command, and does not have the issue, I posted above

Plus it also has support for colors. Shall we use it? I have already wrote the run() function with it. As far as I can tell, this is a much better way to do paging

What do you say @pmeunier

arijid79 added a change on November 20, 2020
XZBUYWBESYK2A6S7BDF22RRBXJICNLPC75CTMIBJD5GFFHEKQGHAC
pmeunier on November 20, 2020

I like the name! One issue I see is that unlike less, you can’t pipe output into moins, meaning that for very large repositories, this might hang before starting to display anything.

I guess we might have to build our own, maybe?

arijid79 on November 20, 2020

Moins is actually a library that captures the standard output and use the terminal’s alternate buffer to display it. As far as the code I have written, this is where the issue could occur, since the display happens after all the logs are in memory.

IMO An asynchronous pager is the way to go. The run function in the crate would take a buffer and check if it’s empty, if not it will display the output and clear the buffer.

If we need to build our own, I am ready to start working on it

pmeunier on November 21, 2020

Moins is actually a library that captures the standard output and use the terminal’s alternate buffer to display it.

It doesn’t look like it.

As far as the code I have written, this is where the issue could occur, since the display happens after all the logs are in memory.

Yes, that is what I meant. Some things in Pijul have required months of work just so that they would be instantaneous (it is simple to describe how pijul fork works, but not easy at all to implement), and this seems like a relatively simpler issue.

If we need to build our own, I am ready to start working on it

Great! I really want this to work in Pijul. I believe a good first step before reimplementing things would be to reach out to the authors of the Pager crate, to try and understand why it doesn’t work for us. Given the number of downloads, it certainly works for other people.

arijid79 on November 21, 2020

Hey @pmeunier. I have filled an issue on Pager’s GitLab issue tracker

https://gitlab.com/imp/pager-rs/-/issues/13

pmeunier on November 21, 2020

Cool, thanks! If you want to take care of this, I’ll be happy to apply your changes.

arijid79 on November 21, 2020

Yeah. I will do updates as they inform me about the issue

robx on November 22, 2020

There seems to be a issue with the run() function in log.rs due to which the q button does not close less when it is at the very end of the output. This is not a issue with pager crate since I did a test where it didn’t seem to happen

My guess is that the q button does end the less process fine, but pijul then fails to quit, perhaps because of blocked write to the closed output pipe, or because of a signal handling issue.

pmeunier on November 23, 2020

I just tested this again. If you press ctrl+c when it blocks, you can press q to quit.

pmeunier on November 23, 2020

Alright, after reading what the Pager crate was doing, I managed to fix this, by adding a std::process::exit(0) at the end of main.

@arijit79, I applied your change (plus some tweaks). Thanks everybody.

pmeunier closed this discussion on November 23, 2020