Thread (98 messages) flat view 98 messages, 6 authors, 2022-02-02

Re: [PATCH v2 8/8] show, log: include conflict/warning messages in --remerge-diff headers

From: Johannes Altmanninger <hidden>
Date: 2021-12-28 10:58:00

On Sat, Dec 25, 2021 at 07:59:19AM +0000, Elijah Newren via GitGitGadget wrote:
quoted hunk ↗ jump to hunk
From: Elijah Newren <redacted>

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 <redacted>
---
 log-tree.c              |  3 ++
 merge-ort.c             |  1 +
 merge-ort.h             | 10 +++++
 t/t4069-remerge-diff.sh | 86 +++++++++++++++++++++++++++++++++++++++++
 4 files changed, 100 insertions(+)
diff --git a/log-tree.c b/log-tree.c
index 33c28f537a6..97fbb756d21 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -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);
diff --git a/merge-ort.c b/merge-ort.c
index 9142d56e0ad..07e53083cbd 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -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);
diff --git a/merge-ort.h b/merge-ort.h
index c011864ffeb..fe599b87868 100644
--- a/merge-ort.h
+++ b/merge-ort.h
@@ -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
diff --git a/t/t4069-remerge-diff.sh b/t/t4069-remerge-diff.sh
index 192dbce2bfe..a040d3bcd91 100755
--- a/t/t4069-remerge-diff.sh
+++ b/t/t4069-remerge-diff.sh
@@ -4,6 +4,15 @@ 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 &&
@@ -55,6 +64,7 @@ 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
@@ -83,4 +93,80 @@ 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
the "~HASH (side1)" suffix will probably mess with some programs that extract
the filename from the diff.
I don't know what programs are supposed to expect.  I can see arguments for
either dropping the suffix or including only "~HASH" since that's part of
the actual filename that's left in the worktree.
Maybe it's not so important.

The file/link typechange conflict test I'll add below exposes what looks
like an accidental interaction with the trailing tab characters that we emit
on --- and +++ lines if the "filename" contains a space (since 1a9eb3b9d5
(git-diff/git-apply: make diff output a bit friendlier to GNU patch (part
2), 2006-09-22)).

	index 70885e4..0000000
	--- a/typechange~738109f (side1)	<-- git diff adds a trailing tab!
	+++ /dev/null

I haven't formed an opinion yet, but since Tig uses the --- and +++ lines
to extract file names, I'd drop the " (side1)" suffix from at least the ---
and +++ lines. Maybe also the ^diff lines, I'm not sure
+	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.
I wonder if it's better to have this line further up, before the "rename"
resolution, to correct the temporal order.
+	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
Took me some time to grok these but the output makes sense (it's loud and
ugly but that's okay since these are serious conflicts).
+	# 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
-- 
gitgitgadget
We're missing a test case for typechange.  Here's is a quick draft I've been
playing around with. Seems ugly that the "diff --git a/typechange b/typechange"
is doubled but okay.

Maybe a rename/delete conflict is interesting as well, I'm not sure.  (Also I
wonder if switching the order of parents will give any interesting difference,
I guess not)

test_expect_success 'remerge-diff with file/link conflict' '
	git branch -d base side1 side2 &&
	git switch --orphan base &&

	echo base >typechange &&
	git add typechange &&
	git commit -m base &&

	git branch side1 &&
	git branch side2 &&

	git checkout side1 &&
	echo orig-file-contents >typechange &&
	git commit -a -m side1 &&

	git checkout side2 &&
	ln -sf . typechange &&
	git add typechange &&
	git commit -m side2 &&

	git checkout -b resolution2 side1 &&
	test_must_fail git merge side2 &&
	rm typechange &&
	mv typechange~HEAD typechange &&
	echo resolved >>typechange &&
	git add typechange~HEAD typechange &&
	git merge --continue &&

	git show --oneline --remerge-diff resolution2 >tmp &&
	sed -e "s/[0-9a-f]\{7,\}/HASH/g" tmp >actual &&

	cat <<-EOF >tmp &&
	7759b27 Merge branch ${SQ}side2${SQ} into resolution2
	diff --git a/typechange b/typechange
	deleted file mode 120000
	CONFLICT (distinct types): typechange had different types on each side; renamed one of them so each can be recorded somewhere.
	index 945c9b4..0000000
	--- a/typechange
	+++ /dev/null
	@@ -1 +0,0 @@
	-.
	\ No newline at end of file
	diff --git a/typechange b/typechange
	new file mode 100644
	CONFLICT (distinct types): typechange had different types on each side; renamed one of them so each can be recorded somewhere.
	index 0000000..70885e4
	--- /dev/null
	+++ b/typechange
	@@ -0,0 +1,2 @@
	+orig-file-contents
	+resolved
	diff --git a/typechange~738109f (side1) b/typechange~738109f (side1)
	deleted file mode 100644
	index 70885e4..0000000
	--- a/typechange~738109f (side1)	
	+++ /dev/null
	@@ -1 +0,0 @@
	-orig-file-contents
	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 &&

	test_cmp expect actual
'
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help