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