[PATCH 0/5] D/F conflict fixes

[PATCH 0/5] D/F conflict fixes

[newren]

This patch series fixes a number of spurious directory/file conflicts
that I have (or have seen others) run across in cherry-pick, rebase,
merge, and fast-import, while fixing already known failures in both
t6020-merge-df.sh and t6035-merge-dir-to-symlink.sh.

It also involves an extra testcase posted by Alexander Gladysh to the
list on March 8; I hope it's not bad form for me to put his testcase
into the testsuite and submit it.

Alexander Gladysh (1):
      Add another rename + D/F conflict testcase

Elijah Newren (4):
      Add additional testcases for D/F conflicts
      merge-recursive: Fix D/F conflicts
      merge_recursive: Fix renames across paths below D/F conflicts
      fast-import: Handle directories changing into symlinks

 fast-import.c                   |    5 ++
 merge-recursive.c               |  106 ++++++++++++++++++++++++++++++++-------
 t/t6020-merge-df.sh             |   34 ++++++++++++-
 t/t6035-merge-dir-to-symlink.sh |   37 +++++++++++++-
 t/t9350-fast-export.sh          |   24 +++++++++
 5 files changed, 185 insertions(+), 21 deletions(-)

[PATCH 1/5] Add additional testcases for D/F conflicts

[newren]

From: Elijah Newren <newren@gmail.com>


Signed-off-by: Elijah Newren <newren@gmail.com>
---
 t/t6035-merge-dir-to-symlink.sh |   37 +++++++++++++++++++++++++++++++++++--
 t/t9350-fast-export.sh          |   24 ++++++++++++++++++++++++
 2 files changed, 59 insertions(+), 2 deletions(-)

diff --git a/t/t6035-merge-dir-to-symlink.sh b/t/t6035-merge-dir-to-symlink.sh
index cd3190c..4f04f41 100755
--- a/t/t6035-merge-dir-to-symlink.sh
+++ b/t/t6035-merge-dir-to-symlink.sh
@@ -56,7 +56,7 @@ test_expect_success 'do not lose a/b-2/c/d in merge (resolve)' '
 	test -f a/b-2/c/d
 '
 
-test_expect_failure 'do not lose a/b-2/c/d in merge (recursive)' '
+test_expect_failure 'Handle D/F conflict, do not lose a/b-2/c/d in merge (recursive)' '
 	git reset --hard &&
 	git checkout baseline^0 &&
 	git merge -s recursive master &&
@@ -64,6 +64,31 @@ test_expect_failure 'do not lose a/b-2/c/d in merge (recursive)' '
 	test -f a/b-2/c/d
 '
 
+test_expect_failure 'Handle F/D conflict, do not lose a/b-2/c/d in merge (recursive)' '
+	git reset --hard &&
+	git checkout master &&
+	git merge -s recursive baseline^0 &&
+	test -h a/b &&
+	test -f a/b-2/c/d
+'
+
+test_expect_success 'do not lose untracked in merge (recursive)' '
+	git reset --hard &&
+	git checkout baseline^0 &&
+	touch a/b/c/e &&
+	test_must_fail git merge -s recursive master &&
+	test -f a/b/c/e &&
+	test -f a/b-2/c/d
+'
+
+test_expect_success 'do not lose modifications in merge (recursive)' '
+	git add --all &&
+	git reset --hard &&
+	git checkout baseline^0 &&
+	echo more content >> a/b/c/d &&
+	test_must_fail git merge -s recursive master
+'
+
 test_expect_success 'setup a merge where dir a/b-2 changed to symlink' '
 	git reset --hard &&
 	git checkout start^0 &&
@@ -82,7 +107,7 @@ test_expect_success 'merge should not have conflicts (resolve)' '
 	test -f a/b/c/d
 '
 
-test_expect_failure 'merge should not have conflicts (recursive)' '
+test_expect_failure 'merge should not have D/F conflicts (recursive)' '
 	git reset --hard &&
 	git checkout baseline^0 &&
 	git merge -s recursive test2 &&
