Re: [PATCH v2 1/8] show, log: provide a --remerge-diff capability

[Johannes Altmanninger <aclopte@gmail.com>]

On Sat, Dec 25, 2021 at 07:59:12AM +0000, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@gmail.com>
> 
> When this option is specified, we remerge all (two parent) merge commits
> and diff the actual merge commit to the automatically created version,
> in order to show how users removed conflict markers, resolved the
> different conflict versions, and potentially added new changes outside
> of conflict regions in order to resolve semantic merge problems (or,
> possibly, just to hide other random changes).
> 
> This capability works by creating a temporary object directory and
> marking it as the primary object store.  This makes it so that any blobs
> or trees created during the automatic merge easily removable afterwards

s/easily/are easily/ ?

> by just deleting all objects from the temporary object directory.
> 
> There are a few ways that this implementation is suboptimal:
>   * `log --remerge-diff` becomes slow, because the temporary object
>     directory can fills with many loose objects while running

s/can fills/can fill/

>   * the log output can be muddied with misplaced "warning: cannot merge
>     binary files" messages, since ll-merge.c unconditionally writes those
>     messages to stderr while running instead of allowing callers to
>     manage them.
>   * important conflict and warning messages are simply dropped; thus for
>     conflicts like modify/delete or rename/rename or file/directory which
>     are not representable with content conflict markers, there may be no
>     way for a user of --remerge-diff to know that there had been a
>     conflict which was resolved (and which possibly motivated other
>     changes in the merge commit).
> Subsequent commits will address these issues.
> 
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>  Documentation/diff-options.txt |  8 ++++
>  builtin/log.c                  | 14 ++++++
>  diff-merges.c                  | 12 +++++
>  log-tree.c                     | 59 +++++++++++++++++++++++
>  revision.h                     |  3 +-
>  t/t4069-remerge-diff.sh        | 86 ++++++++++++++++++++++++++++++++++
>  6 files changed, 181 insertions(+), 1 deletion(-)
>  create mode 100755 t/t4069-remerge-diff.sh
> 
> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
> index c89d530d3d1..b05f1c9f1c9 100644
> --- a/Documentation/diff-options.txt
> +++ b/Documentation/diff-options.txt
> @@ -64,6 +64,14 @@ ifdef::git-log[]
>  	each of the parents. Separate log entry and diff is generated
>  	for each parent.
>  +
> +--diff-merges=remerge:::
> +--diff-merges=r:::
> +--remerge-diff:::

The synopsis above needs an update, too:

	diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
	index c89d530d3d..7a98ab3f85 100644
	--- a/Documentation/diff-options.txt
	+++ b/Documentation/diff-options.txt
	@@ -36,3 +36,3 @@ endif::git-format-patch[]
	 ifdef::git-log[]
	---diff-merges=(off|none|on|first-parent|1|separate|m|combined|c|dense-combined|cc)::
	+--diff-merges=(off|none|on|first-parent|1|separate|m|combined|c|dense-combined|cc|remerge|r)::
	 --no-diff-merges::

> +	With this option, two-parent merge commits are remerged to
> +	create a temporary tree object -- potentially containing files
> +	with conflict markers and such.  A diff is then shown between
> +	that temporary tree and the actual merge commit.

I had not really looked at any of the --diff-merges options before.  The term
"remerge" felt a bit opaque at first, because I didn't know what the diff
would look like. I might have found this easier:

--diff-merges=resolution:::
--diff-merges=r:::
--resolution-diff:::
	This makes two-parent merge commits show the diff with respect to
	a mechanical merge of their parents -- potentially containing files
	with conflict markers and such.

But on a second thought, remerge is actually consistent with the rest,
because it states _what_ we compare to the merge commit, so nevermind.

