[BUG/PATCH] t9350-fast-export: Add failing test for symlink-to-directory

[BUG/PATCH] t9350-fast-export: Add failing test for symlink-to-directory

[Anders Kaseorg]

git fast-export | git fast-import fails to preserve a commit that replaces 
a symlink with a directory.  Add a failing test case demonstrating this 
bug.

The fast-export output for the commit in question looks like

  commit refs/heads/master
  mark :4
  author …
  committer …
  data 4
  two
  M 100644 :1 foo/world
  D foo

fast-import deletes the symlink foo and ignores foo/world.  Swapping the M 
line with the D line would give the correct result.

Signed-off-by: Anders Kaseorg <andersk@mit.edu>
---
 t/t9350-fast-export.sh | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh
index 66c8b0a..5fb8a04 100755
--- a/t/t9350-fast-export.sh
+++ b/t/t9350-fast-export.sh
@@ -419,6 +419,30 @@ test_expect_success 'directory becomes symlink'        '
 	(cd result && git show master:foo)
 '
 
+test_expect_failure 'symlink becomes directory'        '
+	git init symlinktodir &&
+	git init symlinktodirresult &&
+	(
+		cd symlinktodir &&
+		mkdir bar &&
+		echo hello > bar/world &&
+		test_ln_s_add bar foo &&
+		git add foo bar/world &&
+		git commit -q -mone &&
+		git rm foo &&
+		mkdir foo &&
+		echo hello > foo/world &&
+		git add foo/world &&
+		git commit -q -mtwo
+	) &&
+	(
+		cd symlinktodir &&
+		git fast-export master -- foo |
+		(cd ../symlinktodirresult && git fast-import --quiet)
+	) &&
+	(cd symlinktodirresult && git show master:foo)
+'
+
 test_expect_success 'fast-export quotes pathnames' '
 	git init crazy-paths &&
 	(cd crazy-paths &&
-- 
2.5.0

Re: [BUG/PATCH] t9350-fast-export: Add failing test for symlink-to-directory

[Jeff King]

On Wed, Aug 19, 2015 at 03:46:27PM -0400, Anders Kaseorg wrote:

> git fast-export | git fast-import fails to preserve a commit that replaces 
> a symlink with a directory.  Add a failing test case demonstrating this 
> bug.
> 
> The fast-export output for the commit in question looks like
> 
>   commit refs/heads/master
>   mark :4
>   author …
>   committer …
>   data 4
>   two
>   M 100644 :1 foo/world
>   D foo
> 
> fast-import deletes the symlink foo and ignores foo/world.  Swapping the M 
> line with the D line would give the correct result.

Thanks for providing a patch to the tests. That is my favorite form of
bug report. :)

The problem seems to be that we output the entries in a "depth first"
way; "foo/bar" always comes before "foo", to cover the cases explained
in 060df62 (fast-export: Fix output order of D/F changes, 2010-07-09).

I'm tempted to say we would want to do all deletions (at any level)
first, to make room for new files. That patch looks like:

diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index d23f3be..336fd6f 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -267,6 +267,14 @@ static int depth_first(const void *a_, const void *b_)
 	const char *name_a, *name_b;
 	int len_a, len_b, len;
 	int cmp;
+	int deletion;
+
+	/*
+	 * Move all deletions first, to make room for any later modifications.
+	 */
+	deletion = (b->status == 'D') - (a->status == 'D');
+	if (deletion)
+		return deletion;
 
 	name_a = a->one ? a->one->path : a->two->path;
 	name_b = b->one ? b->one->path : b->two->path;


and does indeed pass your test. But I'm not sure that covers all cases,
and I'm not sure it doesn't make some cases worse:

  - if we moved a deletion to the front, is it possible for that path to
    have been the source side of a copy or rename, that is now broken? I
    don't _think_ so. If it's a copy, then the file by definition cannot
    also be deleted (that would make it a rename, not a copy). We could
    have a copy along with a rename, but again, then we don't have a
    delete (we have a rename, which is explicitly bumped to the end for
    this reason).

  - we may still have the opposite problem with renames. That is, a
    rename is _also_ a deletion, but will go to the end. So I would
    expect renaming the symlink "foo" to "bar" and then adding
    "foo/world" would end up with:

       M 100644 :3 foo/world
       R foo bar

    (because we push renames to the end in our sort). And indeed,
    importing that does seem to get it wrong (we end up with "bar/world"
    and no symlink).

We can't fix the ordering in the second case without breaking the first
case. So I'm not sure it's fixable on the fast-export end.

-Peff

Re: [BUG/PATCH] t9350-fast-export: Add failing test for symlink-to-directory

[Anders Kaseorg]

On Fri, 21 Aug 2015, Jeff King wrote:
>   - we may still have the opposite problem with renames. That is, a
>     rename is _also_ a deletion, but will go to the end. So I would
>     expect renaming the symlink "foo" to "bar" and then adding
>     "foo/world" would end up with:
> 
>        M 100644 :3 foo/world
>        R foo bar
> 
>     (because we push renames to the end in our sort). And indeed,
>     importing that does seem to get it wrong (we end up with "bar/world"
>     and no symlink).
> 
> We can't fix the ordering in the second case without breaking the first
> case. So I'm not sure it's fixable on the fast-export end.

Hmm, renames have a more fundamental ordering problem: swapping two 
(normal) files and using fast-export -C -B results in

  R foo bar
  R bar foo

which cannot be reimported correctly without fast-import fixes.

Anders

Re: [BUG/PATCH] t9350-fast-export: Add failing test for symlink-to-directory

[Jeff King]

On Fri, Aug 21, 2015 at 12:47:30PM -0400, Anders Kaseorg wrote:

> On Fri, 21 Aug 2015, Jeff King wrote:
> >   - we may still have the opposite problem with renames. That is, a
> >     rename is _also_ a deletion, but will go to the end. So I would
> >     expect renaming the symlink "foo" to "bar" and then adding
> >     "foo/world" would end up with:
> > 
> >        M 100644 :3 foo/world
> >        R foo bar
> > 
> >     (because we push renames to the end in our sort). And indeed,
> >     importing that does seem to get it wrong (we end up with "bar/world"
> >     and no symlink).
> > 
> > We can't fix the ordering in the second case without breaking the first
> > case. So I'm not sure it's fixable on the fast-export end.
> 
> Hmm, renames have a more fundamental ordering problem: swapping two 
> (normal) files and using fast-export -C -B results in
> 
>   R foo bar
>   R bar foo
> 
> which cannot be reimported correctly without fast-import fixes.

Yeah, you're right. Fast-export's view of the world comes from diff,
which is that the "source" side is immutable. Whereas fast-import seems
to mutate the tree in-place as it reads the set of operations. I wonder
what would break if we simply fixed that. I.e., is anybody else
depending on:

  R foo bar
  M bar ...

to modify "foo" and not "bar". I kind of wonder if it is insane to turn
on renames at all in fast-export.

-Peff