Hi Stefan,
On Mon, 23 Jul 2018, Stefan Beller wrote:
> On Sat, Jul 21, 2018 at 3:04 PM Johannes Schindelin via GitGitGadget
> <gitgitgadget@xxxxxxxxx> wrote:
>
> > 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
> [...]
> > 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.
> > 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.
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)
>
> > 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,
Dscho
