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

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

Side note: I work on implementing range-diff not only to make life easier for reviewers who have to suffer through v2, v3, ... of my patch series, but also to verify my changes before submitting a new iteration. And also, maybe even more importantly, I plan to use it to verify my merging-rebases of Git for
Windows (for which I previously used to redirect the pre-rebase/post-rebase diffs vs upstream and then compare them using `git diff --no-index`). And of course any interested person can see what changes were necessary e.g. in the merging-rebase of Git for Windows onto v2.17.0 by running a command like:

Thanks for making tools that makes the life of a Git developer easier!
(Just filed https://github.com/gitgitgadget/gitgitgadget/issues/26
which asks to break lines for this cover letter)

base-commit: b7bd9486b055c3f967a870311e704e3bb0654e4f
Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-1%2Fdscho%2Fbranch-diff-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1/dscho/branch-diff-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/1

I just realized that if I had ideal memory of the previous review,
I could contain my whole review answer to this email only.

Range-diff vs v3:

  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)


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

      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").

  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.

 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.

Maybe it is worth having an option to expand all "new" patches.
(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)

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

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

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

Thanks,
Stefan