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

From: Stefan Beller <sbeller@google.com>
Date: Jul 23, 2018, 21:03 UTC
Message-ID: <CAGZ79kb4ki0cXLnJHeqzRvWaGWki1_epWOdCy49s_v9cy_tJ2A@mail.gmail.com>
In-Reply-To: <pull.1.v4.git.gitgitgadget@gmail.com>

On Sat, Jul 21, 2018 at 3:04 PM Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com> 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.

Show 12 quoted lines

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

Show 20 quoted lines

>   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