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

Hi Stefan,

On Mon, 23 Jul 2018, Stefan Beller wrote:

On Sat, Jul 21, 2018 at 3:04 PM Johannes Schindelin via GitGitGadget
[off-list ref] wrote:

quoted

  1:  39272eefc !  1:  f7e70689e linear-assignment: a function to solve least-cost assignment problems
     @@ -223,9 +223,7 @@
      +                         BUG("negative j: %d", j);
      +                 i = pred[j];
      +                 column2row[j] = i;
     -+                 k = j;
     -+                 j = row2column[i];
     -+                 row2column[i] = k;
     ++                 SWAP(j, row2column[i]);

The dual color option (as a default) really helps here. Thanks for that!
Does it have to be renamed though? (It's more than two colors; originally
it was inverting the beginning signs)

Maybe --color=emphasize-later
assuming there will be other modes for coloring, such as "diff-only",
which would correspond with --no-dual-color, or "none" that will correspond
would be --no-color. I imagine there could be more fancy things, hence I would
propose a mode rather than a switch.
(Feel free to push back if discussing naming here feels like bike shedding)

I do feel free to push back on that.

2:  7f15b26d4ea !  82:  88134121d2a Introduce `range-diff` to compare
iterations of a topic branch
[...]

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.

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.

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.

(Given that the range-diff
pr-1/dscho/branch-diff-v3...pr-1/dscho/branch-diff-v4 told me you used a
different base, this is a hard problem, as I certainly would want to
skip over all new base commits, but this one is interesting to look at.
An easy way out: Maybe an option to expand any new commits/patches after
the first expansion? Asking for opinions rather than implementing it)

quoted

 19:  144363006 <  -:  --------- range-diff: left-pad patch numbers
  -:  --------- > 19:  07ec215e8 range-diff: left-pad patch numbers

quoted

  -:  --------- > 21:  d8498fb32 range-diff: use dim/bold cues to improve dual color mode

Those are interesting, I'll look at them separately, too.

Thanks,
Dscho