Re: improved diff tool

From: Stefan Beller <sbeller@google.com>
Date: Aug 30, 2018, 17:22 UTC
Message-ID: <CAGZ79kbN7CGiQFoCF-3zkJH3h0kxbfM4xigpYc7XNuF-1bcONg@mail.gmail.com>
In-Reply-To: <nycvar.QRO.7.76.6.1808301324120.71@tvgsbejvaqbjf.bet>

On Thu, Aug 30, 2018 at 4:33 AM Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 9 quoted lines

>
> Hi Piers,
>
> On Thu, 30 Aug 2018, Piers Titus van der Torren wrote:
>
> > I've created a diff algorithm that focuses on creating readable diffs,
> > see https://github.com/pierstitus/klondiff
>
> Looks intriguing.

Yes it does.

The diff of c07c0923 looks quite interesting as it shows what was intended to happen (indented a lot of code as it was wrapped).

Playing around with that commit and some recently added switches
  --color-moved --color-moved-ws=allow-indentation-change which is
supposed to solve a similar problem did not yield as good results.

Show 6 quoted lines

> > The git integration as an external diff command works quite well,
> > though it would be nice to integrate it deeper in git, and also in
> > git-gui and gitk. Any way to use ext-diff from the gui tools?
>
> Git GUI and gitk are both Tcl/Tk programs, and will need quite a bit of
> work to accommodate for your diff mode.

It would be awesome to have gitk/git gui working with more diff options.

>
> To put things into perspective: the `--color-words` mode is not integrated
> into Git GUI nor gitk, and it has been around for a while...

How is it different from the "Color words" mode that sits in the drop down menu that defaults to "Line diff" ? https://imgur.com/a/qvo4oOC

(Oh, I realize I had to draw the window wide to see it as it hides easily :/)

> > Is there interest to incorporate this algorithm in the main git
> > codebase? And if so, any hints on how to proceed?

It depends on the the layering, look at xdiff/xdiffi.c xdl_do_diff() which has

  if (XDF_DIFF_ALG(xpp->flags) == XDF_PATIENCE_DIFF)
    return xdl_do_patience_diff(mf1, mf2, xpp, xe);

  if (XDF_DIFF_ALG(xpp->flags) == XDF_HISTOGRAM_DIFF)
    return xdl_do_histogram_diff(mf1, mf2, xpp, xe);

as that produces the xdiff/ is for producing the diffs.

All the coloring is at a later step, see diff.c (it's a big file), builtin_diff(), which uses fn_out_consume() as a callback from the xdiff/ stuff and assembles a git diff around the raw diff.

diff.c is responsible for everything after the raw diff, including coloring, or getting the format of patches right (such as file names before a diff, matching up (renamed) files for diffing and such)

Show 17 quoted lines

> The best advice I have is to look at the `--color-words` mode. It comes
> with its own "consume" function that accumulates lines from the diff, then
> outputs them in a different way than the regular colored diff. Your mode
> would want to do it very similarly.
>
> This is the accumulating part:
>
>         https://github.com/git/git/blob/v2.19.0-rc1/diff.c#L1886
>
> and this is the display part:
>
>         https://github.com/git/git/blob/v2.19.0-rc1/diff.c#L2013
>
> Basically, I would suggest to do a `git grep color.words` to find the
> places where the `--color-words` mode is special-cased, and add new
> special-casing for your mode. Which, BTW, I would suggest to find a
> catchier name for ;-)

AFAICT this is more than just a coloring scheme as it both produces a different low level diff, which would need code in the xdiff parts as well as colors, that is in diff.c.

Thanks, Stefan