> ++
>  --diff-merges=combined:::
>  --diff-merges=c:::
>  -c:::
> diff --git a/builtin/log.c b/builtin/log.c
> index f75d87e8d7f..d053418fddd 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -35,6 +35,7 @@
>  #include "repository.h"
>  #include "commit-reach.h"
>  #include "range-diff.h"
> +#include "tmp-objdir.h"
>  
>  #define MAIL_DEFAULT_WRAP 72
>  #define COVER_FROM_AUTO_MAX_SUBJECT_LEN 100
> @@ -406,6 +407,14 @@ static int cmd_log_walk(struct rev_info *rev)
>  	struct commit *commit;
>  	int saved_nrl = 0;
>  	int saved_dcctc = 0;
> +	struct tmp_objdir *remerge_objdir = NULL;
> +
> +	if (rev->remerge_diff) {
> +		remerge_objdir = tmp_objdir_create("remerge-diff");
> +		if (!remerge_objdir)
> +			die_errno(_("unable to create temporary object directory"));
> +		tmp_objdir_replace_primary_odb(remerge_objdir, 1);
> +	}
>  
>  	if (rev->early_output)
>  		setup_early_output();
> @@ -449,6 +458,9 @@ static int cmd_log_walk(struct rev_info *rev)
>  	rev->diffopt.no_free = 0;
>  	diff_free(&rev->diffopt);
>  
> +	if (rev->remerge_diff)
> +		tmp_objdir_destroy(remerge_objdir);
> +
>  	if (rev->diffopt.output_format & DIFF_FORMAT_CHECKDIFF &&
>  	    rev->diffopt.flags.check_failed) {
>  		return 02;
> @@ -1943,6 +1955,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>  		die(_("--name-status does not make sense"));
>  	if (rev.diffopt.output_format & DIFF_FORMAT_CHECKDIFF)
>  		die(_("--check does not make sense"));
> +	if (rev.remerge_diff)
> +		die(_("--remerge-diff does not make sense"));
>  
>  	if (!use_patch_format &&
>  		(!rev.diffopt.output_format ||
> diff --git a/diff-merges.c b/diff-merges.c
> index 5060ccd890b..0af4b3f9191 100644
> --- a/diff-merges.c
> +++ b/diff-merges.c
> @@ -17,6 +17,7 @@ static void suppress(struct rev_info *revs)
>  	revs->combined_all_paths = 0;
>  	revs->merges_imply_patch = 0;
>  	revs->merges_need_diff = 0;
> +	revs->remerge_diff = 0;
>  }
>  
>  static void set_separate(struct rev_info *revs)
> @@ -45,6 +46,12 @@ static void set_dense_combined(struct rev_info *revs)
>  	revs->dense_combined_merges = 1;
>  }
>  
> +static void set_remerge_diff(struct rev_info *revs)
> +{
> +	suppress(revs);
> +	revs->remerge_diff = 1;
> +}
> +
>  static diff_merges_setup_func_t func_by_opt(const char *optarg)
>  {
>  	if (!strcmp(optarg, "off") || !strcmp(optarg, "none"))
> @@ -57,6 +64,8 @@ static diff_merges_setup_func_t func_by_opt(const char *optarg)
>  		return set_combined;
>  	else if (!strcmp(optarg, "cc") || !strcmp(optarg, "dense-combined"))
>  		return set_dense_combined;
> +	else if (!strcmp(optarg, "r") || !strcmp(optarg, "remerge"))
> +		return set_remerge_diff;
>  	else if (!strcmp(optarg, "m") || !strcmp(optarg, "on"))
>  		return set_to_default;
>  	return NULL;
> @@ -110,6 +119,9 @@ int diff_merges_parse_opts(struct rev_info *revs, const char **argv)
>  	} else if (!strcmp(arg, "--cc")) {
>  		set_dense_combined(revs);
>  		revs->merges_imply_patch = 1;
> +	} else if (!strcmp(arg, "--remerge-diff")) {
> +		set_remerge_diff(revs);
> +		revs->merges_imply_patch = 1;
>  	} else if (!strcmp(arg, "--no-diff-merges")) {
>  		suppress(revs);
>  	} else if (!strcmp(arg, "--combined-all-paths")) {
> diff --git a/log-tree.c b/log-tree.c
> index 644893fd8cf..84ed864fc81 100644
> --- a/log-tree.c
> +++ b/log-tree.c
> @@ -1,4 +1,5 @@
>  #include "cache.h"
> +#include "commit-reach.h"
>  #include "config.h"
>  #include "diff.h"
>  #include "object-store.h"
> @@ -7,6 +8,7 @@
>  #include "tag.h"
>  #include "graph.h"
>  #include "log-tree.h"
> +#include "merge-ort.h"
>  #include "reflog-walk.h"
>  #include "refs.h"
>  #include "string-list.h"
> @@ -902,6 +904,51 @@ static int do_diff_combined(struct rev_info *opt, struct commit *commit)
>  	return !opt->loginfo;
>  }
>  
> +static int do_remerge_diff(struct rev_info *opt,
> +			   struct commit_list *parents,
> +			   struct object_id *oid,
> +			   struct commit *commit)
> +{
> +	struct merge_options o;
> +	struct commit_list *bases;
> +	struct merge_result res = {0};
> +	struct pretty_print_context ctx = {0};
> +	struct commit *parent1 = parents->item;
> +	struct commit *parent2 = parents->next->item;
> +	struct strbuf parent1_desc = STRBUF_INIT;
> +	struct strbuf parent2_desc = STRBUF_INIT;
> +
> +	/* Setup merge options */
> +	init_merge_options(&o, the_repository);
> +	o.show_rename_progress = 0;

Is there a reason why we are repeating the default here (but not anywhere else)?
For example sequencer.c::do_merge() and builtin/am.c::fall_back_threeway()
don't, and probably also rely on this being disabled(?).

> +
> +	ctx.abbrev = DEFAULT_ABBREV;
> +	format_commit_message(parent1, "%h (%s)", &parent1_desc, &ctx);
> +	format_commit_message(parent2, "%h (%s)", &parent2_desc, &ctx);
> +	o.branch1 = parent1_desc.buf;
> +	o.branch2 = parent2_desc.buf;
> +
> +	/* Parse the relevant commits and get the merge bases */
> +	parse_commit_or_die(parent1);
> +	parse_commit_or_die(parent2);
> +	bases = get_merge_bases(parent1, parent2);
> +
> +	/* Re-merge the parents */
> +	merge_incore_recursive(&o, bases, parent1, parent2, &res);
> +
> +	/* Show the diff */
> +	diff_tree_oid(&res.tree->object.oid, oid, "", &opt->diffopt);
> +	log_tree_diff_flush(opt);
> +
> +	/* Cleanup */
> +	strbuf_release(&parent1_desc);
> +	strbuf_release(&parent2_desc);
> +	merge_finalize(&o, &res);
> +	/* TODO: clean up the temporary object directory */
> +
> +	return !opt->loginfo;
> +}
> +
>  /*
>   * Show the diff of a commit.
>   *
> @@ -936,6 +983,18 @@ static int log_tree_diff(struct rev_info *opt, struct commit *commit, struct log
>  	}
>  
>  	if (is_merge) {
> +		int octopus = (parents->next->next != NULL);
> +
> +		if (opt->remerge_diff) {
> +			if (octopus) {
> +				show_log(opt);
> +				fprintf(opt->diffopt.file,
> +					"diff: warning: Skipping remerge-diff "
> +					"for octopus merges.\n");
> +				return 1;
> +			}
> +			return do_remerge_diff(opt, parents, oid, commit);
> +		}
>  		if (opt->combine_merges)
>  			return do_diff_combined(opt, commit);
>  		if (opt->separate_merges) {
> diff --git a/revision.h b/revision.h
> index 5578bb4720a..13178e6b8f3 100644
> --- a/revision.h
> +++ b/revision.h
> @@ -195,7 +195,8 @@ struct rev_info {
>  			combine_merges:1,
>  			combined_all_paths:1,
>  			dense_combined_merges:1,
> -			first_parent_merges:1;
> +			first_parent_merges:1,
> +			remerge_diff:1;
>  
>  	/* Format info */
>  	int		show_notes;
> diff --git a/t/t4069-remerge-diff.sh b/t/t4069-remerge-diff.sh
> new file mode 100755
> index 00000000000..192dbce2bfe
> --- /dev/null
> +++ b/t/t4069-remerge-diff.sh
> @@ -0,0 +1,86 @@
> +#!/bin/sh
> +
> +test_description='remerge-diff handling'
> +
> +. ./test-lib.sh
> +
> +test_expect_success 'setup basic merges' '
> +	test_write_lines 1 2 3 4 5 6 7 8 9 >numbers &&
> +	git add numbers &&
> +	git commit -m base &&
> +
> +	git branch feature_a &&
> +	git branch feature_b &&
> +	git branch feature_c &&
> +
> +	git branch ab_resolution &&
> +	git branch bc_resolution &&
> +
> +	git checkout feature_a &&
> +	test_write_lines 1 2 three 4 5 6 7 eight 9 >numbers &&
> +	git commit -a -m change_a &&
> +
> +	git checkout feature_b &&
> +	test_write_lines 1 2 tres 4 5 6 7 8 9 >numbers &&
> +	git commit -a -m change_b &&
> +
> +	git checkout feature_c &&
> +	test_write_lines 1 2 3 4 5 6 7 8 9 10 >numbers &&
> +	git commit -a -m change_c &&
> +
> +	git checkout bc_resolution &&
> +	# fast forward
> +	git merge feature_b &&

maybe use --ff-only instead of the comment? Same below.
(But if we did that we probably want to drop the "no conflict" comment too.)

> +	# no conflict
> +	git merge feature_c &&
> +
> +	git checkout ab_resolution &&
> +	# fast forward
> +	git merge feature_a &&
> +	# conflicts!
> +	test_must_fail git merge feature_b &&
> +	# Resolve conflict...and make another change elsewhere
> +	test_write_lines 1 2 drei 4 5 6 7 acht 9 >numbers &&
> +	git add numbers &&
> +	git merge --continue
> +'
> +
> +test_expect_success 'remerge-diff on a clean merge' '
> +	git log -1 --oneline bc_resolution >expect &&
> +	git show --oneline --remerge-diff bc_resolution >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'remerge-diff with both a resolved conflict and an unrelated change' '
> +	git log -1 --oneline ab_resolution >tmp &&
> +	cat <<-EOF >>tmp &&
> +	diff --git a/numbers b/numbers
> +	index a1fb731..6875544 100644
> +	--- a/numbers
> +	+++ b/numbers
> +	@@ -1,13 +1,9 @@
> +	 1
> +	 2
> +	-<<<<<<< b0ed5cb (change_a)
> +	-three
> +	-=======
> +	-tres
> +	->>>>>>> 6cd3f82 (change_b)
> +	+drei

nice

> +	 4
> +	 5
> +	 6
> +	 7
> +	-eight
> +	+acht
> +	 9
> +	EOF
> +	# Hashes above are sha1; rip them out so test works with sha256
> +	sed -e "s/[0-9a-f]\{7,\}/HASH/g" tmp >expect &&

Right, sha256 could cause many noisy test changes. I wonder if there is a
more general way to avoid this; maybe default to SHA1 for existing tests?

> +
> +	git show --oneline --remerge-diff ab_resolution >tmp &&
> +	sed -e "s/[0-9a-f]\{7,\}/HASH/g" tmp >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_done
> -- 
> gitgitgadget
>