diff mbox series

[v3,8/8] merge-recursive: improve rename/rename(1to2)/add[/add] handling

Message ID 20181019193111.12051-9-newren@gmail.com (mailing list archive)
State New, archived
Headers show
Series Improve path collision conflict resolutions | expand

Commit Message

Elijah Newren Oct. 19, 2018, 7:31 p.m. UTC 
When we have a rename/rename(1to2) conflict, each of the renames can
collide with a file addition.  Each of these rename/add conflicts suffered
from the same kinds of problems that normal rename/add suffered from.
Make the code use handle_file_conflicts() as well so that we get all the
same fixes and consistent behavior between the different conflict types.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 merge-recursive.c                    | 154 +++++++++++++--------------
 t/t6042-merge-rename-corner-cases.sh |  29 +++--
 t/t6043-merge-rename-directories.sh  |  24 +++--
 3 files changed, 113 insertions(+), 94 deletions(-)

Comments

Derrick Stolee Oct. 31, 2018, 3:08 p.m. UTC | #1 
On 10/19/2018 3:31 PM, Elijah Newren wrote:
> [snip]
>
> +			char *new_path = NULL;
> +			if (dir_in_way(b->path, !o->call_depth, 0)) {
> +				new_path = unique_path(o, b->path, ci->branch2);
> +				output(o, 1, _("%s is a directory in %s adding "
> +					       "as %s instead"),
> +				       b->path, ci->branch1, new_path);

I tried really hard, but failed to get a test to cover the block below. 
I was able to
find that the "check handling of differently renamed file with D/F 
conflicts" test
in t6022-merge-rename.sh covers the block above. Trying to tweak the 
example using
untracked files seems to hit an error message from unpack-trees.c instead.

> +			} else if (would_lose_untracked(b->path)) {
> +				new_path = unique_path(o, b->path, ci->branch2);
> +				output(o, 1, _("Refusing to lose untracked file"
> +					       " at %s; adding as %s instead"),
> +				       b->path, new_path);

It could also be that I failed because I'm less familiar with this part 
of the
codebase. Elijah, do you think it is possible to hit this block?

Thanks,
-Stolee
Elijah Newren Nov. 1, 2018, 7:01 a.m. UTC | #2 
On Wed, Oct 31, 2018 at 8:08 AM Derrick Stolee <stolee@gmail.com> wrote:
>
> On 10/19/2018 3:31 PM, Elijah Newren wrote:
> > [snip]
> >
> > +                     char *new_path = NULL;
> > +                     if (dir_in_way(b->path, !o->call_depth, 0)) {
> > +                             new_path = unique_path(o, b->path, ci->branch2);
> > +                             output(o, 1, _("%s is a directory in %s adding "
> > +                                            "as %s instead"),
> > +                                    b->path, ci->branch1, new_path);
>
> I tried really hard, but failed to get a test to cover the block below.
> I was able to
> find that the "check handling of differently renamed file with D/F
> conflicts" test
> in t6022-merge-rename.sh covers the block above. Trying to tweak the
> example using
> untracked files seems to hit an error message from unpack-trees.c instead.
>
> > +                     } else if (would_lose_untracked(b->path)) {
> > +                             new_path = unique_path(o, b->path, ci->branch2);
> > +                             output(o, 1, _("Refusing to lose untracked file"
> > +                                            " at %s; adding as %s instead"),
> > +                                    b->path, new_path);
>
> It could also be that I failed because I'm less familiar with this part
> of the
> codebase. Elijah, do you think it is possible to hit this block?

Yeah, this one's going to be a little harder; the upper block would be
done with a D/F, but I think for this block you'd need a directory
rename so that unpack-trees.c can't tell that the untracked file in
the way is actually in the way of anything.  But since this is in the
rename/rename(1to2) area, I think I had rules around avoiding doing
directory renames if the other side renamed the file to avoid getting
into rename/rename(1to3) situations and other weirdness.  So, it might
require a transitive rename (i.e. file renamed on both sides, and on
one side it's renamed into a directory that the other side renamed
away).

I'll try to take a look at it tomorrow, with everything else.  We'll
see how much I can get done.

Thanks for digging in to all these and bringing them up.
Elijah Newren Nov. 2, 2018, 5:27 p.m. UTC | #3 
On Thu, Nov 1, 2018 at 12:01 AM Elijah Newren <newren@gmail.com> wrote:
>
> On Wed, Oct 31, 2018 at 8:08 AM Derrick Stolee <stolee@gmail.com> wrote:
> >
> > On 10/19/2018 3:31 PM, Elijah Newren wrote:
> > > [snip]
> > >
> > > +                     char *new_path = NULL;
> > > +                     if (dir_in_way(b->path, !o->call_depth, 0)) {
> > > +                             new_path = unique_path(o, b->path, ci->branch2);
> > > +                             output(o, 1, _("%s is a directory in %s adding "
> > > +                                            "as %s instead"),
> > > +                                    b->path, ci->branch1, new_path);
> >
> > I tried really hard, but failed to get a test to cover the block below.
> > I was able to
> > find that the "check handling of differently renamed file with D/F
> > conflicts" test
> > in t6022-merge-rename.sh covers the block above. Trying to tweak the
> > example using
> > untracked files seems to hit an error message from unpack-trees.c instead.
> >
> > > +                     } else if (would_lose_untracked(b->path)) {
> > > +                             new_path = unique_path(o, b->path, ci->branch2);
> > > +                             output(o, 1, _("Refusing to lose untracked file"
> > > +                                            " at %s; adding as %s instead"),
> > > +                                    b->path, new_path);
> >

So now I'm confused.  This block was not listed in your coverage
report[1].  And, in fact, I think this block IS covered by testcase
10c of t6043.  However, there is a very similar looking block about 30
lines up that is uncovered (and which was mentioned in your report):

            } else if (would_lose_untracked(a->path)) {
                new_path = unique_path(o, a->path, ci->branch1);
                output(o, 1, _("Refusing to lose untracked file"
                           " at %s; adding as %s instead"),
                       a->path, new_path);

covering it, I think, is just a matter of repeating the 10c test with
the merge repeated in the other direction (checkout B and merge A
instead of checking out A and merging B) -- and touching up the checks
accordingly.

However, now I'm wondering if I'm crazy.  Was it really the block you
had highlighted that you were seeing uncovered?

Thanks,
Elijah

[1] https://public-inbox.org/git/62f0bcf6-aa73-c192-d804-e6d69cac146f@gmail.com/
Derrick Stolee Nov. 2, 2018, 5:30 p.m. UTC | #4 
On 11/2/2018 1:27 PM, Elijah Newren wrote:
> On Thu, Nov 1, 2018 at 12:01 AM Elijah Newren <newren@gmail.com> wrote:
>> On Wed, Oct 31, 2018 at 8:08 AM Derrick Stolee <stolee@gmail.com> wrote:
>>> On 10/19/2018 3:31 PM, Elijah Newren wrote:
>>>> [snip]
>>>>
>>>> +                     char *new_path = NULL;
>>>> +                     if (dir_in_way(b->path, !o->call_depth, 0)) {
>>>> +                             new_path = unique_path(o, b->path, ci->branch2);
>>>> +                             output(o, 1, _("%s is a directory in %s adding "
>>>> +                                            "as %s instead"),
>>>> +                                    b->path, ci->branch1, new_path);
>>> I tried really hard, but failed to get a test to cover the block below.
>>> I was able to
>>> find that the "check handling of differently renamed file with D/F
>>> conflicts" test
>>> in t6022-merge-rename.sh covers the block above. Trying to tweak the
>>> example using
>>> untracked files seems to hit an error message from unpack-trees.c instead.
>>>
>>>> +                     } else if (would_lose_untracked(b->path)) {
>>>> +                             new_path = unique_path(o, b->path, ci->branch2);
>>>> +                             output(o, 1, _("Refusing to lose untracked file"
>>>> +                                            " at %s; adding as %s instead"),
>>>> +                                    b->path, new_path);
> So now I'm confused.  This block was not listed in your coverage
> report[1].  And, in fact, I think this block IS covered by testcase
> 10c of t6043.  However, there is a very similar looking block about 30
> lines up that is uncovered (and which was mentioned in your report):
>
>              } else if (would_lose_untracked(a->path)) {
>                  new_path = unique_path(o, a->path, ci->branch1);
>                  output(o, 1, _("Refusing to lose untracked file"
>                             " at %s; adding as %s instead"),
>                         a->path, new_path);
>
> covering it, I think, is just a matter of repeating the 10c test with
> the merge repeated in the other direction (checkout B and merge A
> instead of checking out A and merging B) -- and touching up the checks
> accordingly.
>
> However, now I'm wondering if I'm crazy.  Was it really the block you
> had highlighted that you were seeing uncovered?

Trust the report (generated by computer) over me (generated by squinting 
at an email, trying to match line numbers).

Thanks,

-Stolee
diff mbox series

Patch

diff --git a/merge-recursive.c b/merge-recursive.c
index c78b347112..59811116b6 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1709,80 +1709,17 @@  static int handle_rename_add(struct merge_options *o,
 				     ci->dst_entry1->stages[other_stage].mode);
 }
 
-static int handle_file(struct merge_options *o,
-			struct diff_filespec *rename,
-			int stage,
-			struct rename_conflict_info *ci)
-{
-	char *dst_name = rename->path;
-	struct stage_data *dst_entry;
-	const char *cur_branch, *other_branch;
-	struct diff_filespec other;
-	struct diff_filespec *add;
-	int ret;
-
-	if (stage == 2) {
-		dst_entry = ci->dst_entry1;
-		cur_branch = ci->branch1;
-		other_branch = ci->branch2;
-	} else {
-		dst_entry = ci->dst_entry2;
-		cur_branch = ci->branch2;
-		other_branch = ci->branch1;
-	}
-
-	add = filespec_from_entry(&other, dst_entry, stage ^ 1);
-	if (add) {
-		int ren_src_was_dirty = was_dirty(o, rename->path);
-		char *add_name = unique_path(o, rename->path, other_branch);
-		if (update_file(o, 0, &add->oid, add->mode, add_name))
-			return -1;
-
-		if (ren_src_was_dirty) {
-			output(o, 1, _("Refusing to lose dirty file at %s"),
-			       rename->path);
-		}
-		/*
-		 * Because the double negatives somehow keep confusing me...
-		 *    1) update_wd iff !ren_src_was_dirty.
-		 *    2) no_wd iff !update_wd
-		 *    3) so, no_wd == !!ren_src_was_dirty == ren_src_was_dirty
-		 */
-		remove_file(o, 0, rename->path, ren_src_was_dirty);
-		dst_name = unique_path(o, rename->path, cur_branch);
-	} else {
-		if (dir_in_way(rename->path, !o->call_depth, 0)) {
-			dst_name = unique_path(o, rename->path, cur_branch);
-			output(o, 1, _("%s is a directory in %s adding as %s instead"),
-			       rename->path, other_branch, dst_name);
-		} else if (!o->call_depth &&
-			   would_lose_untracked(rename->path)) {
-			dst_name = unique_path(o, rename->path, cur_branch);
-			output(o, 1, _("Refusing to lose untracked file at %s; "
-				       "adding as %s instead"),
-			       rename->path, dst_name);
-		}
-	}
-	if ((ret = update_file(o, 0, &rename->oid, rename->mode, dst_name)))
-		; /* fall through, do allow dst_name to be released */
-	else if (stage == 2)
-		ret = update_stages(o, rename->path, NULL, rename, add);
-	else
-		ret = update_stages(o, rename->path, NULL, add, rename);
-
-	if (dst_name != rename->path)
-		free(dst_name);
-
-	return ret;
-}
-
 static int handle_rename_rename_1to2(struct merge_options *o,
 				     struct rename_conflict_info *ci)
 {
 	/* One file was renamed in both branches, but to different names. */
+	struct merge_file_info mfi;
+	struct diff_filespec other;
+	struct diff_filespec *add;
 	struct diff_filespec *one = ci->pair1->one;
 	struct diff_filespec *a = ci->pair1->two;
 	struct diff_filespec *b = ci->pair2->two;
+	char *path_desc;
 
 	output(o, 1, _("CONFLICT (rename/rename): "
 	       "Rename \"%s\"->\"%s\" in branch \"%s\" "
@@ -1790,15 +1727,16 @@  static int handle_rename_rename_1to2(struct merge_options *o,
 	       one->path, a->path, ci->branch1,
 	       one->path, b->path, ci->branch2,
 	       o->call_depth ? _(" (left unresolved)") : "");
-	if (o->call_depth) {
-		struct merge_file_info mfi;
-		struct diff_filespec other;
-		struct diff_filespec *add;
-		if (merge_mode_and_contents(o, one, a, b, one->path,
-					    ci->branch1, ci->branch2,
-					    o->call_depth * 2, &mfi))
-			return -1;
 
+	path_desc = xstrfmt("%s and %s, both renamed from %s",
+			    a->path, b->path, one->path);
+	if (merge_mode_and_contents(o, one, a, b, path_desc,
+				    ci->branch1, ci->branch2,
+				    o->call_depth * 2, &mfi))
+		return -1;
+	free(path_desc);
+
+	if (o->call_depth) {
 		/*
 		 * FIXME: For rename/add-source conflicts (if we could detect
 		 * such), this is wrong.  We should instead find a unique
@@ -1830,8 +1768,70 @@  static int handle_rename_rename_1to2(struct merge_options *o,
 		}
 		else
 			remove_file_from_cache(b->path);
-	} else if (handle_file(o, a, 2, ci) || handle_file(o, b, 3, ci))
-		return -1;
+	} else {
+		/*
+		 * For each destination path, we need to see if there is a
+		 * rename/add collision.  If not, we can write the file out
+		 * to the specified location.
+		 */
+		add = filespec_from_entry(&other, ci->dst_entry1, 2 ^ 1);
+		if (add) {
+			if (handle_file_collision(o, a->path,
+						  NULL, NULL,
+						  ci->branch1, ci->branch2,
+						  &mfi.oid, mfi.mode,
+						  &add->oid, add->mode) < 0)
+				return -1;
+		} else {
+			char *new_path = NULL;
+			if (dir_in_way(a->path, !o->call_depth, 0)) {
+				new_path = unique_path(o, a->path, ci->branch1);
+				output(o, 1, _("%s is a directory in %s adding "
+					       "as %s instead"),
+				       a->path, ci->branch2, new_path);
+			} else if (would_lose_untracked(a->path)) {
+				new_path = unique_path(o, a->path, ci->branch1);
+				output(o, 1, _("Refusing to lose untracked file"
+					       " at %s; adding as %s instead"),
+				       a->path, new_path);
+			}
+
+			if (update_file(o, 0, &mfi.oid, mfi.mode, new_path ? new_path : a->path))
+				return -1;
+			free(new_path);
+			if (update_stages(o, a->path, NULL, a, NULL))
+				return -1;
+		}
+
+		add = filespec_from_entry(&other, ci->dst_entry2, 3 ^ 1);
+		if (add) {
+			if (handle_file_collision(o, b->path,
+						  NULL, NULL,
+						  ci->branch1, ci->branch2,
+						  &add->oid, add->mode,
+						  &mfi.oid, mfi.mode) < 0)
+				return -1;
+		} else {
+			char *new_path = NULL;
+			if (dir_in_way(b->path, !o->call_depth, 0)) {
+				new_path = unique_path(o, b->path, ci->branch2);
+				output(o, 1, _("%s is a directory in %s adding "
+					       "as %s instead"),
+				       b->path, ci->branch1, new_path);
+			} else if (would_lose_untracked(b->path)) {
+				new_path = unique_path(o, b->path, ci->branch2);
+				output(o, 1, _("Refusing to lose untracked file"
+					       " at %s; adding as %s instead"),
+				       b->path, new_path);
+			}
+
+			if (update_file(o, 0, &mfi.oid, mfi.mode, new_path ? new_path : b->path))
+				return -1;
+			free(new_path);
+			if (update_stages(o, b->path, NULL, NULL, b))
+				return -1;
+		}
+	}
 
 	return 0;
 }
diff --git a/t/t6042-merge-rename-corner-cases.sh b/t/t6042-merge-rename-corner-cases.sh
index 4cfcde3082..534f7b55a1 100755
--- a/t/t6042-merge-rename-corner-cases.sh
+++ b/t/t6042-merge-rename-corner-cases.sh
@@ -684,7 +684,7 @@  test_expect_success 'rename/rename/add-dest merge still knows about conflicting
 		git ls-files -u c >out &&
 		test_line_count = 2 out &&
 		git ls-files -o >out &&
-		test_line_count = 5 out &&
+		test_line_count = 1 out &&
 
 		git rev-parse >expect               \
 			A:a   C:b   B:b   C:c   B:c &&
@@ -692,14 +692,27 @@  test_expect_success 'rename/rename/add-dest merge still knows about conflicting
 			:1:a  :2:b  :3:b  :2:c  :3:c &&
 		test_cmp expect actual &&
 
-		git rev-parse >expect               \
-			C:c     B:c     C:b     B:b &&
-		git hash-object >actual                \
-			c~HEAD  c~B\^0  b~HEAD  b~B\^0 &&
-		test_cmp expect actual &&
+		# Record some contents for re-doing merges
+		git cat-file -p A:a >stuff &&
+		git cat-file -p C:b >important_info &&
+		git cat-file -p B:c >precious_data &&
+		>empty &&
 
-		test_path_is_missing b &&
-		test_path_is_missing c
+		# Test the merge in b
+		test_must_fail git merge-file \
+			-L "HEAD" \
+			-L "" \
+			-L "B^0" \
+			important_info empty stuff &&
+		test_cmp important_info b &&
+
+		# Test the merge in c
+		test_must_fail git merge-file \
+			-L "HEAD" \
+			-L "" \
+			-L "B^0" \
+			stuff empty precious_data &&
+		test_cmp stuff c
 	)
 '
 
diff --git a/t/t6043-merge-rename-directories.sh b/t/t6043-merge-rename-directories.sh
index fedaeafc55..5c01a0c14a 100755
--- a/t/t6043-merge-rename-directories.sh
+++ b/t/t6043-merge-rename-directories.sh
@@ -1078,7 +1078,7 @@  test_expect_success '5c-check: Transitive rename would cause rename/rename/renam
 		git ls-files -u >out &&
 		test_line_count = 6 out &&
 		git ls-files -o >out &&
-		test_line_count = 3 out &&
+		test_line_count = 1 out &&
 
 		git rev-parse >actual \
 			:0:y/b :0:y/c :0:y/e &&
@@ -1094,9 +1094,9 @@  test_expect_success '5c-check: Transitive rename would cause rename/rename/renam
 		test_cmp expect actual &&
 
 		git hash-object >actual \
-			w/d~HEAD w/d~B^0 z/d &&
+			z/d &&
 		git rev-parse >expect \
-			O:x/d    B:w/d   O:x/d &&
+			O:x/d &&
 		test_cmp expect actual &&
 		test_path_is_missing x/d &&
 		test_path_is_file y/d &&
@@ -3672,7 +3672,7 @@  test_expect_success '11e-check: Avoid deleting not-uptodate with dir rename/rena
 		git ls-files -u >out &&
 		test_line_count = 4 out &&
 		git ls-files -o >out &&
-		test_line_count = 4 out &&
+		test_line_count = 3 out &&
 
 		echo different >expected &&
 		echo mods >>expected &&
@@ -3684,11 +3684,17 @@  test_expect_success '11e-check: Avoid deleting not-uptodate with dir rename/rena
 			 O:z/a  O:z/b  O:x/d  O:x/c  O:x/c  A:y/c  O:x/c &&
 		test_cmp expect actual &&
 
-		git hash-object >actual \
-			y/c~B^0 y/c~HEAD &&
-		git rev-parse >expect \
-			O:x/c   A:y/c &&
-		test_cmp expect actual
+		# See if y/c~merged has expected contents; requires manually
+		# doing the expected file merge
+		git cat-file -p A:y/c >c1 &&
+		git cat-file -p B:z/c >c2 &&
+		>empty &&
+		test_must_fail git merge-file \
+			-L "HEAD" \
+			-L "" \
+			-L "B^0" \
+			c1 empty c2 &&
+		test_cmp c1 y/c~merged
 	)
 '