range-diff: optionally include merge commits' diffs in the analysis

Let's add corresponding support for git range-diff, too. This makes it more convenient to spot differences between iterations of non-linear contributions, where so-called "evil merges" are sometimes necessary and need to be reviewed, too.

Am 07.11.24 um 18:20 schrieb Johannes Schindelin via GitGitGadget:

Documentation/git-range-diff.txt to revert to color all lines according to the outer diff markers · line 90

Quoted 7 lines

+--diff-merges=<format>::

Documentation/git-range-diff.txt +6 -0

+	Instead of ignoring merge commits, generate diffs for them using the
+	corresponding `--diff-merges=<format>` option of linkgit:git-log[1],
+	and include them in the comparison.
++
+Note: Some of the formats supported by linkgit:git-log[1] make less sense in
+the context of the `range-diff` command than other formats, so choose wisely!

Can we please be a bit more specific which options are usable or which are not usable?

Perhaps you even mean to say that 'first-parent' is the only one that makes sense?

At a minimum, we should mention that it is the one that makes most sense (if that is the case).

Footnote *1*: We should really find a much better name for this than "evil merge". There is nothing evil about me having to add #include "environment.h" in v2.45.1^, for example. It was necessary so that the code would build. Tedious, yes, but not evil.

That's particularly odd since the first category is the most common, and the third (snuck in unrelated changes or "evil changes") are the most rare. Maybe we should just call these "user-modified merges" rather than "evil merges"? Any better suggestions?

Am 07.11.24 um 18:20 schrieb Johannes Schindelin via GitGitGadget:

Documentation/git-range-diff.txt to revert to color all lines according to the outer diff markers · line 90

Quoted 7 lines

+--diff-merges=<format>::

Documentation/git-range-diff.txt +6 -0

+	Instead of ignoring merge commits, generate diffs for them using the
+	corresponding `--diff-merges=<format>` option of linkgit:git-log[1],
+	and include them in the comparison.
++
+Note: Some of the formats supported by linkgit:git-log[1] make less sense in
+the context of the `range-diff` command than other formats, so choose wisely!

Can we please be a bit more specific which options are usable or which are not usable? Perhaps you even mean to say that 'first-parent' is the only one that makes sense? At a minimum, we should mention that it is the one that makes most sense (if that is the case).

Am 07.11.24 um 18:20 schrieb Johannes Schindelin via GitGitGadget:

Documentation/git-range-diff.txt to revert to color all lines according to the outer diff markers · line 90

Quoted 7 lines

+--diff-merges=<format>::

Documentation/git-range-diff.txt +6 -0

+     Instead of ignoring merge commits, generate diffs for them using the
+     corresponding `--diff-merges=<format>` option of linkgit:git-log[1],
+     and include them in the comparison.
++
+Note: Some of the formats supported by linkgit:git-log[1] make less sense in
+the context of the `range-diff` command than other formats, so choose wisely!

Can we please be a bit more specific which options are usable or which are not usable? Perhaps you even mean to say that 'first-parent' is the only one that makes sense? At a minimum, we should mention that it is the one that makes most sense (if that is the case).

From: Johannes Schindelin <[email protected]>

The git log command already offers support for including diffs for merges, via the --diff-merges=<format> option.

Let's add corresponding support for git range-diff, too. This makes it more convenient to spot differences between iterations of non-linear contributions, where so-called "evil merges" are sometimes necessary and need to be reviewed, too.

In my code reviews, I found the --diff-merges=first-parent option particularly useful.

Signed-off-by Johannes Schindelin <[email protected]>

---
Support diff merges option in range diff

The git range-diff command does the same with merge commits as git
rebase: It ignores them.

