[v3,0/2] Support diff merges option in range diff

Message

Derrick Stolee via GitGitGadget Dec. 16, 2024, 2:11 p.m. UTC

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

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

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

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

Changes since v2:

 * Rebased onto js/log-remerge-keep-ancestry, to fix --diff-merges=remerge
 * Now the documentation suggests remerge instead of first-parent mode
 * Added a follow-up commit to introduce the convenience option git
   range-diff --remerge-diff

Changes since v1:

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

Johannes Schindelin (2):
  range-diff: optionally include merge commits' diffs in the analysis
  range-diff: introduce the convenience option `--remerge-diff`

 Documentation/git-range-diff.txt | 17 ++++++++++++++++-
 builtin/range-diff.c             | 12 ++++++++++++
 range-diff.c                     | 17 ++++++++++++-----
 range-diff.h                     |  1 +
 t/t3206-range-diff.sh            | 16 ++++++++++++++++
 5 files changed, 57 insertions(+), 6 deletions(-)


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

Range-diff vs v2:

 1:  d01a395900b ! 1:  ef3c243da1b range-diff: optionally include merge commits' diffs in the analysis
     @@ Commit message
          merges, via the `--diff-merges=<format>` option.
      
          Let's add corresponding support for `git range-diff`, too. This makes it
     -    more convenient to spot differences between iterations of non-linear
     -    contributions, where so-called "evil merges" are sometimes necessary and
     -    need to be reviewed, too.
     +    more convenient to spot differences between commit ranges that contain
     +    merges.
      
     -    In my code reviews, I found the `--diff-merges=first-parent` option
     +    This is especially true in scenarios with non-trivial merges, i.e.
     +    merges introducing changes other than, or in addition to, what merge ORT
     +    would have produced. Merging a topic branch that changes a function
     +    signature into a branch that added a caller of that function, for
     +    example, would require the merge commit itself to adjust that caller to
     +    the modified signature.
     +
     +    In my code reviews, I found the `--diff-merges=remerge` option
          particularly useful.
      
          Signed-off-by: Johannes Schindelin <[e-mail]>
     @@ Documentation/git-range-diff.txt: to revert to color all lines according to the
      +	corresponding `--diff-merges=<format>` option of linkgit:git-log[1],
      +	and include them in the comparison.
      ++
     -+Note: In the common case, the `first-parent` mode will be the most natural one
     -+to use, as it is consistent with the idea that a merge is kind of a "meta
     -+patch", comprising all the merged commits' patches into a single one.
     ++Note: In the common case, the `remerge` mode will be the most natural one
     ++to use, as it shows only the diff on top of what Git's merge machinery would
     ++have produced. In other words, if a merge commit is the result of a
     ++non-conflicting `git merge`, the `remerge` mode will represent it with an empty
     ++diff.
      +
       --[no-]notes[=<ref>]::
       	This flag is passed to the `git log` program
       	(see linkgit:git-log[1]) that generates the patches.
      
       ## builtin/range-diff.c ##
     -@@ builtin/range-diff.c: int cmd_range_diff(int argc,
     +@@ builtin/range-diff.c: int cmd_range_diff(int argc, const char **argv, const char *prefix)
       {
       	struct diff_options diffopt = { NULL };
       	struct strvec other_arg = STRVEC_INIT;
     @@ builtin/range-diff.c: int cmd_range_diff(int argc,
       	struct range_diff_options range_diff_opts = {
       		.creation_factor = RANGE_DIFF_CREATION_FACTOR_DEFAULT,
       		.diffopt = &diffopt,
     -@@ builtin/range-diff.c: int cmd_range_diff(int argc,
     +@@ builtin/range-diff.c: int cmd_range_diff(int argc, const char **argv, const char *prefix)
       		OPT_PASSTHRU_ARGV(0, "notes", &other_arg,
       				  N_("notes"), N_("passed to 'git log'"),
       				  PARSE_OPT_OPTARG),
     @@ builtin/range-diff.c: int cmd_range_diff(int argc,
       		OPT_BOOL(0, "left-only", &left_only,
       			 N_("only emit output related to the first range")),
       		OPT_BOOL(0, "right-only", &right_only,
     -@@ builtin/range-diff.c: int cmd_range_diff(int argc,
     +@@ builtin/range-diff.c: int cmd_range_diff(int argc, const char **argv, const char *prefix)
       	if (!simple_color)
       		diffopt.use_color = 1;
       
     @@ builtin/range-diff.c: int cmd_range_diff(int argc,
       	for (i = 0; i < argc; i++)
       		if (!strcmp(argv[i], "--")) {
       			dash_dash = i;
     -@@ builtin/range-diff.c: int cmd_range_diff(int argc,
     +@@ builtin/range-diff.c: int cmd_range_diff(int argc, const char **argv, const char *prefix)
       	res = show_range_diff(range1.buf, range2.buf, &range_diff_opts);
       
       	strvec_clear(&other_arg);
     @@ range-diff.c: static int read_patches(const char *range, struct string_list *lis
       		     /*
      @@ range-diff.c: static int read_patches(const char *range, struct string_list *list,
       		     "--pretty=medium",
     - 		     "--show-notes-by-default",
     + 		     "--notes",
       		     NULL);
      +	if (!include_merges)
      +		strvec_push(&cp.args, "--no-merges");
     @@ range-diff.c: static int read_patches(const char *range, struct string_list *lis
       				strbuf_reset(&buf);
       			}
       			CALLOC_ARRAY(util, 1);
     +-			if (get_oid(p, &util->oid)) {
      +			if (include_merges && (q = strstr(p, " (from ")))
      +				*q = '\0';
     - 			if (repo_get_oid(the_repository, p, &util->oid)) {
     ++			if (repo_get_oid(the_repository, p, &util->oid)) {
       				error(_("could not parse commit '%s'"), p);
       				FREE_AND_NULL(util);
     + 				string_list_clear(list, 1);
      @@ range-diff.c: int show_range_diff(const char *range1, const char *range2,
       
       	struct string_list branch1 = STRING_LIST_INIT_DUP;
 -:  ----------- > 2:  f99416acd5a range-diff: introduce the convenience option `--remerge-diff`