On 07/05, Johannes Schindelin wrote: > 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. Yeah, I would like that as well ;) > > 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@xxxxxxxxx/ > > [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. Thanks for your review! I did find the suggestions very helpful indeed :)
