Hi Dscho, Le 2024-11-08 à 08:43, Johannes Schindelin via GitGitGadget a écrit : > From: Johannes Schindelin <johannes.schindelin@xxxxxx> > > 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. Maybe "between commit ranges that include merge commits" would be more workflow-agnostic ? Just thinking out loud here, I think what you wrote is also OK. > In my code reviews, I found the `--diff-merges=first-parent` option > particularly useful. > > Signed-off-by: Johannes Schindelin <johannes.schindelin@xxxxxx> > --- > 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(-) > > diff --git a/Documentation/git-range-diff.txt b/Documentation/git-range-diff.txt > index fbdbe0befeb..17a85957877 100644 > --- a/Documentation/git-range-diff.txt > +++ b/Documentation/git-range-diff.txt > @@ -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 > (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. > diff --git a/builtin/range-diff.c b/builtin/range-diff.c > index 1b33ab66a7b..901de5d133d 100644 > --- a/builtin/range-diff.c > +++ b/builtin/range-diff.c 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 ? > diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh > index 86010931ab6..c18a3fdab83 100755 > --- a/t/t3206-range-diff.sh > +++ b/t/t3206-range-diff.sh > @@ -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 > +' The test looks good. Thanks for submitting that patch! Philippe.
