I briefly looked into this and couldn’t find where the details of a change are actually printed; is it the InodeMetadata::write
method?
As an aside, I would recommend using the ansi_term
crate for this, since clap
already depends on it anyway.
No, it’s in libpijul::change::Change::write
. That module is rather long and may be split at some point, but that function is quite easy. The main issue I see is when composing with the reverse read
function, which should remove the vt100 escape codes from the input (or use regular expressions that ignore them).
A bit roundabout, but maybe add another parameter, color: bool
, which adds the color escapes when true
. Then we’d only set it to true
in e.g. pijul diff
, and only when the writer (stdout
) is a TTY.
UQ4M4BRBN42USIWC2S57GLXWDKOTIOPIUCGTKWGPYRTX7CBUEZJQC
The change I pushed enables basic color functionality. It is currently always on, there is no check as of yet whether the output is a terminal (and I’m not sure that would work anyway, due to the fact that we pipe the output to a pager). I had to add the -r
flag to our calls to less
so that it would interpret color codes, otherwise they are printed verbatim even in single-screen situations where you never actually see the pager.
Looks good so far. But we definitely need a way to detect whether it’s being piped somewhere else (not our pager) or not… git
somehow does this, but I’m not sure how it works. GIT_PAGER=cat git log
shows colors, GIT_PAGER=cat git log | cat
does not. As it stands now, pijul diff > file
also writes the color escapes.
I don’t have much time for research, but their color.c and pager.c might be interesting to look at.
… I said that, but immediately went back to research.
Looks like the magic happens right here (color.c
) – in pager.c
, they set an env var for the pager process indicating it was spawned by git
, and the linked line checks if the pager was indeed spawned by git
(amongst other things), and writes colors.
I don’t think there’s an easy way to do this (this being injecting an environment variable) since the pager
crate is external, aside from submit a MR and ask for a version bump after it gets merged.
EDIT: JK, there’s a real nice helper method in pager
: pager_envs
.
I get starting the pager with a specific environment variable, but how would we check whether it was set?
Yeah, that was something I thought would work, but wouldn’t really (since, as you bring up, there’s no way to easily check the env of that child process)…
XMWOJ7NVSIWDP37BTUPONUOSC2PKCOCPBNUY6OBK74BOACDZMY3AC
If I’m not mistaken, we can actually detect whether we started the pager via the Pager::is_on()
method. The pager
crate automatically skips the pager if the output is not a tty. I added a println!
statement to log
that only happens if the pager is on. In my tests, I see “Pager is on” when I call pijul log
, but not when I do pijul log > test.txt
or pijul log | less
. This might be a way forward.
GRSSOI6NZNHBNF43NXZSNMVGIWI3OTEMLQGOKUSB5S7BYJGYPPHAC
TDUCOC5MFA3IZQM6GDMA5DEX47NBQOETCP3UITKSPJMFYCONSUTAC
2YEVYKBBYRSNBNDIUGQRFP7EYI5O4XTVFS63SJDSIK4EPHQRBHCAC
Automatic “color detection” works. I suppose there should also be an option to override the automatic detection.
VDQTHSBEHUK3UMPMEYFHBM66IZ242BJMD4HNMRO7RAE6UWVDP5SAC
F2LZXL3A4HMDJI5IQAHP6ZRZL35I5EHX5H56K5XNCVEAYWMCE2PAC
Ah, I was thinking of doing it with a command line argument (in the vein of --color always|never|auto
, with auto
the default). Supporting this NO_COLOR
variable strikes me as a good idea, though.
Actually, I think we could get this for free if we switch to termcolor
(which is already in our dependency tree as well) – ansi_term
hasn’t been updated in a while (September 2019) and doesn’t support NO_COLOR
, but termcolor
has built-in support for NO_COLOR
(it’s the library that Cargo itself uses).
I’ll work on that and push an all-encompassing change shortly (if you’re not OK with that, let me know and I’ll just make it another patch in this series).
No, that sounds great :)
Hm. It’s not as free as I thought.
My current solution is to construct a Buffer
that we write into, and then write those contents into the &mut w
. This is a lot longer and more verbose (and probably less performant) than what we have here…
The NO_COLOR
stuff is only implemented when you construct a StandardStream
with StandardStream::{stdout,stderr}(ColorChoice::Auto)
, and that is the only way to construct a StandardStream
. The problem here is that print_contents
takes anything that implements the io::Write
trait, which isn’t just limited to stdout
and stderr
. I don’t think boxing ourselves in to only printing to stdout
/ stderr
is acceptable, so I think the current solution is fine.
B2WAO2MPJEXAP5HF6DZ7OPTXL2DRTSN3FD4AUC7ABVT7MJVRQOVQC
B2WAO2MPJEXAP5HF6DZ7OPTXL2DRTSN3FD4AUC7ABVT7MJVRQOVQC
62IOXETBSSLHPQSTHR7LWUDYFFQ5YHD7AGC7NTBJARDGPI53DP7QC
UBRGMN74JCPSYAFCR6SPM2RTF3474VEUBL2AYFOL637XZW76Y5XAC
6VCEINPD32FRCGOKMYXP7UDF6Q2VXANSGGC2Y54OOUJASGHXDAXAC
@loewenheim I noticed the change message for commands: add...
was incorrect (it said initialize_parser
, not initialize_pager
), so I corrected that (which required mucking with history a little bit).
I’m satisfied with this solution, and think it’s good for inclusion, now.
@pmeunier WDYT?
Pulling your most recent change (6VC) gives me an
Error: No such file or directory (os error 2)
Any idea what’s up with that?
Also I just thought of something: We never do anything with the Pager
instance returned by initialize_pager
other than check whether it’s on; the initialization is a side effect. It might be cleaner to just return pager.is_on()
from the function, what do you think?
QE2TAMCXQE5HXRRMA5Z6GCIH3I2KFA22GWYPLXJEEB6RI6S6INMQC
After some tinkering, I have a solution based on termcolor
that I think is more elegant than the previous one. It works like this: various functions in libpijul
that had a Write
argument now have a WriteColor
argument (from termcolor
). Whenever an actual writer is constructed in pijul
, we either make it
StandardStream::stdout(ColorChoice::Auto)
in cases where we previously had std::io::stdout
and care about color, orNoColor::new(wtr)
, where wtr
is the previous writer, if we don’t want color.This has a few consequences:
NO_COLOR
is handled for us, as pointed out by @cole-h.NB: This change subsumes the initialize_pager
one.
I love how this discussion is going. @loewenheim: I’ve tried to investigate the “no such file or directory” issue, I’m not sure what it is. I’m debugging Thrussh at the moment.
@pmeunier I’ve been meaning to tell you that contributing to pijul
has been a great experience, both on a technical and personal level. pjiul
itself and the nest are really solid by now and the atmosphere is extremely friendly and constructive!
Thanks! I’m trying, so this is pleasant to hear.
Btw, we now have enough changes on the main channel, and this discussion also has enough of them, to trigger a bug: the recursive apply was doing a DFS in exponential time. Pulling might become noticeably faster with my next change.
That’s close to what I had when I called it quits. One problem with the current implementation is that cargo run -- change | less
shows escape codes, where it didn’t before (because we were checking if the pager was on and stdout was a TTY). That would have to be resurrected in some form for the current incarnation to be acceptable, IMO. Another issue I just noticed is that, after scrolling down in the pager, scrolling back up messes with the colors (on my system) – the colors seem to bleed over to other lines, whereas before they were properly reset.
I didn’t go this route originally, because I didn’t want to force potential consumers of libpijul into a corner where they now have to depend on termcolor
if they want to use the write
associated method, even if they don’t want colors.
That’s close to what I had when I called it quits. One problem with the current implementation is that cargo run – change | less shows escape codes, where it didn’t before (because we were checking if the pager was on and stdout was a TTY). That would have to be resurrected in some form for the current incarnation to be acceptable, IMO.
Fair point, I should have tested that before pushing. I only piped it through bat
. I’ll re-add this check.
Another issue I just noticed is that, after scrolling down in the pager, scrolling back up messes with the colors (on my system) – the colors seem to bleed over to other lines, whereas before they were properly reset.
This issue, at least, can be fixed by calling less
with -R
instead of -r
. Why exactly that is, I don’t know.
I didn’t go this route originally, because I didn’t want to force potential consumers of libpijul into a corner where they now have to depend on termcolor if they want to use the write associated method, even if they don’t want colors.
This is indeed a puzzle. I don’t know what to do about this.
Alright, I’ve included the previous solution in libpijul-1.0.0-alpha.7 and pijul-1.0.0-alpha.9, which I’m publishing now. I believe people might try to run pijul change BLA > BLA.change
, and we need an output without escape codes.
@lowenheim: why is this solution more elegant? I’m not sure I understand.
Also, as a general comment, the text format is feature-gated in libpijul, so I’ve added the relevant change (ansi_term = { version = "…", optional = true }
and text-changes = [ "regex", "ansi_term" ]
.
In order to avoid unnecessary conflicts, I’m not pushing the change about version for now, until this discussion reaches a conclusion.
@pmeunier I find it more elegant because it moves the decision whether to print colors to the point where the actual printing happens. I fully recognize that this is massively subjective and other people may have different preferences.
About @cole-h’s concern about forcing the termcolor
dependency on people who don’t want it: could this perhaps be feature-gated in some way? I have little experience with this mechanism.
Oh, I see, thanks. I wasn’t implying I had any preference, I’m just trying to understand the state of this discussion.
I just learned something amusing. less -R
handles color codes in an unexpected way: they only apply to the end of the current line. This means that if we want to use less -R
in its current state, we have to color each line individually. This is orthogonal to the question of ansi_term
vs. termcolor
. I’ve also opened an issue to find out whether this behavior of less
is intended and whether it could be changed.
EGEH2NKBUZMZAYWIDRNLCQMQHK64TTPH2N6O4DJDDQODY545TBSQC
@loewenheim: I wouldn’t be super hopeful to see this changed in less ;-)
This looks good to me, I’d like to have @cole-h’s opinion.
@pmeunier Actually, the less
dev explained why it’s the way it is and added a note about this behavior to the manual :)
clap
v3 doesn’t ansi_term
but in fact uses termcolor
(Since v3 hasn’t properly released yet, we’re using clap v2, which does depend on ansi_term
.)
Still not really happy that libpijul::write
is bound by WriteColor
for the reasons I covered above. I think the current implementation in main
is just fine and doesn’t require a trait bound. But don’t let me block this, of course.
No, you guys are using clap:3.0.0-beta.2
which is v3
and uses termcolor
. Trust me, I am clap maintainer ;)
Ah, I was looking at the dependency graph from cargo tree
, which showed clap v2 (and its ansi_term
dependency) – but as a dependency of bindgen
. My bad.
fn use_colors(pager: &pager::Pager) -> bool {
pager.is_on() || atty::is(atty::Stream::Stdout)
}
There might be other commands we want to write on Stderr
. Maybe it’s better if we calculate colors for both streams?
I missed that feature too, so I wrote a little awk script for that. You can check it out at https://nest.pijul.com/levi/pjcolor It should be sufficient for the time being.
ZRUPLBBTT4S6S7A3LOAHG4ONYEGPA5CFO4L2XBCNFKK45MWX3BDAC
I’ve finally found time to come back to this, with a different solution: by adding a trait to print change lines, we can avoid an extra dependency in libpijul, and keep things simpler. The dependency question is not completely obvious, since I would like libpijul to compile on platforms such as WASM, that do not support terminals or colours.
It would be nice to, at a glance, be able to tell if a change is an addition or a removal.
This isn’t really a priority or something “super important”, but it would be nice to have it eventually.