Fix possible coredump with fast-import --import-marks

When e8438420bb7d368bec3647b90c557b9931582267 allowed us to reload
the marks table on subsequent runs of fast-import we really broke
things, as we set pack_id to MAX_PACK_ID for any objects we imported
into the marks table.  Creating a branch from that mark should fail
as we attempt to read the object through a non-existant packed_git
pointer.  Instead we have to use the normal Git object system to
locate the older commit, as we ourselves do not have a reference
to the packed_git it resides in.

This bug only occurred because t9300 was not complete enough.
When we added the --import-marks feature we didn't actually test
its implementation enough to verify the function worked as intended.
I have corrected that, and included the changes as part of this fix.
Prior versions of fast-import fail the new test(s); this commit
allows them to pass.

Credit for this bug find goes to Simon Hausmann <simon@lst.de> as
he recently identified a similiar bug in the tree lazy-loading path.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>

diff --git a/fast-import.c b/fast-import.c
index f5107df74d248afc7e2e2d45f7128fb7779fdfdc..17554f68493a8409c64b47334a94e622d47d8133 100644 (file)
--- a/fast-import.c
+++ b/fast-import.c
@@ -1709,14 +1709,16 @@ static void cmd_from(struct branch *b)
        } else if (*from == ':') {
                uintmax_t idnum = strtoumax(from + 1, NULL, 10);
                struct object_entry *oe = find_mark(idnum);
-               unsigned long size;
-               char *buf;
                if (oe->type != OBJ_COMMIT)
                        die("Mark :%" PRIuMAX " not a commit", idnum);
                hashcpy(b->sha1, oe->sha1);
-               buf = gfi_unpack_entry(oe, &size);
-               cmd_from_commit(b, buf, size);
-               free(buf);
+               if (oe->pack_id != MAX_PACK_ID) {
+                       unsigned long size;
+                       char *buf = gfi_unpack_entry(oe, &size);
+                       cmd_from_commit(b, buf, size);
+                       free(buf);
+               } else
+                       cmd_from_existing(b);
        } else if (!get_sha1(from, b->sha1))
                cmd_from_existing(b);
        else

diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
index 8e958da5361f69e47a27833948eed0bd7dc8b32e..72e49f5d3bebcf6509536c578cc934879ee1aa55 100755 (executable)
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh
@@ -119,6 +119,35 @@ test_expect_success \
                </dev/null &&
        git diff -u expect marks.new'
 
+test_tick
+cat >input <<INPUT_END
+commit refs/heads/verify--import-marks
+committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+data <<COMMIT
+recreate from :5
+COMMIT
+
+from :5
+M 755 :2 copy-of-file2
+
+INPUT_END
+test_expect_success \
+       'A: verify marks import does not crash' \
+       'git-fast-import --import-marks=marks.out <input &&
+        git-whatchanged verify--import-marks'
+test_expect_success \
+       'A: verify pack' \
+       'for p in .git/objects/pack/*.pack;do git-verify-pack $p||exit;done'
+cat >expect <<EOF
+:000000 100755 0000000000000000000000000000000000000000 7123f7f44e39be127c5eb701e5968176ee9d78b1 A     copy-of-file2
+EOF
+git-diff-tree -M -r master verify--import-marks >actual
+test_expect_success \
+       'A: verify diff' \
+       'compare_diff_raw expect actual &&
+        test `git-rev-parse --verify master:file2` \
+           = `git-rev-parse --verify verify--import-marks:copy-of-file2`'
+
 ###
 ### series B
 ###