Hi Thomas,

On Fri, 5 Jul 2019, Thomas Gummerer wrote:

It's been quite a while since I sent the RFC [1] (thanks all for the
comments on that), and the series changed shapes quite a bit since the
last round.

Since it's been such a long time, just to remind everyone, the goal of
this series is to make the range-diff output clearer, by showing
information about the filenames to which the current diff belongs.

Thank you for that reminder ;-)

In the previous round, we did this using "section headers" that
include information about the current file and adding that to the
outer diff's hunk headers.

In this round we still keep the section headers (with slightly more
information), but in addition we also add the filename to the inner
diff hunk headers.  In the outer diff hunk headers we then display
either the section header or the inner diff hunk header using a
userdiff pattern.

I like this idea!

In terms of code changes the biggest change is that we're now re-using
the 'parse_git_header' function from the apply code to parse the diff
headers, instead of trying to parse them with some hacky parsing code
in range-diff.c.  This way we are sure that the diff headers are
properly parsed.

Yep, very good.

I had also considered just outputting the section headers directly
from 'git log', but then decided against that.  Parsing the headers
allows a future enhancement of range-diff, where we would allow
parsing an mbox file [2].

Thanks you for your consideration; I still would like to have the option
at some stage to compare a patch series from public-inbox.org/git to the
commits in `pu`, without having to fiddle with finding a valid base commit
to apply the patches on.

I split the "only pass required data" commits up, in the hopes of
making them easier to review, but I'm also happy to squash them if
people feel like that makes it easier to review them.

I found it very easy to review in the current form, thank you for putting
in the extra effort.

An added advantage of this is that we're also getting rid of things
like the similarity index, which are not important in the range-diff,
and are thus not represented in the "section header".

One thing that did not change is that the new/deleted strings are not
translated in this version either.  This is similar to the regular
diff output, where we also don't translate these.  We can still
consider translating them in the future though.

[1]: https://public-inbox.org/git/20190414210933.20875-1-t.gummerer@gmail.com/
[2]: https://github.com/gitgitgadget/git/issues/207

I'm including the range-diff between this version of the series and
the RFC, and a diff between the range diff and the range-diff without
these changes below.  Probably not useful in reviewing the code, but
good to show off the changes made in this series.

Indeed!

I very much like the idea, and the current iteration. I offered a couple
of minor suggestions, in the hope that you find them helpful.

Thank you,
Dscho