[PATCH] fast-import: catch deletion of non-existent file in input
On Fri, Jul 13, 2012 at 03:39:50PM +0200, Andreas Schwab wrote:
Show 17 quoted lines
> >> The output contains these lines:
> >>
> >> R a/b b/b
> >> D a/b
> >>
> >> Changing the second line to D b/b fixes the bug.
> >
> > Yeah, I agree that is problematic. But I do not think it is a
> > fast-import bug, but rather bogus output generated by bzr fast-export (I
> > am not clear from what you wrote above if you are considering it a bug
> > that fast-import is confused). It seems nonsensical to mention a file
> > both as a rename source and as deleted in the same revision, and
> > certainly I would not expect an importer to deduce a link between the
> > second line and b/b.
>
> IMHO fast-import should raise an error in this case, like it does when
> you switch the lines.
Here's a patch to do so. It means that fast-import is a little more strict about deletions than it used to be. I think it's probably a good idea for fast-import to err on the side of strictness (since erring on the other side may mean corrupt input). I don't know if the omission of error checking in the original was a philosophical choice or just random. :)
-- >8 --
Subject: fast-import: catch deletion of non-existent file in input
Fast-import does not do a lot of input verification; in particular, you can mark a path as deleted by a commit even if that files does not exist in the parent at all.
On the one hand, it is nice for fast-import to be liberal in what it accepts, as it means we are more forgiving of exporter bugs or of people slicing and dicing the input. On the other hand, this can mask bugs in exporters, leading to a subtly wrong import result. In this case, bzr's fast-export generated a sequence like:
R foo bar
D foo
when it meant to say:
R foo bar
D bar
We silently ignored the bogus "D foo" directive, and the resulting tree incorrectly contained "bar". With this patch, we notice the bogus input and die.
This patch adds a new test script specifically for checking bogus inputs to fast-import. We also need to tweak t9300, which appeared to be accidentally deleting a non-existent path in an otherwise unrelated test.
Signed-off-by: Jeff King <peff@peff.net>
---
The tweak in t9300 looks like the "dst2" entry was just leftover cruft
from writing the test; there is no dst2 mentioned anywhere else. It
comes from David's 8dc6a37; pleae correct me if my assumptions isn't
correct.
fast-import.c | 5 ++-
t/t9300-fast-import.sh | 1 -
t/t9302-fast-import-bogus.sh | 77 ++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 81 insertions(+), 2 deletions(-)
create mode 100755 t/t9302-fast-import-bogus.sh
diff --git a/fast-import.c b/fast-import.c
index eed97c8..779adfc 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -2367,6 +2367,7 @@ static void file_change_d(struct branch *b)
const char *p = command_buf.buf + 2;
static struct strbuf uq = STRBUF_INIT;
const char *endp;
+ struct tree_entry leaf;
strbuf_reset(&uq);
if (!unquote_c_style(&uq, p, &endp)) {
@@ -2374,7 +2375,9 @@ static void file_change_d(struct branch *b)
die("Garbage after path in: %s", command_buf.buf);
p = uq.buf;
}
- tree_content_remove(&b->branch_tree, p, NULL);
+ tree_content_remove(&b->branch_tree, p, &leaf);
+ if (!leaf.versions[1].mode)
+ die("Path %s not in branch", p);
}
static void file_change_cr(struct branch *b, int rename)
diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
index 2fcf269..7624ba5 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh
@@ -1203,7 +1203,6 @@ test_expect_success PIPE 'N: empty directory reads as missing' '
printf "%s\n" "$line" >response &&
cat <<-\EOF
D dst1
- D dst2
EOF
) |
git fast-import --cat-blob-fd=3 3>backflow &&
diff --git a/t/t9302-fast-import-bogus.sh b/t/t9302-fast-import-bogus.sh
new file mode 100755
index 0000000..95c5196
--- /dev/null
+++ b/t/t9302-fast-import-bogus.sh
@@ -0,0 +1,77 @@
+#!/bin/sh
+
+test_description='test that fast-import handles bogus input correctly'
+. ./test-lib.sh
+
+# A few shorthands to make writing sample input easier
+counter=0
+mark() {
+ counter=$(( $counter + 1)) &&
+ echo "mark :$counter"
+}
+
+blob() {
+ echo blob &&
+ mark &&
+ cat <<-\EOF
+ data 4
+ foo
+
+ EOF
+}
+
+ident() {
+ test_tick &&
+ echo "author $GIT_AUTHOR_NAME <$GIT_AUTHOR_EMAIL> $GIT_AUTHOR_DATE"
+ echo "committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE"
+}
+
+commit() {
+ echo "commit refs/heads/master" &&
+ mark &&
+ ident &&
+ cat <<-\EOF
+ data 8
+ message
+ EOF
+}
+
+root() {
+ blob &&
+ m=$counter &&
+ echo "reset refs/heads/master" &&
+ commit &&
+ echo "M 100644 :$m file" &&
+ echo
+}
+
+test_expect_success 'deleting a nonexistent file is an error' '
+ {
+ root &&
+ commit &&
+ echo "D does-not-exist"
+ } >input &&
+ test_must_fail git fast-import --force <input
+'
+
+test_expect_success 'renaming a deleted file is an error' '
+ {
+ root &&
+ commit &&
+ echo "D file" &&
+ echo "R file dest"
+ } >input &&
+ test_must_fail git fast-import --force <input
+'
+
+test_expect_success 'deleting a renamed file is an error' '
+ {
+ root &&
+ commit &&
+ echo "R file dest" &&
+ echo "D file"
+ } >input &&
+ test_must_fail git fast-import --force <input
+'
+
+test_done
--
1.7.10.5.40.gbbc17de
