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.

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.

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

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.

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

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.

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.

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

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.

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.

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

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.