From: Eric Sunshine <sunshine@sunshineco.com>
To: Johannes Altmanninger <aclopte@gmail.com>
Cc: Junio C Hamano <gitster@pobox.com>,
Git List <git@vger.kernel.org>, Elijah Newren <newren@gmail.com>,
Jeff King <peff@peff.net>
Subject: Re: [PATCH 2/3] range-diff: ignore context-only changes
Date: Tue, 17 Nov 2020 17:56:31 -0500 [thread overview]
Message-ID: <CAPig+cR3XRWYmRTETWfEMSdg+Ri-L0LZzhNMavg4FCkDC19qdA@mail.gmail.com> (raw)
In-Reply-To: <20201117213551.2539438-3-aclopte@gmail.com>
On Tue, Nov 17, 2020 at 4:38 PM Johannes Altmanninger <aclopte@gmail.com> wrote:
> range-diff compares matching commits by comparing their patches against
> each other. When two patches only differ in their context lines, that
> difference would still show up in range-diff's output.
>
> This commit uses diff's new -I/--ignore-matching-lines regex logic to ignore
> diff hunks that only consist of changes to context lines in the input diffs.
>
> Thanks to the previous commit, lines like "## file => renamed-file ##"
> are not considered context lines because they no longer have a leading space.
>
> This gives some extra @@ lines because we now always calculate
> two diffs: one for the patch metadata, like the commit message,
> and another one for the actual file changes.
> This is because the former contains lines with leading spaces that are not
> context lines, so we never want to ignore them.
> ---
Signed-off-by: is missing from all of your patches.
Just a few lightweight style-related review comments below (I didn't
read the patch any deeper than that)...
> diff --git a/range-diff.c b/range-diff.c
> @@ -363,6 +363,31 @@ static void get_correspondences(struct string_list *a, struct string_list *b,
> +static int are_diffs_equivalent(const char *a_diff, const char *b_diff) {
> + for (
> + const char
> + *a_eol = strchr(a_diff, '\n'),
> + *b_eol = strchr(b_diff, '\n');
> + (a_eol = strchr(a_diff, '\n')) &&
> + (b_eol = strchr(b_diff, '\n'));
> + a_diff = a_eol + 1, b_diff = b_eol + 1
> + ) {
This project doesn't yet declare variable as part of 'for', so:
const char *a_eol = ...;
const char *b_eol = ...;
for ( ; (a_eol = ...) & (b_eol = ...); a_diff = ..., b_diff = ...) {
> + if (!!a_eol != !!b_eol)
> + return 0;
> +
> + // Ignore context lines.
> + if (a_diff[0] == ' ' && b_diff[0] == ' ')
> + continue;
Avoid //-style comments. Use /* comments */ instead.
> + size_t a_len = a_eol - a_diff;
> + size_t b_len = b_eol - b_diff;
This project doesn't yet allow mixing declarations and code. Instead
place the declarations at the top of the scope:
for (...) {
size_t a_len;
size_t b_len;
...
a_len = ...;
b_len = ...;
> @@ -390,8 +415,10 @@ static void output_pair_header(struct diff_options *diffopt,
> - } else if (strcmp(a_util->patch, b_util->patch)) {
> - color = color_commit;
> + } else if (a_util->diff_offset != b_util->diff_offset
> + || strncmp(a_util->patch, b_util->patch, a_util->diff_offset)
> + || !are_diffs_equivalent(a_util->diff, b_util->diff)) {
> + color = color_commit;
Style on this project is to break line after || operator rather than before:
if (... ||
... ||
...) {
> @@ -467,6 +494,14 @@ static void output(struct string_list *a, struct string_list *b,
> + regex_t regex;
> + if (regcomp(®ex, "^ ", REG_EXTENDED | REG_NEWLINE))
> + BUG("invalid regex");
> + ALLOC_GROW(diffopt->ignore_regex, diffopt->ignore_regex_nr + 1,
> + diffopt->ignore_regex_alloc);
> + diffopt->ignore_regex[diffopt->ignore_regex_nr] = ®ex;
> + size_t ignoring_context_only_changes = diffopt->ignore_regex_nr + 1;
Should you be calling regfree(®ex) at the end of the function?
next prev parent reply other threads:[~2020-11-17 23:00 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-11-05 13:34 range-diff should suppress context-only changes? Jeff King
2020-11-05 20:55 ` Junio C Hamano
2020-11-17 21:35 ` Johannes Altmanninger
2020-11-17 21:35 ` [PATCH 1/3] range-diff: move " ## filename ##" headers to the first column Johannes Altmanninger
2020-11-17 21:35 ` [PATCH 2/3] range-diff: ignore context-only changes Johannes Altmanninger
2020-11-17 22:56 ` Eric Sunshine [this message]
2020-11-17 21:35 ` [PATCH 3/3] range-diff: only compute patch diff when patches are different Johannes Altmanninger
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=CAPig+cR3XRWYmRTETWfEMSdg+Ri-L0LZzhNMavg4FCkDC19qdA@mail.gmail.com \
--to=sunshine@sunshineco.com \
--cc=aclopte@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=newren@gmail.com \
--cc=peff@peff.net \
/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).