@@ -90,4 +115,12 @@ test_expect_failure 'merge should not have conflicts (recursive)' '
 	test -f a/b/c/d
 '
 
+test_expect_failure 'merge should not have F/D conflicts (recursive)' '
+	git reset --hard &&
+	git checkout -b foo test2 &&
+	git merge -s recursive baseline^0 &&
+	test -h a/b-2 &&
+	test -f a/b/c/d
+'
+
 test_done
diff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh
index d43f37c..69179c6 100755
--- a/t/t9350-fast-export.sh
+++ b/t/t9350-fast-export.sh
@@ -376,4 +376,28 @@ test_expect_success 'tree_tag-obj'    'git fast-export tree_tag-obj'
 test_expect_success 'tag-obj_tag'     'git fast-export tag-obj_tag'
 test_expect_success 'tag-obj_tag-obj' 'git fast-export tag-obj_tag-obj'
 
+test_expect_failure 'directory becomes symlink'        '
+	git init dirtosymlink &&
+	git init result &&
+	(
+		cd dirtosymlink &&
+		mkdir foo &&
+		mkdir bar &&
+		echo hello > foo/world &&
+		echo hello > bar/world &&
+		git add foo/world bar/world &&
+		git commit -q -mone &&
+		git rm -r foo &&
+		ln -s bar foo &&
+		git add foo &&
+		git commit -q -mtwo
+	) &&
+	(
+		cd dirtosymlink &&
+		git fast-export master -- foo |
+		(cd ../result && git fast-import --quiet)
+	) &&
+	(cd result && git show master:foo)
+'
+
 test_done
-- 
1.7.2.rc0.212.g0c601

[PATCH 2/5] Add another rename + D/F conflict testcase

[newren]

From: Alexander Gladysh <agladysh@gmail.com>

This is a simple testcase where both sides of the rename are paths involved
in (separate) D/F merge conflicts.
---
I hope it's not bad style to take someone else's testcase from the mailing
list and submit it on their behalf as a testsuite addition (nor do I know
what to do about the signed-off-by line in this case).  This is simply the
testcase Alexander Gladysh posted to the list on March 8.  I really like
his example due to how it serves as a simple case where there are two D/F
conflicts with a rename across paths involved in both of those D/F
conflicts.

I'm trying to submit this with Alexander listed as the author, but I'm not
sure how to preserve that when using git-send-email.

 t/t6020-merge-df.sh |   32 ++++++++++++++++++++++++++++++++
 1 files changed, 32 insertions(+), 0 deletions(-)

diff --git a/t/t6020-merge-df.sh b/t/t6020-merge-df.sh
index e71c687..99acb89 100755
--- a/t/t6020-merge-df.sh
+++ b/t/t6020-merge-df.sh
@@ -45,4 +45,36 @@ test_expect_failure 'F/D conflict' '
 	git merge master
 '
 
+test_expect_success 'Setup rename across paths each below D/F conflicts' '
+	git symbolic-ref HEAD refs/heads/newmaster &&
+	rm .git/index &&
+	git clean -fdx &&
+
+	mkdir a &&
+	touch a/f &&
+	git add a &&
+	git commit -m "a" &&
+
+	mkdir b &&
+	ln -s ../a b/a &&
+	git add b &&
+	git commit -m "b" &&
+
+	git checkout -b branch &&
+	rm b/a &&
+	mv a b/ &&
+	ln -s b/a a &&
+	git add . &&
+	git commit -m "swap" &&
+
+	touch f1 &&
+	git add f1 &&
+	git commit -m "f1"
+'
+
+test_expect_failure 'Test rename across paths below D/F conflicts' '
+	git checkout newmaster &&
+	git cherry-pick branch
+'
+
 test_done
-- 
1.7.2.rc0.212.g0c601

[PATCH 3/5] merge-recursive: Fix D/F conflicts

[newren]

From: Elijah Newren <newren@gmail.com>

