Add a new --remerge-diff capability to show & log

Here are some patches to add a --remerge-diff capability to show & log, which works by comparing merge commits to an automatic remerge (note that the automatic remerge tree can contain files with conflict markers).

Changes since original submission[1]:

* Rebased on top of the version of ns/tmp-objdir that Neeraj submitted (Neeraj's patches were based on v2.34, but ns/tmp-objdir got applied on an old commit and does not even build because of that).
* Modify ll-merge API to return a status, instead of printing "Cannot merge binary files" on stdout[2] (as suggested by Peff)
* Make conflict messages and other such warnings into diff headers of the subsequent remerge-diff rather than appearing in the diff as file content of some funny looking filenames (as suggested by Peff[3] and Junio[4])
* Sergey ack'ed the diff-merges.c portion of the patches, but that wasn't limited to one patch so not sure where to record that ack.

[1]
https://lore.kernel.org/git/[email protected]/;
GitHub wouldn't let me change the target branch for the PR, so I had to create a new one with the new base and thus the reason for not sending this as v2 even though it is. [2]
https://lore.kernel.org/git/[email protected]/,
https://lore.kernel.org/git/[email protected]/ [3]
https://lore.kernel.org/git/[email protected]/ [4]
https://lore.kernel.org/git/[email protected]/

=== FURTHER BACKGROUND (original cover letter material) ==

Here are some example commits you can try this out on (with git show --remerge-diff $COMMIT):

* git.git conflicted merge: 07601b5b36
* git.git non-conflicted change: bf04590ecd
* linux.git conflicted merge: eab3540562fb
* linux.git non-conflicted change: 223cea6a4f05

Many more can be found by just running git log --merges --remerge-diff in your repository of choice and searching for diffs (most merges tend to be clean and unmodified and thus produce no diff but a search of '^diff' in the log output tends to find the examples nicely).

Some basic high level details about this new option:

* This option is most naturally compared to --cc, though the output seems to be much more understandable to most users than --cc output.
* Since merges are often clean and unmodified, this new option results in an empty diff for most merges.
* This new option shows things like the removal of conflict markers, which hunks users picked from the various conflicted sides to keep or remove, and shows changes made outside of conflict markers (which might reflect changes needed to resolve semantic conflicts or cleanups of e.g. compilation warnings or other additional changes an integrator felt belonged in the merged result).
* This new option does not (currently) work for octopus merges, since merge-ort is specific to two-parent merges[1].
* This option will not work on a read-only or full filesystem[2].
* We discussed this capability at Git Merge 2020, and one of the suggestions was doing a periodic git gc --auto during the operation (due to potential new blobs and trees created during the operation). I found a way to avoid that; see [2].
* This option is faster than you'd probably expect; it handles 33.5 merge commits per second in linux.git on my computer; see below.

In regards to the performance point above, the timing for running the following command:

time git log --min-parents=2 --max-parents=2 $DIFF_FLAG | wc -l

in linux.git (with v5.4 checked out, since my copy of linux is very out of date) is as follows:

DIFF_FLAG=--cc:            71m 31.536s
DIFF_FLAG=--remerge-diff:  31m  3.170s

Note that there are 62476 merges in this history. Also, output size is:

DIFF_FLAG=--cc:            2169111 lines
DIFF_FLAG=--remerge-diff:  2458020 lines

So roughly the same amount of output as --cc, as you'd expect.

As a side note: git log --remerge-diff, when run in various repositories and allowed to run all the way back to the beginning(s) of history, is a nice stress test of sorts for merge-ort. Especially when users run it for you on their repositories they are working on, whether intentionally or via a bug in a tool triggering that command to be run unexpectedly. Long story short, such a bug in an internal tool existed last December and this command was run on an internal repository and found a platform-specific bug in merge-ort on some really old merge commit from that repo. I fixed that bug (a STABLE_QSORT thing) while upstreaming all the merge-ort patches in the mean time, but it was nice getting extra testing. Having more folks run this on their repositories might be useful extra testing of the new merge strategy.

Also, I previously mentioned --remerge-diff-only (a flag to show how cherry-picks or reverts differ from an automatic cherry-pick or revert, in addition to showing how merges differ from an automatic merge). This series does not include the patches to introduce that option; I'll submit them later.

Two other things that might be interesting but are not included and which I haven't investigated:

* some mechanism for passing extra merge options through (e.g. -Xignore-space-change)
* a capability to compare the automatic merge to a second automatic merge done with different merge options. (Not sure if this would be of interest to end users, but might be interesting while developing new a --strategy-option, or maybe checking how changing some default in the merge algorithm would affect historical merges in various repositories).

[1] I have nebulous ideas of how an Octopus-centric ORT strategy could be written -- basically, just repeatedly invoking ort and trying to make sure nested conflicts can be differentiated. For now, though, a simple warning is printed that octopus merges are not handled and no diff will be shown. [2] New blobs/trees can be written by the three-way merging step. These are written to a temporary area (via tmp-objdir.c) under the git object store that is cleaned up at the end of the operation, with the new loose objects from the remerge being cleaned up after each individual merge.

Elijah Newren (9):
tmp_objdir: add a helper function for discarding all contained objects
ll-merge: make callers responsible for showing warnings
merge-ort: capture and print ll-merge warnings in our preferred
fashion
merge-ort: mark a few more conflict messages as omittable
merge-ort: make path_messages available to external callers
diff: add ability to insert additional headers for paths
merge-ort: format messages slightly different for use in headers show, log: provide a --remerge-diff capability doc/diff-options: explain the new --remerge-diff option

 Documentation/diff-options.txt |  8 ++++
 apply.c                        |  5 ++-
 builtin/checkout.c             | 12 ++++--
 builtin/log.c                  | 16 ++++++++
 diff-merges.c                  | 12 ++++++
 diff.c                         | 34 ++++++++++++++++-
 diff.h                         |  1 +
 ll-merge.c                     | 40 ++++++++++---------
 ll-merge.h                     |  9 ++++-
 log-tree.c                     | 70 ++++++++++++++++++++++++++++++++++
 merge-blobs.c                  |  5 ++-
 merge-ort.c                    | 49 +++++++++++++++++++++---
 merge-ort.h                    | 10 +++++
 merge-recursive.c              |  8 +++-
 merge-recursive.h              |  1 +
 notes-merge.c                  |  5 ++-
 rerere.c                       | 10 +++--
 revision.h                     |  6 ++-
 t/t6404-recursive-merge.sh     |  9 ++++-
 t/t6406-merge-attr.sh          |  9 ++++-
 tmp-objdir.c                   |  5 +++
 tmp-objdir.h                   |  6 +++
 22 files changed, 288 insertions(+), 42 deletions(-)

base-commit: 4e44121c2d7bced65e25eb7ec5156290132bec94
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1103%2Fnewren%2Fremerge-diff-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1103/newren/remerge-diff-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1103

gitgitgadget

From: Elijah Newren <[email protected]>

---
 tmp-objdir.c | 5 +++++
 tmp-objdir.h | 6 ++++++
 2 files changed, 11 insertions(+)

index 3d38eeab66b..adf6033549e 100644
@@ -79,6 +79,11 @@ static void remove_tmp_objdir_on_signal(int signo)
 	raise(signo);
 }
 
+void tmp_objdir_discard_objects(struct tmp_objdir *t)
+{
+	remove_dir_recursively(&t->path, REMOVE_DIR_KEEP_TOPLEVEL);
+}
+
 /*
  * These env_* functions are for setting up the child environment; the
  * "replace" variant overrides the value of any existing variable with that

index cda5ec76778..76efc7edee5 100644
@@ -46,6 +46,12 @@ int tmp_objdir_migrate(struct tmp_objdir *);
  */
 int tmp_objdir_destroy(struct tmp_objdir *);
 
+/*
+ * Remove all objects from the temporary object directory, while leaving it
+ * around so more objects can be added.
+ */
+void tmp_objdir_discard_objects(struct tmp_objdir *);
+
 /*
  * Add the temporary object directory as an alternate object store in the
  * current process.

gitgitgadget

From: Elijah Newren <[email protected]>

Since some callers may want to send warning messages to somewhere other than stdout/stderr, stop printing "warning: Cannot merge binary files" from ll-merge and instead modify the return status of ll_merge() to indicate when a merge of binary files has occurred.

Note that my methodology included first modifying ll_merge() to return a struct, so that the compiler would catch all the callers for me and ensure I had modified all of them. After modifying all of them, I then changed the struct to an enum.

---
 apply.c            |  5 ++++-
 builtin/checkout.c | 12 ++++++++----
 ll-merge.c         | 40 ++++++++++++++++++++++------------------
 ll-merge.h         |  9 ++++++++-
 merge-blobs.c      |  5 ++++-
 merge-ort.c        |  5 ++++-
 merge-recursive.c  |  5 ++++-
 notes-merge.c      |  5 ++++-
 rerere.c           | 10 +++++++---
 9 files changed, 65 insertions(+), 31 deletions(-)

index 43a0aebf4ee..12ea9c72a6b 100644
@@ -3492,7 +3492,7 @@ static int three_way_merge(struct apply_state *state,
 {
 	mmfile_t base_file, our_file, their_file;
 	mmbuffer_t result = { NULL };
-	int status;
+	enum ll_merge_result status;
 
 	/* resolve trivial cases first */
 	if (oideq(base, ours))

@@ -3509,6 +3509,9 @@ static int three_way_merge(struct apply_state *state,
 			  &their_file, "theirs",
 			  state->repo->index,
 			  NULL);
+	if (status == LL_MERGE_BINARY_CONFLICT)
+		warning("Cannot merge binary files: %s (%s vs. %s)",
+			"base", "ours", "theirs");
 	free(base_file.ptr);
 	free(our_file.ptr);
 	free(their_file.ptr);

