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