However, when comparing branch thickets it can be quite illuminating to watch out for inadvertent changes in merge commits, in particular when some "evil" merges have been replayed, i.e. merges that needed to introduce changes outside of the merge conflicts (e.g. when one branch changed a function's signature and another branch introduced a caller of said function), in case the replayed merge is no longer "evil" and therefore potentially incorrect.

Let's introduce support for the --diff-merges option that is passed through to those git log commands.

I had a need for this earlier this year and got it working, leaving the GitGitGadget PR in a draft mode. Phil Blain found it and kindly nerd-sniped me into readying it for submitting, so say thanks to Phil!

Changes since v1:

* Changed the documentation to recommend first-parent mode instead of vaguely talking about various modes' merits.
* Disallowed the no-arg --diff-merges option (because --diff-merges requires an argument).

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1734%2Fdscho%2Fsupport-diff-merges-option-in-range-diff-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1734/dscho/support-diff-merges-option-in-range-diff-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1734

Range-diff vs v1:

1: 11361e07af8 ! 1: d01a395900b range-diff: optionally include merge commits' diffs in the analysis
@@ Documentation/git-range-diff.txt: to revert to color all lines according to the
+ corresponding --diff-merges=<format> option of linkgit:git-log[1],
+ and include them in the comparison.
++
-+Note: Some of the formats supported by linkgit:git-log[1] make less sense in -+the context of the range-diff command than other formats, so choose wisely! ++Note: In the common case, the first-parent mode will be the most natural one ++to use, as it is consistent with the idea that a merge is kind of a "meta ++patch", comprising all the merged commits' patches into a single one.
+
--[no-]notes[=<ref>]::
This flag is passed to the git log program
@@ builtin/range-diff.c: int cmd_range_diff(int argc,
N_("notes"), N_("passed to 'git log'"), PARSE_OPT_OPTARG),
+ OPT_PASSTHRU_ARGV(0, "diff-merges", &diff_merges_arg,
-+ N_("style"), N_("passed to 'git log'"),
-+ PARSE_OPT_OPTARG),
++ N_("style"), N_("passed to 'git log'"), 0),
OPT_BOOL(0, "left-only", &left_only,
N_("only emit output related to the first range")),
OPT_BOOL(0, "right-only", &right_only,

 Documentation/git-range-diff.txt | 11 ++++++++++-
 builtin/range-diff.c             | 10 ++++++++++
 range-diff.c                     | 15 +++++++++++----
 range-diff.h                     |  1 +
 t/t3206-range-diff.sh            | 16 ++++++++++++++++
 5 files changed, 48 insertions(+), 5 deletions(-)

Documentation/git-range-diff.txt +1 -1

index fbdbe0befeb..17a85957877 100644
@@ -10,7 +10,7 @@ SYNOPSIS
 [verse]
 'git range-diff' [--color=[<when>]] [--no-color] [<diff-options>]
        [--no-dual-color] [--creation-factor=<factor>]
-       [--left-only | --right-only]
+       [--left-only | --right-only] [--diff-merges=<format>]
        ( <range1> <range2> | <rev1>...<rev2> | <base> <rev1> <rev2> )
        [[--] <path>...]
 

as it is consistent with the idea that a merge is kind of a "meta +patch", comprising all the merged commits' patches into a single one.

Documentation/git-range-diff.txt +1 -0

+
 --[no-]notes[=<ref>]::
        This flag is passed to the `git log` program
        (see linkgit:git-log[1]) that generates the patches.

builtin/range-diff.c +10 -0

index 1b33ab66a7b..901de5d133d 100644
@@ -21,6 +21,7 @@ int cmd_range_diff(int argc,
 {
        struct diff_options diffopt = { NULL };
        struct strvec other_arg = STRVEC_INIT;
+       struct strvec diff_merges_arg = STRVEC_INIT;
        struct range_diff_options range_diff_opts = {
                .creation_factor = RANGE_DIFF_CREATION_FACTOR_DEFAULT,
                .diffopt = &diffopt,

@@ -36,6 +37,8 @@ int cmd_range_diff(int argc,
                OPT_PASSTHRU_ARGV(0, "notes", &other_arg,
                                  N_("notes"), N_("passed to 'git log'"),
                                  PARSE_OPT_OPTARG),
+               OPT_PASSTHRU_ARGV(0, "diff-merges", &diff_merges_arg,
+                                 N_("style"), N_("passed to 'git log'"), 0),
                OPT_BOOL(0, "left-only", &left_only,
                         N_("only emit output related to the first range")),
                OPT_BOOL(0, "right-only", &right_only,

@@ -62,6 +65,12 @@ int cmd_range_diff(int argc,
        if (!simple_color)
                diffopt.use_color = 1;
 
+       /* If `--diff-merges` was specified, imply `--merges` */
+       if (diff_merges_arg.nr) {
+               range_diff_opts.include_merges = 1;
+               strvec_pushv(&other_arg, diff_merges_arg.v);
+       }
+
        for (i = 0; i < argc; i++)
                if (!strcmp(argv[i], "--")) {
                        dash_dash = i;

@@ -155,6 +164,7 @@ int cmd_range_diff(int argc,
        res = show_range_diff(range1.buf, range2.buf, &range_diff_opts);
 
        strvec_clear(&other_arg);
+       strvec_clear(&diff_merges_arg);
        strbuf_release(&range1);
        strbuf_release(&range2);
 

range-diff.c +11 -4

index bbb0952264b..9e59733059b 100644
@@ -38,7 +38,8 @@ struct patch_util {
  * as struct object_id (will need to be free()d).
  */
 static int read_patches(const char *range, struct string_list *list,
-                       const struct strvec *other_arg)
+                       const struct strvec *other_arg,
+                       unsigned int include_merges)
 {
        struct child_process cp = CHILD_PROCESS_INIT;
        struct strbuf buf = STRBUF_INIT, contents = STRBUF_INIT;

@@ -49,7 +50,7 @@ static int read_patches(const char *range, struct string_list *list,
        size_t size;
        int ret = -1;
 
-       strvec_pushl(&cp.args, "log", "--no-color", "-p", "--no-merges",
+       strvec_pushl(&cp.args, "log", "--no-color", "-p",
                     "--reverse", "--date-order", "--decorate=no",
                     "--no-prefix", "--submodule=short",
                     /*

@@ -64,6 +65,8 @@ static int read_patches(const char *range, struct string_list *list,
                     "--pretty=medium",
                     "--show-notes-by-default",
                     NULL);
+       if (!include_merges)
+               strvec_push(&cp.args, "--no-merges");
        strvec_push(&cp.args, range);
        if (other_arg)
                strvec_pushv(&cp.args, other_arg->v);

@@ -96,11 +99,14 @@ static int read_patches(const char *range, struct string_list *list,
                }
 
                if (skip_prefix(line, "commit ", &p)) {
+                       char *q;
                        if (util) {
                                string_list_append(list, buf.buf)->util = util;
                                strbuf_reset(&buf);
                        }
                        CALLOC_ARRAY(util, 1);
+                       if (include_merges && (q = strstr(p, " (from ")))
+                               *q = '\0';
                        if (repo_get_oid(the_repository, p, &util->oid)) {
                                error(_("could not parse commit '%s'"), p);
                                FREE_AND_NULL(util);

@@ -571,13 +577,14 @@ int show_range_diff(const char *range1, const char *range2,
 
        struct string_list branch1 = STRING_LIST_INIT_DUP;
        struct string_list branch2 = STRING_LIST_INIT_DUP;
+       unsigned int include_merges = range_diff_opts->include_merges;
 
        if (range_diff_opts->left_only && range_diff_opts->right_only)
                res = error(_("options '%s' and '%s' cannot be used together"), "--left-only", "--right-only");
 
-       if (!res && read_patches(range1, &branch1, range_diff_opts->other_arg))
+       if (!res && read_patches(range1, &branch1, range_diff_opts->other_arg, include_merges))
                res = error(_("could not parse log for '%s'"), range1);
-       if (!res && read_patches(range2, &branch2, range_diff_opts->other_arg))
+       if (!res && read_patches(range2, &branch2, range_diff_opts->other_arg, include_merges))
                res = error(_("could not parse log for '%s'"), range2);
 
        if (!res) {

range-diff.h +1 -0

index 2f69f6a434d..cd85000b5a0 100644
@@ -16,6 +16,7 @@ struct range_diff_options {
        int creation_factor;
        unsigned dual_color:1;
        unsigned left_only:1, right_only:1;
+       unsigned include_merges:1;
        const struct diff_options *diffopt; /* may be NULL */
        const struct strvec *other_arg; /* may be NULL */
 };

t/t3206-range-diff.sh +16 -0

index 86010931ab6..c18a3fdab83 100755
@@ -909,4 +909,20 @@ test_expect_success 'submodule changes are shown irrespective of diff.submodule'
        test_cmp expect actual
 '
 
+test_expect_success '--diff-merges' '
+       renamed_oid=$(git rev-parse --short renamed-file) &&
+       tree=$(git merge-tree unmodified renamed-file) &&
+       clean=$(git commit-tree -m merge -p unmodified -p renamed-file $tree) &&
+       clean_oid=$(git rev-parse --short $clean) &&
+       conflict=$(git commit-tree -m merge -p unmodified -p renamed-file^ $tree) &&
+       conflict_oid=$(git rev-parse --short $conflict) &&
+
+       git range-diff --diff-merges=1 $clean...$conflict >actual &&
+       cat >expect <<-EOF &&
+       1:  $renamed_oid < -:  ------- s/12/B/
+       2:  $clean_oid = 1:  $conflict_oid merge
+       EOF
+       test_cmp expect actual
+'
+
 test_done

base-commit: 777489f9e09c8d0dd6b12f9d90de6376330577a2 --
gitgitgadget

Quoted 9 lines · Johannes Schindelin via GitGitGadget

Documentation/git-range-diff.txt +0 -0

index fbdbe0befeb..17a85957877 100644
@@ -81,6 +81,15 @@ to revert to color all lines according to the outer diff markers
        Suppress commits that are missing from the second specified range
        (or the "right range" when using the `<rev1>...<rev2>` format).
 

Documentation/git-range-diff.txt to revert to color all lines according to the outer diff markers

Quoted 7 lines

Documentation/git-range-diff.txt +7 -0

+--diff-merges=<format>::
+       Instead of ignoring merge commits, generate diffs for them using the
+       corresponding `--diff-merges=<format>` option of linkgit:git-log[1],
+       and include them in the comparison.
++
+Note: In the common case, the `first-parent` mode will be the most natural one
+to use,

I think we need more wording around "common case"; I believe this "common case" is when you are diffing against a merely transplanted series of commits (a series created by rebasing without inserting, removing, or minimally modifying those commits in the process) that first-parent makes sense. And it only makes sense in that case.

I think remerge-diff generally makes sense here, both in the cases when first-parent makes sense and when first-parent does not. It could be improved by suppressing the inclusion of short commit ids (and maybe also commit messages) in the labels of conflict markers. I suspect that issue might make remerge-diff less useful than first-parent in simple common cases currently, but I think it's the right thing to build upon for what you are trying to view.

From: Johannes Schindelin <[email protected]>

The git log command already offers support for including diffs for merges, via the --diff-merges=<format> option.

Let's add corresponding support for git range-diff, too. This makes it more convenient to spot differences between iterations of non-linear contributions, where so-called "evil merges" are sometimes necessary and need to be reviewed, too.

In my code reviews, I found the --diff-merges=first-parent option particularly useful.

Signed-off-by Johannes Schindelin <[email protected]>

---
Support diff merges option in range diff

The git range-diff command does the same with merge commits as git
rebase: It ignores them.

However, when comparing branch thickets it can be quite illuminating to watch out for inadvertent changes in merge commits, in particular when some "evil" merges have been replayed, i.e. merges that needed to introduce changes outside of the merge conflicts (e.g. when one branch changed a function's signature and another branch introduced a caller of said function), in case the replayed merge is no longer "evil" and therefore potentially incorrect.

Let's introduce support for the --diff-merges option that is passed through to those git log commands.

I had a need for this earlier this year and got it working, leaving the GitGitGadget PR in a draft mode. Phil Blain found it and kindly nerd-sniped me into readying it for submitting, so say thanks to Phil!

Changes since v1:

* Changed the documentation to recommend first-parent mode instead of vaguely talking about various modes' merits.
* Disallowed the no-arg --diff-merges option (because --diff-merges requires an argument).

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1734%2Fdscho%2Fsupport-diff-merges-option-in-range-diff-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1734/dscho/support-diff-merges-option-in-range-diff-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1734

Range-diff vs v1:

1: 11361e07af8 ! 1: d01a395900b range-diff: optionally include merge commits' diffs in the analysis
@@ Documentation/git-range-diff.txt: to revert to color all lines according to the
+ corresponding --diff-merges=<format> option of linkgit:git-log[1],
+ and include them in the comparison.
++
-+Note: Some of the formats supported by linkgit:git-log[1] make less sense in -+the context of the range-diff command than other formats, so choose wisely! ++Note: In the common case, the first-parent mode will be the most natural one ++to use, as it is consistent with the idea that a merge is kind of a "meta ++patch", comprising all the merged commits' patches into a single one.
+
--[no-]notes[=<ref>]::
This flag is passed to the git log program
@@ builtin/range-diff.c: int cmd_range_diff(int argc,
N_("notes"), N_("passed to 'git log'"), PARSE_OPT_OPTARG),
+ OPT_PASSTHRU_ARGV(0, "diff-merges", &diff_merges_arg,
-+ N_("style"), N_("passed to 'git log'"),
-+ PARSE_OPT_OPTARG),
++ N_("style"), N_("passed to 'git log'"), 0),
OPT_BOOL(0, "left-only", &left_only,
N_("only emit output related to the first range")),
OPT_BOOL(0, "right-only", &right_only,

 Documentation/git-range-diff.txt | 11 ++++++++++-
 builtin/range-diff.c             | 10 ++++++++++
 range-diff.c                     | 15 +++++++++++----
 range-diff.h                     |  1 +
 t/t3206-range-diff.sh            | 16 ++++++++++++++++
 5 files changed, 48 insertions(+), 5 deletions(-)

Documentation/git-range-diff.txt +1 -1

index fbdbe0befeb..17a85957877 100644
@@ -10,7 +10,7 @@ SYNOPSIS
 [verse]
 'git range-diff' [--color=[<when>]] [--no-color] [<diff-options>]
 	[--no-dual-color] [--creation-factor=<factor>]
-	[--left-only | --right-only]
+	[--left-only | --right-only] [--diff-merges=<format>]
 	( <range1> <range2> | <rev1>...<rev2> | <base> <rev1> <rev2> )
 	[[--] <path>...]
 

@@ -81,6 +81,15 @@ to revert to color all lines according to the outer diff markers
 	Suppress commits that are missing from the second specified range

t/t3206-range-diff.sh +5 -0

index 86010931ab6..c18a3fdab83 100755
@@ -909,4 +909,20 @@ test_expect_success 'submodule changes are shown irrespective of diff.submodule'
 	test_cmp expect actual
 '
 
+test_expect_success '--diff-merges' '
+	renamed_oid=$(git rev-parse --short renamed-file) &&
+	tree=$(git merge-tree unmodified renamed-file) &&
+	clean=$(git commit-tree -m merge -p unmodified -p renamed-file $tree) &&
+	clean_oid=$(git rev-parse --short $clean) &&

Le 2024-11-08 à 08:43, Johannes Schindelin via GitGitGadget a écrit :

Quoted 5 lines

From: Johannes Schindelin <[email protected]>

The git log command already offers support for including diffs for merges, via the --diff-merges=<format> option.

Quoted 4 lines

Let's add corresponding support for git range-diff, too. This makes it more convenient to spot differences between iterations of non-linear contributions, where so-called "evil merges" are sometimes necessary and need to be reviewed, too.

Maybe "between commit ranges that include merge commits" would be more workflow-agnostic ?

Quoted 6 lines

Documentation/git-range-diff.txt +0 -0

index fbdbe0befeb..17a85957877 100644
@@ -81,6 +81,15 @@ to revert to color all lines according to the outer diff markers
 	Suppress commits that are missing from the second specified range

Documentation/git-range-diff.txt to revert to color all lines according to the outer diff markers · line 91

Quoted 10 lines

Documentation/git-range-diff.txt +8 -0

 	(or the "right range" when using the `<rev1>...<rev2>` format).
 
+--diff-merges=<format>::
+	Instead of ignoring merge commits, generate diffs for them using the
+	corresponding `--diff-merges=<format>` option of linkgit:git-log[1],
+	and include them in the comparison.
++
+Note: In the common case, the `first-parent` mode will be the most natural one
+to use, as it is consistent with the idea that a merge is kind of a "meta
+patch", comprising all the merged commits' patches into a single one.

I think I agree with Elijah that we probably should also highlight at least 'remerge'.

Also, is it worth making this a proper Asciidoc "[NOTE]" ? (I'm not sure, there are a lot of "Notes:" in our doc that are not Asciidoc "[NOTE]"s.

builtin/range-diff.c

Quoted 4 lines

builtin/range-diff.c +0 -0

index 1b33ab66a7b..901de5d133d 100644

The changes look good to me. Maybe it would be nice to add a corresponding 'range-diff.diffMerges' config option to allow users to configure the behaviour more permanently ?

Hi Philippe,

Quoted 11 lines · Philippe Blain

Le 2024-11-08 à 08:43, Johannes Schindelin via GitGitGadget a écrit :

Quoted 9 lines

From: Johannes Schindelin <[email protected]>

The git log command already offers support for including diffs for merges, via the --diff-merges=<format> option.

Let's add corresponding support for git range-diff, too. This makes it more convenient to spot differences between iterations of non-linear contributions, where so-called "evil merges" are sometimes necessary and need to be reviewed, too.

Quoted 2 lines

Maybe "between commit ranges that include merge commits" would be more workflow-agnostic ?

Good idea, this is much clearer than what I wrote, too.

Quoted 8 lines

builtin/range-diff.c

Quoted 4 lines

builtin/range-diff.c +0 -0

index 1b33ab66a7b..901de5d133d 100644

The changes look good to me. Maybe it would be nice to add a corresponding 'range-diff.diffMerges' config option to allow users to configure the behaviour more permanently ?

Seeing as there are no existing rangeDiff.* options, I am loathe to introduce the first one lest I am asked why I don't balloon this patch series into introducing config settings for the other options, too.

* Disallowed the no-arg --diff-merges option (because --diff-merges requires an argument).

"Johannes Schindelin via GitGitGadget" <[email protected]> writes:

Quoted 4 lines

+--diff-merges=<format>::

Documentation/git-range-diff.txt +3 -0

+     Instead of ignoring merge commits, generate diffs for them using the
+     corresponding `--diff-merges=<format>` option of linkgit:git-log[1],
+     and include them in the comparison.

Documentation/git-range-diff.txt to revert to color all lines according to the outer diff markers · line 91

Quoted 4 lines

++

Documentation/git-range-diff.txt +3 -0

+Note: In the common case, the `first-parent` mode will be the most natural one
+to use, as it is consistent with the idea that a merge is kind of a "meta
+patch", comprising all the merged commits' patches into a single one.

I'll let you and Elijah figure out the best phrasing around here, including whether we recommend only first-parent or give other options.

For me personally, I find that use of first-parent here is consistent with the mindset of those users who prefer to read "git log --first-parent -p", and to a smaller degree, those who use squash merges. To them, a merge is merely bringing in lots of changes into the mainline as a single unit, and other than that these lots of changes are broken out for finer grained inspection if/when they wanted to, they prefer to treat the changes brought in by merges just like a single-parent commit. And from that point of view, (1) reordering the changes inside the side branch is immaterial for the purpose of talking about the result, the net effect the merged topic makes to the mainline, and (2) dropping or adding new changes to the side branch is treated just like a newer iteration of a single parent commit having fewer or more changes. So it is very natural that first-parent helps to make the world view very simplistic, and more readable range-diff is all about how you can present the two iterations in a simple and flattened form.

There _may_ need a tweak of the matching algorithm to allow the "same" merge on both sides to match, even if they diverge a lot, though. A range-diff that pairs a merge in the previous iteration with a single patch in the updated iteration may be hard to read.

Quoted 4 lines

There _may_ need a tweak of the matching algorithm to allow the "same" merge on both sides to match, even if they diverge a lot, though. A range-diff that pairs a merge in the previous iteration with a single patch in the updated iteration may be hard to read.

Sounds like you are arguing that there is an angle/usecase from which first-parent makes sense, namely the viewpoint that "a merge is merely bringing in lots of changes into the mainline as a single unit" as you put it. What was surprising to me, though, is that it's a completely different usecase than the one that was brought up in the commit message for this feature, namely "so-called 'evil merges' are sometimes necessary and need to be reviewed too".

From: Johannes Schindelin <[email protected]>

The git log command already offers support for including diffs for merges, via the --diff-merges=<format> option.

Let's add corresponding support for git range-diff, too. This makes it more convenient to spot differences between iterations of non-linear contributions, where so-called "evil merges" are sometimes necessary and need to be reviewed, too.

In my code reviews, I found the --diff-merges=first-parent option particularly useful.

Signed-off-by Johannes Schindelin <[email protected]>

---
Support diff merges option in range diff

The git range-diff command does the same with merge commits as git
rebase: It ignores them.

However, when comparing branch thickets it can be quite illuminating to watch out for inadvertent changes in merge commits, in particular when some "evil" merges have been replayed, i.e. merges that needed to introduce changes outside of the merge conflicts (e.g. when one branch changed a function's signature and another branch introduced a caller of said function), in case the replayed merge is no longer "evil" and therefore potentially incorrect.

Let's introduce support for the --diff-merges option that is passed through to those git log commands.

I had a need for this earlier this year and got it working, leaving the GitGitGadget PR in a draft mode. Phil Blain found it and kindly nerd-sniped me into readying it for submitting, so say thanks to Phil!

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1734%2Fdscho%2Fsupport-diff-merges-option-in-range-diff-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1734/dscho/support-diff-merges-option-in-range-diff-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1734

 Documentation/git-range-diff.txt | 10 +++++++++-
 builtin/range-diff.c             | 11 +++++++++++
 range-diff.c                     | 15 +++++++++++----
 range-diff.h                     |  1 +
 t/t3206-range-diff.sh            | 16 ++++++++++++++++
 5 files changed, 48 insertions(+), 5 deletions(-)

Documentation/git-range-diff.txt +1 -1

index fbdbe0befeb..a964e856c3c 100644
@@ -10,7 +10,7 @@ SYNOPSIS
 [verse]
 'git range-diff' [--color=[<when>]] [--no-color] [<diff-options>]
        [--no-dual-color] [--creation-factor=<factor>]
-       [--left-only | --right-only]
+       [--left-only | --right-only] [--diff-merges=<format>]
        ( <range1> <range2> | <rev1>...<rev2> | <base> <rev1> <rev2> )
        [[--] <path>...]
 

@@ -81,6 +81,14 @@ to revert to color all lines according to the outer diff markers
        Suppress commits that are missing from the second specified range

Documentation/git-range-diff.txt +0 -0

 --[no-]notes[=<ref>]::
        This flag is passed to the `git log` program
        (see linkgit:git-log[1]) that generates the patches.

builtin/range-diff.c +11 -0

index 1b33ab66a7b..e41719e0f0d 100644
@@ -21,6 +21,7 @@ int cmd_range_diff(int argc,
 {
        struct diff_options diffopt = { NULL };
        struct strvec other_arg = STRVEC_INIT;
+       struct strvec diff_merges_arg = STRVEC_INIT;
        struct range_diff_options range_diff_opts = {
                .creation_factor = RANGE_DIFF_CREATION_FACTOR_DEFAULT,
                .diffopt = &diffopt,

@@ -36,6 +37,9 @@ int cmd_range_diff(int argc,
                OPT_PASSTHRU_ARGV(0, "notes", &other_arg,
                                  N_("notes"), N_("passed to 'git log'"),
                                  PARSE_OPT_OPTARG),
+               OPT_PASSTHRU_ARGV(0, "diff-merges", &diff_merges_arg,
+                                 N_("style"), N_("passed to 'git log'"),
+                                 PARSE_OPT_OPTARG),
                OPT_BOOL(0, "left-only", &left_only,
                         N_("only emit output related to the first range")),
                OPT_BOOL(0, "right-only", &right_only,

@@ -62,6 +66,12 @@ int cmd_range_diff(int argc,
        if (!simple_color)
                diffopt.use_color = 1;
 
+       /* If `--diff-merges` was specified, imply `--merges` */
+       if (diff_merges_arg.nr) {
+               range_diff_opts.include_merges = 1;
+               strvec_pushv(&other_arg, diff_merges_arg.v);
+       }
+
        for (i = 0; i < argc; i++)
                if (!strcmp(argv[i], "--")) {
                        dash_dash = i;

@@ -155,6 +165,7 @@ int cmd_range_diff(int argc,
        res = show_range_diff(range1.buf, range2.buf, &range_diff_opts);
 
        strvec_clear(&other_arg);
+       strvec_clear(&diff_merges_arg);
        strbuf_release(&range1);
        strbuf_release(&range2);
 

range-diff.c +11 -5

index bbb0952264b..9e59733059b 100644
@@ -38,7 +38,8 @@ struct patch_util {
  * as struct object_id (will need to be free()d).
  */
 static int read_patches(const char *range, struct string_list *list,
-                       const struct strvec *other_arg)
+                       const struct strvec *other_arg,
+                       unsigned int include_merges)
 {
        struct child_process cp = CHILD_PROCESS_INIT;
        struct strbuf buf = STRBUF_INIT, contents = STRBUF_INIT;

@@ -49,7 +50,7 @@ static int read_patches(const char *range, struct string_list *list,
        size_t size;
        int ret = -1;
 
-       strvec_pushl(&cp.args, "log", "--no-color", "-p", "--no-merges",
+       strvec_pushl(&cp.args, "log", "--no-color", "-p",
                     "--reverse", "--date-order", "--decorate=no",
                     "--no-prefix", "--submodule=short",
                     /*

@@ -64,6 +65,8 @@ static int read_patches(const char *range, struct string_list *list,
                     "--pretty=medium",
                     "--show-notes-by-default",
                     NULL);
 
 
 
-- Hannes
 
 
+       if (!include_merges)
+               strvec_push(&cp.args, "--no-merges");
        strvec_push(&cp.args, range);
        if (other_arg)
                strvec_pushv(&cp.args, other_arg->v);

@@ -96,11 +99,14 @@ static int read_patches(const char *range, struct string_list *list,
                }
 
                if (skip_prefix(line, "commit ", &p)) {
+                       char *q;
                        if (util) {
                                string_list_append(list, buf.buf)->util = util;
                                strbuf_reset(&buf);
                        }
                        CALLOC_ARRAY(util, 1);
+                       if (include_merges && (q = strstr(p, " (from ")))
+                               *q = '\0';
                        if (repo_get_oid(the_repository, p, &util->oid)) {
                                error(_("could not parse commit '%s'"), p);
                                FREE_AND_NULL(util);

@@ -571,13 +577,14 @@ int show_range_diff(const char *range1, const char *range2,
 
        struct string_list branch1 = STRING_LIST_INIT_DUP;
        struct string_list branch2 = STRING_LIST_INIT_DUP;
+       unsigned int include_merges = range_diff_opts->include_merges;
 
        if (range_diff_opts->left_only && range_diff_opts->right_only)
                res = error(_("options '%s' and '%s' cannot be used together"), "--left-only", "--right-only");
 
-       if (!res && read_patches(range1, &branch1, range_diff_opts->other_arg))
+       if (!res && read_patches(range1, &branch1, range_diff_opts->other_arg, include_merges))
                res = error(_("could not parse log for '%s'"), range1);
-       if (!res && read_patches(range2, &branch2, range_diff_opts->other_arg))
+       if (!res && read_patches(range2, &branch2, range_diff_opts->other_arg, include_merges))
                res = error(_("could not parse log for '%s'"), range2);
 
        if (!res) {

range-diff.h +1 -0

index 2f69f6a434d..cd85000b5a0 100644
@@ -16,6 +16,7 @@ struct range_diff_options {
        int creation_factor;
        unsigned dual_color:1;
        unsigned left_only:1, right_only:1;
+       unsigned include_merges:1;
        const struct diff_options *diffopt; /* may be NULL */
        const struct strvec *other_arg; /* may be NULL */
 };

t/t3206-range-diff.sh +16 -0

index 86010931ab6..c18a3fdab83 100755
@@ -909,4 +909,20 @@ test_expect_success 'submodule changes are shown irrespective of diff.submodule'
        test_cmp expect actual
 '
 
+test_expect_success '--diff-merges' '
+       renamed_oid=$(git rev-parse --short renamed-file) &&
+       tree=$(git merge-tree unmodified renamed-file) &&
+       clean=$(git commit-tree -m merge -p unmodified -p renamed-file $tree) &&
+       clean_oid=$(git rev-parse --short $clean) &&
+       conflict=$(git commit-tree -m merge -p unmodified -p renamed-file^ $tree) &&
+       conflict_oid=$(git rev-parse --short $conflict) &&
+
+       git range-diff --diff-merges=1 $clean...$conflict >actual &&
+       cat >expect <<-EOF &&
+       1:  $renamed_oid < -:  ------- s/12/B/
+       2:  $clean_oid = 1:  $conflict_oid merge
+       EOF
+       test_cmp expect actual
+'
+
 test_done

base-commit: 777489f9e09c8d0dd6b12f9d90de6376330577a2 --
gitgitgadget