{"thread":{"id":"62464","subject":"[PATCH] range-diff: optionally include merge commits' diffs in the analysis","startedAt":"2024-11-07T17:20:15Z","lastAt":"2024-12-16T14:11:29Z","messageCount":21,"participants":["Johannes Schindelin via GitGitGadget","Junio C Hamano","Johannes Sixt","Johannes Schindelin","Elijah Newren","Philippe Blain"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"506808","messageId":"pull.1734.git.1731000007391.gitgitgadget@gmail.com","threadId":"62464","inReplyTo":null,"subject":"[PATCH] range-diff: optionally include merge commits' diffs in the analysis","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-11-07T17:20:07Z","receivedAt":"2024-11-07T17:20:15Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThe `git log` command already offers support for including diffs for\nmerges, via the `--diff-merges=` option.\n\nLet's add corresponding support for `git range-diff`, too. This makes it\nmore convenient to spot differences between iterations of non-linear\ncontributions, where so-called \"evil merges\" are sometimes necessary and\nneed to be reviewed, too.\n\nIn my code reviews, I found the `--diff-merges=first-parent` option\nparticularly useful.\n\nSigned-off-by: Johannes Schindelin \n---\n Support diff merges option in range diff\n \n The git range-diff command does the same with merge commits as git\n rebase: It ignores them.\n \n However, when comparing branch thickets it can be quite illuminating to\n watch out for inadvertent changes in merge commits, in particular when\n some \"evil\" merges have been replayed, i.e. merges that needed to\n introduce changes outside of the merge conflicts (e.g. when one branch\n changed a function's signature and another branch introduced a caller of\n said function), in case the replayed merge is no longer \"evil\" and\n therefore potentially incorrect.\n \n Let's introduce support for the --diff-merges option that is passed\n through to those git log commands.\n \n I had a need for this earlier this year and got it working, leaving the\n GitGitGadget PR in a draft mode. Phil Blain found it and kindly\n nerd-sniped me into readying it for submitting, so say thanks to Phil!\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1734%2Fdscho%2Fsupport-diff-merges-option-in-range-diff-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1734/dscho/support-diff-merges-option-in-range-diff-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1734\n\n Documentation/git-range-diff.txt | 10 +++++++++-\n builtin/range-diff.c | 11 +++++++++++\n range-diff.c | 15 +++++++++++----\n range-diff.h | 1 +\n t/t3206-range-diff.sh | 16 ++++++++++++++++\n 5 files changed, 48 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/git-range-diff.txt b/Documentation/git-range-diff.txt\nindex fbdbe0befeb..a964e856c3c 100644\n--- a/Documentation/git-range-diff.txt\n+++ b/Documentation/git-range-diff.txt\n@@ -10,7 +10,7 @@ SYNOPSIS\n [verse]\n 'git range-diff' [--color=[]] [--no-color] []\n \t[--no-dual-color] [--creation-factor=]\n-\t[--left-only | --right-only]\n+\t[--left-only | --right-only] [--diff-merges=]\n \t( | ... | )\n \t[[--] ...]\n \n@@ -81,6 +81,14 @@ to revert to color all lines according to the outer diff markers\n \tSuppress commits that are missing from the second specified range\n \t(or the \"right range\" when using the `...` format).\n \n+--diff-merges=::\n+\tInstead of ignoring merge commits, generate diffs for them using the\n+\tcorresponding `--diff-merges=` option of linkgit:git-log[1],\n+\tand include them in the comparison.\n++\n+Note: Some of the formats supported by linkgit:git-log[1] make less sense in\n+the context of the `range-diff` command than other formats, so choose wisely!\n+\n --[no-]notes[=]::\n \tThis flag is passed to the `git log` program\n \t(see linkgit:git-log[1]) that generates the patches.\ndiff --git a/builtin/range-diff.c b/builtin/range-diff.c\nindex 1b33ab66a7b..e41719e0f0d 100644\n--- a/builtin/range-diff.c\n+++ b/builtin/range-diff.c\n@@ -21,6 +21,7 @@ int cmd_range_diff(int argc,\n {\n \tstruct diff_options diffopt = { NULL };\n \tstruct strvec other_arg = STRVEC_INIT;\n+\tstruct strvec diff_merges_arg = STRVEC_INIT;\n \tstruct range_diff_options range_diff_opts = {\n \t\t.creation_factor = RANGE_DIFF_CREATION_FACTOR_DEFAULT,\n \t\t.diffopt = &diffopt,\n@@ -36,6 +37,9 @@ int cmd_range_diff(int argc,\n \t\tOPT_PASSTHRU_ARGV(0, \"notes\", &other_arg,\n \t\t\t\t N_(\"notes\"), N_(\"passed to 'git log'\"),\n \t\t\t\t PARSE_OPT_OPTARG),\n+\t\tOPT_PASSTHRU_ARGV(0, \"diff-merges\", &diff_merges_arg,\n+\t\t\t\t N_(\"style\"), N_(\"passed to 'git log'\"),\n+\t\t\t\t PARSE_OPT_OPTARG),\n \t\tOPT_BOOL(0, \"left-only\", &left_only,\n \t\t\t N_(\"only emit output related to the first range\")),\n \t\tOPT_BOOL(0, \"right-only\", &right_only,\n@@ -62,6 +66,12 @@ int cmd_range_diff(int argc,\n \tif (!simple_color)\n \t\tdiffopt.use_color = 1;\n \n+\t/* If `--diff-merges` was specified, imply `--merges` */\n+\tif (diff_merges_arg.nr) {\n+\t\trange_diff_opts.include_merges = 1;\n+\t\tstrvec_pushv(&other_arg, diff_merges_arg.v);\n+\t}\n+\n \tfor (i = 0; i < argc; i++)\n \t\tif (!strcmp(argv[i], \"--\")) {\n \t\t\tdash_dash = i;\n@@ -155,6 +165,7 @@ int cmd_range_diff(int argc,\n \tres = show_range_diff(range1.buf, range2.buf, &range_diff_opts);\n \n \tstrvec_clear(&other_arg);\n+\tstrvec_clear(&diff_merges_arg);\n \tstrbuf_release(&range1);\n \tstrbuf_release(&range2);\n \ndiff --git a/range-diff.c b/range-diff.c\nindex bbb0952264b..9e59733059b 100644\n--- a/range-diff.c\n+++ b/range-diff.c\n@@ -38,7 +38,8 @@ struct patch_util {\n * as struct object_id (will need to be free()d).\n */\n static int read_patches(const char *range, struct string_list *list,\n-\t\t\tconst struct strvec *other_arg)\n+\t\t\tconst struct strvec *other_arg,\n+\t\t\tunsigned int include_merges)\n {\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n \tstruct strbuf buf = STRBUF_INIT, contents = STRBUF_INIT;\n@@ -49,7 +50,7 @@ static int read_patches(const char *range, struct string_list *list,\n \tsize_t size;\n \tint ret = -1;\n \n-\tstrvec_pushl(&cp.args, \"log\", \"--no-color\", \"-p\", \"--no-merges\",\n+\tstrvec_pushl(&cp.args, \"log\", \"--no-color\", \"-p\",\n \t\t \"--reverse\", \"--date-order\", \"--decorate=no\",\n \t\t \"--no-prefix\", \"--submodule=short\",\n \t\t /*\n@@ -64,6 +65,8 @@ static int read_patches(const char *range, struct string_list *list,\n \t\t \"--pretty=medium\",\n \t\t \"--show-notes-by-default\",\n \t\t NULL);\n+\tif (!include_merges)\n+\t\tstrvec_push(&cp.args, \"--no-merges\");\n \tstrvec_push(&cp.args, range);\n \tif (other_arg)\n \t\tstrvec_pushv(&cp.args, other_arg->v);\n@@ -96,11 +99,14 @@ static int read_patches(const char *range, struct string_list *list,\n \t\t}\n \n \t\tif (skip_prefix(line, \"commit \", &p)) {\n+\t\t\tchar *q;\n \t\t\tif (util) {\n \t\t\t\tstring_list_append(list, buf.buf)->util = util;\n \t\t\t\tstrbuf_reset(&buf);\n \t\t\t}\n \t\t\tCALLOC_ARRAY(util, 1);\n+\t\t\tif (include_merges && (q = strstr(p, \" (from \")))\n+\t\t\t\t*q = '\\0';\n \t\t\tif (repo_get_oid(the_repository, p, &util->oid)) {\n \t\t\t\terror(_(\"could not parse commit '%s'\"), p);\n \t\t\t\tFREE_AND_NULL(util);\n@@ -571,13 +577,14 @@ int show_range_diff(const char *range1, const char *range2,\n \n \tstruct string_list branch1 = STRING_LIST_INIT_DUP;\n \tstruct string_list branch2 = STRING_LIST_INIT_DUP;\n+\tunsigned int include_merges = range_diff_opts->include_merges;\n \n \tif (range_diff_opts->left_only && range_diff_opts->right_only)\n \t\tres = error(_(\"options '%s' and '%s' cannot be used together\"), \"--left-only\", \"--right-only\");\n \n-\tif (!res && read_patches(range1, &branch1, range_diff_opts->other_arg))\n+\tif (!res && read_patches(range1, &branch1, range_diff_opts->other_arg, include_merges))\n \t\tres = error(_(\"could not parse log for '%s'\"), range1);\n-\tif (!res && read_patches(range2, &branch2, range_diff_opts->other_arg))\n+\tif (!res && read_patches(range2, &branch2, range_diff_opts->other_arg, include_merges))\n \t\tres = error(_(\"could not parse log for '%s'\"), range2);\n \n \tif (!res) {\ndiff --git a/range-diff.h b/range-diff.h\nindex 2f69f6a434d..cd85000b5a0 100644\n--- a/range-diff.h\n+++ b/range-diff.h\n@@ -16,6 +16,7 @@ struct range_diff_options {\n \tint creation_factor;\n \tunsigned dual_color:1;\n \tunsigned left_only:1, right_only:1;\n+\tunsigned include_merges:1;\n \tconst struct diff_options *diffopt; /* may be NULL */\n \tconst struct strvec *other_arg; /* may be NULL */\n };\ndiff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh\nindex 86010931ab6..c18a3fdab83 100755\n--- a/t/t3206-range-diff.sh\n+++ b/t/t3206-range-diff.sh\n@@ -909,4 +909,20 @@ test_expect_success 'submodule changes are shown irrespective of diff.submodule'\n \ttest_cmp expect actual\n '\n \n+test_expect_success '--diff-merges' '\n+\trenamed_oid=$(git rev-parse --short renamed-file) &&\n+\ttree=$(git merge-tree unmodified renamed-file) &&\n+\tclean=$(git commit-tree -m merge -p unmodified -p renamed-file $tree) &&\n+\tclean_oid=$(git rev-parse --short $clean) &&\n+\tconflict=$(git commit-tree -m merge -p unmodified -p renamed-file^ $tree) &&\n+\tconflict_oid=$(git rev-parse --short $conflict) &&\n+\n+\tgit range-diff --diff-merges=1 $clean...$conflict >actual &&\n+\tcat >expect <<-EOF &&\n+\t1: $renamed_oid < -: ------- s/12/B/\n+\t2: $clean_oid = 1: $conflict_oid merge\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n test_done\n\nbase-commit: 777489f9e09c8d0dd6b12f9d90de6376330577a2\n-- \ngitgitgadget\n"},{"id":"506823","messageId":"xmqq5xoypetw.fsf@gitster.g","threadId":"62464","inReplyTo":"pull.1734.git.1731000007391.gitgitgadget@gmail.com","subject":"Re: [PATCH] range-diff: optionally include merge commits' diffs in the analysis","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-08T03:46:51Z","receivedAt":"2024-11-08T03:46:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> Let's add corresponding support for `git range-diff`, too. This makes it\n> more convenient to spot differences between iterations of non-linear\n> contributions, where so-called \"evil merges\" are sometimes necessary and\n> need to be reviewed, too.\n\nNice addition.\n\nAs the \"diff of the diff\" outer diff part would be two-\"blob\" diff,\nwe won't have to worry about this option causing confusions to\nusers, unlike things like \"-U8\", to which layer of diffs the option\nwould apply.\n\nWill queue. Thanks.\n"},{"id":"506827","messageId":"abc2453b-f7df-4601-8834-595881f9ceca@kdbg.org","threadId":"62464","inReplyTo":"pull.1734.git.1731000007391.gitgitgadget@gmail.com","subject":"Re: [PATCH] range-diff: optionally include merge commits' diffs in the analysis","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2024-11-08T06:53:46Z","receivedAt":"2024-11-08T06:53:54Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 07.11.24 um 18:20 schrieb Johannes Schindelin via GitGitGadget:\n> +--diff-merges=::\n> +\tInstead of ignoring merge commits, generate diffs for them using the\n> +\tcorresponding `--diff-merges=` option of linkgit:git-log[1],\n> +\tand include them in the comparison.\n> ++\n> +Note: Some of the formats supported by linkgit:git-log[1] make less sense in\n> +the context of the `range-diff` command than other formats, so choose wisely!\n\nCan we please be a bit more specific which options are usable or which\nare not usable? Perhaps you even mean to say that 'first-parent' is the\nonly one that makes sense? At a minimum, we should mention that it is\nthe one that makes most sense (if that is the case).\n\n-- Hannes\n\n"},{"id":"506852","messageId":"af576487-5de2-fba3-b341-3c082322c9ec@gmx.de","threadId":"62464","inReplyTo":"abc2453b-f7df-4601-8834-595881f9ceca@kdbg.org","subject":"Re: [PATCH] range-diff: optionally include merge commits' diffs in the analysis","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2024-11-08T10:53:44Z","receivedAt":"2024-11-08T10:53:47Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Hannes\n\nOn Fri, 8 Nov 2024, Johannes Sixt wrote:\n\n> Am 07.11.24 um 18:20 schrieb Johannes Schindelin via GitGitGadget:\n> > +--diff-merges=::\n> > +\tInstead of ignoring merge commits, generate diffs for them using the\n> > +\tcorresponding `--diff-merges=` option of linkgit:git-log[1],\n> > +\tand include them in the comparison.\n> > ++\n> > +Note: Some of the formats supported by linkgit:git-log[1] make less sense in\n> > +the context of the `range-diff` command than other formats, so choose wisely!\n>\n> Can we please be a bit more specific which options are usable or which\n> are not usable?\n\nThere are a couple of reasons:\n\n- Most importantly: I had not even studied that question in depth when I\n wrote the original patch, and not even when I submitted the first\n iteration.\n\n In my reviews of branch thickets, I used `--diff-merges=1` and it served\n my needs well. Therefore I can speak to this mode, but not really to the\n other modes.\n\n- Which options are usable or not may lack a universally-correct answer.\n\n One person might find `--diff-merges=separate` confusing while the next\n person may find it quite useful.\n\n- If the documentation points out an unsuitable mode, then an obvious\n follow-up wish would be to reject this mode in the command, with an\n error message. Some users might, however, need precisely this mode, so\n I am loathe to do that.\n\nMaybe this compromise: Change the \"Note:\" to recommend the `first-parent`\nmode, with a bit more explanation why that is so (it is consistent with\nthe idea that a merge is kind of a \"meta patch\", comprising all the merged\ncommits' patches, which is equivalent to the first-parent diff).\n\n> Perhaps you even mean to say that 'first-parent' is the only one that\n> makes sense?\n\nI would not go _so_ far as to say that.\n\nBefore submitting the patch yesterday, I played a little with a few\nmodes, using the commit graph as per the new t3206 test case.\n\nWhat that test case range-diffs looks like this:\n\n - * - changeA - changeB ------------\n \\ \\ \\\n \\ conflict \\\n \\ / \\\n rename - changeA - changeB ----- clean\n\nIn other words, the main branch modifies a file's contents twice, the\ntopic branch renames it first, then makes the same modifications.\n\nThe clean merge simply merges the topic, which makes the main branch\ntree-same to the topic branch.\n\nThe conflicting merge misses the tip commit of the topic branch, i.e.\nchangeB on the renamed file. The result is tree-same to the clean merge\nbecause changeB on the non-renamed file is already part of the main\nbranch.\n\nOut of habit, I used `--diff-merge=first-parent` in the new test case,\nwhose output reiterates what I just said: both merges introduce the same\nfirst-parent diff (renaming the file, no other changes):\n\n\t1: 1e6226b < -: ------- s/12/B/\n\t2: 043e738 = 1: 6ddec15 merge\n\nWhat about `separate`?\n\n\n\t2: 043e738 = 1: 6ddec15 merge\n\t1: 1e6226b ! 2: 6ddec15 s/12/B/\n\t @@\n\t ## Metadata ##\n\t -Author: Thomas Rast \n\t +Author: A U Thor \n\n\t ## Commit message ##\n\t - s/12/B/\n\t + merge\n\n\t ## renamed-file ##\n\t @@ renamed-file: A\n\nThis might look confusing at first because there are two merge diffs on\nthe right-hand side, but only one on the left-hand side. But that is all\ngood because the diff of the clean merge between merge head and merge is\nempty and therefore not shown. And the `range-diff` demonstrates that the\nconflicting merge had to recapitulate the tip commit of the `renamed-file`\ntopic branch.\n\nThe `combined` and `dense-combined` modes produce identical output to\n`first-parent` in this instance, which is expected.\n\nNow, let's use `remerge`, which should be illuminating with regards to\n\"evil merges\" [*1*], as it shows what was done on top of the automatic\nmerge:\n\n\t1: 1e6226b < -: ------- s/12/B/\n\t2: 043e738 < -: ------- merge\n\t-: ------- > 1: 6ddec15 merge\n\nHuh. That is a bit surprising at first. Let's use a higher creation factor\nto ask `range-diff` to match correspondences more loosely (I used 999,\nwhich kinda forces even non-sensical pairings):\n\n\t1: 1e6226b ! 1: 6ddec15 s/12/B/\n\t @@\n\t ## Metadata ##\n\t -Author: Thomas Rast \n\t +Author: A U Thor \n\n\t ## Commit message ##\n\t - s/12/B/\n\t + merge\n\n\t ## renamed-file ##\n\t + remerge CONFLICT (content): Merge conflict in renamed-file\n\t + index 3f1c0da..25ddf7a 100644\n\t + --- renamed-file\n\t + +++ renamed-file\n\t @@ renamed-file: A\n\t 9\n\t 10\n\t B\n\t +-<<<<<<< 2901f77 (s/12/B/):file\n\t + B\n\t +-=======\n\t -12\n\t -+B\n\t +->>>>>>> 3ce7af6 (s/11/B/):renamed-file\n\t 13\n\t 14\n\t 15\n\t2: 043e738 < -: ------- merge\n\n(Note that if you repeat this experiment, you will see something different\ndue to a bug in `--remerge-diff` that I will fix separately.)\n\nHold on, that looks wrong! It is actually not wrong, `range-diff` just\nfinds the changeB a better pairing than the merge commit with an empty\ndiff...\n\nSo: This mode can be useful to detect where merge conflicts had to be\nresolved.\n\nIn short, I think that in `range-diff`'s context all of these modes have\nmerit, depending on a particular use case. It's just that `first-parent`\nis probably the most natural to use in the common case.\n\n> At a minimum, we should mention that it is the one that makes most sense\n> (if that is the case).\n\nYeah, I'll go with that and drop saying anything about other modes.\n\nCiao,\nJohannes\n\nFootnote *1*: We should really find a much better name for this than \"evil\nmerge\". There is nothing evil about me having to add `#include\n\"environment.h\"` in v2.45.1^, for example. It was necessary so that the\ncode would build. Tedious, yes, but not evil.\n"},{"id":"506853","messageId":"xmqq5xoyndkp.fsf@gitster.g","threadId":"62464","inReplyTo":"abc2453b-f7df-4601-8834-595881f9ceca@kdbg.org","subject":"Re: [PATCH] range-diff: optionally include merge commits' diffs in the analysis","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-08T11:56:54Z","receivedAt":"2024-11-08T11:56:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> Am 07.11.24 um 18:20 schrieb Johannes Schindelin via GitGitGadget:\n>> +--diff-merges=::\n>> +\tInstead of ignoring merge commits, generate diffs for them using the\n>> +\tcorresponding `--diff-merges=` option of linkgit:git-log[1],\n>> +\tand include them in the comparison.\n>> ++\n>> +Note: Some of the formats supported by linkgit:git-log[1] make less sense in\n>> +the context of the `range-diff` command than other formats, so choose wisely!\n>\n> Can we please be a bit more specific which options are usable or which\n> are not usable? Perhaps you even mean to say that 'first-parent' is the\n> only one that makes sense? At a minimum, we should mention that it is\n> the one that makes most sense (if that is the case).\n\nGood suggestion.\n\nI am curious to see how well things like \"--cc\" and \"--remerge-diff\"\nfare in this use case. I suspect that in a case where the original\nround forgot to make necessary semantic conflict resolutions (aka\n\"evil merge\") that the new round has, all of them (including the\n\"--first-parent\") would essentially show the same change, but what\nis shown in such a case would be more readable with \"--first-parent\"\nby treating a merge as if it were just a single parent commit making\na lot of changes, iow, merely one among other commits with equal\nfooting.\n"},{"id":"506860","messageId":"pull.1734.v2.git.1731073383564.gitgitgadget@gmail.com","threadId":"62464","inReplyTo":"pull.1734.git.1731000007391.gitgitgadget@gmail.com","subject":"[PATCH v2] range-diff: optionally include merge commits' diffs in the analysis","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-11-08T13:43:03Z","receivedAt":"2024-11-08T13:43:07Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThe `git log` command already offers support for including diffs for\nmerges, via the `--diff-merges=` option.\n\nLet's add corresponding support for `git range-diff`, too. This makes it\nmore convenient to spot differences between iterations of non-linear\ncontributions, where so-called \"evil merges\" are sometimes necessary and\nneed to be reviewed, too.\n\nIn my code reviews, I found the `--diff-merges=first-parent` option\nparticularly useful.\n\nSigned-off-by: Johannes Schindelin \n---\n Support diff merges option in range diff\n \n The git range-diff command does the same with merge commits as git\n rebase: It ignores them.\n \n However, when comparing branch thickets it can be quite illuminating to\n watch out for inadvertent changes in merge commits, in particular when\n some \"evil\" merges have been replayed, i.e. merges that needed to\n introduce changes outside of the merge conflicts (e.g. when one branch\n changed a function's signature and another branch introduced a caller of\n said function), in case the replayed merge is no longer \"evil\" and\n therefore potentially incorrect.\n \n Let's introduce support for the --diff-merges option that is passed\n through to those git log commands.\n \n I had a need for this earlier this year and got it working, leaving the\n GitGitGadget PR in a draft mode. Phil Blain found it and kindly\n nerd-sniped me into readying it for submitting, so say thanks to Phil!\n \n Changes since v1:\n \n * Changed the documentation to recommend first-parent mode instead of\n vaguely talking about various modes' merits.\n * Disallowed the no-arg --diff-merges option (because --diff-merges\n requires an argument).\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1734%2Fdscho%2Fsupport-diff-merges-option-in-range-diff-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1734/dscho/support-diff-merges-option-in-range-diff-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1734\n\nRange-diff vs v1:\n\n 1: 11361e07af8 ! 1: d01a395900b range-diff: optionally include merge commits' diffs in the analysis\n @@ Documentation/git-range-diff.txt: to revert to color all lines according to the\n +\tcorresponding `--diff-merges=` option of linkgit:git-log[1],\n +\tand include them in the comparison.\n ++\n -+Note: Some of the formats supported by linkgit:git-log[1] make less sense in\n -+the context of the `range-diff` command than other formats, so choose wisely!\n ++Note: In the common case, the `first-parent` mode will be the most natural one\n ++to use, as it is consistent with the idea that a merge is kind of a \"meta\n ++patch\", comprising all the merged commits' patches into a single one.\n +\n --[no-]notes[=]::\n \tThis flag is passed to the `git log` program\n @@ builtin/range-diff.c: int cmd_range_diff(int argc,\n \t\t\t\t N_(\"notes\"), N_(\"passed to 'git log'\"),\n \t\t\t\t PARSE_OPT_OPTARG),\n +\t\tOPT_PASSTHRU_ARGV(0, \"diff-merges\", &diff_merges_arg,\n -+\t\t\t\t N_(\"style\"), N_(\"passed to 'git log'\"),\n -+\t\t\t\t PARSE_OPT_OPTARG),\n ++\t\t\t\t N_(\"style\"), N_(\"passed to 'git log'\"), 0),\n \t\tOPT_BOOL(0, \"left-only\", &left_only,\n \t\t\t N_(\"only emit output related to the first range\")),\n \t\tOPT_BOOL(0, \"right-only\", &right_only,\n\n\n Documentation/git-range-diff.txt | 11 ++++++++++-\n builtin/range-diff.c | 10 ++++++++++\n range-diff.c | 15 +++++++++++----\n range-diff.h | 1 +\n t/t3206-range-diff.sh | 16 ++++++++++++++++\n 5 files changed, 48 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/git-range-diff.txt b/Documentation/git-range-diff.txt\nindex fbdbe0befeb..17a85957877 100644\n--- a/Documentation/git-range-diff.txt\n+++ b/Documentation/git-range-diff.txt\n@@ -10,7 +10,7 @@ SYNOPSIS\n [verse]\n 'git range-diff' [--color=[]] [--no-color] []\n \t[--no-dual-color] [--creation-factor=]\n-\t[--left-only | --right-only]\n+\t[--left-only | --right-only] [--diff-merges=]\n \t( | ... | )\n \t[[--] ...]\n \n@@ -81,6 +81,15 @@ to revert to color all lines according to the outer diff markers\n \tSuppress commits that are missing from the second specified range\n \t(or the \"right range\" when using the `...` format).\n \n+--diff-merges=::\n+\tInstead of ignoring merge commits, generate diffs for them using the\n+\tcorresponding `--diff-merges=` option of linkgit:git-log[1],\n+\tand include them in the comparison.\n++\n+Note: In the common case, the `first-parent` mode will be the most natural one\n+to use, as it is consistent with the idea that a merge is kind of a \"meta\n+patch\", comprising all the merged commits' patches into a single one.\n+\n --[no-]notes[=]::\n \tThis flag is passed to the `git log` program\n \t(see linkgit:git-log[1]) that generates the patches.\ndiff --git a/builtin/range-diff.c b/builtin/range-diff.c\nindex 1b33ab66a7b..901de5d133d 100644\n--- a/builtin/range-diff.c\n+++ b/builtin/range-diff.c\n@@ -21,6 +21,7 @@ int cmd_range_diff(int argc,\n {\n \tstruct diff_options diffopt = { NULL };\n \tstruct strvec other_arg = STRVEC_INIT;\n+\tstruct strvec diff_merges_arg = STRVEC_INIT;\n \tstruct range_diff_options range_diff_opts = {\n \t\t.creation_factor = RANGE_DIFF_CREATION_FACTOR_DEFAULT,\n \t\t.diffopt = &diffopt,\n@@ -36,6 +37,8 @@ int cmd_range_diff(int argc,\n \t\tOPT_PASSTHRU_ARGV(0, \"notes\", &other_arg,\n \t\t\t\t N_(\"notes\"), N_(\"passed to 'git log'\"),\n \t\t\t\t PARSE_OPT_OPTARG),\n+\t\tOPT_PASSTHRU_ARGV(0, \"diff-merges\", &diff_merges_arg,\n+\t\t\t\t N_(\"style\"), N_(\"passed to 'git log'\"), 0),\n \t\tOPT_BOOL(0, \"left-only\", &left_only,\n \t\t\t N_(\"only emit output related to the first range\")),\n \t\tOPT_BOOL(0, \"right-only\", &right_only,\n@@ -62,6 +65,12 @@ int cmd_range_diff(int argc,\n \tif (!simple_color)\n \t\tdiffopt.use_color = 1;\n \n+\t/* If `--diff-merges` was specified, imply `--merges` */\n+\tif (diff_merges_arg.nr) {\n+\t\trange_diff_opts.include_merges = 1;\n+\t\tstrvec_pushv(&other_arg, diff_merges_arg.v);\n+\t}\n+\n \tfor (i = 0; i < argc; i++)\n \t\tif (!strcmp(argv[i], \"--\")) {\n \t\t\tdash_dash = i;\n@@ -155,6 +164,7 @@ int cmd_range_diff(int argc,\n \tres = show_range_diff(range1.buf, range2.buf, &range_diff_opts);\n \n \tstrvec_clear(&other_arg);\n+\tstrvec_clear(&diff_merges_arg);\n \tstrbuf_release(&range1);\n \tstrbuf_release(&range2);\n \ndiff --git a/range-diff.c b/range-diff.c\nindex bbb0952264b..9e59733059b 100644\n--- a/range-diff.c\n+++ b/range-diff.c\n@@ -38,7 +38,8 @@ struct patch_util {\n * as struct object_id (will need to be free()d).\n */\n static int read_patches(const char *range, struct string_list *list,\n-\t\t\tconst struct strvec *other_arg)\n+\t\t\tconst struct strvec *other_arg,\n+\t\t\tunsigned int include_merges)\n {\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n \tstruct strbuf buf = STRBUF_INIT, contents = STRBUF_INIT;\n@@ -49,7 +50,7 @@ static int read_patches(const char *range, struct string_list *list,\n \tsize_t size;\n \tint ret = -1;\n \n-\tstrvec_pushl(&cp.args, \"log\", \"--no-color\", \"-p\", \"--no-merges\",\n+\tstrvec_pushl(&cp.args, \"log\", \"--no-color\", \"-p\",\n \t\t \"--reverse\", \"--date-order\", \"--decorate=no\",\n \t\t \"--no-prefix\", \"--submodule=short\",\n \t\t /*\n@@ -64,6 +65,8 @@ static int read_patches(const char *range, struct string_list *list,\n \t\t \"--pretty=medium\",\n \t\t \"--show-notes-by-default\",\n \t\t NULL);\n+\tif (!include_merges)\n+\t\tstrvec_push(&cp.args, \"--no-merges\");\n \tstrvec_push(&cp.args, range);\n \tif (other_arg)\n \t\tstrvec_pushv(&cp.args, other_arg->v);\n@@ -96,11 +99,14 @@ static int read_patches(const char *range, struct string_list *list,\n \t\t}\n \n \t\tif (skip_prefix(line, \"commit \", &p)) {\n+\t\t\tchar *q;\n \t\t\tif (util) {\n \t\t\t\tstring_list_append(list, buf.buf)->util = util;\n \t\t\t\tstrbuf_reset(&buf);\n \t\t\t}\n \t\t\tCALLOC_ARRAY(util, 1);\n+\t\t\tif (include_merges && (q = strstr(p, \" (from \")))\n+\t\t\t\t*q = '\\0';\n \t\t\tif (repo_get_oid(the_repository, p, &util->oid)) {\n \t\t\t\terror(_(\"could not parse commit '%s'\"), p);\n \t\t\t\tFREE_AND_NULL(util);\n@@ -571,13 +577,14 @@ int show_range_diff(const char *range1, const char *range2,\n \n \tstruct string_list branch1 = STRING_LIST_INIT_DUP;\n \tstruct string_list branch2 = STRING_LIST_INIT_DUP;\n+\tunsigned int include_merges = range_diff_opts->include_merges;\n \n \tif (range_diff_opts->left_only && range_diff_opts->right_only)\n \t\tres = error(_(\"options '%s' and '%s' cannot be used together\"), \"--left-only\", \"--right-only\");\n \n-\tif (!res && read_patches(range1, &branch1, range_diff_opts->other_arg))\n+\tif (!res && read_patches(range1, &branch1, range_diff_opts->other_arg, include_merges))\n \t\tres = error(_(\"could not parse log for '%s'\"), range1);\n-\tif (!res && read_patches(range2, &branch2, range_diff_opts->other_arg))\n+\tif (!res && read_patches(range2, &branch2, range_diff_opts->other_arg, include_merges))\n \t\tres = error(_(\"could not parse log for '%s'\"), range2);\n \n \tif (!res) {\ndiff --git a/range-diff.h b/range-diff.h\nindex 2f69f6a434d..cd85000b5a0 100644\n--- a/range-diff.h\n+++ b/range-diff.h\n@@ -16,6 +16,7 @@ struct range_diff_options {\n \tint creation_factor;\n \tunsigned dual_color:1;\n \tunsigned left_only:1, right_only:1;\n+\tunsigned include_merges:1;\n \tconst struct diff_options *diffopt; /* may be NULL */\n \tconst struct strvec *other_arg; /* may be NULL */\n };\ndiff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh\nindex 86010931ab6..c18a3fdab83 100755\n--- a/t/t3206-range-diff.sh\n+++ b/t/t3206-range-diff.sh\n@@ -909,4 +909,20 @@ test_expect_success 'submodule changes are shown irrespective of diff.submodule'\n \ttest_cmp expect actual\n '\n \n+test_expect_success '--diff-merges' '\n+\trenamed_oid=$(git rev-parse --short renamed-file) &&\n+\ttree=$(git merge-tree unmodified renamed-file) &&\n+\tclean=$(git commit-tree -m merge -p unmodified -p renamed-file $tree) &&\n+\tclean_oid=$(git rev-parse --short $clean) &&\n+\tconflict=$(git commit-tree -m merge -p unmodified -p renamed-file^ $tree) &&\n+\tconflict_oid=$(git rev-parse --short $conflict) &&\n+\n+\tgit range-diff --diff-merges=1 $clean...$conflict >actual &&\n+\tcat >expect <<-EOF &&\n+\t1: $renamed_oid < -: ------- s/12/B/\n+\t2: $clean_oid = 1: $conflict_oid merge\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n test_done\n\nbase-commit: 777489f9e09c8d0dd6b12f9d90de6376330577a2\n-- \ngitgitgadget\n"},{"id":"506865","messageId":"CABPp-BHNVEvYx4KwmiSQhPqidhPffb5hGBt=uT5NKqBPXC4j0w@mail.gmail.com","threadId":"62464","inReplyTo":"pull.1734.git.1731000007391.gitgitgadget@gmail.com","subject":"Re: [PATCH] range-diff: optionally include merge commits' diffs in the analysis","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2024-11-08T15:33:49Z","receivedAt":"2024-11-08T15:34:02Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Nov 7, 2024 at 9:20 AM Johannes Schindelin via GitGitGadget\n wrote:\n>\n> From: Johannes Schindelin \n>\n> The `git log` command already offers support for including diffs for\n> merges, via the `--diff-merges=` option.\n>\n> Let's add corresponding support for `git range-diff`, too. This makes it\n> more convenient to spot differences between iterations of non-linear\n> contributions, where so-called \"evil merges\" are sometimes necessary and\n> need to be reviewed, too.\n>\n> In my code reviews, I found the `--diff-merges=first-parent` option\n> particularly useful.\n\nCurious. Wouldn't --diff-merges=remerge-diff be more useful if you\nare particularly interested in so-called \"evil merges\" and whether\nthey remain \"evil\" (i.e. empty remerge-diff) or gain additional bits\nof \"evilness\" (i.e. more changes shown in the remerge-diff)?\n\nfirst-parent would seem more like a workaround in such a case. Let me\nexplain; first, let me refer to the result that you'd get after\nmerging with no human changes (i.e. a non-evil merge) as a\nhypothetical \"auto-merge\" commit. Now, --diff-merges=first-parent\ncould generally be broken down as the combination of diff from first\nparent to auto-merge + diff from auto-merge to evil-merge (even if the\nauto-merge wasn't actually recorded anywhere and is just a theoretical\nconstruct). Now, you aren't looking at a first-parent diff directly,\nyou are diffing two first-parent diffs. In particular, you are\ncomparing:\n pre-rebase first-parent diff = diff from first parent of merge to\nthe auto-merge + diff from auto-merge to evil-merge\nto\n post-rebase first-parent diff = diff from first parent of merge to\nthe auto-merge + diff from auto-merge to evil-merge\n\nAssuming you didn't drop or insert or modify any commits as part of\nthe rebase, then the two \"diff from first parent of merge to the\nauto-merge\" should match. Since they match, taking the difference of\nthese two causes that part to cancel out, meaning you are left just\nlooking at the differences in the \"evilness\" of the actual merge. But\nif you did make other changes while rebasing, maybe dropping or\ntweaking a commit, then suddenly you aren't just looking at\ndifferences in the \"evilness\" of the actual merge anymore; it's mixed\nwith those other changes making it more challenging to review and easy\nto miss the parts you are looking for. If you want to look for\ndifferences in whether the merge commit in question has changes other\nthan those that a simple \"git merge\" would make, remerge-diff seems\nlike a better choice.\n\n> Signed-off-by: Johannes Schindelin \n> ---\n> Support diff merges option in range diff\n>\n> The git range-diff command does the same with merge commits as git\n> rebase: It ignores them.\n>\n> However, when comparing branch thickets it can be quite illuminating to\n> watch out for inadvertent changes in merge commits, in particular when\n> some \"evil\" merges have been replayed, i.e. merges that needed to\n> introduce changes outside of the merge conflicts (e.g. when one branch\n> changed a function's signature and another branch introduced a caller of\n> said function), in case the replayed merge is no longer \"evil\" and\n> therefore potentially incorrect.\n>\n> Let's introduce support for the --diff-merges option that is passed\n> through to those git log commands.\n>\n> I had a need for this earlier this year and got it working, leaving the\n> GitGitGadget PR in a draft mode. Phil Blain found it and kindly\n> nerd-sniped me into readying it for submitting, so say thanks to Phil!\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1734%2Fdscho%2Fsupport-diff-merges-option-in-range-diff-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1734/dscho/support-diff-merges-option-in-range-diff-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1734\n>\n> Documentation/git-range-diff.txt | 10 +++++++++-\n> builtin/range-diff.c | 11 +++++++++++\n> range-diff.c | 15 +++++++++++----\n> range-diff.h | 1 +\n> t/t3206-range-diff.sh | 16 ++++++++++++++++\n> 5 files changed, 48 insertions(+), 5 deletions(-)\n>\n> diff --git a/Documentation/git-range-diff.txt b/Documentation/git-range-diff.txt\n> index fbdbe0befeb..a964e856c3c 100644\n> --- a/Documentation/git-range-diff.txt\n> +++ b/Documentation/git-range-diff.txt\n> @@ -10,7 +10,7 @@ SYNOPSIS\n> [verse]\n> 'git range-diff' [--color=[]] [--no-color] []\n> [--no-dual-color] [--creation-factor=]\n> - [--left-only | --right-only]\n> + [--left-only | --right-only] [--diff-merges=]\n> ( | ... | )\n> [[--] ...]\n>\n> @@ -81,6 +81,14 @@ to revert to color all lines according to the outer diff markers\n> Suppress commits that are missing from the second specified range\n> (or the \"right range\" when using the `...` format).\n>\n> +--diff-merges=::\n> + Instead of ignoring merge commits, generate diffs for them using the\n> + corresponding `--diff-merges=` option of linkgit:git-log[1],\n> + and include them in the comparison.\n> ++\n> +Note: Some of the formats supported by linkgit:git-log[1] make less sense in\n> +the context of the `range-diff` command than other formats, so choose wisely!\n> +\n\nIndeed. :-)\n\n> --[no-]notes[=]::\n> This flag is passed to the `git log` program\n> (see linkgit:git-log[1]) that generates the patches.\n> diff --git a/builtin/range-diff.c b/builtin/range-diff.c\n> index 1b33ab66a7b..e41719e0f0d 100644\n> --- a/builtin/range-diff.c\n> +++ b/builtin/range-diff.c\n> @@ -21,6 +21,7 @@ int cmd_range_diff(int argc,\n> {\n> struct diff_options diffopt = { NULL };\n> struct strvec other_arg = STRVEC_INIT;\n> + struct strvec diff_merges_arg = STRVEC_INIT;\n> struct range_diff_options range_diff_opts = {\n> .creation_factor = RANGE_DIFF_CREATION_FACTOR_DEFAULT,\n> .diffopt = &diffopt,\n> @@ -36,6 +37,9 @@ int cmd_range_diff(int argc,\n> OPT_PASSTHRU_ARGV(0, \"notes\", &other_arg,\n> N_(\"notes\"), N_(\"passed to 'git log'\"),\n> PARSE_OPT_OPTARG),\n> + OPT_PASSTHRU_ARGV(0, \"diff-merges\", &diff_merges_arg,\n> + N_(\"style\"), N_(\"passed to 'git log'\"),\n> + PARSE_OPT_OPTARG),\n> OPT_BOOL(0, \"left-only\", &left_only,\n> N_(\"only emit output related to the first range\")),\n> OPT_BOOL(0, \"right-only\", &right_only,\n> @@ -62,6 +66,12 @@ int cmd_range_diff(int argc,\n> if (!simple_color)\n> diffopt.use_color = 1;\n>\n> + /* If `--diff-merges` was specified, imply `--merges` */\n> + if (diff_merges_arg.nr) {\n> + range_diff_opts.include_merges = 1;\n> + strvec_pushv(&other_arg, diff_merges_arg.v);\n> + }\n> +\n> for (i = 0; i < argc; i++)\n> if (!strcmp(argv[i], \"--\")) {\n> dash_dash = i;\n> @@ -155,6 +165,7 @@ int cmd_range_diff(int argc,\n> res = show_range_diff(range1.buf, range2.buf, &range_diff_opts);\n>\n> strvec_clear(&other_arg);\n> + strvec_clear(&diff_merges_arg);\n> strbuf_release(&range1);\n> strbuf_release(&range2);\n>\n> diff --git a/range-diff.c b/range-diff.c\n> index bbb0952264b..9e59733059b 100644\n> --- a/range-diff.c\n> +++ b/range-diff.c\n> @@ -38,7 +38,8 @@ struct patch_util {\n> * as struct object_id (will need to be free()d).\n> */\n> static int read_patches(const char *range, struct string_list *list,\n> - const struct strvec *other_arg)\n> + const struct strvec *other_arg,\n> + unsigned int include_merges)\n> {\n> struct child_process cp = CHILD_PROCESS_INIT;\n> struct strbuf buf = STRBUF_INIT, contents = STRBUF_INIT;\n> @@ -49,7 +50,7 @@ static int read_patches(const char *range, struct string_list *list,\n> size_t size;\n> int ret = -1;\n>\n> - strvec_pushl(&cp.args, \"log\", \"--no-color\", \"-p\", \"--no-merges\",\n> + strvec_pushl(&cp.args, \"log\", \"--no-color\", \"-p\",\n> \"--reverse\", \"--date-order\", \"--decorate=no\",\n> \"--no-prefix\", \"--submodule=short\",\n> /*\n> @@ -64,6 +65,8 @@ static int read_patches(const char *range, struct string_list *list,\n> \"--pretty=medium\",\n> \"--show-notes-by-default\",\n> NULL);\n\n\n>\n> -- Hannes\n>\n>\n> + if (!include_merges)\n> + strvec_push(&cp.args, \"--no-merges\");\n> strvec_push(&cp.args, range);\n> if (other_arg)\n> strvec_pushv(&cp.args, other_arg->v);\n> @@ -96,11 +99,14 @@ static int read_patches(const char *range, struct string_list *list,\n> }\n>\n> if (skip_prefix(line, \"commit \", &p)) {\n> + char *q;\n> if (util) {\n> string_list_append(list, buf.buf)->util = util;\n> strbuf_reset(&buf);\n> }\n> CALLOC_ARRAY(util, 1);\n> + if (include_merges && (q = strstr(p, \" (from \")))\n> + *q = '\\0';\n> if (repo_get_oid(the_repository, p, &util->oid)) {\n> error(_(\"could not parse commit '%s'\"), p);\n> FREE_AND_NULL(util);\n> @@ -571,13 +577,14 @@ int show_range_diff(const char *range1, const char *range2,\n>\n> struct string_list branch1 = STRING_LIST_INIT_DUP;\n> struct string_list branch2 = STRING_LIST_INIT_DUP;\n> + unsigned int include_merges = range_diff_opts->include_merges;\n>\n> if (range_diff_opts->left_only && range_diff_opts->right_only)\n> res = error(_(\"options '%s' and '%s' cannot be used together\"), \"--left-only\", \"--right-only\");\n>\n> - if (!res && read_patches(range1, &branch1, range_diff_opts->other_arg))\n> + if (!res && read_patches(range1, &branch1, range_diff_opts->other_arg, include_merges))\n> res = error(_(\"could not parse log for '%s'\"), range1);\n> - if (!res && read_patches(range2, &branch2, range_diff_opts->other_arg))\n> + if (!res && read_patches(range2, &branch2, range_diff_opts->other_arg, include_merges))\n> res = error(_(\"could not parse log for '%s'\"), range2);\n>\n> if (!res) {\n> diff --git a/range-diff.h b/range-diff.h\n> index 2f69f6a434d..cd85000b5a0 100644\n> --- a/range-diff.h\n> +++ b/range-diff.h\n> @@ -16,6 +16,7 @@ struct range_diff_options {\n> int creation_factor;\n> unsigned dual_color:1;\n> unsigned left_only:1, right_only:1;\n> + unsigned include_merges:1;\n> const struct diff_options *diffopt; /* may be NULL */\n> const struct strvec *other_arg; /* may be NULL */\n> };\n> diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh\n> index 86010931ab6..c18a3fdab83 100755\n> --- a/t/t3206-range-diff.sh\n> +++ b/t/t3206-range-diff.sh\n> @@ -909,4 +909,20 @@ test_expect_success 'submodule changes are shown irrespective of diff.submodule'\n> test_cmp expect actual\n> '\n>\n> +test_expect_success '--diff-merges' '\n> + renamed_oid=$(git rev-parse --short renamed-file) &&\n> + tree=$(git merge-tree unmodified renamed-file) &&\n> + clean=$(git commit-tree -m merge -p unmodified -p renamed-file $tree) &&\n> + clean_oid=$(git rev-parse --short $clean) &&\n> + conflict=$(git commit-tree -m merge -p unmodified -p renamed-file^ $tree) &&\n> + conflict_oid=$(git rev-parse --short $conflict) &&\n> +\n> + git range-diff --diff-merges=1 $clean...$conflict >actual &&\n> + cat >expect <<-EOF &&\n> + 1: $renamed_oid < -: ------- s/12/B/\n> + 2: $clean_oid = 1: $conflict_oid merge\n> + EOF\n> + test_cmp expect actual\n> +'\n> +\n> test_done\n>\n> base-commit: 777489f9e09c8d0dd6b12f9d90de6376330577a2\n> --\n> gitgitgadget\n\nDidn't spot any problems with the patch itself.\n"},{"id":"506867","messageId":"CABPp-BFCe6wSQDGJ-Bo4+u1OE9Py2mYroX9qsvxFXJ_kePmTfg@mail.gmail.com","threadId":"62464","inReplyTo":"abc2453b-f7df-4601-8834-595881f9ceca@kdbg.org","subject":"Re: [PATCH] range-diff: optionally include merge commits' diffs in the analysis","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2024-11-08T15:53:25Z","receivedAt":"2024-11-08T15:53:37Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Nov 7, 2024 at 10:54 PM Johannes Sixt wrote:\n>\n> Am 07.11.24 um 18:20 schrieb Johannes Schindelin via GitGitGadget:\n> > +--diff-merges=::\n> > + Instead of ignoring merge commits, generate diffs for them using the\n> > + corresponding `--diff-merges=` option of linkgit:git-log[1],\n> > + and include them in the comparison.\n> > ++\n> > +Note: Some of the formats supported by linkgit:git-log[1] make less sense in\n> > +the context of the `range-diff` command than other formats, so choose wisely!\n>\n> Can we please be a bit more specific which options are usable or which\n> are not usable? Perhaps you even mean to say that 'first-parent' is the\n> only one that makes sense? At a minimum, we should mention that it is\n> the one that makes most sense (if that is the case).\n\nHere's my guesses I mostly typed up last night before getting tired\nand going to bed:\n\nseparate: makes no sense; showing two diffs for each merge and then\ntrying to diff the two sets of two diffs seems crazy, especially since\nthe whole point of rebasing is usually to change the first parent\nsomewhere in the ancestry; that'll cascade up and cause the diffs to\nthe second parents to differ wildly and make this format uselessly\nnoisy in general.\n\ncombined: much better than separate, but probably still quite noisy and\nconfusing, especially since combined-diff and range-diff are both\ncomplicated in that they have two diffs each, and combining the two\nmeans you have a diff of a dual diff. I suspect that might be hard for users to\nprocess.\n\ndense-combined: much better than combined, you at least have the diff\ndown to the smallest relevant pieces. You may still deal with the\ndiff of a dual diff problem.\n\nfirst-parent: useful, but only when keeping the series of patches largely\nintact and transplanting them; any insertion, deletion, or\nmodification of intermediate patches will result in those\nmodifications being shown\nnot just for the patch in question but rolled up into the merge commit\nand shown there as well, making the diff for the merge very noisy\n(possibly to the point of uselessness). When\nkeeping the series of patches intact and transplanting them, though,\nlikely a useful mode.\n\nremerge-diff: generally useful, even if you insert, delete, or modify\nintermediate patches while rebasing. Since it also shows how conflict\nresolutions are done, you'd see how conflicts are resolved\ndifferently. Actually, that might be one drawback to this method;\nsince conflict markers try to provide labels involving a short commit\nID and the commit message, and those short commit IDs will differ,\nthis mode would just end up showing how the two merges had different\nparents, which you already know. We could probably make this option\nmuch more useful by surpressing the conflict labels, or at least the\nshort commit ID parts of those labels. If we do that, and take\nJohannes' nice fix he sent to the list separately, then I suspect this\nmode is generally the most useful one to use with range-diff.\n"},{"id":"506875","messageId":"CABPp-BGeSwDHNeDdEwf+mo5q33GO3f0UiFpVOpjiLF3msEvrfg@mail.gmail.com","threadId":"62464","inReplyTo":"pull.1734.v2.git.1731073383564.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] range-diff: optionally include merge commits' diffs in the analysis","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2024-11-08T17:04:42Z","receivedAt":"2024-11-08T17:04:54Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Nov 8, 2024 at 5:43 AM Johannes Schindelin via GitGitGadget\n wrote:\n>\n> From: Johannes Schindelin \n>\n> The `git log` command already offers support for including diffs for\n> merges, via the `--diff-merges=` option.\n>\n> Let's add corresponding support for `git range-diff`, too. This makes it\n> more convenient to spot differences between iterations of non-linear\n> contributions, where so-called \"evil merges\" are sometimes necessary and\n> need to be reviewed, too.\n>\n> In my code reviews, I found the `--diff-merges=first-parent` option\n> particularly useful.\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> Support diff merges option in range diff\n>\n> The git range-diff command does the same with merge commits as git\n> rebase: It ignores them.\n>\n> However, when comparing branch thickets it can be quite illuminating to\n> watch out for inadvertent changes in merge commits, in particular when\n> some \"evil\" merges have been replayed, i.e. merges that needed to\n> introduce changes outside of the merge conflicts (e.g. when one branch\n> changed a function's signature and another branch introduced a caller of\n> said function), in case the replayed merge is no longer \"evil\" and\n> therefore potentially incorrect.\n>\n> Let's introduce support for the --diff-merges option that is passed\n> through to those git log commands.\n>\n> I had a need for this earlier this year and got it working, leaving the\n> GitGitGadget PR in a draft mode. Phil Blain found it and kindly\n> nerd-sniped me into readying it for submitting, so say thanks to Phil!\n>\n> Changes since v1:\n>\n> * Changed the documentation to recommend first-parent mode instead of\n> vaguely talking about various modes' merits.\n> * Disallowed the no-arg --diff-merges option (because --diff-merges\n> requires an argument).\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1734%2Fdscho%2Fsupport-diff-merges-option-in-range-diff-v2\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1734/dscho/support-diff-merges-option-in-range-diff-v2\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1734\n>\n> Range-diff vs v1:\n>\n> 1: 11361e07af8 ! 1: d01a395900b range-diff: optionally include merge commits' diffs in the analysis\n> @@ Documentation/git-range-diff.txt: to revert to color all lines according to the\n> + corresponding `--diff-merges=` option of linkgit:git-log[1],\n> + and include them in the comparison.\n> ++\n> -+Note: Some of the formats supported by linkgit:git-log[1] make less sense in\n> -+the context of the `range-diff` command than other formats, so choose wisely!\n> ++Note: In the common case, the `first-parent` mode will be the most natural one\n> ++to use, as it is consistent with the idea that a merge is kind of a \"meta\n> ++patch\", comprising all the merged commits' patches into a single one.\n> +\n> --[no-]notes[=]::\n> This flag is passed to the `git log` program\n> @@ builtin/range-diff.c: int cmd_range_diff(int argc,\n> N_(\"notes\"), N_(\"passed to 'git log'\"),\n> PARSE_OPT_OPTARG),\n> + OPT_PASSTHRU_ARGV(0, \"diff-merges\", &diff_merges_arg,\n> -+ N_(\"style\"), N_(\"passed to 'git log'\"),\n> -+ PARSE_OPT_OPTARG),\n> ++ N_(\"style\"), N_(\"passed to 'git log'\"), 0),\n> OPT_BOOL(0, \"left-only\", &left_only,\n> N_(\"only emit output related to the first range\")),\n> OPT_BOOL(0, \"right-only\", &right_only,\n>\n>\n> Documentation/git-range-diff.txt | 11 ++++++++++-\n> builtin/range-diff.c | 10 ++++++++++\n> range-diff.c | 15 +++++++++++----\n> range-diff.h | 1 +\n> t/t3206-range-diff.sh | 16 ++++++++++++++++\n> 5 files changed, 48 insertions(+), 5 deletions(-)\n>\n> diff --git a/Documentation/git-range-diff.txt b/Documentation/git-range-diff.txt\n> index fbdbe0befeb..17a85957877 100644\n> --- a/Documentation/git-range-diff.txt\n> +++ b/Documentation/git-range-diff.txt\n> @@ -10,7 +10,7 @@ SYNOPSIS\n> [verse]\n> 'git range-diff' [--color=[]] [--no-color] []\n> [--no-dual-color] [--creation-factor=]\n> - [--left-only | --right-only]\n> + [--left-only | --right-only] [--diff-merges=]\n> ( | ... | )\n> [[--] ...]\n>\n> @@ -81,6 +81,15 @@ to revert to color all lines according to the outer diff markers\n> Suppress commits that are missing from the second specified range\n> (or the \"right range\" when using the `...` format).\n>\n> +--diff-merges=::\n> + Instead of ignoring merge commits, generate diffs for them using the\n> + corresponding `--diff-merges=` option of linkgit:git-log[1],\n> + and include them in the comparison.\n> ++\n> +Note: In the common case, the `first-parent` mode will be the most natural one\n> +to use,\n\nI think we need more wording around \"common case\"; I believe this\n\"common case\" is when you are diffing against a merely transplanted\nseries of commits (a series created by rebasing without inserting,\nremoving, or minimally modifying those commits in the process) that\n`first-parent` makes sense. And it only makes sense in that case.\n\nI think `remerge-diff` generally makes sense here, both in the cases\nwhen `first-parent` makes sense and when `first-parent` does not. It\ncould be improved by suppressing the inclusion of short commit ids\n(and maybe also commit messages) in the labels of conflict markers. I\nsuspect that issue might make `remerge-diff` less useful than\n`first-parent` in simple common cases currently, but I think it's the\nright thing to build upon for what you are trying to view.\n\nIf `remerge-diff` didn't exist, I think I'd always use\n`dense-combined` over `first-parent` because of this\nmerely-transplanted limitation. I suspect dense-combined would\nprobably be kind of ugly and hard to parse when there is an actual\nevil merge, but it'd at least limit what it shows to the evil merge\ncontent.\n\n> as it is consistent with the idea that a merge is kind of a \"meta\n> +patch\", comprising all the merged commits' patches into a single one.\n\nDoesn't this wording of yours naturally lead to the idea that\n`first-parent` is a bad choice if patches leading to the merge have\nbeen inserted, removed, or more than minimally modified? It points\nout that first-parent is a diff over the whole range, and so\nrange-diff shows you a diff of the diffs over the whole range. If you\nwant to look at the \"evilness\" of merge commits, i.e. the\nuser-generated portion of the diffs included in by merge commits, you\nneed either remerge-diff or dense-combined.\n\n> +\n> --[no-]notes[=]::\n> This flag is passed to the `git log` program\n> (see linkgit:git-log[1]) that generates the patches.\n> diff --git a/builtin/range-diff.c b/builtin/range-diff.c\n> index 1b33ab66a7b..901de5d133d 100644\n> --- a/builtin/range-diff.c\n> +++ b/builtin/range-diff.c\n> @@ -21,6 +21,7 @@ int cmd_range_diff(int argc,\n> {\n> struct diff_options diffopt = { NULL };\n> struct strvec other_arg = STRVEC_INIT;\n> + struct strvec diff_merges_arg = STRVEC_INIT;\n> struct range_diff_options range_diff_opts = {\n> .creation_factor = RANGE_DIFF_CREATION_FACTOR_DEFAULT,\n> .diffopt = &diffopt,\n> @@ -36,6 +37,8 @@ int cmd_range_diff(int argc,\n> OPT_PASSTHRU_ARGV(0, \"notes\", &other_arg,\n> N_(\"notes\"), N_(\"passed to 'git log'\"),\n> PARSE_OPT_OPTARG),\n> + OPT_PASSTHRU_ARGV(0, \"diff-merges\", &diff_merges_arg,\n> + N_(\"style\"), N_(\"passed to 'git log'\"), 0),\n> OPT_BOOL(0, \"left-only\", &left_only,\n> N_(\"only emit output related to the first range\")),\n> OPT_BOOL(0, \"right-only\", &right_only,\n> @@ -62,6 +65,12 @@ int cmd_range_diff(int argc,\n> if (!simple_color)\n> diffopt.use_color = 1;\n>\n> + /* If `--diff-merges` was specified, imply `--merges` */\n> + if (diff_merges_arg.nr) {\n> + range_diff_opts.include_merges = 1;\n> + strvec_pushv(&other_arg, diff_merges_arg.v);\n> + }\n> +\n> for (i = 0; i < argc; i++)\n> if (!strcmp(argv[i], \"--\")) {\n> dash_dash = i;\n> @@ -155,6 +164,7 @@ int cmd_range_diff(int argc,\n> res = show_range_diff(range1.buf, range2.buf, &range_diff_opts);\n>\n> strvec_clear(&other_arg);\n> + strvec_clear(&diff_merges_arg);\n> strbuf_release(&range1);\n> strbuf_release(&range2);\n>\n> diff --git a/range-diff.c b/range-diff.c\n> index bbb0952264b..9e59733059b 100644\n> --- a/range-diff.c\n> +++ b/range-diff.c\n> @@ -38,7 +38,8 @@ struct patch_util {\n> * as struct object_id (will need to be free()d).\n> */\n> static int read_patches(const char *range, struct string_list *list,\n> - const struct strvec *other_arg)\n> + const struct strvec *other_arg,\n> + unsigned int include_merges)\n> {\n> struct child_process cp = CHILD_PROCESS_INIT;\n> struct strbuf buf = STRBUF_INIT, contents = STRBUF_INIT;\n> @@ -49,7 +50,7 @@ static int read_patches(const char *range, struct string_list *list,\n> size_t size;\n> int ret = -1;\n>\n> - strvec_pushl(&cp.args, \"log\", \"--no-color\", \"-p\", \"--no-merges\",\n> + strvec_pushl(&cp.args, \"log\", \"--no-color\", \"-p\",\n> \"--reverse\", \"--date-order\", \"--decorate=no\",\n> \"--no-prefix\", \"--submodule=short\",\n> /*\n> @@ -64,6 +65,8 @@ static int read_patches(const char *range, struct string_list *list,\n> \"--pretty=medium\",\n> \"--show-notes-by-default\",\n> NULL);\n> + if (!include_merges)\n> + strvec_push(&cp.args, \"--no-merges\");\n> strvec_push(&cp.args, range);\n> if (other_arg)\n> strvec_pushv(&cp.args, other_arg->v);\n> @@ -96,11 +99,14 @@ static int read_patches(const char *range, struct string_list *list,\n> }\n>\n> if (skip_prefix(line, \"commit \", &p)) {\n> + char *q;\n> if (util) {\n> string_list_append(list, buf.buf)->util = util;\n> strbuf_reset(&buf);\n> }\n> CALLOC_ARRAY(util, 1);\n> + if (include_merges && (q = strstr(p, \" (from \")))\n> + *q = '\\0';\n> if (repo_get_oid(the_repository, p, &util->oid)) {\n> error(_(\"could not parse commit '%s'\"), p);\n> FREE_AND_NULL(util);\n> @@ -571,13 +577,14 @@ int show_range_diff(const char *range1, const char *range2,\n>\n> struct string_list branch1 = STRING_LIST_INIT_DUP;\n> struct string_list branch2 = STRING_LIST_INIT_DUP;\n> + unsigned int include_merges = range_diff_opts->include_merges;\n>\n> if (range_diff_opts->left_only && range_diff_opts->right_only)\n> res = error(_(\"options '%s' and '%s' cannot be used together\"), \"--left-only\", \"--right-only\");\n>\n> - if (!res && read_patches(range1, &branch1, range_diff_opts->other_arg))\n> + if (!res && read_patches(range1, &branch1, range_diff_opts->other_arg, include_merges))\n> res = error(_(\"could not parse log for '%s'\"), range1);\n> - if (!res && read_patches(range2, &branch2, range_diff_opts->other_arg))\n> + if (!res && read_patches(range2, &branch2, range_diff_opts->other_arg, include_merges))\n> res = error(_(\"could not parse log for '%s'\"), range2);\n>\n> if (!res) {\n> diff --git a/range-diff.h b/range-diff.h\n> index 2f69f6a434d..cd85000b5a0 100644\n> --- a/range-diff.h\n> +++ b/range-diff.h\n> @@ -16,6 +16,7 @@ struct range_diff_options {\n> int creation_factor;\n> unsigned dual_color:1;\n> unsigned left_only:1, right_only:1;\n> + unsigned include_merges:1;\n> const struct diff_options *diffopt; /* may be NULL */\n> const struct strvec *other_arg; /* may be NULL */\n> };\n> diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh\n> index 86010931ab6..c18a3fdab83 100755\n> --- a/t/t3206-range-diff.sh\n> +++ b/t/t3206-range-diff.sh\n> @@ -909,4 +909,20 @@ test_expect_success 'submodule changes are shown irrespective of diff.submodule'\n> test_cmp expect actual\n> '\n>\n> +test_expect_success '--diff-merges' '\n> + renamed_oid=$(git rev-parse --short renamed-file) &&\n> + tree=$(git merge-tree unmodified renamed-file) &&\n> + clean=$(git commit-tree -m merge -p unmodified -p renamed-file $tree) &&\n> + clean_oid=$(git rev-parse --short $clean) &&\n> + conflict=$(git commit-tree -m merge -p unmodified -p renamed-file^ $tree) &&\n> + conflict_oid=$(git rev-parse --short $conflict) &&\n> +\n> + git range-diff --diff-merges=1 $clean...$conflict >actual &&\n> + cat >expect <<-EOF &&\n> + 1: $renamed_oid < -: ------- s/12/B/\n> + 2: $clean_oid = 1: $conflict_oid merge\n> + EOF\n> + test_cmp expect actual\n> +'\n> +\n> test_done\n>\n> base-commit: 777489f9e09c8d0dd6b12f9d90de6376330577a2\n> --\n> gitgitgadget\n"},{"id":"506890","messageId":"CABPp-BEaZXZ2wSfrkkOAX6W9An8MTf-LEVNKvXvRTxyV9XcKcA@mail.gmail.com","threadId":"62464","inReplyTo":"af576487-5de2-fba3-b341-3c082322c9ec@gmx.de","subject":"Re: [PATCH] range-diff: optionally include merge commits' diffs in the analysis","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2024-11-09T08:49:54Z","receivedAt":"2024-11-09T08:50:06Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"One more thing...\n\nOn Fri, Nov 8, 2024 at 3:04 AM Johannes Schindelin\n wrote:\n>\n[...]\n> Footnote *1*: We should really find a much better name for this than \"evil\n> merge\". There is nothing evil about me having to add `#include\n> \"environment.h\"` in v2.45.1^, for example. It was necessary so that the\n> code would build. Tedious, yes, but not evil.\n\nIndeed, I've felt it's problematic for a while too and had to take\ntime on at least one occasion to mention to someone else that I don't\nactually mean \"evil\" it's just that \"evil merge\" as a compound term is\nthe convenient nomenclature we've been using for all of these,\nregardless of whether the user modified the merge to resolve syntactic\nconflicts, or to resolve semantic conflicts, or to sneak in \"evil\"\nchanges. That's particularly odd since the first category is the most\ncommon, and the third (snuck in unrelated changes or \"evil changes\")\nare the most rare. Maybe we should just call these \"user-modified\nmerges\" rather than \"evil merges\"? Any better suggestions?\n"},{"id":"506943","messageId":"444e4a94-0005-bed6-1092-20770c226109@gmail.com","threadId":"62464","inReplyTo":"pull.1734.v2.git.1731073383564.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] range-diff: optionally include merge commits' diffs in the analysis","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2024-11-10T20:30:54Z","receivedAt":"2024-11-10T20:30:56Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Hi Dscho,\n\nLe 2024-11-08 à 08:43, Johannes Schindelin via GitGitGadget a écrit :\n> From: Johannes Schindelin \n> \n> The `git log` command already offers support for including diffs for\n> merges, via the `--diff-merges=` option.\n> \n> Let's add corresponding support for `git range-diff`, too. This makes it\n> more convenient to spot differences between iterations of non-linear\n> contributions, where so-called \"evil merges\" are sometimes necessary and\n> need to be reviewed, too.\n\nMaybe \"between commit ranges that include merge commits\" would be more\nworkflow-agnostic ? Just thinking out loud here, I think what you wrote \nis also OK.\n\n> In my code reviews, I found the `--diff-merges=first-parent` option\n> particularly useful.\n> \n> Signed-off-by: Johannes Schindelin \n> ---\n> Support diff merges option in range diff\n> \n> The git range-diff command does the same with merge commits as git\n> rebase: It ignores them.\n> \n> However, when comparing branch thickets it can be quite illuminating to\n> watch out for inadvertent changes in merge commits, in particular when\n> some \"evil\" merges have been replayed, i.e. merges that needed to\n> introduce changes outside of the merge conflicts (e.g. when one branch\n> changed a function's signature and another branch introduced a caller of\n> said function), in case the replayed merge is no longer \"evil\" and\n> therefore potentially incorrect.\n> \n> Let's introduce support for the --diff-merges option that is passed\n> through to those git log commands.\n> \n> I had a need for this earlier this year and got it working, leaving the\n> GitGitGadget PR in a draft mode. Phil Blain found it and kindly\n> nerd-sniped me into readying it for submitting, so say thanks to Phil!\n> \n> Changes since v1:\n> \n> * Changed the documentation to recommend first-parent mode instead of\n> vaguely talking about various modes' merits.\n> * Disallowed the no-arg --diff-merges option (because --diff-merges\n> requires an argument).\n> \n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1734%2Fdscho%2Fsupport-diff-merges-option-in-range-diff-v2\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1734/dscho/support-diff-merges-option-in-range-diff-v2\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1734\n> \n> Range-diff vs v1:\n> \n> 1: 11361e07af8 ! 1: d01a395900b range-diff: optionally include merge commits' diffs in the analysis\n> @@ Documentation/git-range-diff.txt: to revert to color all lines according to the\n> +\tcorresponding `--diff-merges=` option of linkgit:git-log[1],\n> +\tand include them in the comparison.\n> ++\n> -+Note: Some of the formats supported by linkgit:git-log[1] make less sense in\n> -+the context of the `range-diff` command than other formats, so choose wisely!\n> ++Note: In the common case, the `first-parent` mode will be the most natural one\n> ++to use, as it is consistent with the idea that a merge is kind of a \"meta\n> ++patch\", comprising all the merged commits' patches into a single one.\n> +\n> --[no-]notes[=]::\n> \tThis flag is passed to the `git log` program\n> @@ builtin/range-diff.c: int cmd_range_diff(int argc,\n> \t\t\t\t N_(\"notes\"), N_(\"passed to 'git log'\"),\n> \t\t\t\t PARSE_OPT_OPTARG),\n> +\t\tOPT_PASSTHRU_ARGV(0, \"diff-merges\", &diff_merges_arg,\n> -+\t\t\t\t N_(\"style\"), N_(\"passed to 'git log'\"),\n> -+\t\t\t\t PARSE_OPT_OPTARG),\n> ++\t\t\t\t N_(\"style\"), N_(\"passed to 'git log'\"), 0),\n> \t\tOPT_BOOL(0, \"left-only\", &left_only,\n> \t\t\t N_(\"only emit output related to the first range\")),\n> \t\tOPT_BOOL(0, \"right-only\", &right_only,\n> \n> \n> Documentation/git-range-diff.txt | 11 ++++++++++-\n> builtin/range-diff.c | 10 ++++++++++\n> range-diff.c | 15 +++++++++++----\n> range-diff.h | 1 +\n> t/t3206-range-diff.sh | 16 ++++++++++++++++\n> 5 files changed, 48 insertions(+), 5 deletions(-)\n> \n> diff --git a/Documentation/git-range-diff.txt b/Documentation/git-range-diff.txt\n> index fbdbe0befeb..17a85957877 100644\n> --- a/Documentation/git-range-diff.txt\n> +++ b/Documentation/git-range-diff.txt\n> @@ -10,7 +10,7 @@ SYNOPSIS\n> [verse]\n> 'git range-diff' [--color=[]] [--no-color] []\n> \t[--no-dual-color] [--creation-factor=]\n> -\t[--left-only | --right-only]\n> +\t[--left-only | --right-only] [--diff-merges=]\n> \t( | ... | )\n> \t[[--] ...]\n> \n> @@ -81,6 +81,15 @@ to revert to color all lines according to the outer diff markers\n> \tSuppress commits that are missing from the second specified range\n> \t(or the \"right range\" when using the `...` format).\n> \n> +--diff-merges=::\n> +\tInstead of ignoring merge commits, generate diffs for them using the\n> +\tcorresponding `--diff-merges=` option of linkgit:git-log[1],\n> +\tand include them in the comparison.\n> ++\n> +Note: In the common case, the `first-parent` mode will be the most natural one\n> +to use, as it is consistent with the idea that a merge is kind of a \"meta\n> +patch\", comprising all the merged commits' patches into a single one.\n\nI think I agree with Elijah that we probably should also highlight at least\n'remerge'.\n\nAlso, is it worth making this a proper Asciidoc \"[NOTE]\" ? (I'm not sure, there are \na lot of \"Notes:\" in our doc that are not Asciidoc \"[NOTE]\"s.\n\n> diff --git a/builtin/range-diff.c b/builtin/range-diff.c\n> index 1b33ab66a7b..901de5d133d 100644\n> --- a/builtin/range-diff.c\n> +++ b/builtin/range-diff.c\n\nThe changes look good to me. Maybe it would be nice to add a corresponding\n'range-diff.diffMerges' config option to allow users to configure the \nbehaviour more permanently ?\n\n> diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh\n> index 86010931ab6..c18a3fdab83 100755\n> --- a/t/t3206-range-diff.sh\n> +++ b/t/t3206-range-diff.sh\n> @@ -909,4 +909,20 @@ test_expect_success 'submodule changes are shown irrespective of diff.submodule'\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_success '--diff-merges' '\n> +\trenamed_oid=$(git rev-parse --short renamed-file) &&\n> +\ttree=$(git merge-tree unmodified renamed-file) &&\n> +\tclean=$(git commit-tree -m merge -p unmodified -p renamed-file $tree) &&\n> +\tclean_oid=$(git rev-parse --short $clean) &&\n> +\tconflict=$(git commit-tree -m merge -p unmodified -p renamed-file^ $tree) &&\n> +\tconflict_oid=$(git rev-parse --short $conflict) &&\n> +\n> +\tgit range-diff --diff-merges=1 $clean...$conflict >actual &&\n> +\tcat >expect <<-EOF &&\n> +\t1: $renamed_oid < -: ------- s/12/B/\n> +\t2: $clean_oid = 1: $conflict_oid merge\n> +\tEOF\n> +\ttest_cmp expect actual\n> +'\n\nThe test looks good. \n\nThanks for submitting that patch!\n\nPhilippe.\n"},{"id":"506952","messageId":"xmqqzfm6k4e5.fsf@gitster.g","threadId":"62464","inReplyTo":"CABPp-BEaZXZ2wSfrkkOAX6W9An8MTf-LEVNKvXvRTxyV9XcKcA@mail.gmail.com","subject":"Re: [PATCH] range-diff: optionally include merge commits' diffs in the analysis","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-11T00:20:18Z","receivedAt":"2024-11-11T00:20:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n> That's particularly odd since the first category is the most\n> common, and the third (snuck in unrelated changes or \"evil changes\")\n> are the most rare. Maybe we should just call these \"user-modified\n> merges\" rather than \"evil merges\"? Any better suggestions?\n\nIt is \"evil\" in the sense that it makes it really tough for those\nwho dig history to find out what happened later to figure out where\nthe change came from. It could be an attempt to hide a malicious\nand unrelated change, it could be a resolution for a non-textual\nconflict, or it could be a \"while at it I am fixing an unrelated\ntypo I spotted nearby while resolving this textual conflict\".\n\nThe change does not have to have an evil intent. It is just\nsomething different from and more than what the textual and\nmechanical merge would produce.\n\n\"more than\" may give us a hint for what we want to convey in the\nname, if we were to pick a new name. If a new topic changed a\nfunction signature to add one extra parameter, and updated all the\ncallers of the function that existed back when the topic forked, an\n\"evil\" merge needs to address the semantic conflicts to do a very\nsimilar update to a new call to the function added on the mainline\nsince the topic forked, when the topic is merged back to the\nmainline. Taken as a whole, the first-parent diff of such a merge\nwill show that a function gained a parameter, and all of its callers\nhave been adjusted for that change. Most of such changes would have\ncome from the side branch, but on top of that, the merge needed to\ncontribute more of the similar changes to callers.\n\n\n"},{"id":"506953","messageId":"xmqqv7wuk3lx.fsf@gitster.g","threadId":"62464","inReplyTo":"pull.1734.v2.git.1731073383564.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] range-diff: optionally include merge commits' diffs in the analysis","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-11T00:37:14Z","receivedAt":"2024-11-11T00:37:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> * Disallowed the no-arg --diff-merges option (because --diff-merges\n> requires an argument).\n\nYup, I was happy to see it in the range-diff output, because I was\nwondering where the optarg came from in the initial submission.\n\n> +--diff-merges=::\n> +\tInstead of ignoring merge commits, generate diffs for them using the\n> +\tcorresponding `--diff-merges=` option of linkgit:git-log[1],\n> +\tand include them in the comparison.\n> ++\n> +Note: In the common case, the `first-parent` mode will be the most natural one\n> +to use, as it is consistent with the idea that a merge is kind of a \"meta\n> +patch\", comprising all the merged commits' patches into a single one.\n\nI'll let you and Elijah figure out the best phrasing around here,\nincluding whether we recommend only first-parent or give other\noptions.\n\nFor me personally, I find that use of `first-parent` here is\nconsistent with the mindset of those users who prefer to read \"git\nlog --first-parent -p\", and to a smaller degree, those who use\nsquash merges. To them, a merge is merely bringing in lots of\nchanges into the mainline as a single unit, and other than that\nthese lots of changes are broken out for finer grained inspection\nif/when they wanted to, they prefer to treat the changes brought in\nby merges just like a single-parent commit. And from that point of\nview, (1) reordering the changes inside the side branch is\nimmaterial for the purpose of talking about the result, the net\neffect the merged topic makes to the mainline, and (2) dropping or\nadding new changes to the side branch is treated just like a newer\niteration of a single parent commit having fewer or more changes.\nSo it is very natural that first-parent helps to make the world view\nvery simplistic, and more readable range-diff is all about how you\ncan present the two iterations in a simple and flattened form.\n\nThere _may_ need a tweak of the matching algorithm to allow the\n\"same\" merge on both sides to match, even if they diverge a lot,\nthough. A range-diff that pairs a merge in the previous iteration\nwith a single patch in the updated iteration may be hard to read.\n"},{"id":"507057","messageId":"CABPp-BFXZ+aaTWGAUhJxh2YYZ131BNYFCyLNVbjntBmStUu0SA@mail.gmail.com","threadId":"62464","inReplyTo":"xmqqv7wuk3lx.fsf@gitster.g","subject":"Re: [PATCH v2] range-diff: optionally include merge commits' diffs in the analysis","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2024-11-11T16:51:12Z","receivedAt":"2024-11-11T16:51:24Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Sun, Nov 10, 2024 at 4:37 PM Junio C Hamano wrote:\n>\n> \"Johannes Schindelin via GitGitGadget\" \n> writes:\n>\n[...]\n> > +--diff-merges=::\n> > + Instead of ignoring merge commits, generate diffs for them using the\n> > + corresponding `--diff-merges=` option of linkgit:git-log[1],\n> > + and include them in the comparison.\n> > ++\n> > +Note: In the common case, the `first-parent` mode will be the most natural one\n> > +to use, as it is consistent with the idea that a merge is kind of a \"meta\n> > +patch\", comprising all the merged commits' patches into a single one.\n>\n> I'll let you and Elijah figure out the best phrasing around here,\n> including whether we recommend only first-parent or give other\n> options.\n\nI'd probably suggest:\n\nNote: When trying to see how users adjusted the contents of a merge\ncommit, `remerge-diff` is a natural choice. When viewing merges as a\n\"meta patch\", comprising a squash of all the merged commits' patches\ninto a single one (similar to `git log --first-parent -p`),\n`first-parent` is a natural choice.\n\n> For me personally, I find that use of `first-parent` here is\n> consistent with the mindset of those users who prefer to read \"git\n> log --first-parent -p\", and to a smaller degree, those who use\n> squash merges. To them, a merge is merely bringing in lots of\n> changes into the mainline as a single unit, and other than that\n> these lots of changes are broken out for finer grained inspection\n> if/when they wanted to, they prefer to treat the changes brought in\n> by merges just like a single-parent commit. And from that point of\n> view, (1) reordering the changes inside the side branch is\n> immaterial for the purpose of talking about the result, the net\n> effect the merged topic makes to the mainline, and (2) dropping or\n> adding new changes to the side branch is treated just like a newer\n> iteration of a single parent commit having fewer or more changes.\n> So it is very natural that first-parent helps to make the world view\n> very simplistic, and more readable range-diff is all about how you\n> can present the two iterations in a simple and flattened form.\n>\n> There _may_ need a tweak of the matching algorithm to allow the\n> \"same\" merge on both sides to match, even if they diverge a lot,\n> though. A range-diff that pairs a merge in the previous iteration\n> with a single patch in the updated iteration may be hard to read.\n\nSounds like you are arguing that there is an angle/usecase from which\n`first-parent` makes sense, namely the viewpoint that \"a merge is\nmerely bringing in lots of changes into the mainline as a single unit\"\nas you put it. What was surprising to me, though, is that it's a\ncompletely different usecase than the one that was brought up in the\ncommit message for this feature, namely \"so-called 'evil merges' are\nsometimes necessary and need to be reviewed too\".\n\nIf you want to review \"evilness\" or the differences between the\nchanges that `git merge` would make and what got recorded in the\ncommit, then `first-parent` is not that great an approximation of what\nyou are looking for, BUT it happens to work beautifully in common\ncases where you are merely transplanting commits due to the fact that\nthe huge amounts of extras being put into the diff happen to be equal\non the two sides, so they cancel out and leave you with the \"evilness\"\nyou are interested in. But if you stray from that common case (by\ne.g. inserting, removing, or modifying commits while rebasing) and are\nstill interested in \"evilness\", your approximation quickly drops from\naccurately pulling out just the bits you are interested in towards\nbeing completely swamped by the noise.\n\nMy concern with the commit message and patch as they stand, is that I\nfeel the `first-parent` mode is being recommended as a solution for a\ncertain usecase where it is really only an approximation that luckily\nis highly accurate in special but common scenarios while being very\ninaccurate in others. I think that will lead to users getting\nnegatively surprised when they move outside those common cases.\n"},{"id":"507071","messageId":"c7c71d10-5795-6c93-6ff4-1effc534c6a0@gmx.de","threadId":"62464","inReplyTo":"CABPp-BGeSwDHNeDdEwf+mo5q33GO3f0UiFpVOpjiLF3msEvrfg@mail.gmail.com","subject":"Re: [PATCH v2] range-diff: optionally include merge commits' diffs in the analysis","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2024-11-11T19:55:56Z","receivedAt":"2024-11-11T19:56:00Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Elijah,\n\nOn Fri, 8 Nov 2024, Elijah Newren wrote:\n\n> On Fri, Nov 8, 2024 at 5:43 AM Johannes Schindelin via GitGitGadget\n> wrote:\n> >\n> > diff --git a/Documentation/git-range-diff.txt b/Documentation/git-range-diff.txt\n> > index fbdbe0befeb..17a85957877 100644\n> > --- a/Documentation/git-range-diff.txt\n> > +++ b/Documentation/git-range-diff.txt\n> > @@ -81,6 +81,15 @@ to revert to color all lines according to the outer diff markers\n> > Suppress commits that are missing from the second specified range\n> > (or the \"right range\" when using the `...` format).\n> >\n> > +--diff-merges=::\n> > + Instead of ignoring merge commits, generate diffs for them using the\n> > + corresponding `--diff-merges=` option of linkgit:git-log[1],\n> > + and include them in the comparison.\n> > ++\n> > +Note: In the common case, the `first-parent` mode will be the most natural one\n> > +to use,\n>\n> I think we need more wording around \"common case\"; I believe this\n> \"common case\" is when you are diffing against a merely transplanted\n> series of commits (a series created by rebasing without inserting,\n> removing, or minimally modifying those commits in the process) that\n> `first-parent` makes sense. And it only makes sense in that case.\n\nI tried to reproduce some of the common cases (which were more along the\nlines of stacked PRs than transplanted series of commits), and you're\nright: with the remerge bug fix, the range-diffs are much more easily\ninterpreted.\n\nFor example, I had to squash in a few fixes in\nhttps://github.com/git/git-scm.com/pull/1915, and I try to combine all of\nmy currently-open git-scm.com PRs into the `gh-pages` branch in my fork.\n\nWith `--diff-merges=1`, all of those single-commit merges that update the\nvarious translations of the book had diffs identical to the diffs of the\ncommits they merged. Which confused range-diff quite a bit, often picking\na non-merge as matching a merge (which is of course suboptimal).\n\nWith `--diff-merges=remerge`, these trivial merges had no diffs at all,\nwhich made the review much easier.\n\n> I think `remerge-diff` generally makes sense here, both in the cases\n> when `first-parent` makes sense and when `first-parent` does not. It\n> could be improved by suppressing the inclusion of short commit ids\n> (and maybe also commit messages) in the labels of conflict markers. I\n> suspect that issue might make `remerge-diff` less useful than\n> `first-parent` in simple common cases currently, but I think it's the\n> right thing to build upon for what you are trying to view.\n\nInteresting idea about the suppressed commit IDs. I'll play with that.\n\nCiao,\nJohannes\n"},{"id":"507072","messageId":"eb502feb-1563-b1b5-5345-784673c89c79@gmx.de","threadId":"62464","inReplyTo":"444e4a94-0005-bed6-1092-20770c226109@gmail.com","subject":"Re: [PATCH v2] range-diff: optionally include merge commits' diffs in the analysis","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2024-11-11T20:07:02Z","receivedAt":"2024-11-11T20:07:05Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Philippe,\n\nOn Sun, 10 Nov 2024, Philippe Blain wrote:\n\n> Le 2024-11-08 à 08:43, Johannes Schindelin via GitGitGadget a écrit :\n> > From: Johannes Schindelin \n> >\n> > The `git log` command already offers support for including diffs for\n> > merges, via the `--diff-merges=` option.\n> >\n> > Let's add corresponding support for `git range-diff`, too. This makes it\n> > more convenient to spot differences between iterations of non-linear\n> > contributions, where so-called \"evil merges\" are sometimes necessary and\n> > need to be reviewed, too.\n>\n> Maybe \"between commit ranges that include merge commits\" would be more\n> workflow-agnostic ?\n\nGood idea, this is much clearer than what I wrote, too.\n\n> > diff --git a/Documentation/git-range-diff.txt b/Documentation/git-range-diff.txt\n> > index fbdbe0befeb..17a85957877 100644\n> > --- a/Documentation/git-range-diff.txt\n> > +++ b/Documentation/git-range-diff.txt\n> > @@ -81,6 +81,15 @@ to revert to color all lines according to the outer diff markers\n> > \tSuppress commits that are missing from the second specified range\n> > \t(or the \"right range\" when using the `...` format).\n> >\n> > +--diff-merges=::\n> > +\tInstead of ignoring merge commits, generate diffs for them using the\n> > +\tcorresponding `--diff-merges=` option of linkgit:git-log[1],\n> > +\tand include them in the comparison.\n> > ++\n> > +Note: In the common case, the `first-parent` mode will be the most natural one\n> > +to use, as it is consistent with the idea that a merge is kind of a \"meta\n> > +patch\", comprising all the merged commits' patches into a single one.\n>\n> I think I agree with Elijah that we probably should also highlight at least\n> 'remerge'.\n>\n> Also, is it worth making this a proper Asciidoc \"[NOTE]\" ? (I'm not sure, there are\n> a lot of \"Notes:\" in our doc that are not Asciidoc \"[NOTE]\"s.\n\nRight, I did not want to deviate too much from the surrounding style.\n\nBesides, I am not _so_ versed in AsciiDoc, I consider Markdown to be much\nmore common, so I am much more familiar with that. I had no idea that\nthere was a proper AsciiDoc [NOTE].\n\n> > diff --git a/builtin/range-diff.c b/builtin/range-diff.c\n> > index 1b33ab66a7b..901de5d133d 100644\n> > --- a/builtin/range-diff.c\n> > +++ b/builtin/range-diff.c\n>\n> The changes look good to me. Maybe it would be nice to add a corresponding\n> 'range-diff.diffMerges' config option to allow users to configure the\n> behaviour more permanently ?\n\nSeeing as there are no existing `rangeDiff.*` options, I am loathe to\nintroduce the first one lest I am asked why I don't balloon this patch\nseries into introducing config settings for the other options, too.\n\nCiao,\nJohannes\n"},{"id":"507092","messageId":"xmqqwmh98fcg.fsf@gitster.g","threadId":"62464","inReplyTo":"CABPp-BFXZ+aaTWGAUhJxh2YYZ131BNYFCyLNVbjntBmStUu0SA@mail.gmail.com","subject":"Re: [PATCH v2] range-diff: optionally include merge commits' diffs in the analysis","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-12T00:29:03Z","receivedAt":"2024-11-12T00:29:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n>> There _may_ need a tweak of the matching algorithm to allow the\n>> \"same\" merge on both sides to match, even if they diverge a lot,\n>> though. A range-diff that pairs a merge in the previous iteration\n>> with a single patch in the updated iteration may be hard to read.\n>\n> Sounds like you are arguing that there is an angle/usecase from which\n> `first-parent` makes sense, namely the viewpoint that \"a merge is\n> merely bringing in lots of changes into the mainline as a single unit\"\n> as you put it. What was surprising to me, though, is that it's a\n> completely different usecase than the one that was brought up in the\n> commit message for this feature, namely \"so-called 'evil merges' are\n> sometimes necessary and need to be reviewed too\".\n\nWhat I had in mind when I wrote the example you are responding to is\nbased on what sometimes happens while I make repeated merges (and as\nyou may know, I make lots of them). In the first attempt I miss the\nfact that I need semantic adjustments and then in the second attempt\nI know what additional changes are necessary in the same merge (i.e.\nmerging exactly the same iteration of the same topic). If you run\nthe first-parent range-diff between one iteration of 'seen' and\nanother, the \"additional changes\" I would make in the second attempt\nwould be the only thing that will appear in the output, showing the\n\"evil merge\".\n\nThere can also be updates in the topic itself when I rebuild 'seen',\nin addition to merge-fixes to adjust for semantic conflicts. Such a\nchange would also appear in the first-parent view. If you used\nother views, like dense-combined or remerge-diff, these updates in\nthe topic itself may be hidden, as these other views are\nspecifically designed to highlight conflict resolutions and evil\nmerges by discarding mechanically resolvable changes. So it all\ndepends on what you are looking for and what you are deliberately\nexcluding from the lower level of diff generation when you prepare\nyour range-diff, which is a \"diff of diff\". Giving an impression\nthat first-parent is the most useful may risk misleading readers in\nthat sense.\n\n"},{"id":"508156","messageId":"xmqq34jezarn.fsf@gitster.g","threadId":"62464","inReplyTo":"eb502feb-1563-b1b5-5345-784673c89c79@gmx.de","subject":"Re: [PATCH v2] range-diff: optionally include merge commits' diffs in the analysis","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-26T07:58:52Z","receivedAt":"2024-11-26T07:58:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Hi Philippe,\n>\n> On Sun, 10 Nov 2024, Philippe Blain wrote:\n>\n>> Le 2024-11-08 à 08:43, Johannes Schindelin via GitGitGadget a écrit :\n>> > From: Johannes Schindelin \n>> >\n>> > The `git log` command already offers support for including diffs for\n>> > merges, via the `--diff-merges=` option.\n>> >\n>> > Let's add corresponding support for `git range-diff`, too. This makes it\n>> > more convenient to spot differences between iterations of non-linear\n>> > contributions, where so-called \"evil merges\" are sometimes necessary and\n>> > need to be reviewed, too.\n>>\n>> Maybe \"between commit ranges that include merge commits\" would be more\n>> workflow-agnostic ?\n>\n> Good idea, this is much clearer than what I wrote, too.\n\nSounds good.\n\n>> > diff --git a/builtin/range-diff.c b/builtin/range-diff.c\n>> > index 1b33ab66a7b..901de5d133d 100644\n>> > --- a/builtin/range-diff.c\n>> > +++ b/builtin/range-diff.c\n>>\n>> The changes look good to me. Maybe it would be nice to add a corresponding\n>> 'range-diff.diffMerges' config option to allow users to configure the\n>> behaviour more permanently ?\n>\n> Seeing as there are no existing `rangeDiff.*` options, I am loathe to\n> introduce the first one lest I am asked why I don't balloon this patch\n> series into introducing config settings for the other options, too.\n\nYeah, I think it can be left for a follow-on exercise, done even by\nother people who are interested.\n\nThanks.\n"},{"id":"509160","messageId":"pull.1734.v3.git.1734358282.gitgitgadget@gmail.com","threadId":"62464","inReplyTo":"pull.1734.v2.git.1731073383564.gitgitgadget@gmail.com","subject":"[PATCH v3 0/2] Support diff merges option in range diff","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-12-16T14:11:20Z","receivedAt":"2024-12-16T14:11:27Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"The git range-diff command does the same with merge commits as git rebase:\nIt ignores them.\n\nHowever, when comparing branch thickets it can be quite illuminating to\nwatch out for inadvertent changes in merge commits, in particular when some\n\"evil\" merges have been replayed, i.e. merges that needed to introduce\nchanges outside of the merge conflicts (e.g. when one branch changed a\nfunction's signature and another branch introduced a caller of said\nfunction), in case the replayed merge is no longer \"evil\" and therefore\npotentially incorrect.\n\nLet's introduce support for the --diff-merges option that is passed through\nto those git log commands.\n\nI had a need for this earlier this year and got it working, leaving the\nGitGitGadget PR in a draft mode. Phil Blain found it and kindly nerd-sniped\nme into readying it for submitting, so say thanks to Phil!\n\nChanges since v2:\n\n * Rebased onto js/log-remerge-keep-ancestry, to fix --diff-merges=remerge\n * Now the documentation suggests remerge instead of first-parent mode\n * Added a follow-up commit to introduce the convenience option git\n range-diff --remerge-diff\n\nChanges since v1:\n\n * Changed the documentation to recommend first-parent mode instead of\n vaguely talking about various modes' merits.\n * Disallowed the no-arg --diff-merges option (because --diff-merges\n requires an argument).\n\nJohannes Schindelin (2):\n range-diff: optionally include merge commits' diffs in the analysis\n range-diff: introduce the convenience option `--remerge-diff`\n\n Documentation/git-range-diff.txt | 17 ++++++++++++++++-\n builtin/range-diff.c | 12 ++++++++++++\n range-diff.c | 17 ++++++++++++-----\n range-diff.h | 1 +\n t/t3206-range-diff.sh | 16 ++++++++++++++++\n 5 files changed, 57 insertions(+), 6 deletions(-)\n\n\nbase-commit: f94bfa151623d7e675db9465ae7ff0b33e4825f3\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1734%2Fdscho%2Fsupport-diff-merges-option-in-range-diff-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1734/dscho/support-diff-merges-option-in-range-diff-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/1734\n\nRange-diff vs v2:\n\n 1: d01a395900b ! 1: ef3c243da1b range-diff: optionally include merge commits' diffs in the analysis\n @@ Commit message\n merges, via the `--diff-merges=` option.\n \n Let's add corresponding support for `git range-diff`, too. This makes it\n - more convenient to spot differences between iterations of non-linear\n - contributions, where so-called \"evil merges\" are sometimes necessary and\n - need to be reviewed, too.\n + more convenient to spot differences between commit ranges that contain\n + merges.\n \n - In my code reviews, I found the `--diff-merges=first-parent` option\n + This is especially true in scenarios with non-trivial merges, i.e.\n + merges introducing changes other than, or in addition to, what merge ORT\n + would have produced. Merging a topic branch that changes a function\n + signature into a branch that added a caller of that function, for\n + example, would require the merge commit itself to adjust that caller to\n + the modified signature.\n +\n + In my code reviews, I found the `--diff-merges=remerge` option\n particularly useful.\n \n Signed-off-by: Johannes Schindelin \n @@ Documentation/git-range-diff.txt: to revert to color all lines according to the\n +\tcorresponding `--diff-merges=` option of linkgit:git-log[1],\n +\tand include them in the comparison.\n ++\n -+Note: In the common case, the `first-parent` mode will be the most natural one\n -+to use, as it is consistent with the idea that a merge is kind of a \"meta\n -+patch\", comprising all the merged commits' patches into a single one.\n ++Note: In the common case, the `remerge` mode will be the most natural one\n ++to use, as it shows only the diff on top of what Git's merge machinery would\n ++have produced. In other words, if a merge commit is the result of a\n ++non-conflicting `git merge`, the `remerge` mode will represent it with an empty\n ++diff.\n +\n --[no-]notes[=]::\n \tThis flag is passed to the `git log` program\n \t(see linkgit:git-log[1]) that generates the patches.\n \n ## builtin/range-diff.c ##\n -@@ builtin/range-diff.c: int cmd_range_diff(int argc,\n +@@ builtin/range-diff.c: int cmd_range_diff(int argc, const char **argv, const char *prefix)\n {\n \tstruct diff_options diffopt = { NULL };\n \tstruct strvec other_arg = STRVEC_INIT;\n @@ builtin/range-diff.c: int cmd_range_diff(int argc,\n \tstruct range_diff_options range_diff_opts = {\n \t\t.creation_factor = RANGE_DIFF_CREATION_FACTOR_DEFAULT,\n \t\t.diffopt = &diffopt,\n -@@ builtin/range-diff.c: int cmd_range_diff(int argc,\n +@@ builtin/range-diff.c: int cmd_range_diff(int argc, const char **argv, const char *prefix)\n \t\tOPT_PASSTHRU_ARGV(0, \"notes\", &other_arg,\n \t\t\t\t N_(\"notes\"), N_(\"passed to 'git log'\"),\n \t\t\t\t PARSE_OPT_OPTARG),\n @@ builtin/range-diff.c: int cmd_range_diff(int argc,\n \t\tOPT_BOOL(0, \"left-only\", &left_only,\n \t\t\t N_(\"only emit output related to the first range\")),\n \t\tOPT_BOOL(0, \"right-only\", &right_only,\n -@@ builtin/range-diff.c: int cmd_range_diff(int argc,\n +@@ builtin/range-diff.c: int cmd_range_diff(int argc, const char **argv, const char *prefix)\n \tif (!simple_color)\n \t\tdiffopt.use_color = 1;\n \n @@ builtin/range-diff.c: int cmd_range_diff(int argc,\n \tfor (i = 0; i < argc; i++)\n \t\tif (!strcmp(argv[i], \"--\")) {\n \t\t\tdash_dash = i;\n -@@ builtin/range-diff.c: int cmd_range_diff(int argc,\n +@@ builtin/range-diff.c: int cmd_range_diff(int argc, const char **argv, const char *prefix)\n \tres = show_range_diff(range1.buf, range2.buf, &range_diff_opts);\n \n \tstrvec_clear(&other_arg);\n @@ range-diff.c: static int read_patches(const char *range, struct string_list *lis\n \t\t /*\n @@ range-diff.c: static int read_patches(const char *range, struct string_list *list,\n \t\t \"--pretty=medium\",\n - \t\t \"--show-notes-by-default\",\n + \t\t \"--notes\",\n \t\t NULL);\n +\tif (!include_merges)\n +\t\tstrvec_push(&cp.args, \"--no-merges\");\n @@ range-diff.c: static int read_patches(const char *range, struct string_list *lis\n \t\t\t\tstrbuf_reset(&buf);\n \t\t\t}\n \t\t\tCALLOC_ARRAY(util, 1);\n +-\t\t\tif (get_oid(p, &util->oid)) {\n +\t\t\tif (include_merges && (q = strstr(p, \" (from \")))\n +\t\t\t\t*q = '\\0';\n - \t\t\tif (repo_get_oid(the_repository, p, &util->oid)) {\n ++\t\t\tif (repo_get_oid(the_repository, p, &util->oid)) {\n \t\t\t\terror(_(\"could not parse commit '%s'\"), p);\n \t\t\t\tFREE_AND_NULL(util);\n + \t\t\t\tstring_list_clear(list, 1);\n @@ range-diff.c: int show_range_diff(const char *range1, const char *range2,\n \n \tstruct string_list branch1 = STRING_LIST_INIT_DUP;\n -: ----------- > 2: f99416acd5a range-diff: introduce the convenience option `--remerge-diff`\n\n-- \ngitgitgadget\n"},{"id":"509159","messageId":"ef3c243da1b3215000684a6b2d683dc6bb0017ac.1734358282.git.gitgitgadget@gmail.com","threadId":"62464","inReplyTo":"pull.1734.v3.git.1734358282.gitgitgadget@gmail.com","subject":"[PATCH v3 1/2] range-diff: optionally include merge commits' diffs in the analysis","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-12-16T14:11:21Z","receivedAt":"2024-12-16T14:11:28Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThe `git log` command already offers support for including diffs for\nmerges, via the `--diff-merges=` option.\n\nLet's add corresponding support for `git range-diff`, too. This makes it\nmore convenient to spot differences between commit ranges that contain\nmerges.\n\nThis is especially true in scenarios with non-trivial merges, i.e.\nmerges introducing changes other than, or in addition to, what merge ORT\nwould have produced. Merging a topic branch that changes a function\nsignature into a branch that added a caller of that function, for\nexample, would require the merge commit itself to adjust that caller to\nthe modified signature.\n\nIn my code reviews, I found the `--diff-merges=remerge` option\nparticularly useful.\n\nSigned-off-by: Johannes Schindelin \n---\n Documentation/git-range-diff.txt | 13 ++++++++++++-\n builtin/range-diff.c | 10 ++++++++++\n range-diff.c | 17 ++++++++++++-----\n range-diff.h | 1 +\n t/t3206-range-diff.sh | 16 ++++++++++++++++\n 5 files changed, 51 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/git-range-diff.txt b/Documentation/git-range-diff.txt\nindex 0b393715d70..150b0acbd86 100644\n--- a/Documentation/git-range-diff.txt\n+++ b/Documentation/git-range-diff.txt\n@@ -10,7 +10,7 @@ SYNOPSIS\n [verse]\n 'git range-diff' [--color=[]] [--no-color] []\n \t[--no-dual-color] [--creation-factor=]\n-\t[--left-only | --right-only]\n+\t[--left-only | --right-only] [--diff-merges=]\n \t( | ... | )\n \t[[--] ...]\n \n@@ -81,6 +81,17 @@ to revert to color all lines according to the outer diff markers\n \tSuppress commits that are missing from the second specified range\n \t(or the \"right range\" when using the `...` format).\n \n+--diff-merges=::\n+\tInstead of ignoring merge commits, generate diffs for them using the\n+\tcorresponding `--diff-merges=` option of linkgit:git-log[1],\n+\tand include them in the comparison.\n++\n+Note: In the common case, the `remerge` mode will be the most natural one\n+to use, as it shows only the diff on top of what Git's merge machinery would\n+have produced. In other words, if a merge commit is the result of a\n+non-conflicting `git merge`, the `remerge` mode will represent it with an empty\n+diff.\n+\n --[no-]notes[=]::\n \tThis flag is passed to the `git log` program\n \t(see linkgit:git-log[1]) that generates the patches.\ndiff --git a/builtin/range-diff.c b/builtin/range-diff.c\nindex aecfae12d3a..9d6236e5116 100644\n--- a/builtin/range-diff.c\n+++ b/builtin/range-diff.c\n@@ -16,6 +16,7 @@ int cmd_range_diff(int argc, const char **argv, const char *prefix)\n {\n \tstruct diff_options diffopt = { NULL };\n \tstruct strvec other_arg = STRVEC_INIT;\n+\tstruct strvec diff_merges_arg = STRVEC_INIT;\n \tstruct range_diff_options range_diff_opts = {\n \t\t.creation_factor = RANGE_DIFF_CREATION_FACTOR_DEFAULT,\n \t\t.diffopt = &diffopt,\n@@ -31,6 +32,8 @@ int cmd_range_diff(int argc, const char **argv, const char *prefix)\n \t\tOPT_PASSTHRU_ARGV(0, \"notes\", &other_arg,\n \t\t\t\t N_(\"notes\"), N_(\"passed to 'git log'\"),\n \t\t\t\t PARSE_OPT_OPTARG),\n+\t\tOPT_PASSTHRU_ARGV(0, \"diff-merges\", &diff_merges_arg,\n+\t\t\t\t N_(\"style\"), N_(\"passed to 'git log'\"), 0),\n \t\tOPT_BOOL(0, \"left-only\", &left_only,\n \t\t\t N_(\"only emit output related to the first range\")),\n \t\tOPT_BOOL(0, \"right-only\", &right_only,\n@@ -57,6 +60,12 @@ int cmd_range_diff(int argc, const char **argv, const char *prefix)\n \tif (!simple_color)\n \t\tdiffopt.use_color = 1;\n \n+\t/* If `--diff-merges` was specified, imply `--merges` */\n+\tif (diff_merges_arg.nr) {\n+\t\trange_diff_opts.include_merges = 1;\n+\t\tstrvec_pushv(&other_arg, diff_merges_arg.v);\n+\t}\n+\n \tfor (i = 0; i < argc; i++)\n \t\tif (!strcmp(argv[i], \"--\")) {\n \t\t\tdash_dash = i;\n@@ -150,6 +159,7 @@ int cmd_range_diff(int argc, const char **argv, const char *prefix)\n \tres = show_range_diff(range1.buf, range2.buf, &range_diff_opts);\n \n \tstrvec_clear(&other_arg);\n+\tstrvec_clear(&diff_merges_arg);\n \tstrbuf_release(&range1);\n \tstrbuf_release(&range2);\n \ndiff --git a/range-diff.c b/range-diff.c\nindex 4bd65ab7496..3bd7bff4735 100644\n--- a/range-diff.c\n+++ b/range-diff.c\n@@ -31,7 +31,8 @@ struct patch_util {\n * as struct object_id (will need to be free()d).\n */\n static int read_patches(const char *range, struct string_list *list,\n-\t\t\tconst struct strvec *other_arg)\n+\t\t\tconst struct strvec *other_arg,\n+\t\t\tunsigned int include_merges)\n {\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n \tstruct strbuf buf = STRBUF_INIT, contents = STRBUF_INIT;\n@@ -42,7 +43,7 @@ static int read_patches(const char *range, struct string_list *list,\n \tsize_t size;\n \tint ret = -1;\n \n-\tstrvec_pushl(&cp.args, \"log\", \"--no-color\", \"-p\", \"--no-merges\",\n+\tstrvec_pushl(&cp.args, \"log\", \"--no-color\", \"-p\",\n \t\t \"--reverse\", \"--date-order\", \"--decorate=no\",\n \t\t \"--no-prefix\", \"--submodule=short\",\n \t\t /*\n@@ -57,6 +58,8 @@ static int read_patches(const char *range, struct string_list *list,\n \t\t \"--pretty=medium\",\n \t\t \"--notes\",\n \t\t NULL);\n+\tif (!include_merges)\n+\t\tstrvec_push(&cp.args, \"--no-merges\");\n \tstrvec_push(&cp.args, range);\n \tif (other_arg)\n \t\tstrvec_pushv(&cp.args, other_arg->v);\n@@ -89,12 +92,15 @@ static int read_patches(const char *range, struct string_list *list,\n \t\t}\n \n \t\tif (skip_prefix(line, \"commit \", &p)) {\n+\t\t\tchar *q;\n \t\t\tif (util) {\n \t\t\t\tstring_list_append(list, buf.buf)->util = util;\n \t\t\t\tstrbuf_reset(&buf);\n \t\t\t}\n \t\t\tCALLOC_ARRAY(util, 1);\n-\t\t\tif (get_oid(p, &util->oid)) {\n+\t\t\tif (include_merges && (q = strstr(p, \" (from \")))\n+\t\t\t\t*q = '\\0';\n+\t\t\tif (repo_get_oid(the_repository, p, &util->oid)) {\n \t\t\t\terror(_(\"could not parse commit '%s'\"), p);\n \t\t\t\tFREE_AND_NULL(util);\n \t\t\t\tstring_list_clear(list, 1);\n@@ -559,13 +565,14 @@ int show_range_diff(const char *range1, const char *range2,\n \n \tstruct string_list branch1 = STRING_LIST_INIT_DUP;\n \tstruct string_list branch2 = STRING_LIST_INIT_DUP;\n+\tunsigned int include_merges = range_diff_opts->include_merges;\n \n \tif (range_diff_opts->left_only && range_diff_opts->right_only)\n \t\tres = error(_(\"options '%s' and '%s' cannot be used together\"), \"--left-only\", \"--right-only\");\n \n-\tif (!res && read_patches(range1, &branch1, range_diff_opts->other_arg))\n+\tif (!res && read_patches(range1, &branch1, range_diff_opts->other_arg, include_merges))\n \t\tres = error(_(\"could not parse log for '%s'\"), range1);\n-\tif (!res && read_patches(range2, &branch2, range_diff_opts->other_arg))\n+\tif (!res && read_patches(range2, &branch2, range_diff_opts->other_arg, include_merges))\n \t\tres = error(_(\"could not parse log for '%s'\"), range2);\n \n \tif (!res) {\ndiff --git a/range-diff.h b/range-diff.h\nindex 04ffe217be6..9753922fee9 100644\n--- a/range-diff.h\n+++ b/range-diff.h\n@@ -10,6 +10,7 @@ struct range_diff_options {\n \tint creation_factor;\n \tunsigned dual_color:1;\n \tunsigned left_only:1, right_only:1;\n+\tunsigned include_merges:1;\n \tconst struct diff_options *diffopt; /* may be NULL */\n \tconst struct strvec *other_arg; /* may be NULL */\n };\ndiff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh\nindex b5f4d6a6530..d3c4d2dbb49 100755\n--- a/t/t3206-range-diff.sh\n+++ b/t/t3206-range-diff.sh\n@@ -866,4 +866,20 @@ test_expect_success 'submodule changes are shown irrespective of diff.submodule'\n \ttest_cmp expect actual\n '\n \n+test_expect_success '--diff-merges' '\n+\trenamed_oid=$(git rev-parse --short renamed-file) &&\n+\ttree=$(git merge-tree unmodified renamed-file) &&\n+\tclean=$(git commit-tree -m merge -p unmodified -p renamed-file $tree) &&\n+\tclean_oid=$(git rev-parse --short $clean) &&\n+\tconflict=$(git commit-tree -m merge -p unmodified -p renamed-file^ $tree) &&\n+\tconflict_oid=$(git rev-parse --short $conflict) &&\n+\n+\tgit range-diff --diff-merges=1 $clean...$conflict >actual &&\n+\tcat >expect <<-EOF &&\n+\t1: $renamed_oid < -: ------- s/12/B/\n+\t2: $clean_oid = 1: $conflict_oid merge\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"509161","messageId":"f99416acd5a4fa7d6173be52ab41e238cd99e151.1734358282.git.gitgitgadget@gmail.com","threadId":"62464","inReplyTo":"pull.1734.v3.git.1734358282.gitgitgadget@gmail.com","subject":"[PATCH v3 2/2] range-diff: introduce the convenience option `--remerge-diff`","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-12-16T14:11:22Z","receivedAt":"2024-12-16T14:11:29Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nJust like `git log`, now also `git range-diff` has that option as a\nshortcut for the common operation that would otherwise require the quite\nunwieldy (if theoretically \"more correct\") `--diff-mode=remerge` option.\n\nSigned-off-by: Johannes Schindelin \n---\n Documentation/git-range-diff.txt | 4 ++++\n builtin/range-diff.c | 2 ++\n 2 files changed, 6 insertions(+)\n\ndiff --git a/Documentation/git-range-diff.txt b/Documentation/git-range-diff.txt\nindex 150b0acbd86..e9e97d94480 100644\n--- a/Documentation/git-range-diff.txt\n+++ b/Documentation/git-range-diff.txt\n@@ -11,6 +11,7 @@ SYNOPSIS\n 'git range-diff' [--color=[]] [--no-color] []\n \t[--no-dual-color] [--creation-factor=]\n \t[--left-only | --right-only] [--diff-merges=]\n+\t[--remerge-diff]\n \t( | ... | )\n \t[[--] ...]\n \n@@ -92,6 +93,9 @@ have produced. In other words, if a merge commit is the result of a\n non-conflicting `git merge`, the `remerge` mode will represent it with an empty\n diff.\n \n+--remerge-diff::\n+\tConvenience option, equivalent to `--diff-merges=remerge`.\n+\n --[no-]notes[=]::\n \tThis flag is passed to the `git log` program\n \t(see linkgit:git-log[1]) that generates the patches.\ndiff --git a/builtin/range-diff.c b/builtin/range-diff.c\nindex 9d6236e5116..3ab871ab925 100644\n--- a/builtin/range-diff.c\n+++ b/builtin/range-diff.c\n@@ -34,6 +34,8 @@ int cmd_range_diff(int argc, const char **argv, const char *prefix)\n \t\t\t\t PARSE_OPT_OPTARG),\n \t\tOPT_PASSTHRU_ARGV(0, \"diff-merges\", &diff_merges_arg,\n \t\t\t\t N_(\"style\"), N_(\"passed to 'git log'\"), 0),\n+\t\tOPT_PASSTHRU_ARGV(0, \"remerge-diff\", &diff_merges_arg, NULL,\n+\t\t\t\t N_(\"passed to 'git log'\"), PARSE_OPT_NOARG),\n \t\tOPT_BOOL(0, \"left-only\", &left_only,\n \t\t\t N_(\"only emit output related to the first range\")),\n \t\tOPT_BOOL(0, \"right-only\", &right_only,\n-- \ngitgitgadget\n"}]}