git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Elijah Newren <newren@gmail.com>
To: <git@vger.kernel.org>
Cc: <gitster@pobox.com>, <stolee@gmail.com>,
	Elijah Newren <newren@gmail.com>
Subject: [PATCH v4 05/10] merge-recursive: fix rename/add conflict handling
Date: Fri, 2 Nov 2018 11:53:13 -0700	[thread overview]
Message-ID: <20181102185317.31015-6-newren@gmail.com> (raw)
In-Reply-To: <20181102185317.31015-1-newren@gmail.com>

This makes the rename/add conflict handling make use of the new
handle_file_collision() function, which fixes several bugs and improves
things for the rename/add case significantly.  Previously, rename/add
would:

  * Not leave any higher order stage entries in the index, making it
    appear as if there were no conflict.
  * Would place the rename file at the colliding path, and move the
    added file elsewhere, which combined with the lack of higher order
    stage entries felt really odd.  It's not clear to me why the
    rename should take precedence over the add; if one should be moved
    out of the way, they both probably should.
  * In the recursive case, it would do a two way merge of the added
    file and the version of the renamed file on the renamed side,
    completely excluding modifications to the renamed file on the
    unrenamed side of history.

Use the new handle_file_collision() to fix all of these issues.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 merge-recursive.c                    | 137 +++++++++++++++++----------
 t/t6036-recursive-corner-cases.sh    |  24 ++---
 t/t6042-merge-rename-corner-cases.sh |   4 +-
 3 files changed, 101 insertions(+), 64 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 24d979022e..0805095168 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -186,6 +186,7 @@ static int oid_eq(const struct object_id *a, const struct object_id *b)
 enum rename_type {
 	RENAME_NORMAL = 0,
 	RENAME_VIA_DIR,
+	RENAME_ADD,
 	RENAME_DELETE,
 	RENAME_ONE_FILE_TO_ONE,
 	RENAME_ONE_FILE_TO_TWO,
@@ -228,6 +229,7 @@ static inline void setup_rename_conflict_info(enum rename_type rename_type,
 					      struct stage_data *src_entry1,
 					      struct stage_data *src_entry2)
 {
+	int ostage1 = 0, ostage2;
 	struct rename_conflict_info *ci;
 
 	/*
@@ -264,18 +266,22 @@ static inline void setup_rename_conflict_info(enum rename_type rename_type,
 		dst_entry2->rename_conflict_info = ci;
 	}
 
-	if (rename_type == RENAME_TWO_FILES_TO_ONE) {
-		/*
-		 * For each rename, there could have been
-		 * modifications on the side of history where that
-		 * file was not renamed.
-		 */
-		int ostage1 = o->branch1 == branch1 ? 3 : 2;
-		int ostage2 = ostage1 ^ 1;
+	/*
+	 * For each rename, there could have been
+	 * modifications on the side of history where that
+	 * file was not renamed.
+	 */
+	if (rename_type == RENAME_ADD ||
+	    rename_type == RENAME_TWO_FILES_TO_ONE) {
+		ostage1 = o->branch1 == branch1 ? 3 : 2;
 
 		ci->ren1_other.path = pair1->one->path;
 		oidcpy(&ci->ren1_other.oid, &src_entry1->stages[ostage1].oid);
 		ci->ren1_other.mode = src_entry1->stages[ostage1].mode;
+	}
+
+	if (rename_type == RENAME_TWO_FILES_TO_ONE) {
+		ostage2 = ostage1 ^ 1;
 
 		ci->ren2_other.path = pair2->one->path;
 		oidcpy(&ci->ren2_other.oid, &src_entry2->stages[ostage2].oid);
@@ -1559,7 +1565,6 @@ static struct diff_filespec *filespec_from_entry(struct diff_filespec *target,
 	return target;
 }
 
-#if 0 // #if-0-ing avoids unused function warning; will make live in next commit
 static int handle_file_collision(struct merge_options *o,
 				 const char *collide_path,
 				 const char *prev_path1,
@@ -1575,6 +1580,20 @@ static int handle_file_collision(struct merge_options *o,
 	char *alt_path = NULL;
 	const char *update_path = collide_path;
 
+	/*
+	 * It's easiest to get the correct things into stage 2 and 3, and
+	 * to make sure that the content merge puts HEAD before the other
+	 * branch if we just ensure that branch1 == o->branch1.  So, simply
+	 * flip arguments around if we don't have that.
+	 */
+	if (branch1 != o->branch1) {
+		return handle_file_collision(o, collide_path,
+					     prev_path2, prev_path1,
+					     branch2, branch1,
+					     b_oid, b_mode,
+					     a_oid, a_mode);
+	}
+
 	/*
 	 * In the recursive case, we just opt to undo renames
 	 */
@@ -1678,7 +1697,38 @@ static int handle_file_collision(struct merge_options *o,
 	 */
 	return mfi.clean;
 }
-#endif
+
+static int handle_rename_add(struct merge_options *o,
+			     struct rename_conflict_info *ci)
+{
+	/* a was renamed to c, and a separate c was added. */
+	struct diff_filespec *a = ci->pair1->one;
+	struct diff_filespec *c = ci->pair1->two;
+	char *path = c->path;
+	char *prev_path_desc;
+	struct merge_file_info mfi;
+
+	int other_stage = (ci->branch1 == o->branch1 ? 3 : 2);
+
+	output(o, 1, _("CONFLICT (rename/add): "
+	       "Rename %s->%s in %s.  Added %s in %s"),
+	       a->path, c->path, ci->branch1,
+	       c->path, ci->branch2);
+
+	prev_path_desc = xstrfmt("version of %s from %s", path, a->path);
+	if (merge_mode_and_contents(o, a, c, &ci->ren1_other, prev_path_desc,
+				    o->branch1, o->branch2,
+				    1 + o->call_depth * 2, &mfi))
+		return -1;
+	free(prev_path_desc);
+
+	return handle_file_collision(o,
+				     c->path, a->path, NULL,
+				     ci->branch1, ci->branch2,
+				     &mfi.oid, mfi.mode,
+				     &ci->dst_entry1->stages[other_stage].oid,
+				     ci->dst_entry1->stages[other_stage].mode);
+}
 
 static int handle_file(struct merge_options *o,
 			struct diff_filespec *rename,
@@ -2860,47 +2910,23 @@ static int process_renames(struct merge_options *o,
 						      0  /* update_wd    */))
 					clean_merge = -1;
 			} else if (!oid_eq(&dst_other.oid, &null_oid)) {
-				clean_merge = 0;
-				try_merge = 1;
-				output(o, 1, _("CONFLICT (rename/add): Rename %s->%s in %s. "
-				       "%s added in %s"),
-				       ren1_src, ren1_dst, branch1,
-				       ren1_dst, branch2);
-				if (o->call_depth) {
-					struct merge_file_info mfi;
-					struct diff_filespec one, a, b;
-
-					oidcpy(&one.oid, &null_oid);
-					one.mode = 0;
-					one.path = ren1->pair->two->path;
-
-					oidcpy(&a.oid, &ren1->pair->two->oid);
-					a.mode = ren1->pair->two->mode;
-					a.path = one.path;
-
-					oidcpy(&b.oid, &dst_other.oid);
-					b.mode = dst_other.mode;
-					b.path = one.path;
-
-					if (merge_mode_and_contents(o, &one, &a, &b, ren1_dst,
-								    branch1, branch2,
-								    o->call_depth * 2, &mfi)) {
-						clean_merge = -1;
-						goto cleanup_and_return;
-					}
-					output(o, 1, _("Adding merged %s"), ren1_dst);
-					if (update_file(o, 0, &mfi.oid,
-							mfi.mode, ren1_dst))
-						clean_merge = -1;
-					try_merge = 0;
-				} else {
-					char *new_path = unique_path(o, ren1_dst, branch2);
-					output(o, 1, _("Adding as %s instead"), new_path);
-					if (update_file(o, 0, &dst_other.oid,
-							dst_other.mode, new_path))
-						clean_merge = -1;
-					free(new_path);
-				}
+				/*
+				 * Probably not a clean merge, but it's
+				 * premature to set clean_merge to 0 here,
+				 * because if the rename merges cleanly and
+				 * the merge exactly matches the newly added
+				 * file, then the merge will be clean.
+				 */
+				setup_rename_conflict_info(RENAME_ADD,
+							   ren1->pair,
+							   NULL,
+							   branch1,
+							   branch2,
+							   ren1->dst_entry,
+							   NULL,
+							   o,
+							   ren1->src_entry,
+							   NULL);
 			} else
 				try_merge = 1;
 
@@ -3312,6 +3338,15 @@ static int process_entry(struct merge_options *o,
 						  conflict_info->branch2))
 				clean_merge = -1;
 			break;
+		case RENAME_ADD:
+			/*
+			 * Probably unclean merge, but if the renamed file
+			 * merges cleanly and the result can then be
+			 * two-way merged cleanly with the added file, I
+			 * guess it's a clean merge?
+			 */
+			clean_merge = handle_rename_add(o, conflict_info);
+			break;
 		case RENAME_DELETE:
 			clean_merge = 0;
 			if (handle_rename_delete(o,
diff --git a/t/t6036-recursive-corner-cases.sh b/t/t6036-recursive-corner-cases.sh
index 3091cbc06a..d6b7e568f5 100755
--- a/t/t6036-recursive-corner-cases.sh
+++ b/t/t6036-recursive-corner-cases.sh
@@ -185,7 +185,7 @@ test_expect_success 'setup differently handled merges of rename/add conflict' '
 		git branch B &&
 		git checkout -b C &&
 		echo 10 >>a &&
-		echo "other content" >>new_a &&
+		test_write_lines 0 1 2 3 4 5 6 7 foobar >new_a &&
 		git add a new_a &&
 		test_tick && git commit -m C &&
 
@@ -195,14 +195,14 @@ test_expect_success 'setup differently handled merges of rename/add conflict' '
 
 		git checkout B^0 &&
 		test_must_fail git merge C &&
-		git clean -f &&
+		git show :2:new_a >new_a &&
+		git add new_a &&
 		test_tick && git commit -m D &&
 		git tag D &&
 
 		git checkout C^0 &&
 		test_must_fail git merge B &&
-		rm new_a~HEAD new_a &&
-		printf "Incorrectly merged content" >>new_a &&
+		test_write_lines 0 1 2 3 4 5 6 7 bad_merge >new_a &&
 		git add -u &&
 		test_tick && git commit -m E &&
 		git tag E
@@ -225,21 +225,23 @@ test_expect_success 'git detects differently handled merges conflict' '
 		test_line_count = 1 out &&
 
 		git rev-parse >expect       \
-			D:new_a  E:new_a &&
+			C:new_a  D:new_a  E:new_a &&
 		git rev-parse   >actual     \
-			:2:new_a :3:new_a &&
+			:1:new_a :2:new_a :3:new_a &&
 		test_cmp expect actual &&
 
-		git cat-file -p C:new_a >ours &&
-		git cat-file -p B:new_a >theirs &&
+		# Test that the two-way merge in new_a is as expected
+		git cat-file -p D:new_a >ours &&
+		git cat-file -p E:new_a >theirs &&
 		>empty &&
 		test_must_fail git merge-file \
-			-L "Temporary merge branch 1" \
+			-L "HEAD" \
 			-L "" \
-			-L "Temporary merge branch 2" \
+			-L "E^0" \
 			ours empty theirs &&
 		sed -e "s/^\([<=>]\)/\1\1\1/" ours >expect &&
-		git cat-file -p :1:new_a >actual &&
+		git hash-object new_a >actual &&
+		git hash-object ours  >expect &&
 		test_cmp expect actual
 	)
 '
diff --git a/t/t6042-merge-rename-corner-cases.sh b/t/t6042-merge-rename-corner-cases.sh
index 23c3b6dffa..3e934ae89e 100755
--- a/t/t6042-merge-rename-corner-cases.sh
+++ b/t/t6042-merge-rename-corner-cases.sh
@@ -1095,8 +1095,8 @@ test_conflicts_with_adds_and_renames() {
 }
 
 test_conflicts_with_adds_and_renames rename rename failure
-test_conflicts_with_adds_and_renames rename add    failure
-test_conflicts_with_adds_and_renames add    rename failure
+test_conflicts_with_adds_and_renames rename add    success
+test_conflicts_with_adds_and_renames add    rename success
 test_conflicts_with_adds_and_renames add    add    success
 
 # Setup:
-- 
2.19.0.232.gd14c2061fc


  parent reply	other threads:[~2018-11-02 18:53 UTC|newest]

Thread overview: 52+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-10-14  2:05 [RFC PATCH v2 0/7] Improve path collision conflict resolutions Elijah Newren
2018-10-14  2:05 ` [RFC PATCH v2 1/7] Add testcases for consistency in file collision conflict handling Elijah Newren
2018-10-14  2:05 ` [RFC PATCH v2 2/7] t6036, t6042: testcases for rename collision of already conflicting files Elijah Newren
2018-10-14  2:05 ` [RFC PATCH v2 3/7] merge-recursive: new function for better colliding conflict resolutions Elijah Newren
2018-10-14  2:05 ` [RFC PATCH v2 4/7] merge-recursive: fix rename/add conflict handling Elijah Newren
2018-10-14  2:05 ` [RFC PATCH v2 5/7] merge-recursive: improve handling for rename/rename(2to1) conflicts Elijah Newren
2018-10-14  2:05 ` [RFC PATCH v2 6/7] merge-recursive: use handle_file_collision for add/add conflicts Elijah Newren
2018-10-14  2:05 ` [RFC PATCH v2 7/7] merge-recursive: improve rename/rename(1to2)/add[/add] handling Elijah Newren
2018-10-19 19:31 ` [PATCH v3 0/8] Improve path collision conflict resolutions Elijah Newren
2018-10-19 19:31   ` [PATCH v3 1/8] Add testcases for consistency in file collision conflict handling Elijah Newren
2018-10-19 19:31   ` [PATCH v3 2/8] t6036, t6042: testcases for rename collision of already conflicting files Elijah Newren
2018-10-31 14:01     ` Derrick Stolee
2018-11-01  6:57       ` Elijah Newren
2018-10-19 19:31   ` [PATCH v3 3/8] merge-recursive: increase marker length with depth of recursion Elijah Newren
2018-10-19 19:31   ` [PATCH v3 4/8] merge-recursive: new function for better colliding conflict resolutions Elijah Newren
2018-10-31 13:53     ` Derrick Stolee
2018-10-31 13:57       ` Derrick Stolee
2018-11-01  6:56         ` Elijah Newren
2018-10-19 19:31   ` [PATCH v3 5/8] merge-recursive: fix rename/add conflict handling Elijah Newren
2018-10-19 19:31   ` [PATCH v3 6/8] merge-recursive: improve handling for rename/rename(2to1) conflicts Elijah Newren
2018-10-19 19:31   ` [PATCH v3 7/8] merge-recursive: use handle_file_collision for add/add conflicts Elijah Newren
2018-10-19 19:31   ` [PATCH v3 8/8] merge-recursive: improve rename/rename(1to2)/add[/add] handling Elijah Newren
2018-10-31 15:08     ` Derrick Stolee
2018-11-01  7:01       ` Elijah Newren
2018-11-02 17:27         ` Elijah Newren
2018-11-02 17:30           ` Derrick Stolee
2018-11-02 18:53   ` [PATCH v4 00/10] Improve path collision conflict resolutions Elijah Newren
2018-11-02 18:53     ` [PATCH v4 01/10] Add testcases for consistency in file collision conflict handling Elijah Newren
2018-11-02 18:53     ` [PATCH v4 02/10] t6036, t6042: testcases for rename collision of already conflicting files Elijah Newren
2018-11-02 18:53     ` [PATCH v4 03/10] merge-recursive: increase marker length with depth of recursion Elijah Newren
2018-11-02 18:53     ` [PATCH v4 04/10] merge-recursive: new function for better colliding conflict resolutions Elijah Newren
2018-11-02 18:53     ` Elijah Newren [this message]
2018-11-02 18:53     ` [PATCH v4 06/10] merge-recursive: improve handling for rename/rename(2to1) conflicts Elijah Newren
2018-11-02 18:53     ` [PATCH v4 07/10] merge-recursive: use handle_file_collision for add/add conflicts Elijah Newren
2018-11-02 18:53     ` [PATCH v4 08/10] merge-recursive: improve rename/rename(1to2)/add[/add] handling Elijah Newren
2018-11-02 20:01       ` [PATCH] merge-recursive: combine error handling Derrick Stolee
2018-11-02 18:53     ` [RFC PATCH v4 09/10] fixup! merge-recursive: fix rename/add conflict handling Elijah Newren
2018-11-02 19:05     ` [PATCH v4 10/10] fixup! merge-recursive: improve rename/rename(1to2)/add[/add] handling Elijah Newren
2018-11-02 19:09     ` [PATCH v4 00/10] Improve path collision conflict resolutions Derrick Stolee
2018-11-02 20:06       ` Elijah Newren
2018-11-08  4:40         ` [PATCH v5 " Elijah Newren
2018-11-08  4:40           ` [PATCH v5 01/10] Add testcases for consistency in file collision conflict handling Elijah Newren
2018-11-08  4:40           ` [PATCH v5 02/10] t6036, t6042: testcases for rename collision of already conflicting files Elijah Newren
2018-11-08  4:40           ` [PATCH v5 03/10] merge-recursive: increase marker length with depth of recursion Elijah Newren
2018-11-08  4:40           ` [PATCH v5 04/10] merge-recursive: new function for better colliding conflict resolutions Elijah Newren
2018-11-08  4:40           ` [PATCH v5 05/10] merge-recursive: fix rename/add conflict handling Elijah Newren
2018-11-08  4:40           ` [PATCH v5 06/10] merge-recursive: improve handling for rename/rename(2to1) conflicts Elijah Newren
2018-11-08  4:40           ` [PATCH v5 07/10] merge-recursive: use handle_file_collision for add/add conflicts Elijah Newren
2018-11-08  4:40           ` [PATCH v5 08/10] merge-recursive: improve rename/rename(1to2)/add[/add] handling Elijah Newren
2018-11-08  4:40           ` [PATCH v5 09/10] t6036, t6043: increase code coverage for file collision handling Elijah Newren
2018-11-08  4:40           ` [PATCH v5 10/10] merge-recursive: combine error handling Elijah Newren
2018-11-08  5:25             ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20181102185317.31015-6-newren@gmail.com \
    --to=newren@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=stolee@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).