From: Jeff King <peff@peff.net>
To: Anders Kaseorg <andersk@mit.edu>
Cc: gitster@pobox.com, git@vger.kernel.org
Subject: Re: [BUG/PATCH] t9350-fast-export: Add failing test for symlink-to-directory
Date: Fri, 21 Aug 2015 10:58:27 -0400 [thread overview]
Message-ID: <20150821145827.GA565@sigill.intra.peff.net> (raw)
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:
> 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
next prev parent reply other threads:[~2015-08-21 14:58 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-08-19 19:46 [BUG/PATCH] t9350-fast-export: Add failing test for symlink-to-directory Anders Kaseorg
2015-08-21 14:58 ` Jeff King [this message]
2015-08-21 16:47 ` Anders Kaseorg
2015-08-24 5:25 ` Jeff King
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20150821145827.GA565@sigill.intra.peff.net \
--to=peff@peff.net \
--cc=andersk@mit.edu \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).