The D/F conflicts we know how to resolve (file or directory unmodified on
one side of history), have the nice property that process_entry() can
correctly handle all subpaths of the D/F conflict, and will in fact delete
all non-conflicting files below the relevant directory and the directory
itself in such cases.  So if we handle D/F conflicts after all other
conflicts, they become fairly simple to handle.  We do this by adding an
extra process_df_entry() step after process_renames() and process_entry().

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 merge-recursive.c               |   93 ++++++++++++++++++++++++++++++++-------
 t/t6035-merge-dir-to-symlink.sh |    8 ++--
 2 files changed, 81 insertions(+), 20 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 856e98c..8d70fc0 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1072,6 +1072,7 @@ static int process_entry(struct merge_options *o,
 	unsigned char *a_sha = stage_sha(entry->stages[2].sha, a_mode);
 	unsigned char *b_sha = stage_sha(entry->stages[3].sha, b_mode);
 
+	entry->processed = 1;
 	if (o_sha && (!a_sha || !b_sha)) {
 		/* Case A: Deleted in one */
 		if ((!a_sha && !b_sha) ||
@@ -1104,33 +1105,28 @@ static int process_entry(struct merge_options *o,
 	} else if ((!o_sha && a_sha && !b_sha) ||
 		   (!o_sha && !a_sha && b_sha)) {
 		/* Case B: Added in one. */
-		const char *add_branch;
-		const char *other_branch;
 		unsigned mode;
 		const unsigned char *sha;
-		const char *conf;
 
 		if (a_sha) {
-			add_branch = o->branch1;
-			other_branch = o->branch2;
 			mode = a_mode;
 			sha = a_sha;
-			conf = "file/directory";
 		} else {
-			add_branch = o->branch2;
-			other_branch = o->branch1;
 			mode = b_mode;
 			sha = b_sha;
-			conf = "directory/file";
 		}
 		if (string_list_has_string(&o->current_directory_set, path)) {
-			const char *new_path = unique_path(o, path, add_branch);
-			clean_merge = 0;
-			output(o, 1, "CONFLICT (%s): There is a directory with name %s in %s. "
-			       "Adding %s as %s",
-			       conf, path, other_branch, path, new_path);
-			remove_file(o, 0, path, 0);
-			update_file(o, 0, sha, mode, new_path);
+			/* Handle D->F conflicts after all subfiles */
+			entry->processed = 0;
+			/* But get any file out of the way now, so conflicted
+			 * entries below the directory of the same name can
+			 * be put in the working directory.
+			 */
+			if (a_sha)
+				output(o, 2, "Removing %s", path);
+			/* do not touch working file if it did not exist */
+			remove_file(o, 0, path, !a_sha);
+			return 1; /* Assume clean till processed */
 		} else {
 			output(o, 2, "Adding %s", path);
 			update_file(o, 1, sha, mode, path);
@@ -1178,6 +1174,64 @@ static int process_entry(struct merge_options *o,
 	return clean_merge;
 }
 
+/* Per entry merge function for D/F conflicts, to be called only after
+ * all files below dir have been processed.  We do this because in the
+ * cases we can cleanly resolve D/F conflicts, process_entry() can clean
+ * out all the files below the directory for us.
+ */
+static int process_df_entry(struct merge_options *o,
+			 const char *path, struct stage_data *entry)
+{
+	int clean_merge = 1;
+	unsigned o_mode = entry->stages[1].mode;
+	unsigned a_mode = entry->stages[2].mode;
+	unsigned b_mode = entry->stages[3].mode;
+	unsigned char *o_sha = stage_sha(entry->stages[1].sha, o_mode);
+	unsigned char *a_sha = stage_sha(entry->stages[2].sha, a_mode);
+	unsigned char *b_sha = stage_sha(entry->stages[3].sha, b_mode);
+
+	/* We currently only handle D->F cases */
+	assert((!o_sha && a_sha && !b_sha) ||
+	       (!o_sha && !a_sha && b_sha));
+
+	const char *add_branch;
+	const char *other_branch;
+	unsigned mode;
+	const unsigned char *sha;
+	const char *conf;
+
+	entry->processed = 1;
+
+	if (a_sha) {
+		add_branch = o->branch1;
+		other_branch = o->branch2;
+		mode = a_mode;
+		sha = a_sha;
+		conf = "file/directory";
+	} else {
+		add_branch = o->branch2;
+		other_branch = o->branch1;
+		mode = b_mode;
+		sha = b_sha;
+		conf = "directory/file";
+	}
+	struct stat st;
+	if (lstat(path, &st) == 0 && S_ISDIR(st.st_mode)) {
+		const char *new_path = unique_path(o, path, add_branch);
+		clean_merge = 0;
+		output(o, 1, "CONFLICT (%s): There is a directory with name %s in %s. "
+		       "Adding %s as %s",
+		       conf, path, other_branch, path, new_path);
+		remove_file(o, 0, path, 0);
+		update_file(o, 0, sha, mode, new_path);
+	} else {
+		output(o, 2, "Adding %s", path);
+		update_file(o, 1, sha, mode, path);
+	}
+
+	return clean_merge;
+}
+
 struct unpack_trees_error_msgs get_porcelain_error_msgs(void)
 {
 	struct unpack_trees_error_msgs msgs = {
@@ -1249,6 +1303,13 @@ int merge_trees(struct merge_options *o,
 				&& !process_entry(o, path, e))
 				clean = 0;
 		}
+		for (i = 0; i < entries->nr; i++) {
+			const char *path = entries->items[i].string;
+			struct stage_data *e = entries->items[i].util;
+			if (!e->processed
+				&& !process_df_entry(o, path, e))
+				clean = 0;
+		}
 
 		string_list_clear(re_merge, 0);
 		string_list_clear(re_head, 0);
diff --git a/t/t6035-merge-dir-to-symlink.sh b/t/t6035-merge-dir-to-symlink.sh
index 4f04f41..64387a0 100755
--- a/t/t6035-merge-dir-to-symlink.sh
+++ b/t/t6035-merge-dir-to-symlink.sh
@@ -56,7 +56,7 @@ test_expect_success 'do not lose a/b-2/c/d in merge (resolve)' '
 	test -f a/b-2/c/d
 '
 
-test_expect_failure 'Handle D/F conflict, do not lose a/b-2/c/d in merge (recursive)' '
+test_expect_success 'Handle D/F conflict, do not lose a/b-2/c/d in merge (recursive)' '
 	git reset --hard &&
 	git checkout baseline^0 &&
 	git merge -s recursive master &&
@@ -64,7 +64,7 @@ test_expect_failure 'Handle D/F conflict, do not lose a/b-2/c/d in merge (recurs
 	test -f a/b-2/c/d
 '
 
-test_expect_failure 'Handle F/D conflict, do not lose a/b-2/c/d in merge (recursive)' '
+test_expect_success 'Handle F/D conflict, do not lose a/b-2/c/d in merge (recursive)' '
 	git reset --hard &&
 	git checkout master &&
 	git merge -s recursive baseline^0 &&
@@ -107,7 +107,7 @@ test_expect_success 'merge should not have conflicts (resolve)' '
 	test -f a/b/c/d
 '
 
-test_expect_failure 'merge should not have D/F conflicts (recursive)' '
+test_expect_success 'merge should not have D/F conflicts (recursive)' '
 	git reset --hard &&
 	git checkout baseline^0 &&
 	git merge -s recursive test2 &&
@@ -115,7 +115,7 @@ test_expect_failure 'merge should not have D/F conflicts (recursive)' '
 	test -f a/b/c/d
 '
 
-test_expect_failure 'merge should not have F/D conflicts (recursive)' '
+test_expect_success 'merge should not have F/D conflicts (recursive)' '
 	git reset --hard &&
 	git checkout -b foo test2 &&
 	git merge -s recursive baseline^0 &&
-- 
1.7.2.rc0.212.g0c601

[PATCH 4/5] merge_recursive: Fix renames across paths below D/F conflicts

[newren]

From: Elijah Newren <newren@gmail.com>


Signed-off-by: Elijah Newren <newren@gmail.com>
---
I'm a little uneasy with this change, mainly because I don't fully
understand the rename processing logic (I was actually kind of surprised
when I made these changes and it worked).  Although I verified that
these changes (and my others in this patch series) introduce no new
breakages in the testsuite and even fix a known issue, I'm still not
quite sure I follow the logic well enough to feel fully confident in
this change.  I'm particularly worried I may have neglected some closely
related cases that I should have fixed but which may still be broken.

 merge-recursive.c   |   13 +++++++++++--
 t/t6020-merge-df.sh |    4 ++--
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 8d70fc0..ab0743f 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1019,14 +1019,23 @@ static int process_renames(struct merge_options *o,
 
 				if (mfi.clean &&
 				    sha_eq(mfi.sha, ren1->pair->two->sha1) &&
-				    mfi.mode == ren1->pair->two->mode)
+				    mfi.mode == ren1->pair->two->mode) {
 					/*
 					 * This messaged is part of
 					 * t6022 test. If you change
 					 * it update the test too.
 					 */
 					output(o, 3, "Skipped %s (merged same as existing)", ren1_dst);
-				else {
+
+					/* If this was a rename across a path involved
+					 * in a D/F conflict, there may be more work to
+					 * do.
+					 */
+					for (i=1; i<=3; ++i) {
+						if (ren1->dst_entry->stages[i].mode)
+							ren1->dst_entry->processed = 0;
+					}
+				} else {
 					if (mfi.merge || !mfi.clean)
 						output(o, 1, "Renaming %s => %s", ren1_src, ren1_dst);
 					if (mfi.merge)
diff --git a/t/t6020-merge-df.sh b/t/t6020-merge-df.sh
index 99acb89..7278eee 100755
--- a/t/t6020-merge-df.sh
+++ b/t/t6020-merge-df.sh
@@ -22,7 +22,7 @@ git commit -m "File: dir"'
 
 test_expect_code 1 'Merge with d/f conflicts' 'git merge "merge msg" B master'
 
-test_expect_failure 'F/D conflict' '
+test_expect_success 'F/D conflict' '
 	git reset --hard &&
 	git checkout master &&
 	rm .git/index &&
@@ -72,7 +72,7 @@ test_expect_success 'Setup rename across paths each below D/F conflicts' '
 	git commit -m "f1"
 '
 
-test_expect_failure 'Test rename across paths below D/F conflicts' '
+test_expect_success 'Test rename across paths below D/F conflicts' '
 	git checkout newmaster &&
 	git cherry-pick branch
 '
-- 
1.7.2.rc0.212.g0c601

[PATCH 5/5] fast-import: Handle directories changing into symlinks

[newren]

From: Elijah Newren <newren@gmail.com>


Signed-off-by: Elijah Newren <newren@gmail.com>
---
This is a resend of an earlier patch.  Since the previous one wasn't
reviewed and didn't make it to pu, I decided to resend it along with the
merge-recursive directory/symlink conflict fixes as part of a patch series.

 fast-import.c          |    5 +++++
 t/t9350-fast-export.sh |    2 +-
 2 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/fast-import.c b/fast-import.c
index 1e5d66e..9a2ecc8 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1528,6 +1528,11 @@ static int tree_content_remove(
 	for (i = 0; i < t->entry_count; i++) {
 		e = t->entries[i];
 		if (e->name->str_len == n && !strncmp(p, e->name->str_dat, n)) {
+			if (slash1 && S_ISLNK(e->versions[1].mode))
+				/* p was already removed by an earlier change
+				 * of a parent directory to a symlink.
+				 */
+				return 1;
 			if (!slash1 || !S_ISDIR(e->versions[1].mode))
 				goto del_entry;
 			if (!e->tree)
diff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh
index 69179c6..1ee1461 100755
--- a/t/t9350-fast-export.sh
+++ b/t/t9350-fast-export.sh
@@ -376,7 +376,7 @@ test_expect_success 'tree_tag-obj'    'git fast-export tree_tag-obj'
 test_expect_success 'tag-obj_tag'     'git fast-export tag-obj_tag'
 test_expect_success 'tag-obj_tag-obj' 'git fast-export tag-obj_tag-obj'
 
-test_expect_failure 'directory becomes symlink'        '
+test_expect_success 'directory becomes symlink'        '
 	git init dirtosymlink &&
 	git init result &&
 	(
-- 
1.7.2.rc0.212.g0c601

Re: [PATCH 4/5] merge_recursive: Fix renames across paths below D/F conflicts

[Miklos Vajna]

[-- Attachment #1: Type: text/plain, Size: 916 bytes --]

On Mon, Jun 28, 2010 at 07:12:15PM -0600, newren@gmail.com wrote:
> From: Elijah Newren <newren@gmail.com>
> 
> 
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
> I'm a little uneasy with this change, mainly because I don't fully
> understand the rename processing logic (I was actually kind of surprised
> when I made these changes and it worked).  Although I verified that
> these changes (and my others in this patch series) introduce no new
> breakages in the testsuite and even fix a known issue, I'm still not
> quite sure I follow the logic well enough to feel fully confident in
> this change.  I'm particularly worried I may have neglected some closely
> related cases that I should have fixed but which may still be broken.

Same here, I touched merge-recursive, but not this part of it, so others
will give you a better review, I'm sure. :)

Other than that, I like it, thanks!

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 2/5] Add another rename + D/F conflict testcase

[Alexander Gladysh]

> I hope it's not bad style to take someone else's testcase from the mailing
> list and submit it on their behalf as a testsuite addition (nor do I know
> what to do about the signed-off-by line in this case).  This is simply the
> testcase Alexander Gladysh posted to the list on March 8.  I really like
> his example due to how it serves as a simple case where there are two D/F
> conflicts with a rename across paths involved in both of those D/F
> conflicts.

I have no problem if you use my test case for the Git testsuite. The
more tests — the merrier. :-)

Consider this testcase as signed-off by me.

If you feel that you need to mention my name somewhere and proper Git
headers are a problem — commit comments would do.

If, to honor all formalities, you would need my actual commit — please
tell me, I'll try to submit it.

Alexander.

Re: [PATCH 4/5] merge_recursive: Fix renames across paths below D/F conflicts

[Elijah Newren]

On Tue, Jun 29, 2010 at 1:54 AM, Miklos Vajna <vmiklos@frugalware.org> wrote:
> On Mon, Jun 28, 2010 at 07:12:15PM -0600, newren@gmail.com wrote:
>> I'm a little uneasy with this change, mainly because I don't fully
>> understand the rename processing logic (I was actually kind of surprised
>> when I made these changes and it worked).  Although I verified that
>> these changes (and my others in this patch series) introduce no new
>> breakages in the testsuite and even fix a known issue, I'm still not
>> quite sure I follow the logic well enough to feel fully confident in
>> this change.  I'm particularly worried I may have neglected some closely
>> related cases that I should have fixed but which may still be broken.
>
> Same here, I touched merge-recursive, but not this part of it, so others
> will give you a better review, I'm sure. :)
>
> Other than that, I like it, thanks!

Oh, it looks like I was off by a couple lines when trying to read the
authorship out of git blame -C -C.  You touched lines that were pretty
close, but it looks like this if block was actually due to Alex.  So
I'll add him to the cc.

Alex: I think the basic idea is just that the rename logic isn't aware
that there may be higher stage entries in the index due to D/F
conflicts; by checking for such cases and marking the entry as not
processed, it allows process_entry() later to look at it and handle
those higher stages.  But I'm not sure if that's the right way to
handle it, or if just having process_renames() should take care of
clearing out the higher stage entries, or if something else entirely
should be done.

Thanks,
Elijah

Re: [PATCH 4/5] merge_recursive: Fix renames across paths below D/F conflicts

[Alex Riesen]

On Tue, Jun 29, 2010 at 14:52, Elijah Newren <newren@gmail.com> wrote:
> On Tue, Jun 29, 2010 at 1:54 AM, Miklos Vajna <vmiklos@frugalware.org> wrote:
>> On Mon, Jun 28, 2010 at 07:12:15PM -0600, newren@gmail.com wrote:
>>> I'm a little uneasy with this change, mainly because I don't fully
>>> understand the rename processing logic (I was actually kind of surprised
>>> when I made these changes and it worked).  Although I verified that
>>> these changes (and my others in this patch series) introduce no new
>>> breakages in the testsuite and even fix a known issue, I'm still not
>>> quite sure I follow the logic well enough to feel fully confident in
>>> this change.  I'm particularly worried I may have neglected some closely
>>> related cases that I should have fixed but which may still be broken.
>
> Alex: I think the basic idea is just that the rename logic isn't aware
> that there may be higher stage entries in the index due to D/F
> conflicts; by checking for such cases and marking the entry as not
> processed, it allows process_entry() later to look at it and handle
> those higher stages.  But I'm not sure if that's the right way to
> handle it, or if just having process_renames() should take care of
> clearing out the higher stage entries, or if something else entirely
> should be done.

Nor am I. You may be still off by some commits in detecting the authorship :)
This code was seldom touched since it was written (by Johannes). It has
survived in this sorry state all (at least my) attempts to fix it. OTOH I never
tried really hard. Maybe because the D/F conflicts are rare and are relatively
simple to work around.

I cannot say much about your change... Are you sure about D/F conflict
detection, though? You just test if target mode not 0.

Re: [PATCH 4/5] merge_recursive: Fix renames across paths below D/F conflicts

[Elijah Newren]

On Tue, Jun 29, 2010 at 7:36 AM, Alex Riesen <raa.lkml@gmail.com> wrote:
> On Tue, Jun 29, 2010 at 14:52, Elijah Newren <newren@gmail.com> wrote:
>> Alex: I think the basic idea is just that the rename logic isn't aware
>> that there may be higher stage entries in the index due to D/F
>> conflicts; by checking for such cases and marking the entry as not
>> processed, it allows process_entry() later to look at it and handle
>> those higher stages.  But I'm not sure if that's the right way to
>> handle it, or if just having process_renames() should take care of
>> clearing out the higher stage entries, or if something else entirely
>> should be done.
>
> Nor am I. You may be still off by some commits in detecting the authorship :)
> This code was seldom touched since it was written (by Johannes). It has
> survived in this sorry state all (at least my) attempts to fix it. OTOH I never
> tried really hard. Maybe because the D/F conflicts are rare and are relatively
> simple to work around.
>
> I cannot say much about your change... Are you sure about D/F conflict
> detection, though? You just test if target mode not 0.

Well, as far as this particular if-block is concerned, blame suggests
that you and Miklos were responsible (I apologize if gmail screws up
and inserts line wrapping)::

$ git blame -C -C -L 1020,1038 merge-recursive.c
9047ebbc (Miklos Vajna  2008-08-12 18:45:14 +0200 1020)
                 if (mfi.clean &&
9047ebbc (Miklos Vajna  2008-08-12 18:45:14 +0200 1021)
                     sha_eq(mfi.sha, ren1->pair->two->sha1) &&
de4d7dc3 (Elijah Newren 2010-06-28 09:38:58 -0600 1022)
                     mfi.mode == ren1->pair->two->mode) {
8a359819 (Alex Riesen   2007-04-25 22:07:45 +0200 1023)
                         /*
8a359819 (Alex Riesen   2007-04-25 22:07:45 +0200 1024)
                          * This messaged is part of
8a359819 (Alex Riesen   2007-04-25 22:07:45 +0200 1025)
                          * t6022 test. If you change
8a359819 (Alex Riesen   2007-04-25 22:07:45 +0200 1026)
                          * it update the test too.
8a359819 (Alex Riesen   2007-04-25 22:07:45 +0200 1027)
                          */
8a2fce18 (Miklos Vajna  2008-08-25 16:25:57 +0200 1028)
                         output(o, 3, "Skipped %s (merged same as
existing)", ren1_
de4d7dc3 (Elijah Newren 2010-06-28 09:38:58 -0600 1029)
de4d7dc3 (Elijah Newren 2010-06-28 09:38:58 -0600 1030)
                         /* If this was a rename across a path
involved
de4d7dc3 (Elijah Newren 2010-06-28 09:38:58 -0600 1031)
                          * in a D/F conflict, there may be more work
to
de4d7dc3 (Elijah Newren 2010-06-28 09:38:58 -0600 1032)
                          * do.
de4d7dc3 (Elijah Newren 2010-06-28 09:38:58 -0600 1033)
                          */
de4d7dc3 (Elijah Newren 2010-06-28 09:38:58 -0600 1034)
                         for (i=1; i<=3; ++i) {
de4d7dc3 (Elijah Newren 2010-06-28 09:38:58 -0600 1035)
                                 if (ren1->dst_entry->stages[i].mode)
de4d7dc3 (Elijah Newren 2010-06-28 09:38:58 -0600 1036)
                                         ren1->dst_entry->processed =
0;
de4d7dc3 (Elijah Newren 2010-06-28 09:38:58 -0600 1037)
                         }
de4d7dc3 (Elijah Newren 2010-06-28 09:38:58 -0600 1038)
                 } else {

With D/F conflicts, the files would be loaded into higher stages in
the index (before it gets to process_renames()), which I detected via
a non-zero mode.  If there's a different way I should be checking for
higher stage entries that still need to be resolved, I'd be happy to
use it.

Re: [PATCH 4/5] merge_recursive: Fix renames across paths below D/F conflicts

[Miklos Vajna]

[-- Attachment #1: Type: text/plain, Size: 810 bytes --]

On Tue, Jun 29, 2010 at 09:55:38AM -0600, Elijah Newren <newren@gmail.com> wrote:
> Well, as far as this particular if-block is concerned, blame suggests
> that you and Miklos were responsible (I apologize if gmail screws up
> and inserts line wrapping)::
> 
> $ git blame -C -C -L 1020,1038 merge-recursive.c
> 9047ebbc (Miklos Vajna  2008-08-12 18:45:14 +0200 1020)
>                  if (mfi.clean &&
> 9047ebbc (Miklos Vajna  2008-08-12 18:45:14 +0200 1021)
>                      sha_eq(mfi.sha, ren1->pair->two->sha1) &&

And if you have a look at what commit 9047ebbc does, that's really just
about changing it to be part of libgit, so I could call it without
fork() from builtin-merge.

To sum up, I sadly have to say I don't know too much about the
merge-recursive internal sematics.

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 4/5] merge_recursive: Fix renames across paths below D/F conflicts

[Alex Riesen]

On Tue, Jun 29, 2010 at 17:55, Elijah Newren <newren@gmail.com> wrote:
> On Tue, Jun 29, 2010 at 7:36 AM, Alex Riesen <raa.lkml@gmail.com> wrote:
>> I cannot say much about your change... Are you sure about D/F conflict
>> detection, though? You just test if target mode not 0.
>
> Well, as far as this particular if-block is concerned, blame suggests
> that you and Miklos were responsible (I apologize if gmail screws up
> and inserts line wrapping)::

Don't just look at the blame output, look at what the commits actually changed.
It's either a reformatting or a trivial change.

> With D/F conflicts, the files would be loaded into higher stages in
> the index (before it gets to process_renames()), which I detected via
> a non-zero mode.

This just detects if there was any conflict. Not specifically D/F or F/D.

> If there's a different way I should be checking for higher stage entries
> that still need to be resolved, I'd be happy to use it.

I'd expect a check for a file-to-directory (or back) mode change.