From: Thomas Gummerer <t.gummerer@gmail.com>
To: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Cc: git@vger.kernel.org, Duy Nguyen <pclouds@gmail.com>,
Junio C Hamano <gitster@pobox.com>,
Eric Sunshine <sunshine@sunshineco.com>,
Johannes Sixt <j6t@kdbg.org>
Subject: Re: [PATCH v2 00/14] output improvements for git range-diff
Date: Mon, 8 Jul 2019 12:45:46 +0100 [thread overview]
Message-ID: <20190708114546.GD16825@hank.intra.tgummerer.com> (raw)
In-Reply-To: <nycvar.QRO.7.76.6.1907052138370.44@tvgsbejvaqbjf.bet>
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@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.
Thanks for your review! I did find the suggestions very helpful
indeed :)
next prev parent reply other threads:[~2019-07-08 11:45 UTC|newest]
Thread overview: 61+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20190414210933.20875-1-t.gummerer@gmail.com/>
2019-07-05 17:06 ` [PATCH v2 00/14] output improvements for git range-diff Thomas Gummerer
2019-07-05 17:06 ` [PATCH v2 01/14] apply: replace marc.info link with public-inbox Thomas Gummerer
2019-07-05 17:06 ` [PATCH v2 02/14] apply: only pass required data to skip_tree_prefix Thomas Gummerer
2019-07-05 17:06 ` [PATCH v2 03/14] apply: only pass required data to git_header_name Thomas Gummerer
2019-07-05 17:06 ` [PATCH v2 04/14] apply: only pass required data to check_header_line Thomas Gummerer
2019-07-05 17:06 ` [PATCH v2 05/14] apply: only pass required data to find_name_* Thomas Gummerer
2019-07-05 17:06 ` [PATCH v2 06/14] apply: only pass required data to gitdiff_* functions Thomas Gummerer
2019-07-05 18:51 ` Johannes Schindelin
2019-07-05 17:06 ` [PATCH v2 07/14] apply: make parse_git_header public Thomas Gummerer
2019-07-05 18:48 ` Johannes Schindelin
2019-07-05 17:06 ` [PATCH v2 08/14] range-diff: fix function parameter indentation Thomas Gummerer
2019-07-05 17:06 ` [PATCH v2 09/14] range-diff: split lines manually Thomas Gummerer
2019-07-05 19:05 ` Johannes Schindelin
2019-07-08 11:24 ` Thomas Gummerer
2019-07-05 17:06 ` [PATCH v2 10/14] range-diff: don't remove funcname from inner diff Thomas Gummerer
2019-07-05 19:09 ` Johannes Schindelin
2019-07-05 17:06 ` [PATCH v2 11/14] range-diff: suppress line count in outer diff Thomas Gummerer
2019-07-05 17:06 ` [PATCH v2 12/14] range-diff: add section header instead of diff header Thomas Gummerer
2019-07-05 19:35 ` Johannes Schindelin
2019-07-08 11:44 ` Thomas Gummerer
2019-07-08 13:12 ` Johannes Schindelin
2019-07-05 17:06 ` [PATCH v2 13/14] range-diff: add filename to inner diff Thomas Gummerer
2019-07-05 17:06 ` [PATCH v2 14/14] range-diff: add headers to the outer hunk header Thomas Gummerer
2019-07-05 19:48 ` [PATCH v2 00/14] output improvements for git range-diff Johannes Schindelin
2019-07-08 11:45 ` Thomas Gummerer [this message]
2019-07-08 16:33 ` [PATCH v3 " Thomas Gummerer
2019-07-08 16:33 ` [PATCH v3 01/14] apply: replace marc.info link with public-inbox Thomas Gummerer
2019-07-08 16:33 ` [PATCH v3 02/14] apply: only pass required data to skip_tree_prefix Thomas Gummerer
2019-07-08 16:33 ` [PATCH v3 03/14] apply: only pass required data to git_header_name Thomas Gummerer
2019-07-08 16:33 ` [PATCH v3 04/14] apply: only pass required data to check_header_line Thomas Gummerer
2019-07-08 16:33 ` [PATCH v3 05/14] apply: only pass required data to find_name_* Thomas Gummerer
2019-07-08 16:33 ` [PATCH v3 06/14] apply: only pass required data to gitdiff_* functions Thomas Gummerer
2019-07-08 16:33 ` [PATCH v3 07/14] apply: make parse_git_header public Thomas Gummerer
2019-07-09 19:39 ` Junio C Hamano
2019-07-09 21:23 ` Thomas Gummerer
2019-07-09 23:22 ` Junio C Hamano
2019-07-10 8:48 ` Thomas Gummerer
2019-07-08 16:33 ` [PATCH v3 08/14] range-diff: fix function parameter indentation Thomas Gummerer
2019-07-08 16:33 ` [PATCH v3 09/14] range-diff: split lines manually Thomas Gummerer
2019-07-08 16:33 ` [PATCH v3 10/14] range-diff: don't remove funcname from inner diff Thomas Gummerer
2019-07-08 16:33 ` [PATCH v3 11/14] range-diff: suppress line count in outer diff Thomas Gummerer
2019-07-08 16:33 ` [PATCH v3 12/14] range-diff: add section header instead of diff header Thomas Gummerer
2019-07-08 16:33 ` [PATCH v3 13/14] range-diff: add filename to inner diff Thomas Gummerer
2019-07-08 16:33 ` [PATCH v3 14/14] range-diff: add headers to the outer hunk header Thomas Gummerer
2019-07-11 16:08 ` [PATCH v4 00/14] output improvements for git range-diff Thomas Gummerer
2019-07-11 16:08 ` [PATCH v4 01/14] apply: replace marc.info link with public-inbox Thomas Gummerer
2019-07-11 16:08 ` [PATCH v4 02/14] apply: only pass required data to skip_tree_prefix Thomas Gummerer
2019-07-11 16:08 ` [PATCH v4 03/14] apply: only pass required data to git_header_name Thomas Gummerer
2019-07-11 16:08 ` [PATCH v4 04/14] apply: only pass required data to check_header_line Thomas Gummerer
2019-07-11 16:08 ` [PATCH v4 05/14] apply: only pass required data to find_name_* Thomas Gummerer
2019-07-11 16:08 ` [PATCH v4 06/14] apply: only pass required data to gitdiff_* functions Thomas Gummerer
2019-07-11 16:08 ` [PATCH v4 07/14] apply: make parse_git_diff_header public Thomas Gummerer
2019-07-11 16:08 ` [PATCH v4 08/14] range-diff: fix function parameter indentation Thomas Gummerer
2019-07-11 16:08 ` [PATCH v4 09/14] range-diff: split lines manually Thomas Gummerer
2019-07-11 16:08 ` [PATCH v4 10/14] range-diff: don't remove funcname from inner diff Thomas Gummerer
2019-07-11 16:08 ` [PATCH v4 11/14] range-diff: suppress line count in outer diff Thomas Gummerer
2019-07-11 16:08 ` [PATCH v4 12/14] range-diff: add section header instead of diff header Thomas Gummerer
2019-07-11 16:08 ` [PATCH v4 13/14] range-diff: add filename to inner diff Thomas Gummerer
2019-07-11 16:08 ` [PATCH v4 14/14] range-diff: add headers to the outer hunk header Thomas Gummerer
2019-07-11 22:09 ` [PATCH v4 00/14] output improvements for git range-diff Ramsay Jones
2019-07-12 10:44 ` Johannes Schindelin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190708114546.GD16825@hank.intra.tgummerer.com \
--to=t.gummerer@gmail.com \
--cc=Johannes.Schindelin@gmx.de \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=j6t@kdbg.org \
--cc=pclouds@gmail.com \
--cc=sunshine@sunshineco.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).