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

From: Jeff King <peff@peff.net>
Date: Aug 21, 2015, 14:58 UTC
Message-ID: <20150821145827.GA565@sigill.intra.peff.net>
In-Reply-To: <alpine.DEB.2.10.1508191532330.31851@buzzword-bingo.mit.edu>

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

Show 17 quoted lines

> 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