Re: [PATCH 2/3] range-diff: ignore context-only changes

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Nov 17, 2020, 22:56 UTC
Message-ID: <CAPig+cR3XRWYmRTETWfEMSdg+Ri-L0LZzhNMavg4FCkDC19qdA@mail.gmail.com>
In-Reply-To: <20201117213551.2539438-3-aclopte@gmail.com>

On Tue, Nov 17, 2020 at 4:38 PM Johannes Altmanninger <aclopte@gmail.com> wrote:

Show 16 quoted lines

> 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)...

Show 11 quoted lines

> 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 = ...) {

Show 6 quoted lines

> +               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 = ...;

Show 7 quoted lines

> @@ -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 (... ||
        ... ||
        ...) {

Show 8 quoted lines

> @@ -467,6 +494,14 @@ static void output(struct string_list *a, struct string_list *b,
> +       regex_t regex;
> +       if (regcomp(&regex, "^ ", 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] = &regex;
> +       size_t ignoring_context_only_changes = diffopt->ignore_regex_nr + 1;

Should you be calling regfree(&regex) at the end of the function?