index cbf73b8c9f6..3a559d69303 100644
@@ -237,6 +237,7 @@ static int checkout_merged(int pos, const struct checkout *state,
 	struct cache_entry *ce = active_cache[pos];
 	const char *path = ce->name;
 	mmfile_t ancestor, ours, theirs;
+	enum ll_merge_result merge_status;
 	int status;
 	struct object_id oid;
 	mmbuffer_t result_buf;

@@ -267,13 +268,16 @@ static int checkout_merged(int pos, const struct checkout *state,
 	memset(&ll_opts, 0, sizeof(ll_opts));
 	git_config_get_bool("merge.renormalize", &renormalize);
 	ll_opts.renormalize = renormalize;
-	status = ll_merge(&result_buf, path, &ancestor, "base",
-			  &ours, "ours", &theirs, "theirs",
-			  state->istate, &ll_opts);
+	merge_status = ll_merge(&result_buf, path, &ancestor, "base",
+				&ours, "ours", &theirs, "theirs",
+				state->istate, &ll_opts);
 	free(ancestor.ptr);
 	free(ours.ptr);
 	free(theirs.ptr);
-	if (status < 0 || !result_buf.ptr) {
+	if (merge_status == LL_MERGE_BINARY_CONFLICT)
+		warning("Cannot merge binary files: %s (%s vs. %s)",
+			path, "ours", "theirs");
+	if (merge_status < 0 || !result_buf.ptr) {
 		free(result_buf.ptr);
 		return error(_("path '%s': cannot merge"), path);
 	}

index 261657578c7..669c09eed6c 100644
@@ -14,7 +14,7 @@
 
 struct ll_merge_driver;
 
-typedef int (*ll_merge_fn)(const struct ll_merge_driver *,
+typedef enum ll_merge_result (*ll_merge_fn)(const struct ll_merge_driver *,
 			   mmbuffer_t *result,
 			   const char *path,
 			   mmfile_t *orig, const char *orig_name,

@@ -49,7 +49,7 @@ void reset_merge_attributes(void)
 /*
  * Built-in low-levels
  */
-static int ll_binary_merge(const struct ll_merge_driver *drv_unused,
+static enum ll_merge_result ll_binary_merge(const struct ll_merge_driver *drv_unused,
 			   mmbuffer_t *result,
 			   const char *path,
 			   mmfile_t *orig, const char *orig_name,

@@ -58,6 +58,7 @@ static int ll_binary_merge(const struct ll_merge_driver *drv_unused,
 			   const struct ll_merge_options *opts,
 			   int marker_size)
 {
+	enum ll_merge_result ret;
 	mmfile_t *stolen;
 	assert(opts);
 

@@ -68,16 +69,19 @@ static int ll_binary_merge(const struct ll_merge_driver *drv_unused,
 	 */
 	if (opts->virtual_ancestor) {
 		stolen = orig;
+		ret = LL_MERGE_OK;
 	} else {
 		switch (opts->variant) {
 		default:
-			warning("Cannot merge binary files: %s (%s vs. %s)",
-				path, name1, name2);
-			/* fallthru */
+			ret = LL_MERGE_BINARY_CONFLICT;
+			stolen = src1;
+			break;
 		case XDL_MERGE_FAVOR_OURS:
+			ret = LL_MERGE_OK;
 			stolen = src1;
 			break;
 		case XDL_MERGE_FAVOR_THEIRS:
+			ret = LL_MERGE_OK;
 			stolen = src2;
 			break;
 		}

@@ -87,16 +91,10 @@ static int ll_binary_merge(const struct ll_merge_driver *drv_unused,
 	result->size = stolen->size;
 	stolen->ptr = NULL;
 
-	/*
-	 * With -Xtheirs or -Xours, we have cleanly merged;
-	 * otherwise we got a conflict.
-	 */
-	return opts->variant == XDL_MERGE_FAVOR_OURS ||
-	       opts->variant == XDL_MERGE_FAVOR_THEIRS ?
-	       0 : 1;
+	return ret;
 }
 
-static int ll_xdl_merge(const struct ll_merge_driver *drv_unused,
+static enum ll_merge_result ll_xdl_merge(const struct ll_merge_driver *drv_unused,
 			mmbuffer_t *result,
 			const char *path,
 			mmfile_t *orig, const char *orig_name,

@@ -105,7 +103,9 @@ static int ll_xdl_merge(const struct ll_merge_driver *drv_unused,
 			const struct ll_merge_options *opts,
 			int marker_size)
 {
+	enum ll_merge_result ret;
 	xmparam_t xmp;
+	int status;
 	assert(opts);
 
 	if (orig->size > MAX_XDIFF_SIZE ||

@@ -133,10 +133,12 @@ static int ll_xdl_merge(const struct ll_merge_driver *drv_unused,
 	xmp.ancestor = orig_name;
 	xmp.file1 = name1;
 	xmp.file2 = name2;
-	return xdl_merge(orig, src1, src2, &xmp, result);
+	status = xdl_merge(orig, src1, src2, &xmp, result);
+	ret = (status > 1 ) ? LL_MERGE_CONFLICT : status;
+	return ret;
 }
 
-static int ll_union_merge(const struct ll_merge_driver *drv_unused,
+static enum ll_merge_result ll_union_merge(const struct ll_merge_driver *drv_unused,
 			  mmbuffer_t *result,
 			  const char *path,
 			  mmfile_t *orig, const char *orig_name,

@@ -178,7 +180,7 @@ static void create_temp(mmfile_t *src, char *path, size_t len)
 /*
  * User defined low-level merge driver support.
  */
-static int ll_ext_merge(const struct ll_merge_driver *fn,
+static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,
 			mmbuffer_t *result,
 			const char *path,
 			mmfile_t *orig, const char *orig_name,

@@ -194,6 +196,7 @@ static int ll_ext_merge(const struct ll_merge_driver *fn,
 	const char *args[] = { NULL, NULL };
 	int status, fd, i;
 	struct stat st;
+	enum ll_merge_result ret;
 	assert(opts);
 
 	sq_quote_buf(&path_sq, path);

@@ -236,7 +239,8 @@ static int ll_ext_merge(const struct ll_merge_driver *fn,
 		unlink_or_warn(temp[i]);
 	strbuf_release(&cmd);
 	strbuf_release(&path_sq);
-	return status;
+	ret = (status > 1) ? LL_MERGE_CONFLICT : status;
+	return ret;
 }
 
 /*

@@ -362,7 +366,7 @@ static void normalize_file(mmfile_t *mm, const char *path, struct index_state *i
 	}
 }
 
-int ll_merge(mmbuffer_t *result_buf,
+enum ll_merge_result ll_merge(mmbuffer_t *result_buf,
 	     const char *path,
 	     mmfile_t *ancestor, const char *ancestor_label,
 	     mmfile_t *ours, const char *our_label,

index aceb1b24132..e4a20e81a3a 100644
@@ -82,13 +82,20 @@ struct ll_merge_options {
 	long xdl_opts;
 };
 
+enum ll_merge_result {
+	LL_MERGE_ERROR = -1,
+	LL_MERGE_OK = 0,
+	LL_MERGE_CONFLICT,
+	LL_MERGE_BINARY_CONFLICT,
+};
+
 /**
  * Perform a three-way single-file merge in core.  This is a thin wrapper
  * around `xdl_merge` that takes the path and any merge backend specified in
  * `.gitattributes` or `.git/info/attributes` into account.
  * Returns 0 for a clean merge.
  */
-int ll_merge(mmbuffer_t *result_buf,
+enum ll_merge_result ll_merge(mmbuffer_t *result_buf,
 	     const char *path,
 	     mmfile_t *ancestor, const char *ancestor_label,
 	     mmfile_t *ours, const char *our_label,

index ee0a0e90c94..8138090f81c 100644
@@ -36,7 +36,7 @@ static void *three_way_filemerge(struct index_state *istate,
 				 mmfile_t *their,
 				 unsigned long *size)
 {
-	int merge_status;
+	enum ll_merge_result merge_status;
 	mmbuffer_t res;
 
 	/*

@@ -50,6 +50,9 @@ static void *three_way_filemerge(struct index_state *istate,
 				istate, NULL);
 	if (merge_status < 0)
 		return NULL;
+	if (merge_status == LL_MERGE_BINARY_CONFLICT)
+		warning("Cannot merge binary files: %s (%s vs. %s)",
+			path, ".our", ".their");
 
 	*size = res.size;
 	return res.ptr;

index 0342f104836..c24da2ba3cb 100644
@@ -1743,7 +1743,7 @@ static int merge_3way(struct merge_options *opt,
 	mmfile_t orig, src1, src2;
 	struct ll_merge_options ll_opts = {0};
 	char *base, *name1, *name2;
-	int merge_status;
+	enum ll_merge_result merge_status;
 
 	if (!opt->priv->attr_index.initialized)
 		initialize_attr_index(opt);

@@ -1787,6 +1787,9 @@ static int merge_3way(struct merge_options *opt,
 	merge_status = ll_merge(result_buf, path, &orig, base,
 				&src1, name1, &src2, name2,
 				&opt->priv->attr_index, &ll_opts);
+	if (merge_status == LL_MERGE_BINARY_CONFLICT)
+		warning("Cannot merge binary files: %s (%s vs. %s)",
+			path, name1, name2);
 
 	free(base);
 	free(name1);

index d9457797dbb..bc73c52dd84 100644
@@ -1044,7 +1044,7 @@ static int merge_3way(struct merge_options *opt,
 	mmfile_t orig, src1, src2;
 	struct ll_merge_options ll_opts = {0};
 	char *base, *name1, *name2;
-	int merge_status;
+	enum ll_merge_result merge_status;
 
 	ll_opts.renormalize = opt->renormalize;
 	ll_opts.extra_marker_size = extra_marker_size;

@@ -1090,6 +1090,9 @@ static int merge_3way(struct merge_options *opt,
 	merge_status = ll_merge(result_buf, a->path, &orig, base,
 				&src1, name1, &src2, name2,
 				opt->repo->index, &ll_opts);
+	if (merge_status == LL_MERGE_BINARY_CONFLICT)
+		warning("Cannot merge binary files: %s (%s vs. %s)",
+			a->path, name1, name2);
 
 	free(base);
 	free(name1);

index b4a3a903e86..01d596920ea 100644
@@ -344,7 +344,7 @@ static int ll_merge_in_worktree(struct notes_merge_options *o,
 {
 	mmbuffer_t result_buf;
 	mmfile_t base, local, remote;
-	int status;
+	enum ll_merge_result status;
 
 	read_mmblob(&base, &p->base);
 	read_mmblob(&local, &p->local);

@@ -358,6 +358,9 @@ static int ll_merge_in_worktree(struct notes_merge_options *o,
 	free(local.ptr);
 	free(remote.ptr);
 
+	if (status == LL_MERGE_BINARY_CONFLICT)
+		warning("Cannot merge binary files: %s (%s vs. %s)",
+			oid_to_hex(&p->obj), o->local_ref, o->remote_ref);
 	if ((status < 0) || !result_buf.ptr)
 		die("Failed to execute internal merge");
 

index d83d58df4fb..46fd01819b8 100644
@@ -609,19 +609,23 @@ static int try_merge(struct index_state *istate,
 		     const struct rerere_id *id, const char *path,
 		     mmfile_t *cur, mmbuffer_t *result)
 {
-	int ret;
+	enum ll_merge_result ret;
 	mmfile_t base = {NULL, 0}, other = {NULL, 0};
 
 	if (read_mmfile(&base, rerere_path(id, "preimage")) ||
 	    read_mmfile(&other, rerere_path(id, "postimage")))
-		ret = 1;
-	else
+		ret = LL_MERGE_CONFLICT;
+	else {
 		/*
 		 * A three-way merge. Note that this honors user-customizable
 		 * low-level merge driver settings.
 		 */
 		ret = ll_merge(result, path, &base, NULL, cur, "", &other, "",
 			       istate, NULL);
+		if (ret == LL_MERGE_BINARY_CONFLICT)
+			warning("Cannot merge binary files: %s (%s vs. %s)",
+				path, "", "");
+	}
 
 	free(base.ptr);
 	free(other.ptr);

gitgitgadget

From: Elijah Newren <[email protected]>

Instead of immediately printing ll-merge warnings to stderr, we save them in our output strbuf. Besides allowing us to move these warnings to a special file for --remerge-diff, this has two other benefits for regular merges done by merge-ort:

* The deferral of messages ensures we can print all messages about any given path together (merge-recursive was known to sometimes intersperse messages about other paths, particularly when renames were involved).

* The deferral of messages means we can avoid printing spurious conflict messages when we just end up aborting due to local user modifications in the way. (In contrast to merge-recursive.c which prematurely checks for local modifications in the way via unpack_trees() and gets the check wrong both in terms of false positives and false negatives relative to renames, merge-ort does not perform the local modifications in the way check until the checkout() step after the full merge has been computed.)

---
 merge-ort.c                | 5 +++--
 t/t6404-recursive-merge.sh | 9 +++++++--
 t/t6406-merge-attr.sh      | 9 +++++++--
 3 files changed, 17 insertions(+), 6 deletions(-)

index c24da2ba3cb..a18f47e23c5 100644
@@ -1788,8 +1788,9 @@ static int merge_3way(struct merge_options *opt,
 				&src1, name1, &src2, name2,
 				&opt->priv->attr_index, &ll_opts);
 	if (merge_status == LL_MERGE_BINARY_CONFLICT)
-		warning("Cannot merge binary files: %s (%s vs. %s)",
-			path, name1, name2);
+		path_msg(opt, path, 0,
+			 "warning: Cannot merge binary files: %s (%s vs. %s)",
+			 path, name1, name2);
 
 	free(base);
 	free(name1);

index eaf48e941e2..b8735c6db4d 100755
@@ -108,8 +108,13 @@ test_expect_success 'refuse to merge binary files' '
 	printf "\0\0" >binary-file &&
 	git add binary-file &&
 	git commit -m binary2 &&
-	test_must_fail git merge F >merge.out 2>merge.err &&
-	grep "Cannot merge binary files: binary-file (HEAD vs. F)" merge.err
+	if test "$GIT_TEST_MERGE_ALGORITHM" = ort
+	then
+		test_must_fail git merge F >merge_output
+	else
+		test_must_fail git merge F 2>merge_output
+	fi &&
+	grep "Cannot merge binary files: binary-file (HEAD vs. F)" merge_output
 '
 
 test_expect_success 'mark rename/delete as unmerged' '

index 84946458371..c41584eb33e 100755
@@ -221,8 +221,13 @@ test_expect_success 'binary files with union attribute' '
 	printf "two\0" >bin.txt &&
 	git commit -am two &&
 
-	test_must_fail git merge bin-main 2>stderr &&
-	grep -i "warning.*cannot merge.*HEAD vs. bin-main" stderr
+	if test "$GIT_TEST_MERGE_ALGORITHM" = ort
+	then
+		test_must_fail git merge bin-main >output
+	else
+		test_must_fail git merge bin-main 2>output
+	fi &&
+	grep -i "warning.*cannot merge.*HEAD vs. bin-main" output
 '
 
 test_done

gitgitgadget

From: Elijah Newren <[email protected]>

path_msg() has the ability to mark messages as omittable, designed for remerge-diff where we'll instead be showing conflict messages as diff headers for a subsequent diff. While all these messages are very useful when trying to create a merge initially, early use with the --remerge-diff feature (the only user of this omittable conflict message capability), suggests that the particular messages marked in this commit are just noise when trying to see what changes users made to create a merge commit. Mark them as omittable.

---
 merge-ort.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

index a18f47e23c5..fe27870e73e 100644
@@ -2420,7 +2420,7 @@ static void apply_directory_rename_modifications(struct merge_options *opt,
 		 */
 		ci->path_conflict = 1;
 		if (pair->status == 'A')
-			path_msg(opt, new_path, 0,
+			path_msg(opt, new_path, 1,
 				 _("CONFLICT (file location): %s added in %s "
 				   "inside a directory that was renamed in %s, "
 				   "suggesting it should perhaps be moved to "

@@ -2428,7 +2428,7 @@ static void apply_directory_rename_modifications(struct merge_options *opt,
 				 old_path, branch_with_new_path,
 				 branch_with_dir_rename, new_path);
 		else
-			path_msg(opt, new_path, 0,
+			path_msg(opt, new_path, 1,
 				 _("CONFLICT (file location): %s renamed to %s "
 				   "in %s, inside a directory that was renamed "
 				   "in %s, suggesting it should perhaps be "

@@ -3825,7 +3825,7 @@ static void process_entry(struct merge_options *opt,
 				reason = _("add/add");
 			if (S_ISGITLINK(merged_file.mode))
 				reason = _("submodule");
-			path_msg(opt, path, 0,
+			path_msg(opt, path, 1,
 				 _("CONFLICT (%s): Merge conflict in %s"),
 				 reason, path);
 		}

gitgitgadget

From: Elijah Newren <[email protected]>

merge-ort is designed to be more flexible so that it could be called as more of a library function. Part of that design is not writing to the working tree or index unless and until requested. Part of it is returning tree objects (rather than creating commits and making them part of HEAD), and allowing callers to do their own special thing with that merged tree. Along the same lines, we want to enable callers to do something special with output messages (conflicts and other warnings) besides just automatically displaying on stdout/stderr. Do so by making the output path messages accessible via a new member of struct merge_result named path_messages.

---
 merge-ort.c |  1 +
 merge-ort.h | 10 ++++++++++
 2 files changed, 11 insertions(+)

index fe27870e73e..c4d6c5c81cc 100644
@@ -4547,6 +4547,7 @@ redo:
 	trace2_region_leave("merge", "process_entries", opt->repo);
 
 	/* Set return values */
+	result->path_messages = &opt->priv->output;
 	result->tree = parse_tree_indirect(&working_tree_oid);
 	/* existence of conflicted entries implies unclean */
 	result->clean &= strmap_empty(&opt->priv->conflicted);

index c011864ffeb..fe599b87868 100644
@@ -5,6 +5,7 @@
 
 struct commit;
 struct tree;
+struct strmap;
 
 struct merge_result {
 	/*

@@ -23,6 +24,15 @@ struct merge_result {
 	 */
 	struct tree *tree;
 
+	/*
+	 * Special messages and conflict notices for various paths
+	 *
+	 * This is a map of pathnames to strbufs.  It contains various
+	 * warning/conflict/notice messages (possibly multiple per path)
+	 * that callers may want to use.
+	 */
+	struct strmap *path_messages;
+
 	/*
 	 * Additional metadata used by merge_switch_to_result() or future calls
 	 * to merge_incore_*().  Includes data needed to update the index (if

gitgitgadget

From: Elijah Newren <[email protected]>

In support of a remerge-diff ability we will add in a few commits, we want to be able to provide additional headers to show along with a diff. Add the plumbing necessary to enable this.

---
 diff.c | 34 +++++++++++++++++++++++++++++++++-
 diff.h |  1 +
 2 files changed, 34 insertions(+), 1 deletion(-)

index 861282db1c3..a9490b9b2ba 100644
@@ -27,6 +27,7 @@
 #include "help.h"
 #include "promisor-remote.h"
 #include "dir.h"
+#include "strmap.h"
 
 #ifdef NO_FAST_WORKING_DIRECTORY
 #define FAST_WORKING_DIRECTORY 0

@@ -3406,6 +3407,33 @@ struct userdiff_driver *get_textconv(struct repository *r,
 	return userdiff_get_textconv(r, one->driver);
 }
 
+static struct strbuf* additional_headers(struct diff_options *o,
+					 const char *path)
+{
+	if (!o->additional_path_headers)
+		return NULL;
+	return strmap_get(o->additional_path_headers, path);
+}
+
+static void add_formatted_headers(struct strbuf *msg,
+				  struct strbuf *more_headers,
+				  const char *line_prefix,
+				  const char *meta,
+				  const char *reset)
+{
+	char *next, *newline;
+
+	next = more_headers->buf;
+	while ((newline = strchr(next, '\n'))) {
+		*newline = '\0';
+		strbuf_addf(msg, "%s%s%s%s\n", line_prefix, meta, next, reset);
+		*newline = '\n';
+		next = newline + 1;
+	}
+	if (*next)
+		strbuf_addf(msg, "%s%s%s%s\n", line_prefix, meta, next, reset);
+}
+
 static void builtin_diff(const char *name_a,
 			 const char *name_b,
 			 struct diff_filespec *one,

@@ -4328,9 +4356,13 @@ static void fill_metainfo(struct strbuf *msg,
 	const char *set = diff_get_color(use_color, DIFF_METAINFO);
 	const char *reset = diff_get_color(use_color, DIFF_RESET);
 	const char *line_prefix = diff_line_prefix(o);
+	struct strbuf *more_headers = NULL;
 
 	*must_show_header = 1;
 	strbuf_init(msg, PATH_MAX * 2 + 300);
+	if ((more_headers = additional_headers(o, name)))
+		add_formatted_headers(msg, more_headers,
+				      line_prefix, set, reset);
 	switch (p->status) {
 	case DIFF_STATUS_COPIED:
 		strbuf_addf(msg, "%s%ssimilarity index %d%%",

@@ -5852,7 +5884,7 @@ int diff_unmodified_pair(struct diff_filepair *p)
 
 static void diff_flush_patch(struct diff_filepair *p, struct diff_options *o)
 {
-	if (diff_unmodified_pair(p))
+	if (diff_unmodified_pair(p) && !additional_headers(o, p->one->path))
 		return;
 
 	if ((DIFF_FILE_VALID(p->one) && S_ISDIR(p->one->mode)) ||

index 8ba85c5e605..289badf5643 100644
@@ -395,6 +395,7 @@ struct diff_options {
 
 	struct repository *repo;
 	struct option *parseopts;
+	struct strmap *additional_path_headers;
 
 	int no_free;
 };

gitgitgadget

From: Elijah Newren <[email protected]>

We want to add an ability for users to run

    git show --remerge-diff $MERGE_COMMIT

or even
git log -p --remerge-diff ...
and have git show the differences between where the merge machinery would stop and what is recorded in merge commits. However, in such cases, stdout is not an appropriate location to dump conflict messages. We instead want these messages to appear as headers in the subsequent diff. For them to work as headers, though, we need for any multiline messages to replace newlines with both a newline and a space. Add a new flag to signal when we want these messages modified in such a fashion, and use it in path_msg() to modify these messages this way.

---
 merge-ort.c       | 36 ++++++++++++++++++++++++++++++++++--
 merge-recursive.c |  3 +++
 merge-recursive.h |  1 +
 3 files changed, 38 insertions(+), 2 deletions(-)

index c4d6c5c81cc..0ae3e4ffa75 100644
@@ -634,17 +634,46 @@ static void path_msg(struct merge_options *opt,
 		     const char *fmt, ...)
 {
 	va_list ap;
-	struct strbuf *sb = strmap_get(&opt->priv->output, path);
+	struct strbuf *sb, *dest;
+	struct strbuf tmp = STRBUF_INIT;
+
+	if (opt->record_conflict_msgs_as_headers && omittable_hint)
+		return; /* Do not record mere hints in tree */
+	sb = strmap_get(&opt->priv->output, path);
 	if (!sb) {
 		sb = xmalloc(sizeof(*sb));
 		strbuf_init(sb, 0);
 		strmap_put(&opt->priv->output, path, sb);
 	}
 
+	dest = (opt->record_conflict_msgs_as_headers ? &tmp : sb);
+
 	va_start(ap, fmt);
-	strbuf_vaddf(sb, fmt, ap);
+	strbuf_vaddf(dest, fmt, ap);
 	va_end(ap);
 
+	if (opt->record_conflict_msgs_as_headers) {
+		int i_sb = 0, i_tmp = 0;
+
+		/* Copy tmp to sb, adding spaces after newlines */
+		strbuf_grow(sb, 2*tmp.len); /* more than sufficient */
+		for (; i_tmp < tmp.len; i_tmp++, i_sb++) {
+			/* Copy next character from tmp to sb */
+			sb->buf[sb->len + i_sb] = tmp.buf[i_tmp];
+
+			/* If we copied a newline, add a space */
+			if (tmp.buf[i_tmp] == '\n')
+				sb->buf[++i_sb] = ' ';
+		}
+		/* Update length and ensure it's NUL-terminated */
+		sb->len += i_sb;
+		sb->buf[sb->len] = '\0';
+
+		/* Clean up tmp */
+		strbuf_release(&tmp);
+	}
+
+	/* Add final newline character to sb */
 	strbuf_addch(sb, '\n');
 }
 

@@ -4246,6 +4275,9 @@ void merge_switch_to_result(struct merge_options *opt,
 		struct string_list olist = STRING_LIST_INIT_NODUP;
 		int i;
 
+		if (opt->record_conflict_msgs_as_headers)
+			BUG("Either display conflict messages or record them as headers, not both");
+
 		trace2_region_enter("merge", "display messages", opt->repo);
 
 		/* Hack to pre-allocate olist to the desired size */

index bc73c52dd84..c9ba7e904a6 100644
@@ -3714,6 +3714,9 @@ static int merge_start(struct merge_options *opt, struct tree *head)
 
 	assert(opt->priv == NULL);
 
+	/* Not supported; option specific to merge-ort */
+	assert(!opt->record_conflict_msgs_as_headers);
+
 	/* Sanity check on repo state; index must match head */
 	if (repo_index_has_changes(opt->repo, head, &sb)) {
 		err(opt, _("Your local changes to the following files would be overwritten by merge:\n  %s"),

index 0795a1d3ec1..ebfdb7f994e 100644
@@ -46,6 +46,7 @@ struct merge_options {
 	/* miscellaneous control options */
 	const char *subtree_shift;
 	unsigned renormalize : 1;
+	unsigned record_conflict_msgs_as_headers : 1;
 
 	/* internal fields used by the implementation */
 	struct merge_options_internal *priv;

gitgitgadget

From: Elijah Newren <[email protected]>

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, so that any blobs or trees created during the automatic merge, can be easily removed afterwards by just deleting all objects from the temporary object directory. We can do this after handling each merge commit, in order to avoid the need to worry about doing git gc --auto runs while running git log --remerge-diff.

---
 builtin/log.c | 16 ++++++++++++
 diff-merges.c | 12 +++++++++
 log-tree.c    | 70 +++++++++++++++++++++++++++++++++++++++++++++++++++
 revision.h    |  6 ++++-
 4 files changed, 103 insertions(+), 1 deletion(-)

index f75d87e8d7f..2b51d8b6aae 100644
@@ -35,6 +35,8 @@
 #include "repository.h"
 #include "commit-reach.h"
 #include "range-diff.h"
+#include "dir.h"
+#include "tmp-objdir.h"
 
 #define MAIL_DEFAULT_WRAP 72
 #define COVER_FROM_AUTO_MAX_SUBJECT_LEN 100

@@ -407,6 +409,13 @@ static int cmd_log_walk(struct rev_info *rev)
 	int saved_nrl = 0;
 	int saved_dcctc = 0;
 
+	if (rev->remerge_diff) {
+		rev->remerge_objdir = tmp_objdir_create("remerge-diff");
+		if (!rev->remerge_objdir)
+			die(_("unable to create temporary object directory"));
+		tmp_objdir_replace_primary_odb(rev->remerge_objdir, 1);
+	}
+
 	if (rev->early_output)
 		setup_early_output();
 

@@ -449,6 +458,11 @@ 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(rev->remerge_objdir);
+		rev->remerge_objdir = NULL;
+	}
+
 	if (rev->diffopt.output_format & DIFF_FORMAT_CHECKDIFF &&
 	    rev->diffopt.flags.check_failed) {
 		return 02;

@@ -1943,6 +1957,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 ||

index 5060ccd890b..0af4b3f9191 100644
@@ -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")) {

index 5578bb4720a..44efce3f410 100644
@@ -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;

@@ -317,6 +318,9 @@ struct rev_info {
 
 	/* misc. flags related to '--no-kept-objects' */
 	unsigned keep_pack_cache_flags;
+
+	/* Location where temporary objects for remerge-diff are written. */
+	struct tmp_objdir *remerge_objdir;
 };
 
 int ref_excluded(struct string_list *, const char *path);

gitgitgadget

From: Elijah Newren <[email protected]>

---
 Documentation/diff-options.txt | 8 ++++++++
 1 file changed, 8 insertions(+)

index c89d530d3d1..b05f1c9f1c9 100644
@@ -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:::
+	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.
++
 --diff-merges=combined:::
 --diff-merges=c:::
 -c:::

gitgitgadget

Yes, but I'm not introducing a new message here; I'm merely moving an existing one. It's important to me that readers of this patch be able to verify that I have made no functional changes in this patch, so fixing the case should definitely be a different patch from this one. I kind of think that fixing the case distracts a bit from the point of the series, and the series is already kind of long, but do you feel strongly that I should fix the case with a new patch inserted into the series?

I can switch to die_errno().

Indeed, thanks.

Sure, I can make that change.

Yes, but this groups all the ancestors so nicely as opposed to the typical happenstance of whatever fits on a line. ;-)

Ok, will change.

I presume you mean the "main" patch for this one is 8/9. I was trying to find a way to break up that large patch, but this is pretty small so...sure I'll squash it in.

What are you referring to as the "main" patch for 5/9, though? It only seems related to 6/9 and 7/9 to me, but I very deliberately split those patches off and don't want to confuse them with unrelated changes. I disagree with combining 5/9 with either of those.

I just missed the bit where it was moved from below in the diff. Sorry about the noise.

Nah, no worries; thanks for taking a look at the patches!

Oh, that's bad. I wish people do not rebase their updates on top of newer 'master' only for the sake of it, once an older version is queued.

I wondered if we want to do the same for other error messages to give callers greater control, but this change by itself already looks quite good.

OK.

On that single patch?

Yeah, the main patch near the end that changed 4 files; he acked the changes to just one of those files in that patch.

This isn't just a convenience; since tmp_objdir is defined in tmp-objdir.c rather than tmp-objdir.h, t->path is not accessible outside of tmp-objdir.c. Because of this, some kind of helper function is necessary.

I would imagine that with something like this, we can show such a warning message differently when it happens during an inner "synthesizing a virtual common ancestor" merge (the most likely value for "show differently" would be to "squelch"), which may be a good thing.

It is likely because when somebody is looking at the output of remerge-diff, they are mostly concentrating on the _content_ level merges and they are not keenly looking for a merge whose result is deposited at a wrong path. Since what is shown is something that has already recorded in the history, we can safely assume that it is no longer a relevant (or "it is way too late to matter"), I would say, to show these messages about "file location".

It is somewhat disappointing that there is no test or documentation update that show how a typical remerge-diff output should look like. I was specifically interested in finding out how the "conflict hint messages from merge backend in the diff header" output would look.

I left some messages here and there on the patches I read carefully, and they looked mostly good. I only skimmed 7 and 8 and did not find anything glaringly wrong, but that wouldn't count as a review.

Thanks.

I just gave it a quick initial skim.

I have sometimes found it a bit harder to review your patches due to over-splitting.

E.g. (went back and looked) here tmp_objdir_discard_objects() is introduced in 1/9 but used in 8/9. "path_messages" is then introduced in 5/9 and used in 8/9, no?

Anyway, just a bit of feedback. FWIW not just bikeshedding. I do find myself stopping at 1/9, paging to 2/9, searching for the function, not there, checking 3/9 etc.

I realize this is a bit of a stones & glass houses comment, but I find it a bit easier to review things when a patch is larger v.s. having it split up in a way where preceding steps don't do anything yet except wait for use by a subsequent patch.

0.02 etc.

But adding this function as an extra level of abstration is *not* the only way to expose the feature. Instead the internal of "struct tmp_objdir" could be exposed to the caller that wants to discard the files inside the path.

I think we now have enough material to fill between these two lines to help readers ;-)

Thanks.

Oh, 8/9. That one could make sense.

And thanks for the feedback. Perhaps I could restructure this series with a top-down design instead of bottom-up. Doing that would mean either adding functions with an instant-die() implementation in the first step or just leaving a placeholder comment, and then filling those things in for later steps.

Ah, indeed; thanks for reading carefully.

Will fix.

Yes, that is a possibility that opens up after this. Which reminds me, merge-recursive nicely nested conflict/warnings messages from inner merges by adding 2*call_depth space characters before messages. I lost that in merge-ort (which becomes more problematic since merge-ort tries to group messages about the same path together, thus mixing inner merge messages with outer ones and providing no way to differentiate the two). I've got a patch to fix that up, but of course it conflicts with this series, so I'll be submitting it after this one settles.

Yeah, I'm not so sure about it either, and my notes are long, long gone. I think I'll pull this one out, and we can always tweak it later if needed.

Ah, yes, we talked about that during the tmp-objdir discussion back in September/October. Peff didn't want struct tmp_objdir exposed, and I was operating with that in mind.

I've restructured the series a bit based on Ævar's feedback, and this function is now only introduced along with its caller. Hopefully that makes it a bit clearer.

That's a fair question. Most of the involved messages are of the form
CONFLICT (<reason>): more details
and "CONFLICT" seems like a pretty identifiable string. There are some others, which made me wonder if we wanted some kind of additional prefix, but I was having a hard time coming up with a meaningful prefix; most that I thought of didn't seem like they'd help.

I've provided some testcases in the next re-roll so you can see some examples; maybe that will help others judge if a prefix is needed and spur creative juices for coming up with a good once since I seem to be unable to.

I've added a comment.

From: Elijah Newren <[email protected]>

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 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
* 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.

---
 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

index c89d530d3d1..b05f1c9f1c9 100644
@@ -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:::
+	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.
++
 --diff-merges=combined:::
 --diff-merges=c:::
 -c:::

index f75d87e8d7f..d053418fddd 100644
@@ -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 ||

index 5060ccd890b..0af4b3f9191 100644
@@ -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")) {

index 5578bb4720a..13178e6b8f3 100644
@@ -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;

gitgitgadget

Here are some patches to add a --remerge-diff capability to show & log, which works by comparing merge commits to an automatic remerge (note that the automatic remerge tree can contain files with conflict markers).

Changes since v1 (of the restarted submission, which technically was v2):

* Restructured the series, so the first patch introduces the feature -- with a bunch of caveats. Subsequent patches clean up those caveats. This avoids introducing not-yet-used functions, and hopefully makes review easier.
* added testcases
* numerous small improvements suggested by Ævar and Junio

Changes since original submission[1]:

* Rebased on top of the version of ns/tmp-objdir that Neeraj submitted (Neeraj's patches were based on v2.34, but ns/tmp-objdir got applied on an old commit and does not even build because of that).
* Modify ll-merge API to return a status, instead of printing "Cannot merge binary files" on stdout[2] (as suggested by Peff)
* Make conflict messages and other such warnings into diff headers of the subsequent remerge-diff rather than appearing in the diff as file content of some funny looking filenames (as suggested by Peff[3] and Junio[4])
* Sergey ack'ed the diff-merges.c portion of the patches, but that wasn't limited to one patch so not sure where to record that ack.

[1]
https://lore.kernel.org/git/[email protected]/;
GitHub wouldn't let me change the target branch for the PR, so I had to create a new one with the new base and thus the reason for not sending this as v2 even though it is. [2]
https://lore.kernel.org/git/[email protected]/,
https://lore.kernel.org/git/[email protected]/ [3]
https://lore.kernel.org/git/[email protected]/ [4]
https://lore.kernel.org/git/[email protected]/

=== FURTHER BACKGROUND (original cover letter material) ==

Here are some example commits you can try this out on (with git show --remerge-diff $COMMIT):

* git.git conflicted merge: 07601b5b36
* git.git non-conflicted change: bf04590ecd
* linux.git conflicted merge: eab3540562fb
* linux.git non-conflicted change: 223cea6a4f05

Many more can be found by just running git log --merges --remerge-diff in your repository of choice and searching for diffs (most merges tend to be clean and unmodified and thus produce no diff but a search of '^diff' in the log output tends to find the examples nicely).

Some basic high level details about this new option:

* This option is most naturally compared to --cc, though the output seems to be much more understandable to most users than --cc output.
* Since merges are often clean and unmodified, this new option results in an empty diff for most merges.
* This new option shows things like the removal of conflict markers, which hunks users picked from the various conflicted sides to keep or remove, and shows changes made outside of conflict markers (which might reflect changes needed to resolve semantic conflicts or cleanups of e.g. compilation warnings or other additional changes an integrator felt belonged in the merged result).
* This new option does not (currently) work for octopus merges, since merge-ort is specific to two-parent merges[1].
* This option will not work on a read-only or full filesystem[2].
* We discussed this capability at Git Merge 2020, and one of the suggestions was doing a periodic git gc --auto during the operation (due to potential new blobs and trees created during the operation). I found a way to avoid that; see [2].
* This option is faster than you'd probably expect; it handles 33.5 merge commits per second in linux.git on my computer; see below.

In regards to the performance point above, the timing for running the following command:

time git log --min-parents=2 --max-parents=2 $DIFF_FLAG | wc -l

in linux.git (with v5.4 checked out, since my copy of linux is very out of date) is as follows:

DIFF_FLAG=--cc:            71m 31.536s
DIFF_FLAG=--remerge-diff:  31m  3.170s

Note that there are 62476 merges in this history. Also, output size is:

DIFF_FLAG=--cc:            2169111 lines
DIFF_FLAG=--remerge-diff:  2458020 lines

So roughly the same amount of output as --cc, as you'd expect.

As a side note: git log --remerge-diff, when run in various repositories and allowed to run all the way back to the beginning(s) of history, is a nice stress test of sorts for merge-ort. Especially when users run it for you on their repositories they are working on, whether intentionally or via a bug in a tool triggering that command to be run unexpectedly. Long story short, such a bug in an internal tool existed last December and this command was run on an internal repository and found a platform-specific bug in merge-ort on some really old merge commit from that repo. I fixed that bug (a STABLE_QSORT thing) while upstreaming all the merge-ort patches in the mean time, but it was nice getting extra testing. Having more folks run this on their repositories might be useful extra testing of the new merge strategy.

Also, I previously mentioned --remerge-diff-only (a flag to show how cherry-picks or reverts differ from an automatic cherry-pick or revert, in addition to showing how merges differ from an automatic merge). This series does not include the patches to introduce that option; I'll submit them later.

Two other things that might be interesting but are not included and which I haven't investigated:

* some mechanism for passing extra merge options through (e.g. -Xignore-space-change)
* a capability to compare the automatic merge to a second automatic merge done with different merge options. (Not sure if this would be of interest to end users, but might be interesting while developing new a --strategy-option, or maybe checking how changing some default in the merge algorithm would affect historical merges in various repositories).

[1] I have nebulous ideas of how an Octopus-centric ORT strategy could be written -- basically, just repeatedly invoking ort and trying to make sure nested conflicts can be differentiated. For now, though, a simple warning is printed that octopus merges are not handled and no diff will be shown. [2] New blobs/trees can be written by the three-way merging step. These are written to a temporary area (via tmp-objdir.c) under the git object store that is cleaned up at the end of the operation, with the new loose objects from the remerge being cleaned up after each individual merge.

Elijah Newren (8):
show, log: provide a --remerge-diff capability
log: clean unneeded objects during log --remerge-diff
ll-merge: make callers responsible for showing warnings
merge-ort: capture and print ll-merge warnings in our preferred
fashion
merge-ort: mark a few more conflict messages as omittable
merge-ort: format messages slightly different for use in headers
diff: add ability to insert additional headers for paths show, log: include conflict/warning messages in --remerge-diff headers

 Documentation/diff-options.txt |   8 ++
 apply.c                        |   5 +-
 builtin/checkout.c             |  12 ++-
 builtin/log.c                  |  15 +++
 diff-merges.c                  |  12 +++
 diff.c                         | 116 +++++++++++++++++++++-
 diff.h                         |   3 +-
 ll-merge.c                     |  40 ++++----
 ll-merge.h                     |   9 +-
 log-tree.c                     |  70 +++++++++++++-
 merge-blobs.c                  |   5 +-
 merge-ort.c                    |  47 ++++++++-
 merge-ort.h                    |  10 ++
 merge-recursive.c              |   8 +-
 merge-recursive.h              |   1 +
 notes-merge.c                  |   5 +-
 rerere.c                       |  12 ++-
 revision.h                     |   6 +-
 t/t4069-remerge-diff.sh        | 172 +++++++++++++++++++++++++++++++++
 t/t6404-recursive-merge.sh     |   9 +-
 t/t6406-merge-attr.sh          |   9 +-
 tmp-objdir.c                   |   5 +
 tmp-objdir.h                   |   6 ++
 23 files changed, 538 insertions(+), 47 deletions(-)
 create mode 100755 t/t4069-remerge-diff.sh

base-commit: 4e44121c2d7bced65e25eb7ec5156290132bec94
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1103%2Fnewren%2Fremerge-diff-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1103/newren/remerge-diff-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1103

Range-diff vs v1:

8: 5d5846be0bd ! 1: b3ae62083e1 show, log: provide a --remerge-diff capability

     @@ Commit message
          possibly, just to hide other random changes).
      
          This capability works by creating a temporary object directory and
     -    marking it as the primary object store, so that any blobs or trees
     -    created during the automatic merge, can be easily removed afterwards by
     -    just deleting all objects from the temporary object directory.  We can
     -    do this after handling each merge commit, in order to avoid the need to
     -    worry about doing `git gc --auto` runs while running `git log
     -    --remerge-diff`.
     +    marking it as the primary object store.  This makes it so that any blobs
     +    or trees created during the automatic merge easily removable afterwards
     +    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
     +      * 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 <[email protected]>
      
     + ## Documentation/diff-options.txt ##
     +@@ Documentation/diff-options.txt: 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:::
     ++	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.
     +++
     + --diff-merges=combined:::
     + --diff-merges=c:::
     + -c:::
     +
       ## builtin/log.c ##
      @@
       #include "repository.h"
       #include "commit-reach.h"
       #include "range-diff.h"
     -+#include "dir.h"
      +#include "tmp-objdir.h"
       
       #define MAIL_DEFAULT_WRAP 72
       #define COVER_FROM_AUTO_MAX_SUBJECT_LEN 100
      @@ builtin/log.c: 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) {
     -+		rev->remerge_objdir = tmp_objdir_create("remerge-diff");
     -+		if (!rev->remerge_objdir)
     -+			die(_("unable to create temporary object directory"));
     -+		tmp_objdir_replace_primary_odb(rev->remerge_objdir, 1);
     ++		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();
     - 
      @@ builtin/log.c: 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(rev->remerge_objdir);
     -+		rev->remerge_objdir = NULL;
     -+	}
     ++	if (rev->remerge_diff)
     ++		tmp_objdir_destroy(remerge_objdir);
      +
       	if (rev->diffopt.output_format & DIFF_FORMAT_CHECKDIFF &&
       	    rev->diffopt.flags.check_failed) {
     @@ builtin/log.c: int cmd_format_patch(int argc, const char **argv, const char *pre
       	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"));
     ++		die(_("--remerge-diff does not make sense"));
       
       	if (!use_patch_format &&
       		(!rev.diffopt.output_format ||
     @@ log-tree.c
       #include "config.h"
       #include "diff.h"
       #include "object-store.h"
     - #include "repository.h"
     -+#include "tmp-objdir.h"
     - #include "commit.h"
     +@@
       #include "tag.h"
       #include "graph.h"
       #include "log-tree.h"
     @@ log-tree.c
       #include "reflog-walk.h"
       #include "refs.h"
       #include "string-list.h"
     -@@
     - #include "line-log.h"
     - #include "help.h"
     - #include "range-diff.h"
     -+#include "dir.h"
     - 
     - static struct decoration name_decoration = { "object names" };
     - static int decoration_loaded;
      @@ log-tree.c: static int do_diff_combined(struct rev_info *opt, struct commit *commit)
       	return !opt->loginfo;
       }
     @@ log-tree.c: static int do_diff_combined(struct rev_info *opt, struct commit *com
      +{
      +	struct merge_options o;
      +	struct commit_list *bases;
     -+	struct merge_result res;
     ++	struct merge_result res = {0};
      +	struct pretty_print_context ctx = {0};
     -+	struct strbuf commit1 = STRBUF_INIT;
     -+	struct strbuf commit2 = STRBUF_INIT;
     ++	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);
     -+	memset(&res, 0, sizeof(res));
      +	o.show_rename_progress = 0;
      +
      +	ctx.abbrev = DEFAULT_ABBREV;
     -+	format_commit_message(parents->item,       "%h (%s)", &commit1, &ctx);
     -+	format_commit_message(parents->next->item, "%h (%s)", &commit2, &ctx);
     -+	o.branch1 = commit1.buf;
     -+	o.branch2 = commit2.buf;
     -+	o.record_conflict_msgs_as_headers = 1;
     ++	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(parents->item);
     -+	parse_commit_or_die(parents->next->item);
     -+	bases = get_merge_bases(parents->item, parents->next->item);
     ++	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, parents->item, parents->next->item,
     -+			       &res);
     ++	merge_incore_recursive(&o, bases, parent1, parent2, &res);
      +
      +	/* Show the diff */
     -+	opt->diffopt.additional_path_headers = res.path_messages;
      +	diff_tree_oid(&res.tree->object.oid, oid, "", &opt->diffopt);
      +	log_tree_diff_flush(opt);
      +
      +	/* Cleanup */
     -+	opt->diffopt.additional_path_headers = NULL;
     -+	strbuf_release(&commit1);
     -+	strbuf_release(&commit2);
     ++	strbuf_release(&parent1_desc);
     ++	strbuf_release(&parent2_desc);
      +	merge_finalize(&o, &res);
     -+
     -+	/* Clean up the temporary object directory */
     -+	if (opt->remerge_objdir != NULL)
     -+		tmp_objdir_discard_objects(opt->remerge_objdir);
     -+	else
     -+		BUG("unable to remove temporary object directory");
     ++	/* TODO: clean up the temporary object directory */
      +
      +	return !opt->loginfo;
      +}
     @@ revision.h: struct rev_info {
       
       	/* Format info */
       	int		show_notes;
     -@@ revision.h: struct rev_info {
     - 
     - 	/* misc. flags related to '--no-kept-objects' */
     - 	unsigned keep_pack_cache_flags;
     +
     + ## t/t4069-remerge-diff.sh (new) ##
     +@@
     ++#!/bin/sh
      +
     -+	/* Location where temporary objects for remerge-diff are written. */
     -+	struct tmp_objdir *remerge_objdir;
     - };
     - 
     - int ref_excluded(struct string_list *, const char *path);
     ++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 &&
     ++	# 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
     ++	 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 &&
     ++
     ++	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

1: fab1b2c69ea ! 2: 54f1fb31d04 tmp_objdir: add a helper function for discarding all contained objects
@@ Metadata
Author: Elijah Newren <[email protected]>

## Commit message ##
- tmp_objdir: add a helper function for discarding all contained objects
+ log: clean unneeded objects during log --remerge-diff
+
+ The --remerge-diff option will need to create new blobs and trees
+ representing the "automatic merge" state. If one is traversing a
+ long project history, one can easily get hundreds of thousands of
+ loose objects generated during log --remerge-diff. However, none of
+ those loose objects are needed after we have completed our diff
+ operation; they can be summarily deleted.
+
+ Add a new helper function to tmp_objdir to discard all the contained
+ objects, and call it after each merge is handled.

+ ## builtin/log.c ##
+@@ builtin/log.c: 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)
++ rev->remerge_objdir = tmp_objdir_create("remerge-diff");
++ if (!rev->remerge_objdir)
+ die_errno(_("unable to create temporary object directory"));
+- tmp_objdir_replace_primary_odb(remerge_objdir, 1);
++ tmp_objdir_replace_primary_odb(rev->remerge_objdir, 1);
+ }
+
+ if (rev->early_output)
+@@ builtin/log.c: 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->remerge_diff) {
++ tmp_objdir_destroy(rev->remerge_objdir);
++ rev->remerge_objdir = NULL;
++ }
+
+ if (rev->diffopt.output_format & DIFF_FORMAT_CHECKDIFF &&
+ rev->diffopt.flags.check_failed) {
+
+ ## log-tree.c ##
+@@
+ #include "diff.h"
+ #include "object-store.h"
+ #include "repository.h"
++#include "tmp-objdir.h"
+ #include "commit.h"
+ #include "tag.h"
+ #include "graph.h"
+@@ log-tree.c: static int do_remerge_diff(struct rev_info *opt,
+ strbuf_release(&parent1_desc);
+ strbuf_release(&parent2_desc);
+ merge_finalize(&o, &res);
+- /* TODO: clean up the temporary object directory */
++
++ /* Clean up the contents of the temporary object directory */
++ if (opt->remerge_objdir)
++ tmp_objdir_discard_objects(opt->remerge_objdir);
++ else
++ BUG("unable to remove temporary object directory");
+
+ return !opt->loginfo;
+ }
+
+ ## revision.h ##
+@@ revision.h: struct rev_info {
+
+ /* misc. flags related to '--no-kept-objects' */
+ unsigned keep_pack_cache_flags;
++
++ /* Location where temporary objects for remerge-diff are written. */
++ struct tmp_objdir *remerge_objdir;
+ };
+
+ int ref_excluded(struct string_list *, const char *path);
+
## tmp-objdir.c ##
@@ tmp-objdir.c: static void remove_tmp_objdir_on_signal(int signo)
raise(signo);
2: d022176618d ! 3: d5566f5d136 ll-merge: make callers responsible for showing warnings

     @@ Commit message
          from ll-merge and instead modify the return status of ll_merge() to
          indicate when a merge of binary files has occurred.
      
     +    This commit continues printing the message as-is; future changes will
     +    start handling the new commit differently in the merge-ort codepath.
     +
          Note that my methodology included first modifying ll_merge() to return
          a struct, so that the compiler would catch all the callers for me and
          ensure I had modified all of them.  After modifying all of them, I then
     @@ apply.c: static int three_way_merge(struct apply_state *state,
       			  NULL);
      +	if (status == LL_MERGE_BINARY_CONFLICT)
      +		warning("Cannot merge binary files: %s (%s vs. %s)",
     -+			"base", "ours", "theirs");
     ++			path, "ours", "theirs");
       	free(base_file.ptr);
       	free(our_file.ptr);
       	free(their_file.ptr);
     @@ rerere.c: static int try_merge(struct index_state *istate,
       	mmfile_t base = {NULL, 0}, other = {NULL, 0};
       
       	if (read_mmfile(&base, rerere_path(id, "preimage")) ||
     - 	    read_mmfile(&other, rerere_path(id, "postimage")))
     +-	    read_mmfile(&other, rerere_path(id, "postimage")))
      -		ret = 1;
      -	else
     ++	    read_mmfile(&other, rerere_path(id, "postimage"))) {
      +		ret = LL_MERGE_CONFLICT;
     -+	else {
     ++	} else {
       		/*
       		 * A three-way merge. Note that this honors user-customizable
       		 * low-level merge driver settings.
  3:  f36395fdee0 =  4:  a02845f12db merge-ort: capture and print ll-merge warnings in our preferred fashion
  4:  1e7eef7705e !  5:  000933c5d7f merge-ort: mark a few more conflict messages as omittable

@@ Commit message
are just noise when trying to see what changes users made to create a merge commit. Mark them as omittable.

+ Note that there were already a few messages marked as omittable in
+ merge-ort when doing a remerge-diff, because the development of
+ --remerge-diff preceded the upstreaming of merge-ort and I was trying to
+ ensure merge-ort could handle all the necessary requirements. See
+ commit c5a6f65527 ("merge-ort: add modify/delete handling and delayed
+ output processing", 2020-12-03) for the initial details. For some
+ examples of already-marked-as-omittable messages, see either
+ "Auto-merging <path>" or some of the submodule update hints. This
+ commit just adds two more messages that should also be omittable.
+

## merge-ort.c ##
@@ merge-ort.c: static void apply_directory_rename_modifications(struct merge_optio
_("CONFLICT (file location): %s renamed to %s "
"in %s, inside a directory that was renamed "
"in %s, suggesting it should perhaps be "
-@@ merge-ort.c: static void process_entry(struct merge_options *opt,
- reason = _("add/add");
- if (S_ISGITLINK(merged_file.mode))
- reason = _("submodule");
-- path_msg(opt, path, 0,
-+ path_msg(opt, path, 1,
- _("CONFLICT (%s): Merge conflict in %s"),
- reason, path);
- }
7: b307f63569f ! 6: 887e46435c0 merge-ort: format messages slightly different for use in headers
@@ Metadata
## Commit message ##
merge-ort: format messages slightly different for use in headers

- We want to add an ability for users to run
+ When users run
git show --remerge-diff $MERGE_COMMIT
- or even
+ or
git log -p --remerge-diff ...
- and have git show the differences between where the merge machinery
- would stop and what is recorded in merge commits. However, in such
- cases, stdout is not an appropriate location to dump conflict messages.
- We instead want these messages to appear as headers in the subsequent
- diff. For them to work as headers, though, we need for any multiline
+ stdout is not an appropriate location to dump conflict messages, but we
+ do want to provide them to users. We will include them in the diff
+ headers instead...but for that to work, we need for any multiline
messages to replace newlines with both a newline and a space. Add a new flag to signal when we want these messages modified in such a fashion, and use it in path_msg() to modify these messages this way.
6: 15600df925f ! 7: e9470651303 diff: add ability to insert additional headers for paths

     @@ Metadata
       ## Commit message ##
          diff: add ability to insert additional headers for paths
      
     -    In support of a remerge-diff ability we will add in a few commits, we
     -    want to be able to provide additional headers to show along with a diff.
     -    Add the plumbing necessary to enable this.
     +    When additional headers are provided, we need to
     +      * add diff_filepairs to diff_queued_diff for each paths in the
     +        additional headers map which, unless that path is part of
     +        another diff_filepair already found in diff_queued_diff
     +      * format the headers (colorization, line_prefix for --graph)
     +      * make sure the various codepaths that attempt to return early
     +        if there are "no changes" take into account the headers that
     +        need to be shown.
      
          Signed-off-by: Elijah Newren <[email protected]>
      
     @@ diff.c: struct userdiff_driver *get_textconv(struct repository *r,
       	return userdiff_get_textconv(r, one->driver);
       }
       
     -+static struct strbuf* additional_headers(struct diff_options *o,
     ++static struct strbuf *additional_headers(struct diff_options *o,
      +					 const char *path)
      +{
      +	if (!o->additional_path_headers)
     @@ diff.c: struct userdiff_driver *get_textconv(struct repository *r,
      +{
      +	char *next, *newline;
      +
     -+	next = more_headers->buf;
     -+	while ((newline = strchr(next, '\n'))) {
     -+		*newline = '\0';
     -+		strbuf_addf(msg, "%s%s%s%s\n", line_prefix, meta, next, reset);
     -+		*newline = '\n';
     -+		next = newline + 1;
     ++	for (next = more_headers->buf; *next; next = newline) {
     ++		newline = strchrnul(next, '\n');
     ++		strbuf_addf(msg, "%s%s%.*s%s\n", line_prefix, meta,
     ++			    (int)(newline - next), next, reset);
     ++		if (*newline)
     ++			newline++;
      +	}
     -+	if (*next)
     -+		strbuf_addf(msg, "%s%s%s%s\n", line_prefix, meta, next, reset);
      +}
      +
       static void builtin_diff(const char *name_a,
       			 const char *name_b,
       			 struct diff_filespec *one,
     +@@ diff.c: static void builtin_diff(const char *name_a,
     + 	b_two = quote_two(b_prefix, name_b + (*name_b == '/'));
     + 	lbl[0] = DIFF_FILE_VALID(one) ? a_one : "/dev/null";
     + 	lbl[1] = DIFF_FILE_VALID(two) ? b_two : "/dev/null";
     ++	if (!DIFF_FILE_VALID(one) && !DIFF_FILE_VALID(two)) {
     ++		/*
     ++		 * We should only reach this point for pairs from
     ++		 * create_filepairs_for_header_only_notifications().  For
     ++		 * these, we should avoid the "/dev/null" special casing
     ++		 * above, meaning we avoid showing such pairs as either
     ++		 * "new file" or "deleted file" below.
     ++		 */
     ++		lbl[0] = a_one;
     ++		lbl[1] = b_two;
     ++	}
     + 	strbuf_addf(&header, "%s%sdiff --git %s %s%s\n", line_prefix, meta, a_one, b_two, reset);
     + 	if (lbl[0][0] == '/') {
     + 		/* /dev/null */
      @@ diff.c: static void fill_metainfo(struct strbuf *msg,
       	const char *set = diff_get_color(use_color, DIFF_METAINFO);
       	const char *reset = diff_get_color(use_color, DIFF_RESET);
     @@ diff.c: static void fill_metainfo(struct strbuf *msg,
       
       	*must_show_header = 1;
       	strbuf_init(msg, PATH_MAX * 2 + 300);
     -+	if ((more_headers = additional_headers(o, name)))
     +@@ diff.c: static void fill_metainfo(struct strbuf *msg,
     + 	default:
     + 		*must_show_header = 0;
     + 	}
     ++	if ((more_headers = additional_headers(o, name))) {
      +		add_formatted_headers(msg, more_headers,
      +				      line_prefix, set, reset);
     - 	switch (p->status) {
     - 	case DIFF_STATUS_COPIED:
     - 		strbuf_addf(msg, "%s%ssimilarity index %d%%",
     ++		*must_show_header = 1;
     ++	}
     + 	if (one && two && !oideq(&one->oid, &two->oid)) {
     + 		const unsigned hexsz = the_hash_algo->hexsz;
     + 		int abbrev = o->abbrev ? o->abbrev : DEFAULT_ABBREV;
      @@ diff.c: int diff_unmodified_pair(struct diff_filepair *p)
       
       static void diff_flush_patch(struct diff_filepair *p, struct diff_options *o)
       {
      -	if (diff_unmodified_pair(p))
     ++	/*
     ++	 * Check if we can return early without showing a diff.  Note that
     ++	 * diff_filepair only stores {oid, path, mode, is_valid}
     ++	 * information for each path, and thus diff_unmodified_pair() only
     ++	 * considers those bits of info.  However, we do not want pairs
     ++	 * created by create_filepairs_for_header_only_notifications() to
     ++	 * be ignored, so return early if both p is unmodified AND
     ++	 * p->one->path is not in additional headers.
     ++	 */
      +	if (diff_unmodified_pair(p) && !additional_headers(o, p->one->path))
       		return;
       
     ++	/* Actually, we can also return early to avoid showing tree diffs */
       	if ((DIFF_FILE_VALID(p->one) && S_ISDIR(p->one->mode)) ||
     + 	    (DIFF_FILE_VALID(p->two) && S_ISDIR(p->two->mode)))
     +-		return; /* no tree diffs in patch format */
     ++		return;
     + 
     + 	run_diff(p, o);
     + }
     +@@ diff.c: static void diff_flush_checkdiff(struct diff_filepair *p,
     + 	run_checkdiff(p, o);
     + }
     + 
     +-int diff_queue_is_empty(void)
     ++int diff_queue_is_empty(struct diff_options *o)
     + {
     + 	struct diff_queue_struct *q = &diff_queued_diff;
     + 	int i;
     ++
     ++	if (o->additional_path_headers &&
     ++	    !strmap_empty(o->additional_path_headers))
     ++		return 0;
     + 	for (i = 0; i < q->nr; i++)
     + 		if (!diff_unmodified_pair(q->queue[i]))
     + 			return 0;
     +@@ diff.c: void diff_warn_rename_limit(const char *varname, int needed, int degraded_cc)
     + 		warning(_(rename_limit_advice), varname, needed);
     + }
     + 
     ++static void create_filepairs_for_header_only_notifications(struct diff_options *o)
     ++{
     ++	struct strset present;
     ++	struct diff_queue_struct *q = &diff_queued_diff;
     ++	struct hashmap_iter iter;
     ++	struct strmap_entry *e;
     ++	int i;
     ++
     ++	strset_init_with_options(&present, /*pool*/ NULL, /*strdup*/ 0);
     ++
     ++	/*
     ++	 * Find out which paths exist in diff_queued_diff, preferring
     ++	 * one->path for any pair that has multiple paths.
     ++	 */
     ++	for (i = 0; i < q->nr; i++) {
     ++		struct diff_filepair *p = q->queue[i];
     ++		char *path = p->one->path ? p->one->path : p->two->path;
     ++
     ++		if (strmap_contains(o->additional_path_headers, path))
     ++			strset_add(&present, path);
     ++	}
     ++
     ++	/*
     ++	 * Loop over paths in additional_path_headers; for each NOT already
     ++	 * in diff_queued_diff, create a synthetic filepair and insert that
     ++	 * into diff_queued_diff.
     ++	 */
     ++	strmap_for_each_entry(o->additional_path_headers, &iter, e) {
     ++		if (!strset_contains(&present, e->key)) {
     ++			struct diff_filespec *one, *two;
     ++			struct diff_filepair *p;
     ++
     ++			one = alloc_filespec(e->key);
     ++			two = alloc_filespec(e->key);
     ++			fill_filespec(one, null_oid(), 0, 0);
     ++			fill_filespec(two, null_oid(), 0, 0);
     ++			p = diff_queue(q, one, two);
     ++			p->status = DIFF_STATUS_MODIFIED;
     ++		}
     ++	}
     ++
     ++	/* Re-sort the filepairs */
     ++	diffcore_fix_diff_index();
     ++
     ++	/* Cleanup */
     ++	strset_clear(&present);
     ++}
     ++
     + static void diff_flush_patch_all_file_pairs(struct diff_options *o)
     + {
     + 	int i;
     +@@ diff.c: static void diff_flush_patch_all_file_pairs(struct diff_options *o)
     + 	if (o->color_moved)
     + 		o->emitted_symbols = &esm;
     + 
     ++	if (o->additional_path_headers)
     ++		create_filepairs_for_header_only_notifications(o);
     ++
     + 	for (i = 0; i < q->nr; i++) {
     + 		struct diff_filepair *p = q->queue[i];
     + 		if (check_pair_status(p))
     +@@ diff.c: void diff_flush(struct diff_options *options)
     + 	 * Order: raw, stat, summary, patch
     + 	 * or:    name/name-status/checkdiff (other bits clear)
     + 	 */
     +-	if (!q->nr)
     ++	if (!q->nr && !options->additional_path_headers)
     + 		goto free_queue;
     + 
     + 	if (output_format & (DIFF_FORMAT_RAW |
      
       ## diff.h ##
      @@ diff.h: struct diff_options {
     @@ diff.h: struct diff_options {
       
       	int no_free;
       };
     +@@ diff.h: void diffcore_fix_diff_index(void);
     + "                show all files diff when -S is used and hit is found.\n" \
     + "  -a  --text    treat all files as text.\n"
     + 
     +-int diff_queue_is_empty(void);
     ++int diff_queue_is_empty(struct diff_options*);
     + void diff_flush(struct diff_options*);
     + void diff_free(struct diff_options*);
     + void diff_warn_rename_limit(const char *varname, int needed, int degraded_cc);
     +
     + ## log-tree.c ##
     +@@ log-tree.c: int log_tree_diff_flush(struct rev_info *opt)
     + 	opt->shown_dashes = 0;
     + 	diffcore_std(&opt->diffopt);
     + 
     +-	if (diff_queue_is_empty()) {
     ++	if (diff_queue_is_empty(&opt->diffopt)) {
     + 		int saved_fmt = opt->diffopt.output_format;
     + 		opt->diffopt.output_format = DIFF_FORMAT_NO_OUTPUT;
     + 		diff_flush(&opt->diffopt);
  5:  dd5461d45de !  8:  4cc53c55a6e merge-ort: make path_messages available to external callers
     @@ Metadata
      Author: Elijah Newren <[email protected]>
      
       ## Commit message ##
     -    merge-ort: make path_messages available to external callers
     +    show, log: include conflict/warning messages in --remerge-diff headers
      
     -    merge-ort is designed to be more flexible so that it could be called as
     -    more of a library function.  Part of that design is not writing to the
     -    working tree or index unless and until requested.  Part of it is
     -    returning tree objects (rather than creating commits and making them
     -    part of HEAD), and allowing callers to do their own special thing with
     -    that merged tree.  Along the same lines, we want to enable callers to do
     -    something special with output messages (conflicts and other warnings)
     -    besides just automatically displaying on stdout/stderr.  Do so by making
     -    the output path messages accessible via a new member of struct
     -    merge_result named path_messages.
     +    Conflicts such as modify/delete, rename/rename, or file/directory are
     +    not representable via content conflict markers, and the normal output
     +    messages notifying users about these were dropped with --remerge-diff.
     +    While we don't want these messages randomly shown before the commit
     +    and diff headers, we do want them to still be shown; include them as
     +    part of the diff headers instead.
      
          Signed-off-by: Elijah Newren <[email protected]>
      
     + ## log-tree.c ##
     +@@ log-tree.c: static int do_remerge_diff(struct rev_info *opt,
     + 	/* Setup merge options */
     + 	init_merge_options(&o, the_repository);
     + 	o.show_rename_progress = 0;
     ++	o.record_conflict_msgs_as_headers = 1;
     + 
     + 	ctx.abbrev = DEFAULT_ABBREV;
     + 	format_commit_message(parent1, "%h (%s)", &parent1_desc, &ctx);
     +@@ log-tree.c: static int do_remerge_diff(struct rev_info *opt,
     + 	merge_incore_recursive(&o, bases, parent1, parent2, &res);
     + 
     + 	/* Show the diff */
     ++	opt->diffopt.additional_path_headers = res.path_messages;
     + 	diff_tree_oid(&res.tree->object.oid, oid, "", &opt->diffopt);
     + 	log_tree_diff_flush(opt);
     + 
     + 	/* Cleanup */
     ++	opt->diffopt.additional_path_headers = NULL;
     + 	strbuf_release(&parent1_desc);
     + 	strbuf_release(&parent2_desc);
     + 	merge_finalize(&o, &res);
     +
       ## merge-ort.c ##
      @@ merge-ort.c: redo:
       	trace2_region_leave("merge", "process_entries", opt->repo);
     @@ merge-ort.h: struct merge_result {
       	/*
       	 * Additional metadata used by merge_switch_to_result() or future calls
       	 * to merge_incore_*().  Includes data needed to update the index (if
     +
     + ## t/t4069-remerge-diff.sh ##
     +@@ t/t4069-remerge-diff.sh: test_description='remerge-diff handling'
     + 
     + . ./test-lib.sh
     + 
     ++# --remerge-diff uses ort under the hood regardless of setting.  However,
     ++# we set up a file/directory conflict beforehand, and the different backends
     ++# handle the conflict differently, which would require separate code paths
     ++# to resolve.  There's not much point in making the code uglier to do that,
     ++# though, when the real thing we are testing (--remerge-diff) will hardcode
     ++# calls directly into the merge-ort API anyway.  So just force the use of
     ++# ort on the setup too.
     ++GIT_TEST_MERGE_ALGORITHM=ort
     ++
     + test_expect_success 'setup basic merges' '
     + 	test_write_lines 1 2 3 4 5 6 7 8 9 >numbers &&
     + 	git add numbers &&
     +@@ t/t4069-remerge-diff.sh: test_expect_success 'remerge-diff with both a resolved conflict and an unrelated
     + 	git log -1 --oneline ab_resolution >tmp &&
     + 	cat <<-EOF >>tmp &&
     + 	diff --git a/numbers b/numbers
     ++	CONFLICT (content): Merge conflict in numbers
     + 	index a1fb731..6875544 100644
     + 	--- a/numbers
     + 	+++ b/numbers
     +@@ t/t4069-remerge-diff.sh: test_expect_success 'remerge-diff with both a resolved conflict and an unrelated
     + 	test_cmp expect actual
     + '
     + 
     ++test_expect_success 'setup non-content conflicts' '
     ++	git switch --orphan base &&
     ++
     ++	test_write_lines 1 2 3 4 5 6 7 8 9 >numbers &&
     ++	test_write_lines a b c d e f g h i >letters &&
     ++	test_write_lines in the way >content &&
     ++	git add numbers letters content &&
     ++	git commit -m base &&
     ++
     ++	git branch side1 &&
     ++	git branch side2 &&
     ++
     ++	git checkout side1 &&
     ++	test_write_lines 1 2 three 4 5 6 7 8 9 >numbers &&
     ++	git mv letters letters_side1 &&
     ++	git mv content file_or_directory &&
     ++	git add numbers &&
     ++	git commit -m side1 &&
     ++
     ++	git checkout side2 &&
     ++	git rm numbers &&
     ++	git mv letters letters_side2 &&
     ++	mkdir file_or_directory &&
     ++	echo hello >file_or_directory/world &&
     ++	git add file_or_directory/world &&
     ++	git commit -m side2 &&
     ++
     ++	git checkout -b resolution side1 &&
     ++	test_must_fail git merge side2 &&
     ++	test_write_lines 1 2 three 4 5 6 7 8 9 >numbers &&
     ++	git add numbers &&
     ++	git add letters_side1 &&
     ++	git rm letters &&
     ++	git rm letters_side2 &&
     ++	git add file_or_directory~HEAD &&
     ++	git mv file_or_directory~HEAD wanted_content &&
     ++	git commit -m resolved
     ++'
     ++
     ++test_expect_success 'remerge-diff with non-content conflicts' '
     ++	git log -1 --oneline resolution >tmp &&
     ++	cat <<-EOF >>tmp &&
     ++	diff --git a/file_or_directory~HASH (side1) b/wanted_content
     ++	similarity index 100%
     ++	rename from file_or_directory~HASH (side1)
     ++	rename to wanted_content
     ++	CONFLICT (file/directory): directory in the way of file_or_directory from HASH (side1); moving it to file_or_directory~HASH (side1) instead.
     ++	diff --git a/letters b/letters
     ++	CONFLICT (rename/rename): letters renamed to letters_side1 in HASH (side1) and to letters_side2 in HASH (side2).
     ++	diff --git a/letters_side2 b/letters_side2
     ++	deleted file mode 100644
     ++	index b236ae5..0000000
     ++	--- a/letters_side2
     ++	+++ /dev/null
     ++	@@ -1,9 +0,0 @@
     ++	-a
     ++	-b
     ++	-c
     ++	-d
     ++	-e
     ++	-f
     ++	-g
     ++	-h
     ++	-i
     ++	diff --git a/numbers b/numbers
     ++	CONFLICT (modify/delete): numbers deleted in HASH (side2) and modified in HASH (side1).  Version HASH (side1) of numbers left in tree.
     ++	EOF
     ++	# We still have some sha1 hashes above; rip them out so test works
     ++	# with sha256
     ++	sed -e "s/[0-9a-f]\{7,\}/HASH/g" tmp >expect &&
     ++
     ++	git show --oneline --remerge-diff resolution >tmp &&
     ++	sed -e "s/[0-9a-f]\{7,\}/HASH/g" tmp >actual &&
     ++	test_cmp expect actual
     ++'
     ++
     + test_done

9: 4f21969e357 < -: ----------- doc/diff-options: explain the new --remerge-diff option

gitgitgadget

From: Elijah Newren <[email protected]>

The --remerge-diff option will need to create new blobs and trees representing the "automatic merge" state. If one is traversing a long project history, one can easily get hundreds of thousands of loose objects generated during log --remerge-diff. However, none of those loose objects are needed after we have completed our diff operation; they can be summarily deleted.

Add a new helper function to tmp_objdir to discard all the contained objects, and call it after each merge is handled.

---
 builtin/log.c | 13 +++++++------
 log-tree.c    |  8 +++++++-
 revision.h    |  3 +++
 tmp-objdir.c  |  5 +++++
 tmp-objdir.h  |  6 ++++++
 5 files changed, 28 insertions(+), 7 deletions(-)

index d053418fddd..e6a080df914 100644
@@ -407,13 +407,12 @@ 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)
+		rev->remerge_objdir = tmp_objdir_create("remerge-diff");
+		if (!rev->remerge_objdir)
 			die_errno(_("unable to create temporary object directory"));
-		tmp_objdir_replace_primary_odb(remerge_objdir, 1);
+		tmp_objdir_replace_primary_odb(rev->remerge_objdir, 1);
 	}
 
 	if (rev->early_output)

@@ -458,8 +457,10 @@ 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->remerge_diff) {
+		tmp_objdir_destroy(rev->remerge_objdir);
+		rev->remerge_objdir = NULL;
+	}
 
 	if (rev->diffopt.output_format & DIFF_FORMAT_CHECKDIFF &&
 	    rev->diffopt.flags.check_failed) {

index 84ed864fc81..d4655b63d75 100644
@@ -4,6 +4,7 @@
 #include "diff.h"
 #include "object-store.h"
 #include "repository.h"
+#include "tmp-objdir.h"
 #include "commit.h"
 #include "tag.h"
 #include "graph.h"

@@ -944,7 +945,12 @@ static int do_remerge_diff(struct rev_info *opt,
 	strbuf_release(&parent1_desc);
 	strbuf_release(&parent2_desc);
 	merge_finalize(&o, &res);
-	/* TODO: clean up the temporary object directory */
+
+	/* Clean up the contents of the temporary object directory */
+	if (opt->remerge_objdir)
+		tmp_objdir_discard_objects(opt->remerge_objdir);
+	else
+		BUG("unable to remove temporary object directory");
 
 	return !opt->loginfo;
 }

index 13178e6b8f3..44efce3f410 100644
@@ -318,6 +318,9 @@ struct rev_info {
 
 	/* misc. flags related to '--no-kept-objects' */
 	unsigned keep_pack_cache_flags;
+
+	/* Location where temporary objects for remerge-diff are written. */
+	struct tmp_objdir *remerge_objdir;
 };
 
 int ref_excluded(struct string_list *, const char *path);

index 3d38eeab66b..adf6033549e 100644
@@ -79,6 +79,11 @@ static void remove_tmp_objdir_on_signal(int signo)
 	raise(signo);
 }
 
+void tmp_objdir_discard_objects(struct tmp_objdir *t)
+{
+	remove_dir_recursively(&t->path, REMOVE_DIR_KEEP_TOPLEVEL);
+}
+
 /*
  * These env_* functions are for setting up the child environment; the
  * "replace" variant overrides the value of any existing variable with that

index cda5ec76778..76efc7edee5 100644
@@ -46,6 +46,12 @@ int tmp_objdir_migrate(struct tmp_objdir *);
  */
 int tmp_objdir_destroy(struct tmp_objdir *);
 
+/*
+ * Remove all objects from the temporary object directory, while leaving it
+ * around so more objects can be added.
+ */
+void tmp_objdir_discard_objects(struct tmp_objdir *);
+
 /*
  * Add the temporary object directory as an alternate object store in the
  * current process.

gitgitgadget

From: Elijah Newren <[email protected]>

Since some callers may want to send warning messages to somewhere other than stdout/stderr, stop printing "warning: Cannot merge binary files" from ll-merge and instead modify the return status of ll_merge() to indicate when a merge of binary files has occurred.

This commit continues printing the message as-is; future changes will start handling the new commit differently in the merge-ort codepath.

Note that my methodology included first modifying ll_merge() to return a struct, so that the compiler would catch all the callers for me and ensure I had modified all of them. After modifying all of them, I then changed the struct to an enum.

---
 apply.c            |  5 ++++-
 builtin/checkout.c | 12 ++++++++----
 ll-merge.c         | 40 ++++++++++++++++++++++------------------
 ll-merge.h         |  9 ++++++++-
 merge-blobs.c      |  5 ++++-
 merge-ort.c        |  5 ++++-
 merge-recursive.c  |  5 ++++-
 notes-merge.c      |  5 ++++-
 rerere.c           | 12 ++++++++----
 9 files changed, 66 insertions(+), 32 deletions(-)

index 43a0aebf4ee..8079395755f 100644
@@ -3492,7 +3492,7 @@ static int three_way_merge(struct apply_state *state,
 {
 	mmfile_t base_file, our_file, their_file;
 	mmbuffer_t result = { NULL };
-	int status;
+	enum ll_merge_result status;
 
 	/* resolve trivial cases first */
 	if (oideq(base, ours))

@@ -3509,6 +3509,9 @@ static int three_way_merge(struct apply_state *state,
 			  &their_file, "theirs",
 			  state->repo->index,
 			  NULL);
+	if (status == LL_MERGE_BINARY_CONFLICT)
+		warning("Cannot merge binary files: %s (%s vs. %s)",
+			path, "ours", "theirs");
 	free(base_file.ptr);
 	free(our_file.ptr);
 	free(their_file.ptr);

index cbf73b8c9f6..3a559d69303 100644
@@ -237,6 +237,7 @@ static int checkout_merged(int pos, const struct checkout *state,
 	struct cache_entry *ce = active_cache[pos];
 	const char *path = ce->name;
 	mmfile_t ancestor, ours, theirs;
+	enum ll_merge_result merge_status;
 	int status;
 	struct object_id oid;
 	mmbuffer_t result_buf;

@@ -267,13 +268,16 @@ static int checkout_merged(int pos, const struct checkout *state,
 	memset(&ll_opts, 0, sizeof(ll_opts));
 	git_config_get_bool("merge.renormalize", &renormalize);
 	ll_opts.renormalize = renormalize;
-	status = ll_merge(&result_buf, path, &ancestor, "base",
-			  &ours, "ours", &theirs, "theirs",
-			  state->istate, &ll_opts);
+	merge_status = ll_merge(&result_buf, path, &ancestor, "base",
+				&ours, "ours", &theirs, "theirs",
+				state->istate, &ll_opts);
 	free(ancestor.ptr);
 	free(ours.ptr);
 	free(theirs.ptr);
-	if (status < 0 || !result_buf.ptr) {
+	if (merge_status == LL_MERGE_BINARY_CONFLICT)
+		warning("Cannot merge binary files: %s (%s vs. %s)",
+			path, "ours", "theirs");
+	if (merge_status < 0 || !result_buf.ptr) {
 		free(result_buf.ptr);
 		return error(_("path '%s': cannot merge"), path);
 	}

index 261657578c7..669c09eed6c 100644
@@ -14,7 +14,7 @@
 
 struct ll_merge_driver;
 
-typedef int (*ll_merge_fn)(const struct ll_merge_driver *,
+typedef enum ll_merge_result (*ll_merge_fn)(const struct ll_merge_driver *,
 			   mmbuffer_t *result,
 			   const char *path,
 			   mmfile_t *orig, const char *orig_name,

@@ -49,7 +49,7 @@ void reset_merge_attributes(void)
 /*
  * Built-in low-levels
  */
-static int ll_binary_merge(const struct ll_merge_driver *drv_unused,
+static enum ll_merge_result ll_binary_merge(const struct ll_merge_driver *drv_unused,
 			   mmbuffer_t *result,
 			   const char *path,
 			   mmfile_t *orig, const char *orig_name,

@@ -58,6 +58,7 @@ static int ll_binary_merge(const struct ll_merge_driver *drv_unused,
 			   const struct ll_merge_options *opts,
 			   int marker_size)
 {
+	enum ll_merge_result ret;
 	mmfile_t *stolen;
 	assert(opts);
 

@@ -68,16 +69,19 @@ static int ll_binary_merge(const struct ll_merge_driver *drv_unused,
 	 */
 	if (opts->virtual_ancestor) {
 		stolen = orig;
+		ret = LL_MERGE_OK;
 	} else {
 		switch (opts->variant) {
 		default:
-			warning("Cannot merge binary files: %s (%s vs. %s)",
-				path, name1, name2);
-			/* fallthru */
+			ret = LL_MERGE_BINARY_CONFLICT;
+			stolen = src1;
+			break;
 		case XDL_MERGE_FAVOR_OURS:
+			ret = LL_MERGE_OK;
 			stolen = src1;
 			break;
 		case XDL_MERGE_FAVOR_THEIRS:
+			ret = LL_MERGE_OK;
 			stolen = src2;
 			break;
 		}

@@ -87,16 +91,10 @@ static int ll_binary_merge(const struct ll_merge_driver *drv_unused,
 	result->size = stolen->size;
 	stolen->ptr = NULL;
 
-	/*
-	 * With -Xtheirs or -Xours, we have cleanly merged;
-	 * otherwise we got a conflict.
-	 */
-	return opts->variant == XDL_MERGE_FAVOR_OURS ||
-	       opts->variant == XDL_MERGE_FAVOR_THEIRS ?
-	       0 : 1;
+	return ret;
 }
 
-static int ll_xdl_merge(const struct ll_merge_driver *drv_unused,
+static enum ll_merge_result ll_xdl_merge(const struct ll_merge_driver *drv_unused,
 			mmbuffer_t *result,
 			const char *path,
 			mmfile_t *orig, const char *orig_name,

@@ -105,7 +103,9 @@ static int ll_xdl_merge(const struct ll_merge_driver *drv_unused,
 			const struct ll_merge_options *opts,
 			int marker_size)
 {
+	enum ll_merge_result ret;
 	xmparam_t xmp;
+	int status;
 	assert(opts);
 
 	if (orig->size > MAX_XDIFF_SIZE ||

@@ -133,10 +133,12 @@ static int ll_xdl_merge(const struct ll_merge_driver *drv_unused,
 	xmp.ancestor = orig_name;
 	xmp.file1 = name1;
 	xmp.file2 = name2;
-	return xdl_merge(orig, src1, src2, &xmp, result);
+	status = xdl_merge(orig, src1, src2, &xmp, result);
+	ret = (status > 1 ) ? LL_MERGE_CONFLICT : status;
+	return ret;
 }
 
-static int ll_union_merge(const struct ll_merge_driver *drv_unused,
+static enum ll_merge_result ll_union_merge(const struct ll_merge_driver *drv_unused,
 			  mmbuffer_t *result,
 			  const char *path,
 			  mmfile_t *orig, const char *orig_name,

@@ -178,7 +180,7 @@ static void create_temp(mmfile_t *src, char *path, size_t len)
 /*
  * User defined low-level merge driver support.
  */
-static int ll_ext_merge(const struct ll_merge_driver *fn,
+static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,
 			mmbuffer_t *result,
 			const char *path,
 			mmfile_t *orig, const char *orig_name,

@@ -194,6 +196,7 @@ static int ll_ext_merge(const struct ll_merge_driver *fn,
 	const char *args[] = { NULL, NULL };
 	int status, fd, i;
 	struct stat st;
+	enum ll_merge_result ret;
 	assert(opts);
 
 	sq_quote_buf(&path_sq, path);

@@ -236,7 +239,8 @@ static int ll_ext_merge(const struct ll_merge_driver *fn,
 		unlink_or_warn(temp[i]);
 	strbuf_release(&cmd);
 	strbuf_release(&path_sq);
-	return status;
+	ret = (status > 1) ? LL_MERGE_CONFLICT : status;
+	return ret;
 }
 
 /*

@@ -362,7 +366,7 @@ static void normalize_file(mmfile_t *mm, const char *path, struct index_state *i
 	}
 }
 
-int ll_merge(mmbuffer_t *result_buf,
+enum ll_merge_result ll_merge(mmbuffer_t *result_buf,
 	     const char *path,
 	     mmfile_t *ancestor, const char *ancestor_label,
 	     mmfile_t *ours, const char *our_label,

index aceb1b24132..e4a20e81a3a 100644
@@ -82,13 +82,20 @@ struct ll_merge_options {
 	long xdl_opts;
 };
 
+enum ll_merge_result {
+	LL_MERGE_ERROR = -1,
+	LL_MERGE_OK = 0,
+	LL_MERGE_CONFLICT,
+	LL_MERGE_BINARY_CONFLICT,
+};
+
 /**
  * Perform a three-way single-file merge in core.  This is a thin wrapper
  * around `xdl_merge` that takes the path and any merge backend specified in
  * `.gitattributes` or `.git/info/attributes` into account.
  * Returns 0 for a clean merge.
  */
-int ll_merge(mmbuffer_t *result_buf,
+enum ll_merge_result ll_merge(mmbuffer_t *result_buf,
 	     const char *path,
 	     mmfile_t *ancestor, const char *ancestor_label,
 	     mmfile_t *ours, const char *our_label,

index ee0a0e90c94..8138090f81c 100644
@@ -36,7 +36,7 @@ static void *three_way_filemerge(struct index_state *istate,
 				 mmfile_t *their,
 				 unsigned long *size)
 {
-	int merge_status;
+	enum ll_merge_result merge_status;
 	mmbuffer_t res;
 
 	/*

@@ -50,6 +50,9 @@ static void *three_way_filemerge(struct index_state *istate,
 				istate, NULL);
 	if (merge_status < 0)
 		return NULL;
+	if (merge_status == LL_MERGE_BINARY_CONFLICT)
+		warning("Cannot merge binary files: %s (%s vs. %s)",
+			path, ".our", ".their");
 
 	*size = res.size;
 	return res.ptr;

index 0342f104836..c24da2ba3cb 100644
@@ -1743,7 +1743,7 @@ static int merge_3way(struct merge_options *opt,
 	mmfile_t orig, src1, src2;
 	struct ll_merge_options ll_opts = {0};
 	char *base, *name1, *name2;
-	int merge_status;
+	enum ll_merge_result merge_status;
 
 	if (!opt->priv->attr_index.initialized)
 		initialize_attr_index(opt);

@@ -1787,6 +1787,9 @@ static int merge_3way(struct merge_options *opt,
 	merge_status = ll_merge(result_buf, path, &orig, base,
 				&src1, name1, &src2, name2,
 				&opt->priv->attr_index, &ll_opts);
+	if (merge_status == LL_MERGE_BINARY_CONFLICT)
+		warning("Cannot merge binary files: %s (%s vs. %s)",
+			path, name1, name2);
 
 	free(base);
 	free(name1);

index d9457797dbb..bc73c52dd84 100644
@@ -1044,7 +1044,7 @@ static int merge_3way(struct merge_options *opt,
 	mmfile_t orig, src1, src2;
 	struct ll_merge_options ll_opts = {0};
 	char *base, *name1, *name2;
-	int merge_status;
+	enum ll_merge_result merge_status;
 
 	ll_opts.renormalize = opt->renormalize;
 	ll_opts.extra_marker_size = extra_marker_size;

@@ -1090,6 +1090,9 @@ static int merge_3way(struct merge_options *opt,
 	merge_status = ll_merge(result_buf, a->path, &orig, base,
 				&src1, name1, &src2, name2,
 				opt->repo->index, &ll_opts);
+	if (merge_status == LL_MERGE_BINARY_CONFLICT)
+		warning("Cannot merge binary files: %s (%s vs. %s)",
+			a->path, name1, name2);
 
 	free(base);
 	free(name1);

index b4a3a903e86..01d596920ea 100644
@@ -344,7 +344,7 @@ static int ll_merge_in_worktree(struct notes_merge_options *o,
 {
 	mmbuffer_t result_buf;
 	mmfile_t base, local, remote;
-	int status;
+	enum ll_merge_result status;
 
 	read_mmblob(&base, &p->base);
 	read_mmblob(&local, &p->local);

@@ -358,6 +358,9 @@ static int ll_merge_in_worktree(struct notes_merge_options *o,
 	free(local.ptr);
 	free(remote.ptr);
 
+	if (status == LL_MERGE_BINARY_CONFLICT)
+		warning("Cannot merge binary files: %s (%s vs. %s)",
+			oid_to_hex(&p->obj), o->local_ref, o->remote_ref);
 	if ((status < 0) || !result_buf.ptr)
 		die("Failed to execute internal merge");
 

index d83d58df4fb..b1f8961ed9e 100644
@@ -609,19 +609,23 @@ static int try_merge(struct index_state *istate,
 		     const struct rerere_id *id, const char *path,
 		     mmfile_t *cur, mmbuffer_t *result)
 {
-	int ret;
+	enum ll_merge_result ret;
 	mmfile_t base = {NULL, 0}, other = {NULL, 0};
 
 	if (read_mmfile(&base, rerere_path(id, "preimage")) ||
-	    read_mmfile(&other, rerere_path(id, "postimage")))
-		ret = 1;
-	else
+	    read_mmfile(&other, rerere_path(id, "postimage"))) {
+		ret = LL_MERGE_CONFLICT;
+	} else {
 		/*
 		 * A three-way merge. Note that this honors user-customizable
 		 * low-level merge driver settings.
 		 */
 		ret = ll_merge(result, path, &base, NULL, cur, "", &other, "",
 			       istate, NULL);
+		if (ret == LL_MERGE_BINARY_CONFLICT)
+			warning("Cannot merge binary files: %s (%s vs. %s)",
+				path, "", "");
+	}
 
 	free(base.ptr);
 	free(other.ptr);

gitgitgadget

From: Elijah Newren <[email protected]>

Instead of immediately printing ll-merge warnings to stderr, we save them in our output strbuf. Besides allowing us to move these warnings to a special file for --remerge-diff, this has two other benefits for regular merges done by merge-ort:

* The deferral of messages ensures we can print all messages about any given path together (merge-recursive was known to sometimes intersperse messages about other paths, particularly when renames were involved).

* The deferral of messages means we can avoid printing spurious conflict messages when we just end up aborting due to local user modifications in the way. (In contrast to merge-recursive.c which prematurely checks for local modifications in the way via unpack_trees() and gets the check wrong both in terms of false positives and false negatives relative to renames, merge-ort does not perform the local modifications in the way check until the checkout() step after the full merge has been computed.)

---
 merge-ort.c                | 5 +++--
 t/t6404-recursive-merge.sh | 9 +++++++--
 t/t6406-merge-attr.sh      | 9 +++++++--
 3 files changed, 17 insertions(+), 6 deletions(-)

index c24da2ba3cb..a18f47e23c5 100644
@@ -1788,8 +1788,9 @@ static int merge_3way(struct merge_options *opt,
 				&src1, name1, &src2, name2,
 				&opt->priv->attr_index, &ll_opts);
 	if (merge_status == LL_MERGE_BINARY_CONFLICT)
-		warning("Cannot merge binary files: %s (%s vs. %s)",
-			path, name1, name2);
+		path_msg(opt, path, 0,
+			 "warning: Cannot merge binary files: %s (%s vs. %s)",
+			 path, name1, name2);
 
 	free(base);
 	free(name1);

index eaf48e941e2..b8735c6db4d 100755
@@ -108,8 +108,13 @@ test_expect_success 'refuse to merge binary files' '
 	printf "\0\0" >binary-file &&
 	git add binary-file &&
 	git commit -m binary2 &&
-	test_must_fail git merge F >merge.out 2>merge.err &&
-	grep "Cannot merge binary files: binary-file (HEAD vs. F)" merge.err
+	if test "$GIT_TEST_MERGE_ALGORITHM" = ort
+	then
+		test_must_fail git merge F >merge_output
+	else
+		test_must_fail git merge F 2>merge_output
+	fi &&
+	grep "Cannot merge binary files: binary-file (HEAD vs. F)" merge_output
 '
 
 test_expect_success 'mark rename/delete as unmerged' '

index 84946458371..c41584eb33e 100755
@@ -221,8 +221,13 @@ test_expect_success 'binary files with union attribute' '
 	printf "two\0" >bin.txt &&
 	git commit -am two &&
 
-	test_must_fail git merge bin-main 2>stderr &&
-	grep -i "warning.*cannot merge.*HEAD vs. bin-main" stderr
+	if test "$GIT_TEST_MERGE_ALGORITHM" = ort
+	then
+		test_must_fail git merge bin-main >output
+	else
+		test_must_fail git merge bin-main 2>output
+	fi &&
+	grep -i "warning.*cannot merge.*HEAD vs. bin-main" output
 '
 
 test_done

gitgitgadget

From: Elijah Newren <[email protected]>

path_msg() has the ability to mark messages as omittable, designed for remerge-diff where we'll instead be showing conflict messages as diff headers for a subsequent diff. While all these messages are very useful when trying to create a merge initially, early use with the --remerge-diff feature (the only user of this omittable conflict message capability), suggests that the particular messages marked in this commit are just noise when trying to see what changes users made to create a merge commit. Mark them as omittable.

Note that there were already a few messages marked as omittable in merge-ort when doing a remerge-diff, because the development of --remerge-diff preceded the upstreaming of merge-ort and I was trying to ensure merge-ort could handle all the necessary requirements. See commit c5a6f65527 ("merge-ort: add modify/delete handling and delayed output processing", 2020-12-03) for the initial details. For some examples of already-marked-as-omittable messages, see either "Auto-merging <path>" or some of the submodule update hints. This commit just adds two more messages that should also be omittable.

---
 merge-ort.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index a18f47e23c5..998e92ec593 100644
@@ -2420,7 +2420,7 @@ static void apply_directory_rename_modifications(struct merge_options *opt,
 		 */
 		ci->path_conflict = 1;
 		if (pair->status == 'A')
-			path_msg(opt, new_path, 0,
+			path_msg(opt, new_path, 1,
 				 _("CONFLICT (file location): %s added in %s "
 				   "inside a directory that was renamed in %s, "
 				   "suggesting it should perhaps be moved to "

@@ -2428,7 +2428,7 @@ static void apply_directory_rename_modifications(struct merge_options *opt,
 				 old_path, branch_with_new_path,
 				 branch_with_dir_rename, new_path);
 		else
-			path_msg(opt, new_path, 0,
+			path_msg(opt, new_path, 1,
 				 _("CONFLICT (file location): %s renamed to %s "
 				   "in %s, inside a directory that was renamed "
 				   "in %s, suggesting it should perhaps be "

gitgitgadget

From: Elijah Newren <[email protected]>

When users run

    git show --remerge-diff $MERGE_COMMIT

or
git log -p --remerge-diff ...
stdout is not an appropriate location to dump conflict messages, but we do want to provide them to users. We will include them in the diff headers instead...but for that to work, we need for any multiline messages to replace newlines with both a newline and a space. Add a new flag to signal when we want these messages modified in such a fashion, and use it in path_msg() to modify these messages this way.

---
 merge-ort.c       | 36 ++++++++++++++++++++++++++++++++++--
 merge-recursive.c |  3 +++
 merge-recursive.h |  1 +
 3 files changed, 38 insertions(+), 2 deletions(-)

index 998e92ec593..9142d56e0ad 100644
@@ -634,17 +634,46 @@ static void path_msg(struct merge_options *opt,
 		     const char *fmt, ...)
 {
 	va_list ap;
-	struct strbuf *sb = strmap_get(&opt->priv->output, path);
+	struct strbuf *sb, *dest;
+	struct strbuf tmp = STRBUF_INIT;
+
+	if (opt->record_conflict_msgs_as_headers && omittable_hint)
+		return; /* Do not record mere hints in tree */
+	sb = strmap_get(&opt->priv->output, path);
 	if (!sb) {
 		sb = xmalloc(sizeof(*sb));
 		strbuf_init(sb, 0);
 		strmap_put(&opt->priv->output, path, sb);
 	}
 
+	dest = (opt->record_conflict_msgs_as_headers ? &tmp : sb);
+
 	va_start(ap, fmt);
-	strbuf_vaddf(sb, fmt, ap);
+	strbuf_vaddf(dest, fmt, ap);
 	va_end(ap);
 
+	if (opt->record_conflict_msgs_as_headers) {
+		int i_sb = 0, i_tmp = 0;
+
+		/* Copy tmp to sb, adding spaces after newlines */
+		strbuf_grow(sb, 2*tmp.len); /* more than sufficient */
+		for (; i_tmp < tmp.len; i_tmp++, i_sb++) {
+			/* Copy next character from tmp to sb */
+			sb->buf[sb->len + i_sb] = tmp.buf[i_tmp];
+
+			/* If we copied a newline, add a space */
+			if (tmp.buf[i_tmp] == '\n')
+				sb->buf[++i_sb] = ' ';
+		}
+		/* Update length and ensure it's NUL-terminated */
+		sb->len += i_sb;
+		sb->buf[sb->len] = '\0';
+
+		/* Clean up tmp */
+		strbuf_release(&tmp);
+	}
+
+	/* Add final newline character to sb */
 	strbuf_addch(sb, '\n');
 }
 

@@ -4246,6 +4275,9 @@ void merge_switch_to_result(struct merge_options *opt,
 		struct string_list olist = STRING_LIST_INIT_NODUP;
 		int i;
 
+		if (opt->record_conflict_msgs_as_headers)
+			BUG("Either display conflict messages or record them as headers, not both");
+
 		trace2_region_enter("merge", "display messages", opt->repo);
 
 		/* Hack to pre-allocate olist to the desired size */

index bc73c52dd84..c9ba7e904a6 100644
@@ -3714,6 +3714,9 @@ static int merge_start(struct merge_options *opt, struct tree *head)
 
 	assert(opt->priv == NULL);
 
+	/* Not supported; option specific to merge-ort */
+	assert(!opt->record_conflict_msgs_as_headers);
+
 	/* Sanity check on repo state; index must match head */
 	if (repo_index_has_changes(opt->repo, head, &sb)) {
 		err(opt, _("Your local changes to the following files would be overwritten by merge:\n  %s"),

index 0795a1d3ec1..ebfdb7f994e 100644
@@ -46,6 +46,7 @@ struct merge_options {
 	/* miscellaneous control options */
 	const char *subtree_shift;
 	unsigned renormalize : 1;
+	unsigned record_conflict_msgs_as_headers : 1;
 
 	/* internal fields used by the implementation */
 	struct merge_options_internal *priv;

gitgitgadget

From: Elijah Newren <[email protected]>

When additional headers are provided, we need to
* add diff_filepairs to diff_queued_diff for each paths in the additional headers map which, unless that path is part of another diff_filepair already found in diff_queued_diff
* format the headers (colorization, line_prefix for --graph)
* make sure the various codepaths that attempt to return early if there are "no changes" take into account the headers that need to be shown.

---
 diff.c     | 116 +++++++++++++++++++++++++++++++++++++++++++++++++++--
 diff.h     |   3 +-
 log-tree.c |   2 +-
 3 files changed, 115 insertions(+), 6 deletions(-)

index 861282db1c3..aaa6a19f158 100644
@@ -27,6 +27,7 @@
 #include "help.h"
 #include "promisor-remote.h"
 #include "dir.h"
+#include "strmap.h"
 
 #ifdef NO_FAST_WORKING_DIRECTORY
 #define FAST_WORKING_DIRECTORY 0

@@ -3406,6 +3407,31 @@ struct userdiff_driver *get_textconv(struct repository *r,
 	return userdiff_get_textconv(r, one->driver);
 }
 
+static struct strbuf *additional_headers(struct diff_options *o,
+					 const char *path)
+{
+	if (!o->additional_path_headers)
+		return NULL;
+	return strmap_get(o->additional_path_headers, path);
+}
+
+static void add_formatted_headers(struct strbuf *msg,
+				  struct strbuf *more_headers,
+				  const char *line_prefix,
+				  const char *meta,
+				  const char *reset)
+{
+	char *next, *newline;
+
+	for (next = more_headers->buf; *next; next = newline) {
+		newline = strchrnul(next, '\n');
+		strbuf_addf(msg, "%s%s%.*s%s\n", line_prefix, meta,
+			    (int)(newline - next), next, reset);
+		if (*newline)
+			newline++;
+	}
+}
+
 static void builtin_diff(const char *name_a,
 			 const char *name_b,
 			 struct diff_filespec *one,

@@ -3464,6 +3490,17 @@ static void builtin_diff(const char *name_a,
 	b_two = quote_two(b_prefix, name_b + (*name_b == '/'));
 	lbl[0] = DIFF_FILE_VALID(one) ? a_one : "/dev/null";
 	lbl[1] = DIFF_FILE_VALID(two) ? b_two : "/dev/null";
+	if (!DIFF_FILE_VALID(one) && !DIFF_FILE_VALID(two)) {
+		/*
+		 * We should only reach this point for pairs from
+		 * create_filepairs_for_header_only_notifications().  For
+		 * these, we should avoid the "/dev/null" special casing
+		 * above, meaning we avoid showing such pairs as either
+		 * "new file" or "deleted file" below.
+		 */
+		lbl[0] = a_one;
+		lbl[1] = b_two;
+	}
 	strbuf_addf(&header, "%s%sdiff --git %s %s%s\n", line_prefix, meta, a_one, b_two, reset);
 	if (lbl[0][0] == '/') {
 		/* /dev/null */

@@ -4328,6 +4365,7 @@ static void fill_metainfo(struct strbuf *msg,
 	const char *set = diff_get_color(use_color, DIFF_METAINFO);
 	const char *reset = diff_get_color(use_color, DIFF_RESET);
 	const char *line_prefix = diff_line_prefix(o);
+	struct strbuf *more_headers = NULL;
 
 	*must_show_header = 1;
 	strbuf_init(msg, PATH_MAX * 2 + 300);

@@ -4364,6 +4402,11 @@ static void fill_metainfo(struct strbuf *msg,
 	default:
 		*must_show_header = 0;
 	}
+	if ((more_headers = additional_headers(o, name))) {
+		add_formatted_headers(msg, more_headers,
+				      line_prefix, set, reset);
+		*must_show_header = 1;
+	}
 	if (one && two && !oideq(&one->oid, &two->oid)) {
 		const unsigned hexsz = the_hash_algo->hexsz;
 		int abbrev = o->abbrev ? o->abbrev : DEFAULT_ABBREV;

@@ -5852,12 +5895,22 @@ int diff_unmodified_pair(struct diff_filepair *p)
 
 static void diff_flush_patch(struct diff_filepair *p, struct diff_options *o)
 {
-	if (diff_unmodified_pair(p))
+	/*
+	 * Check if we can return early without showing a diff.  Note that
+	 * diff_filepair only stores {oid, path, mode, is_valid}
+	 * information for each path, and thus diff_unmodified_pair() only
+	 * considers those bits of info.  However, we do not want pairs
+	 * created by create_filepairs_for_header_only_notifications() to
+	 * be ignored, so return early if both p is unmodified AND
+	 * p->one->path is not in additional headers.
+	 */
+	if (diff_unmodified_pair(p) && !additional_headers(o, p->one->path))
 		return;
 
+	/* Actually, we can also return early to avoid showing tree diffs */
 	if ((DIFF_FILE_VALID(p->one) && S_ISDIR(p->one->mode)) ||
 	    (DIFF_FILE_VALID(p->two) && S_ISDIR(p->two->mode)))
-		return; /* no tree diffs in patch format */
+		return;
 
 	run_diff(p, o);
 }

@@ -5888,10 +5941,14 @@ static void diff_flush_checkdiff(struct diff_filepair *p,
 	run_checkdiff(p, o);
 }
 
-int diff_queue_is_empty(void)
+int diff_queue_is_empty(struct diff_options *o)
 {
 	struct diff_queue_struct *q = &diff_queued_diff;
 	int i;
+
+	if (o->additional_path_headers &&
+	    !strmap_empty(o->additional_path_headers))
+		return 0;
 	for (i = 0; i < q->nr; i++)
 		if (!diff_unmodified_pair(q->queue[i]))
 			return 0;

@@ -6325,6 +6382,54 @@ void diff_warn_rename_limit(const char *varname, int needed, int degraded_cc)
 		warning(_(rename_limit_advice), varname, needed);
 }
 
+static void create_filepairs_for_header_only_notifications(struct diff_options *o)
+{
+	struct strset present;
+	struct diff_queue_struct *q = &diff_queued_diff;
+	struct hashmap_iter iter;
+	struct strmap_entry *e;
+	int i;
+
+	strset_init_with_options(&present, /*pool*/ NULL, /*strdup*/ 0);
+
+	/*
+	 * Find out which paths exist in diff_queued_diff, preferring
+	 * one->path for any pair that has multiple paths.
+	 */
+	for (i = 0; i < q->nr; i++) {
+		struct diff_filepair *p = q->queue[i];
+		char *path = p->one->path ? p->one->path : p->two->path;
+
+		if (strmap_contains(o->additional_path_headers, path))
+			strset_add(&present, path);
+	}
+
+	/*
+	 * Loop over paths in additional_path_headers; for each NOT already
+	 * in diff_queued_diff, create a synthetic filepair and insert that
+	 * into diff_queued_diff.
+	 */
+	strmap_for_each_entry(o->additional_path_headers, &iter, e) {
+		if (!strset_contains(&present, e->key)) {
+			struct diff_filespec *one, *two;
+			struct diff_filepair *p;
+
+			one = alloc_filespec(e->key);
+			two = alloc_filespec(e->key);
+			fill_filespec(one, null_oid(), 0, 0);
+			fill_filespec(two, null_oid(), 0, 0);
+			p = diff_queue(q, one, two);
+			p->status = DIFF_STATUS_MODIFIED;
+		}
+	}
+
+	/* Re-sort the filepairs */
+	diffcore_fix_diff_index();
+
+	/* Cleanup */
+	strset_clear(&present);
+}
+
 static void diff_flush_patch_all_file_pairs(struct diff_options *o)
 {
 	int i;

@@ -6337,6 +6442,9 @@ static void diff_flush_patch_all_file_pairs(struct diff_options *o)
 	if (o->color_moved)
 		o->emitted_symbols = &esm;
 
+	if (o->additional_path_headers)
+		create_filepairs_for_header_only_notifications(o);
+
 	for (i = 0; i < q->nr; i++) {
 		struct diff_filepair *p = q->queue[i];
 		if (check_pair_status(p))

@@ -6413,7 +6521,7 @@ void diff_flush(struct diff_options *options)
 	 * Order: raw, stat, summary, patch
 	 * or:    name/name-status/checkdiff (other bits clear)
 	 */
-	if (!q->nr)
+	if (!q->nr && !options->additional_path_headers)
 		goto free_queue;
 
 	if (output_format & (DIFF_FORMAT_RAW |

index 8ba85c5e605..06a0a67afda 100644
@@ -395,6 +395,7 @@ struct diff_options {
 
 	struct repository *repo;
 	struct option *parseopts;
+	struct strmap *additional_path_headers;
 
 	int no_free;
 };

@@ -593,7 +594,7 @@ void diffcore_fix_diff_index(void);
 "                show all files diff when -S is used and hit is found.\n" \
 "  -a  --text    treat all files as text.\n"
 
-int diff_queue_is_empty(void);
+int diff_queue_is_empty(struct diff_options*);
 void diff_flush(struct diff_options*);
 void diff_free(struct diff_options*);
 void diff_warn_rename_limit(const char *varname, int needed, int degraded_cc);

index d4655b63d75..33c28f537a6 100644
@@ -850,7 +850,7 @@ int log_tree_diff_flush(struct rev_info *opt)
 	opt->shown_dashes = 0;
 	diffcore_std(&opt->diffopt);
 
-	if (diff_queue_is_empty()) {
+	if (diff_queue_is_empty(&opt->diffopt)) {
 		int saved_fmt = opt->diffopt.output_format;
 		opt->diffopt.output_format = DIFF_FORMAT_NO_OUTPUT;
 		diff_flush(&opt->diffopt);

gitgitgadget

From: Elijah Newren <[email protected]>

Conflicts such as modify/delete, rename/rename, or file/directory are not representable via content conflict markers, and the normal output messages notifying users about these were dropped with --remerge-diff. While we don't want these messages randomly shown before the commit and diff headers, we do want them to still be shown; include them as part of the diff headers instead.

---
 log-tree.c              |  3 ++
 merge-ort.c             |  1 +
 merge-ort.h             | 10 +++++
 t/t4069-remerge-diff.sh | 86 +++++++++++++++++++++++++++++++++++++++++
 4 files changed, 100 insertions(+)

index 33c28f537a6..97fbb756d21 100644
@@ -922,6 +922,7 @@ static int do_remerge_diff(struct rev_info *opt,
 	/* Setup merge options */
 	init_merge_options(&o, the_repository);
 	o.show_rename_progress = 0;
+	o.record_conflict_msgs_as_headers = 1;
 
 	ctx.abbrev = DEFAULT_ABBREV;
 	format_commit_message(parent1, "%h (%s)", &parent1_desc, &ctx);

@@ -938,10 +939,12 @@ static int do_remerge_diff(struct rev_info *opt,
 	merge_incore_recursive(&o, bases, parent1, parent2, &res);
 
 	/* Show the diff */
+	opt->diffopt.additional_path_headers = res.path_messages;
 	diff_tree_oid(&res.tree->object.oid, oid, "", &opt->diffopt);
 	log_tree_diff_flush(opt);
 
 	/* Cleanup */
+	opt->diffopt.additional_path_headers = NULL;
 	strbuf_release(&parent1_desc);
 	strbuf_release(&parent2_desc);
 	merge_finalize(&o, &res);

index 9142d56e0ad..07e53083cbd 100644
@@ -4579,6 +4579,7 @@ redo:
 	trace2_region_leave("merge", "process_entries", opt->repo);
 
 	/* Set return values */
+	result->path_messages = &opt->priv->output;
 	result->tree = parse_tree_indirect(&working_tree_oid);
 	/* existence of conflicted entries implies unclean */
 	result->clean &= strmap_empty(&opt->priv->conflicted);

index c011864ffeb..fe599b87868 100644
@@ -5,6 +5,7 @@
 
 struct commit;
 struct tree;
+struct strmap;
 
 struct merge_result {
 	/*

@@ -23,6 +24,15 @@ struct merge_result {
 	 */
 	struct tree *tree;
 
+	/*
+	 * Special messages and conflict notices for various paths
+	 *
+	 * This is a map of pathnames to strbufs.  It contains various
+	 * warning/conflict/notice messages (possibly multiple per path)
+	 * that callers may want to use.
+	 */
+	struct strmap *path_messages;
+
 	/*
 	 * Additional metadata used by merge_switch_to_result() or future calls
 	 * to merge_incore_*().  Includes data needed to update the index (if

gitgitgadget