On Wed, Aug 8, 2018 at 6:05 AM Johannes Schindelin
<[email protected]> wrote:
> > [...]
> > > 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.
I anticipated this answer, so I wrote some patches myself, starting at
https://public-inbox.org/git/[email protected]/
specifically
https://public-inbox.org/git/[email protected]/
I plan on resending these on top of your resend (if any) at a later convenient
time for both you and Junio, as noted in
https://public-inbox.org/git/cagz79kznvesvpicnu7lxkrchurqgvesfvg3dl5o_2kpvyrw...@mail.gmail.com/
>
> > > 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.
Yes, I agree. I am unsure about the current state of your series, though;
Junio thinks (expects?) a resend, whereas you seem to call it good enough
but also said (some time back) that you want to resend due to Thomas
feedback.
I do have 2 series on top of the current range-diff.
* The first (queued by Junio as origin/sb/range-diff-colors)
adds a basic test for colors and improves diff.c readability
* The second (linked above) changes colors for some lines.
I do not want to build more on top as long as I do not know if
you resend (and how much it'll change)
Thanks,
Stefan
