Re: [PATCH v4 00/21] Add `range-diff`, a `tbdiff` lookalike

On Wed, Aug 8, 2018 at 6:05 AM Johannes Schindelin
[off-list ref] wrote:

quoted

[...]

quoted

      diff --git a/Makefile b/Makefile
      --- a/Makefile
      +++ b/Makefile

The line starting with --- is red (default removed color) and the line
with +++ is green (default add color).

Ideally these two lines and the third line above starting with "diff --git"
would render in GIT_COLOR_BOLD ("METAINFO").

I agree that is not the best coloring here, but as you remarked elsewhere,
it would require content-aware dual coloring, and I am loathe to try to
implement that for two reasons: 1) it would take most likely a long time
to design and implement that, and 2) I don't have that time.

So I would like to declare that good enough is good enough in this case.

I anticipated this answer, so I wrote some patches myself, starting at
https://public-inbox.org/git/20180804015317.182683-1-sbeller@google.com/
specifically
https://public-inbox.org/git/20180804015317.182683-5-sbeller@google.com/

I plan on resending these on top of your resend (if any) at a later convenient
time for both you and Junio, as noted in
https://public-inbox.org/git/CAGZ79kZnVEsvpicNu7LXkRcHuRqGvESfvG3DL5O_2kPVYrW-Gg@mail.gmail.com/

quoted

quoted

  3:  076e1192d !  3:  4e3fb47a1 range-diff: first rudimentary implementation
     @@ -4,7 +4,7 @@

          At this stage, `git range-diff` can determine corresponding commits
          of two related commit ranges. This makes use of the recently introduced
     -    implementation of the Hungarian algorithm.
     +    implementation of the linear assignment algorithm.

          The core of this patch is a straight port of the ideas of tbdiff, the
          apparently dormant project at https://github.com/trast/tbdiff.
     @@ -51,19 +51,17 @@
      + int res = 0;
      + struct strbuf range1 = STRBUF_INIT, range2 = STRBUF_INIT;

     -- argc = parse_options(argc, argv, NULL, options,
     --                      builtin_range_diff_usage, 0);
     -+ argc = parse_options(argc, argv, NULL, options, builtin_range_diff_usage,
     -+                      0);
     +  argc = parse_options(argc, argv, NULL, options,
     +                       builtin_range_diff_usage, 0);

This is really nice in colors when viewed locally.

quoted

 16:  dfa7b1e71 <  -:  --------- range-diff --dual-color: work around bogus white-space warning
  -:  --------- > 16:  f4252f2b2 range-diff --dual-color: fix bogus white-space warning

Ah; here my initial assumption of only reviewing the range-diff breaks down now.
I'll dig into patch 16 separately.

Right. This was an almost complete rewrite, and then next iteration will
hopefully bring another complete rewrite: disabling whitespace warnings in
dual color mode.

quoted

Maybe it is worth having an option to expand all "new" patches.

Sure.

And I also have a use case for --left-only/--right-only.

And I also have a strong use case (and so does Junio, it seems, or for
that matter, anybody contributing to Git due to Junio's insistence on
signing off on each patch, rather than on the merge commit) for something
like --ignore-lines=<regex>.

And you probably guess what I will say next: these features will make for
really fantastic patch series *on top* of mine. There really is no good
reason to delay the current patch series just to cram more features into
it that had not been planned in the first place.

Yes, I agree. I am unsure about the current state of your series, though;

Junio thinks (expects?) a resend, whereas you seem to call it good enough
but also said (some time back) that you want to resend due to Thomas
feedback.

I do have 2 series on top of the current range-diff.
* The first (queued by Junio as origin/sb/range-diff-colors)
   adds a basic test for colors and improves diff.c readability
* The second (linked above) changes colors for some lines.

I do not want to build more on top as long as I do not know if
you resend (and how much it'll change)

Thanks,
Stefan