[PATCH 0/5] D/F conflict fixes

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

From: Elijah Newren <redacted>


Signed-off-by: Elijah Newren <redacted>
---
 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

From: Elijah Newren <redacted>


Signed-off-by: Elijah Newren <redacted>
---
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

From: Elijah Newren <redacted>

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

From: Elijah Newren <redacted>


Signed-off-by: Elijah Newren <redacted>
---
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

From: Alexander Gladysh <redacted>

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

On Mon, Jun 28, 2010 at 07:12:15PM -0600, newren@gmail.com wrote:

From: Elijah Newren <redacted>


Signed-off-by: Elijah Newren <redacted>
---
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!

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.

On Tue, Jun 29, 2010 at 1:54 AM, Miklos Vajna [off-list ref] wrote:

On Mon, Jun 28, 2010 at 07:12:15PM -0600, newren@gmail.com wrote:

quoted

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

On Tue, Jun 29, 2010 at 14:52, Elijah Newren [off-list ref] wrote:

On Tue, Jun 29, 2010 at 1:54 AM, Miklos Vajna [off-list ref] wrote:

quoted

On Mon, Jun 28, 2010 at 07:12:15PM -0600, newren@gmail.com wrote:

quoted

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.

On Tue, Jun 29, 2010 at 7:36 AM, Alex Riesen [off-list ref] wrote:

On Tue, Jun 29, 2010 at 14:52, Elijah Newren [off-list ref] wrote:

quoted

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.

On Tue, Jun 29, 2010 at 09:55:38AM -0600, Elijah Newren [off-list ref] 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.

On Tue, Jun 29, 2010 at 17:55, Elijah Newren [off-list ref] wrote:

On Tue, Jun 29, 2010 at 7:36 AM, Alex Riesen [off-list ref] wrote:

quoted

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.