Use colored patch output in diff ?

[hzeller]

Since #1866 we have a nicely colored patch.

We have similar code to output diffs in diff.cc which does not do any coloring.

It might be a good code-quality work to refactor the code in patch and diff to use the same code to output diffs.

[hzeller]

The unified diff output is for instance used in the output of showing alternatives for lint output.

Say we have the following file /tmp/undersized_binary_literal.sv

module undersized_binary_literal;
  localparam logic [3:0] Foo = 4'b1;
endmodule

... then running the linter shows the alternatives as a unified diff. Would be cool if they would also be colored:

$ verible-verilog-lint --autofix=patch-interactive /tmp/undersized_binary_literal.sv 
/tmp/undersized_binary_literal.sv:2:35: Binary literal 4'b1 has less digits than expected for 4 bits. [Style: number-literals] [undersized-binary-literal]
[ 1. Alternative Left-expand leading zeroes ]
@@ -1,3 +1,3 @@
 module undersized_binary_literal;
-  localparam logic [3:0] Foo = 4'b1;
+  localparam logic [3:0] Foo = 4'b0001;
 endmodule
[ 2. Alternative Replace with decimal ]
@@ -1,3 +1,3 @@
 module undersized_binary_literal;
-  localparam logic [3:0] Foo = 4'b1;
+  localparam logic [3:0] Foo = 4'd1;
 endmodule
[ 3. Alternative Adjust width to inferred width ]
@@ -1,3 +1,3 @@
 module undersized_binary_literal;
-  localparam logic [3:0] Foo = 4'b1;
+  localparam logic [3:0] Foo = 1'b1;
 endmodule
Autofix is available. Apply? [1,2,3,y,n,a,d,A,D,p,P,?] 

[hzeller]

@IEncinas10 is this something that would interest you ? It essentially would be some refactoring to improve the diff output everywhere.

[IEncinas10]

Sure! Missed this part, as I only knew the script for interactive formatting, so I thought maybe patch.cc was it.

[hzeller]

The code came together at different times, so this is why there is somewhat duplicate efforts. Might be worthwhile using that opportunity to see if they can be refactored together.

[IEncinas10]

Q1: What do you think about adding some highlighting to the linting rule?

We have to do something different because the line above will be highlighted with "bold", the line below with "inverse" so leaving it in plain text makes it hard to see, and as it is the rule the user is breaking it makes sense for me to highlight it somehow.

Quick reference for alternatives:

• https://en.wikipedia.org/wiki/ANSI_escape_code#3-bit_and_4-bit

Q2: The way of refactoring together I see is by adding new constructors to patch.h classes

MarkedLine, HunkIndices, HunkHeader that come from processed data instead of text.

Don't know if this is what you had in mind and I see reasons for disliking it, I'll let you be the judge :). I'll push in my fork and edit this comment so you can see what I'm saying exactly, or if you prefer I could do a draft-PR.

Edit:

MarkedLine example: Make MarkedLine accept a internally constructed string (Op + Line). Added constructor so we avoid copying again.

My idea would be do something similar for HunkHeader and HunkIndices:

• Making HunkIndices constructible from a pair of ints (start, end)
• Making HunkHeader constructible from 2 HunkIndices

Although it seems there is a missmatch between diff.cc and patch.cc regarding HunkIndices. diff.c handles the special case where start=End and only outputs start (avoids ", end") like git does by default:

diff --git a/tmp/1 b/tmp/2
index 7898192..0cfbf08 100644
--- a/tmp/1
+++ b/tmp/2
@@ -1 +1 @@
-a
+2

On the other hand, HunkIndices::Parse always expects "start, end". I don't know if this is meaningful or not.

[hzeller]

A1: I think we should not overdo the coloring. The regular output of filename:<line>:<col> is the expected regular output of the linter looks exactly as the expected output of the linter, just that we now added a bit more visible UI.

A2: Let's start with a draft pull request and discuss the details there. I think in general if we can end up with a higher-level representation of how edits look like and passed around (beyond simple text), i.e. use diff::Edit, the better and more universally we can deal with edit scripts in general. My instinct would actually be to represent patches as diff::Edit scripts, and then use that as base to render patches, but I am not sure if this will preserve everything we need to represent a Hunk. It might be a multi-step refeactoring. So maybe start out with: what is the minimum you'd need to do to have both of these use the same code to render a colorful patch; then you'll have enough familarity with the code to then suggest further refactoring steps.