#104 Diffs and changes should have color when printed to stdout

Opened by cole-h on November 20, 2020
cole-h on November 20, 2020

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.

loewenheim on November 21, 2020

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.

pmeunier on November 21, 2020

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).

cole-h on November 21, 2020

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.

loewenheim added a change on November 24, 2020
UQ4M4BRBN42USIWC2S57GLXWDKOTIOPIUCGTKWGPYRTX7CBUEZJQC
loewenheim on November 24, 2020

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.

cole-h on November 24, 2020

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.

cole-h on November 24, 2020

… 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.

loewenheim on November 25, 2020

I get starting the pager with a specific environment variable, but how would we check whether it was set?

cole-h on November 25, 2020

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)…

loewenheim added a change on November 27, 2020
XMWOJ7NVSIWDP37BTUPONUOSC2PKCOCPBNUY6OBK74BOACDZMY3AC
loewenheim on November 27, 2020

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.

loewenheim added a change on November 27, 2020
GRSSOI6NZNHBNF43NXZSNMVGIWI3OTEMLQGOKUSB5S7BYJGYPPHAC
loewenheim added a change on November 27, 2020
TDUCOC5MFA3IZQM6GDMA5DEX47NBQOETCP3UITKSPJMFYCONSUTAC
loewenheim added a change on November 27, 2020
2YEVYKBBYRSNBNDIUGQRFP7EYI5O4XTVFS63SJDSIK4EPHQRBHCAC
loewenheim on November 27, 2020

Automatic “color detection” works. I suppose there should also be an option to override the automatic detection.

cole-h added a change on November 27, 2020
VDQTHSBEHUK3UMPMEYFHBM66IZ242BJMD4HNMRO7RAE6UWVDP5SAC
cole-h on November 27, 2020

Indeed it does, fantastic work!

I’ve pushed a change that adds support for the NO_COLOR env var for that purpose.

EDIT: Amended and re-pushed because I forgot to remove my “test” diff stuff.

cole-h added a change on November 27, 2020
F2LZXL3A4HMDJI5IQAHP6ZRZL35I5EHX5H56K5XNCVEAYWMCE2PAC
loewenheim on November 27, 2020

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.

cole-h on November 27, 2020

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).

loewenheim on November 27, 2020

No, that sounds great :)

cole-h on November 27, 2020

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.

cole-h added a change on November 27, 2020
B2WAO2MPJEXAP5HF6DZ7OPTXL2DRTSN3FD4AUC7ABVT7MJVRQOVQC
cole-h added a change on November 27, 2020
B2WAO2MPJEXAP5HF6DZ7OPTXL2DRTSN3FD4AUC7ABVT7MJVRQOVQC
cole-h added a change on November 27, 2020
62IOXETBSSLHPQSTHR7LWUDYFFQ5YHD7AGC7NTBJARDGPI53DP7QC
cole-h added a change on November 27, 2020
UBRGMN74JCPSYAFCR6SPM2RTF3474VEUBL2AYFOL637XZW76Y5XAC
cole-h added a change on November 27, 2020
6VCEINPD32FRCGOKMYXP7UDF6Q2VXANSGGC2Y54OOUJASGHXDAXAC
cole-h on November 27, 2020

@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?

loewenheim on November 28, 2020

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?

loewenheim on November 28, 2020

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?

loewenheim added a change on November 28, 2020
QE2TAMCXQE5HXRRMA5Z6GCIH3I2KFA22GWYPLXJEEB6RI6S6INMQC
loewenheim on November 28, 2020

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, or
  • NoColor::new(wtr), where wtr is the previous writer, if we don’t want color.

This has a few consequences:

  • The style of insertion and deletion lines is fixed, it’s always green/red. But the writer we’re writing to may choose to ignore colors. This means that unless we implement a command line option or user configuration, we don’t need any custom logic to decide whether to use colors at all.
  • NO_COLOR is handled for us, as pointed out by @cole-h.

NB: This change subsumes the initialize_pager one.

pmeunier on November 28, 2020

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.

loewenheim on November 28, 2020

@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!

pmeunier on November 28, 2020

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.

cole-h on November 28, 2020

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.

loewenheim on November 28, 2020

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.

pmeunier on November 29, 2020

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.

loewenheim on November 29, 2020

@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.

pmeunier on November 29, 2020

Oh, I see, thanks. I wasn’t implying I had any preference, I’m just trying to understand the state of this discussion.

loewenheim on November 29, 2020

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.

loewenheim added a change on November 30, 2020
EGEH2NKBUZMZAYWIDRNLCQMQHK64TTPH2N6O4DJDDQODY545TBSQC
pmeunier on November 30, 2020

@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.

loewenheim on November 30, 2020

@pmeunier Actually, the less dev explained why it’s the way it is and added a note about this behavior to the manual :)

pksunkara on November 30, 2020

clap v3 doesn’t ansi_term but in fact uses termcolor

cole-h on November 30, 2020

(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.

pksunkara on November 30, 2020

No, you guys are using clap:3.0.0-beta.2 which is v3 and uses termcolor. Trust me, I am clap maintainer ;)

cole-h on November 30, 2020

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.

pksunkara on November 30, 2020
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?