[PATCHv2 0/5] D/F conflict fixes

[PATCHv2 0/5] D/F conflict fixes

[newren]

This patch series fixes a number of spurious directory/file conflicts
and associated bugs appearing in cherry-pick, rebase, merge, and
fast-import.  It includes testsuite fixes for currently known failures
in both t6020-merge-df.sh and t6035-merge-dir-to-symlink.sh.

The right person to review the changes other than the simple
fast-import one is probably Dscho; in his absence, the next best as
far as I can tell is probably Junio or perhaps Shawn.  I hate to
overwork them even more, so if anyone else has some time to take a
look or even do some simple testing, it'd be much appreciated.  Shawn
is a natural choice for reviewing the (fairly trivial) fast-import
change.

Changes since the previous submission:
  * Significantly extended, clarified, or otherwise modified several
    of the commit messages
  * Rebased the series on top of maint (sorry about submitting
    relative to next!)
  * Added Alexander's signoff on his testcase
  * Moved the new rename+D/F conflict testcase to a different file
    since the testcase uses cherry-pick rather than merge.

Alexander Gladysh (1):
      Add a 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: Fix minor data-loss issue with directories becoming symlinks

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

[PATCHv2 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 3202e1d..3c176d7 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.1.1.6.gc3cd6

[PATCHv2 2/5] Add a 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

Signed-off-by: Alexander Gladysh <agladysh@gmail.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
As noted previously this is simply a testcase Alexander sent to the list
on March 8 which I'm submitting in the form of a testsuite addition.
He's happy to have his testcase added to the testsuite and/or even
submit it himself if that makes more sense -- just let us know what is
wanted.  I'm less familiar with format-patch and am; hopefully this
submission comes across with him recorded as the author.

Also, I moved the location of this testcase since the last submission;
last time I made it part of t/t6020-merge-df.sh, which probably doesn't
make sense since the testcase invokes cherry-pick rather than merge.

 t/t3508-cherry-pick-merge-df.sh |   34 ++++++++++++++++++++++++++++++++++
 1 files changed, 34 insertions(+), 0 deletions(-)
 create mode 100755 t/t3508-cherry-pick-merge-df.sh

diff --git a/t/t3508-cherry-pick-merge-df.sh b/t/t3508-cherry-pick-merge-df.sh
new file mode 100755
index 0000000..646a56d
--- /dev/null
+++ b/t/t3508-cherry-pick-merge-df.sh
@@ -0,0 +1,34 @@
+#!/bin/sh
+
+test_description='Test cherry-pick with directory/file conflicts'
+. ./test-lib.sh
+
+test_expect_success 'Setup rename across paths each below D/F conflicts' '
+	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 'Cherry-pick succeeds with rename across D/F conflicts' '
+	git checkout master &&
+	git cherry-pick branch
+'
+
+test_done
-- 
1.7.1.1.6.gc3cd6

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

[newren]

From: Elijah Newren <newren@gmail.com>

The D/F conflicts that can be automatically resolved (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.  In
the case of D->F conversions, it will correctly delete all non-conflicting
files below the relevant directory and the directory itself (note that both
untracked and conflicting files below the directory will prevent its
removal).  So if we handle D/F conflicts after all other conflicts, they
become fairly simple to handle -- we just need to check for whether or not
a path (file/directory) is in the way of creating the new content.  We do
this by having process_entry() defer handling such entries to a subsequent
process_df_entry() step.

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 206c103..865729a 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 3c176d7..384547d 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.1.1.6.gc3cd6

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

[newren]

From: Elijah Newren <newren@gmail.com>

The rename logic in process_renames() handles renames and merging of file
contents and then marks files as processed.  However, there may be higher
stage entries left in the index for other reasons (e.g., 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.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 merge-recursive.c               |   13 +++++++++++--
 t/t3508-cherry-pick-merge-df.sh |    2 +-
 t/t6020-merge-df.sh             |    2 +-
 3 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 865729a..ecddd9e 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 {
+
+					/* There may be higher stage entries left
+					 * in the index (e.g. due to a D/F
+					 * conflict) that need to be resolved.
+					 */
+					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/t3508-cherry-pick-merge-df.sh b/t/t3508-cherry-pick-merge-df.sh
index 646a56d..5895ea3 100755
--- a/t/t3508-cherry-pick-merge-df.sh
+++ b/t/t3508-cherry-pick-merge-df.sh
@@ -26,7 +26,7 @@ test_expect_success 'Setup rename across paths each below D/F conflicts' '
 	git commit -m "f1"
 '
 
-test_expect_failure 'Cherry-pick succeeds with rename across D/F conflicts' '
+test_expect_success 'Cherry-pick succeeds with rename across D/F conflicts' '
 	git checkout master &&
 	git cherry-pick branch
 '
diff --git a/t/t6020-merge-df.sh b/t/t6020-merge-df.sh
index e71c687..490d397 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 &&
-- 
1.7.1.1.6.gc3cd6

[PATCHv2 5/5] fast-import: Fix minor data-loss issue with directories becoming symlinks

[newren]

From: Elijah Newren <newren@gmail.com>

When fast-export runs across a directory changing to a symlink, it will
output the changes in the form
  M 120000 :239821 dir-changing-to-symlink
  D dir-changing-to-symlink/filename1
When fast-import sees the first line, it deletes the directory named
dir-changing-to-symlink (and any files below it) and creates a symlink in
its place.  When fast-import came across the second line, it was previously
trying to remove the file and relevant leading directories in
tree_content_remove(), and as a side effect it would delete the symlink
that was just created.  This resulted in the symlink silently missing from
the resulting repository.

We fix this by ignoring file deletions underneath directory names that
correspond to symlinks.  This can also be viewed as a minor optimization:
since there cannot be a symlink and a directory with the same name in the
same directory, the file clearly can't exist so nothing needs to be done to
delete it.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 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 309f2c5..10462d8 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))
+				/* If a parent directory of p is a symlink, then
+				 * p cannot exist and need not be deleted.
+				 */
+				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.1.1.6.gc3cd6

Re: [PATCHv2 5/5] fast-import: Fix minor data-loss issue with directories becoming symlinks

[Shawn O. Pearce]

newren@gmail.com wrote:
> From: Elijah Newren <newren@gmail.com>
> 
> When fast-export runs across a directory changing to a symlink, it will
> output the changes in the form
>   M 120000 :239821 dir-changing-to-symlink
>   D dir-changing-to-symlink/filename1
> When fast-import sees the first line, it deletes the directory named
> dir-changing-to-symlink (and any files below it) and creates a symlink in
> its place.  When fast-import came across the second line, it was previously
> trying to remove the file and relevant leading directories in
> tree_content_remove(), and as a side effect it would delete the symlink
> that was just created.  This resulted in the symlink silently missing from
> the resulting repository.

Ugh.

I'm not against making the input parser more robust, but this is
a violation of the stream format from fast-export.  The stream is
incremental, a command like 'M' takes place immediately.  It is
wrong for a frontend to output 'M foo', then 'D foo/bar'.

IMHO, if fast-export is doing what you say above, the bug lies in
fast-export, and therefore the fix should too.
 
-- 
Shawn.

Re: [PATCHv2 5/5] fast-import: Fix minor data-loss issue with directories becoming symlinks

[Elijah Newren]

On Tue, Jul 6, 2010 at 1:34 PM, Shawn O. Pearce <spearce@spearce.org> wrote:
> newren@gmail.com wrote:
>> From: Elijah Newren <newren@gmail.com>
>>
>> When fast-export runs across a directory changing to a symlink, it will
>> output the changes in the form
>>   M 120000 :239821 dir-changing-to-symlink
>>   D dir-changing-to-symlink/filename1
>> When fast-import sees the first line, it deletes the directory named
>> dir-changing-to-symlink (and any files below it) and creates a symlink in
>> its place.  When fast-import came across the second line, it was previously
>> trying to remove the file and relevant leading directories in
>> tree_content_remove(), and as a side effect it would delete the symlink
>> that was just created.  This resulted in the symlink silently missing from
>> the resulting repository.
>
> Ugh.
>
> I'm not against making the input parser more robust, but this is
> a violation of the stream format from fast-export.  The stream is
> incremental, a command like 'M' takes place immediately.  It is
> wrong for a frontend to output 'M foo', then 'D foo/bar'.
>
> IMHO, if fast-export is doing what you say above, the bug lies in
> fast-export, and therefore the fix should too.

Okay, I'll fix up fast-export.  Do you want me to drop this
fast-import patch, or does it still make sense as an extra robustness
check?

Elijah

Re: [PATCHv2 5/5] fast-import: Fix minor data-loss issue with directories becoming symlinks

[Shawn O. Pearce]

Elijah Newren <newren@gmail.com> wrote:
> On Tue, Jul 6, 2010 at 1:34 PM, Shawn O. Pearce <spearce@spearce.org> wrote:
> > newren@gmail.com wrote:
> >> From: Elijah Newren <newren@gmail.com>
> >>
> >> When fast-export runs across a directory changing to a symlink, it will
> >> output the changes in the form
> >> ?? M 120000 :239821 dir-changing-to-symlink
> >> ?? D dir-changing-to-symlink/filename1
> >> When fast-import sees the first line, it deletes the directory named
> >> dir-changing-to-symlink (and any files below it) and creates a symlink in
> >> its place. ??When fast-import came across the second line, it was previously
> >> trying to remove the file and relevant leading directories in
> >> tree_content_remove(), and as a side effect it would delete the symlink
> >> that was just created. ??This resulted in the symlink silently missing from
> >> the resulting repository.
> >
> > Ugh.
> >
> > I'm not against making the input parser more robust, but this is
> > a violation of the stream format from fast-export. ??The stream is
> > incremental, a command like 'M' takes place immediately. ??It is
> > wrong for a frontend to output 'M foo', then 'D foo/bar'.
> >
> > IMHO, if fast-export is doing what you say above, the bug lies in
> > fast-export, and therefore the fix should too.
> 
> Okay, I'll fix up fast-export.  Do you want me to drop this
> fast-import patch, or does it still make sense as an extra robustness
> check?

It probably makes sense to still do this in fast-import, deleting
something that doesn't exist is probably OK, its still going to
be deleted in the end anyway.

-- 
Shawn.