Add color output support

[ferki]

As a user, I would like have the hook's output colorized, in order to improve its readability.

[choroba]

Greetings from the Pull Request Club 👋🏽

I have an implementation using extra_command_option (see here), but the test only works when diff supports -b (which is POSIX, so it probably often should). I'm not sure how to test it better. To colour the output, use extra_command_option => '--color=always'.

Also, I'm not sure the options should be specified via an import - I don't use Rex much so I'm not familiar with the ways it handles similar situations. Let me know what you think.

[ferki]

Hey, @choroba, thanks for looking into this!

About extra_command_option

While it certainly can produce colorized diffs in some cases, I'm afraid configurable diff options have a much wider impact than only coloring, which may lead us to a lot more details consider. Coloring feels a bit more like a beneficial side-effect of such feature.

While we can consider supporting such feature overall, perhaps it's best to discuss that separately from coloring. We'll see what we find out :)

About a coloring approach in general

I ended up with a local proof of concept experiment a while ago, which separates generating the diff from coloring it. It uses a highlighter command to post-process the diff output, for example delta --color-only --diff-so-fancy in my case. Ideally this highlighter would be configurable too. I'll look into pushing this hack into a branch later.

I find the separation of concerns between the logic (how we obtain the diff) and the presentation (how we transform the diff) important. The diff output is coming from either the diff command (when available on non-Windows managed endpoint), or from the Text::Diff module. I would prefer a coloring feature which cares only about coloring, regardless of how the diff itself is obtained. In the same spirit, I expect the tests should be able to focus solely on the behavior too ("did it produce the right output, including colors?")

Alternatives I considered

Allowing configurable arbitrary diff command could work too, for example colordiff or diff --color=always. This sounds like pivoting into a generic command hook instead of a diff hook, though. Finding the right amount of trade-off around it may require a lot more work.

Another approach could be to parse the diff on our own by applying color tags to certain chunks or lines. I had an initial experiment with this a while back, and it felt a lot more fragile and complex to me than offloading it to a pre-existing specialized solution.

Alternatives to check

Perhaps there's something on CPAN for coloring diffs already? 🤔 It may worth a look.

About configuration options

In Rex contexts we have Rex::Commands::set() to set configuration values. In my opinion this module should support at least this approach for consistency and convenience, and I would focus on that first.

Nevertheless, I find an import argument like you proposed an interesting option to consider for configuration when we learn about a use case where set() may not fit well 👍

Where we are now?

Combining the above what I got curious about is the following approach:

• add a config option to define/retrieve a presentation command with set/get
• pass the diff output to the configured coloring/formatting command as an argument or via a pipe (not sure which one fits best yet?)
• displaying the colored/formatted output

[choroba]

Thanks for the detailed answer. I basically agree with everything, it would be great if you could push your hack somewhere for me to see.

Just one idea: you mention "coloring/formatting command", but can't it be a Perl subroutine, too? I can imagine something simple like colouring red what starts with a - and green what starts with a +. There's no need to wrap it into a command, right?

[ferki]

Work-in-progress proof-of-concept hack

[...] it would be great if you could push your hack somewhere for me to see.

I updated some minor things in the maintain branch, and then pushed my proof-of-concept coloring code on top of that to the color_output branch.

Big hack without tests or error checking, also breaks current tests, and needs delta for formatting – works though 😂 Since this is a WIP outside the default branch, it may change without further notice (though unlikely.)

Coloring on our own

Just one idea: you mention "coloring/formatting command", but can't it be a Perl subroutine, too? I can imagine something simple like colouring red what starts with a - and green what starts with a +. There's no need to wrap it into a command, right?

In my experience diff formatting/highlighting is a wide topic, and I strongly prefer to offload it to existing specialized tools. More importantly, each person tends to have their favorite format, and different formats may fit better for different situations – so making it configurable sounds wise.

Providing an acceptable basic built-in coloring approach sounds nice 👍 Like how you propose matching some lines and colorize them with Term::ANSIColor + optionally Win32::Console::ANSI on Windows (which are already transitive dependencies through Rex itself, so we don't add new ones to the stack.)

Previous similar attempt

In fact, using the OUTPUT callback of Text::Diff in a similar way was exactly my original experiment. It was slow because it gets "called once with the (optional) file header, and once for each hunk body". It also doesn't work if we get the diff text by other means, e.g. the default way of calling the diff binary.

General expectations

• It may work more efficiently and universally if we take the whole diff output and pass through it only once on our own.
• I expect testing can go to e.g. a new t/color.t:
  • testing a separate subroutine should probably makes some tests simpler
  • exercising the built-in coloring with a single case for checking the expected output for a known synthetic input sounds like a good starting point
  • ideally I would like to include a test to call the actual diff routine and check for colored output there too
  • even then, testing may be the more complex part to solve properly, rather than the implementation: how to test for colored output in a portable way, and is there a CPAN module to help us?
• If the built-in is not too slow (to be defined later), it sounds wise to match Rex's own colored logging preferences via the value of $Rex::Logger::no_color (=color by default, turn it off with rex -m), and that way we don't need our own config option to opt in or out coloring
• I'd keep it basic, and only aim for the following for now:
  • assume unified diff format as input
  • follow diff --color colors (header as white, removed lines as red, added lines as green)

Additional thoughts

Identified configuration opportunities to consider separately

Coloring and configurability sounds like two different (while related) feature, and it may be possible to discover and implement them separately.

Your original idea was to allow configuring the command which gets the diff, and use a value that happens to return colored diffs. This is likely a good idea to support separately from coloring as soon as there's an actual real-world use case.

If we succeed with a built-in basic coloring option, it may be good enough for a good while, and we can add configurable external highlighting separately later.

Environment variables may be interesting to explore for configuration purposes as well (on top of Rex's built-in set/get.)

Possible performance cliff

I wonder how useful it would be to set an upper limit on the size of diffs we attempt to colorize (or even show?). Unusually large ones may use too much resources or appear as stuck. It sounds like a separate concern, and I expect it's enough to solve when it becomes a problem.