Hi Stefan,
On Wed, 8 Aug 2018, Stefan Beller wrote:
> On Wed, Aug 8, 2018 at 6:05 AM Johannes Schindelin
> <Johannes.Schindelin@xxxxxx> 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/20180804015317.182683-1-sbeller@xxxxxxxxxx/
> specifically
> https://public-inbox.org/git/20180804015317.182683-5-sbeller@xxxxxxxxxx/
>
> 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-Gg@xxxxxxxxxxxxxx/
Thank you!
> > > > 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 happy to hear that.
> 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.
Sorry about being so unclear. When time gets scarce, sometimes I get too
stressed (and too short on time) to communicate properly.
Yes, I want to limit the new features put into this patch series, in the
interest of getting things into `next` (and maybe still into `master`
before v2.19, but I am not allowing myself to hope for that too much).
And yes, I want to send another iteration, as there have been too many
changes that I do not want to ask Junio to touch up, in particular because
I am a little bit of a detail-oriented person and want my fixes just so.
> 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)
It will not change much. The biggest change is that the white-space
warning thing is done completely differently, so that I do not even have
to be in ws.c's author list.
I'll just try to get that option parsing change in that Eric suggested,
force-push, then wait for macOS and Linux builds to pass (trusting that
Windows will follow suite) and hit /submit.
Ciao,
Dscho
