1
0
Fork 0
mirror of https://we.phorge.it/source/arcanist.git synced 2024-11-26 00:32:41 +01:00

Improve test coverage of diff/patch generation

Summary:
We have coverage for generating patches, applying them, and making sure they reproduce the original repository state. However:

  - The code uses simplified patch generation which omits some flags like `-M` and `-C`, and generally produces less rich patches than we really produce. Instead, produce patches the same way `arc diff` does.
  - We don't test the intermediate change representation. In theory it's not too important because if we get it wrong the output should be wrong, but in practice it makes it easier to nail down issues. We can also generate less-rich patches which still apply correctly, but would prefer not to.
  - Similarly, we don't test the intermediate patch representation. This is almost entirely redundant with simply applying the patch, but easier to visualize.

Add coverage for all that stuff and fix some bugs with `-M` / `-C` patch generation that weren't caught under the simpler patch generation.

Test Plan: Ran unit tests.

Reviewers: btrahan, vrana

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T866

Differential Revision: https://secure.phabricator.com/D3751
This commit is contained in:
epriestley 2012-10-20 08:43:13 -07:00
parent 5981aa1df4
commit 4b03a3fa0d
16 changed files with 491 additions and 30 deletions

View file

@ -267,6 +267,22 @@ final class ArcanistBundle {
$result = array(); $result = array();
$changes = $this->getChanges(); $changes = $this->getChanges();
$binary_sources = array();
foreach ($changes as $change) {
if (!$this->isGitBinaryChange($change)) {
continue;
}
$type = $change->getType();
if ($type == ArcanistDiffChangeType::TYPE_MOVE_AWAY ||
$type == ArcanistDiffChangeType::TYPE_COPY_AWAY ||
$type == ArcanistDiffChangeType::TYPE_MULTICOPY) {
foreach ($change->getAwayPaths() as $path) {
$binary_sources[$path] = $change;
}
}
}
foreach (array_keys($changes) as $multicopy_key) { foreach (array_keys($changes) as $multicopy_key) {
$multicopy_change = $changes[$multicopy_key]; $multicopy_change = $changes[$multicopy_key];
@ -310,20 +326,6 @@ final class ArcanistBundle {
} }
} }
$old_file_phids = array();
foreach ($changes as $change) {
if (!$this->isGitBinaryChange($change)) {
continue;
}
$type = $change->getType();
if ($type == ArcanistDiffChangeType::TYPE_MOVE_AWAY) {
foreach ($change->getAwayPaths() as $path) {
$old_file_phids[$path] = $change->getMetadata('old:binary-phid');
}
}
}
foreach ($changes as $change) { foreach ($changes as $change) {
$type = $change->getType(); $type = $change->getType();
$file_type = $change->getFileType(); $file_type = $change->getFileType();
@ -350,8 +352,8 @@ final class ArcanistBundle {
$is_binary = $this->isGitBinaryChange($change); $is_binary = $this->isGitBinaryChange($change);
if ($is_binary) { if ($is_binary) {
$old_phid = idx($old_file_phids, $this->getCurrentPath($change)); $old_binary = idx($binary_sources, $this->getCurrentPath($change));
$change_body = $this->buildBinaryChange($change, $old_phid); $change_body = $this->buildBinaryChange($change, $old_binary);
} else { } else {
$change_body = $this->buildHunkChanges($change->getHunks()); $change_body = $this->buildHunkChanges($change->getHunks());
} }
@ -648,14 +650,25 @@ final class ArcanistBundle {
throw new Exception("Nowhere to load blob '{$phid}' from!"); throw new Exception("Nowhere to load blob '{$phid}' from!");
} }
private function buildBinaryChange(ArcanistDiffChange $change, $old_phid) { private function buildBinaryChange(ArcanistDiffChange $change, $old_binary) {
$old_phid = idx($change->getAllMetadata(), 'old:binary-phid', $old_phid); // In Git, when we write out a binary file move or copy, we need the
$new_phid = $change->getMetadata('new:binary-phid'); // original binary for the source and the current binary for the
// destination.
$old_data = null; if ($old_binary) {
if ($change->getOriginalFileData() !== null) { if ($old_binary->getOriginalFileData() !== null) {
$old_data = $old_binary->getOriginalFileData();
$old_phid = null;
} else {
$old_data = null;
$old_binary->getMetadata('old:binary-phid');
}
} else {
$old_data = $change->getOriginalFileData(); $old_data = $change->getOriginalFileData();
} else if ($old_phid) { $old_phid = $change->getMetadata('old:binary-phid');
}
if ($old_data === null && $old_phid) {
$name = basename($change->getOldPath()); $name = basename($change->getOldPath());
$old_data = $this->getBlob($old_phid, $name); $old_data = $this->getBlob($old_phid, $name);
} }
@ -669,6 +682,8 @@ final class ArcanistBundle {
$old_sha1 = sha1("blob {$old_length}\0{$old_data}"); $old_sha1 = sha1("blob {$old_length}\0{$old_data}");
} }
$new_phid = $change->getMetadata('new:binary-phid');
$new_data = null; $new_data = null;
if ($change->getCurrentFileData() !== null) { if ($change->getCurrentFileData() !== null) {
$new_data = $change->getCurrentFileData(); $new_data = $change->getCurrentFileData();

View file

@ -56,6 +56,7 @@ final class ArcanistBundleTestCase extends ArcanistTestCase {
} }
$archive = dirname(__FILE__).'/bundle.git.tgz'; $archive = dirname(__FILE__).'/bundle.git.tgz';
$patches = dirname(__FILE__).'/patches/';
$fixture = PhutilDirectoryFixture::newFromArchive($archive); $fixture = PhutilDirectoryFixture::newFromArchive($archive);
chdir($fixture->getPath()); chdir($fixture->getPath());
@ -73,27 +74,51 @@ final class ArcanistBundleTestCase extends ArcanistTestCase {
list($commit_hash, $tree_hash, $subject) = explode(' ', $commit, 3); list($commit_hash, $tree_hash, $subject) = explode(' ', $commit, 3);
execx('git reset --hard %s --', $commit_hash); execx('git reset --hard %s --', $commit_hash);
list($diff) = execx(
'git diff %s^ %s --',
$commit_hash,
$commit_hash);
$repository_api = new ArcanistGitAPI($fixture->getPath()); $repository_api = new ArcanistGitAPI($fixture->getPath());
$repository_api->setDefaultBaseCommit(); $repository_api->setDefaultBaseCommit();
$diff = $repository_api->getFullGitDiff();
$parser = new ArcanistDiffParser(); $parser = new ArcanistDiffParser();
$parser->setRepositoryAPI($repository_api); $parser->setRepositoryAPI($repository_api);
$changes = $parser->parseDiff($diff); $changes = $parser->parseDiff($diff);
$this->makeChangeAssertions($commit_hash, $changes);
$bundle = ArcanistBundle::newFromChanges($changes); $bundle = ArcanistBundle::newFromChanges($changes);
execx('git reset --hard %s^ --', $commit_hash); execx('git reset --hard %s^ --', $commit_hash);
$patch = $bundle->toGitPatch(); $patch = $bundle->toGitPatch();
id(new ExecFuture('git apply --index --reject')) $expect_path = $patches.'/'.$commit_hash.'.gitpatch';
->write($patch) $expect = null;
->resolvex(); if (Filesystem::pathExists($expect_path)) {
$expect = Filesystem::readFile($expect_path);
}
if ($patch === $expect) {
$this->assertEqual($expect, $patch);
} else {
Filesystem::writeFile($expect_path.'.real', $patch);
throw new Exception(
"Expected patch and actual patch for {$commit_hash} differ. ".
"Wrote actual patch to '{$expect_path}.real'.");
}
try {
id(new ExecFuture('git apply --index --reject'))
->write($patch)
->resolvex();
} catch (CommandException $ex) {
$temp = new TempFile(substr($commit_hash, 0, 8).'.patch');
$temp->setPreserveFile(true);
Filesystem::writeFile($temp, $patch);
PhutilConsole::getConsole()->writeErr(
"Wrote failing patch to '%s'.\n",
$temp);
throw $ex;
}
execx('git commit -m %s', $subject); execx('git commit -m %s', $subject);
list($result_hash) = execx('git log -n1 --format=%s', '%T'); list($result_hash) = execx('git log -n1 --format=%s', '%T');
@ -106,6 +131,241 @@ final class ArcanistBundleTestCase extends ArcanistTestCase {
} }
} }
private function makeChangeAssertions($commit, array $raw_changes) {
$changes = array();
// Verify that there are no duplicate changes, and rekey the changes on
// affected path because we don't care about the order in which the
// changes appear.
foreach ($raw_changes as $change) {
$this->assertEqual(
true,
empty($changes[$change->getCurrentPath()]),
"Unique Path: ".$change->getCurrentPath());
$changes[$change->getCurrentPath()] = $change;
}
switch ($commit) {
case '5dec8bf28557f078d1987c4e8cfb53d08310f522':
// "Copy an image, and replace the original."
// `image_2.png` is copied to `image.png` and then replaced.
$this->assertEqual(2, count($changes));
$c = $changes['image.png'];
$this->assertEqual(
ArcanistDiffChangeType::TYPE_COPY_HERE,
$c->getType());
$this->assertEqual(
ArcanistDiffChangeType::FILE_BINARY,
$c->getFileType());
$this->assertEqual(
null,
$c->getOriginalFileData());
$this->assertEqual(
'8645053452b2cc2f955ef3944ac0831a',
md5($c->getCurrentFileData()));
$c = $changes['image_2.png'];
$this->assertEqual(
ArcanistDiffChangeType::TYPE_COPY_AWAY,
$c->getType());
$this->assertEqual(
ArcanistDiffChangeType::FILE_BINARY,
$c->getFileType());
$this->assertEqual(
'8645053452b2cc2f955ef3944ac0831a',
md5($c->getOriginalFileData()));
$this->assertEqual(
'c9ec1b952480da09b393ba672d9b13da',
md5($c->getCurrentFileData()));
break;
case 'fb28468d25a5fdd063aca4ca559454c998a0af51':
// "Multicopy image."
// `image.png` is copied to `image_2.png` and `image_3.png` and then
// deleted. Git detects this as a move and an add.
$this->assertEqual(3, count($changes));
$c = $changes['image.png'];
$this->assertEqual(
ArcanistDiffChangeType::TYPE_MULTICOPY,
$c->getType());
$this->assertEqual(
ArcanistDiffChangeType::FILE_BINARY,
$c->getFileType());
$this->assertEqual(
'8645053452b2cc2f955ef3944ac0831a',
md5($c->getOriginalFileData()));
$this->assertEqual(
null,
$c->getCurrentFileData());
$c = $changes['image_2.png'];
$this->assertEqual(
ArcanistDiffChangeType::TYPE_COPY_HERE,
$c->getType());
$this->assertEqual(
ArcanistDiffChangeType::FILE_BINARY,
$c->getFileType());
$this->assertEqual(
null,
$c->getOriginalFileData());
$this->assertEqual(
'8645053452b2cc2f955ef3944ac0831a',
md5($c->getCurrentFileData()));
$c = $changes['image_3.png'];
$this->assertEqual(
ArcanistDiffChangeType::TYPE_MOVE_HERE,
$c->getType());
$this->assertEqual(
ArcanistDiffChangeType::FILE_BINARY,
$c->getFileType());
$this->assertEqual(
null,
$c->getOriginalFileData());
$this->assertEqual(
'8645053452b2cc2f955ef3944ac0831a',
md5($c->getCurrentFileData()));
break;
case 'df340e88d8aba12e8f2b8827f01f0cd9f35eb758':
// "Remove binary image."
// `image_2.png` is deleted.
$this->assertEqual(1, count($changes));
$c = $changes['image_2.png'];
$this->assertEqual(
ArcanistDiffChangeType::TYPE_DELETE,
$c->getType());
$this->assertEqual(
ArcanistDiffChangeType::FILE_BINARY,
$c->getFileType());
$this->assertEqual(
'8645053452b2cc2f955ef3944ac0831a',
md5($c->getOriginalFileData()));
$this->assertEqual(
null,
$c->getCurrentFileData());
break;
case '3f5c6d735e64c25a04f83be48ef184b25b5282f0':
// "Copy binary image."
// `image_2.png` is copied to `image.png`. Git does not detect this as
// a copy without --find-copies-harder.
$this->assertEqual(1, count($changes));
$c = $changes['image.png'];
$this->assertEqual(
ArcanistDiffChangeType::TYPE_ADD,
$c->getType());
$this->assertEqual(
ArcanistDiffChangeType::FILE_BINARY,
$c->getFileType());
$this->assertEqual(
null,
$c->getOriginalFileData());
$this->assertEqual(
'8645053452b2cc2f955ef3944ac0831a',
md5($c->getCurrentFileData()));
break;
case 'b454edb3bb29890ee5b3af5ef66ce6a24d15d882':
// "Move binary image."
// `image.png` is moved to `image_2.png`.
$this->assertEqual(2, count($changes));
$c = $changes['image.png'];
$this->assertEqual(
ArcanistDiffChangeType::TYPE_MOVE_AWAY,
$c->getType());
$this->assertEqual(
ArcanistDiffChangeType::FILE_BINARY,
$c->getFileType());
$this->assertEqual(
'8645053452b2cc2f955ef3944ac0831a',
md5($c->getOriginalFileData()));
$this->assertEqual(
null,
$c->getCurrentFileData());
$c = $changes['image_2.png'];
$this->assertEqual(
ArcanistDiffChangeType::TYPE_MOVE_HERE,
$c->getType());
$this->assertEqual(
ArcanistDiffChangeType::FILE_BINARY,
$c->getFileType());
$this->assertEqual(
null,
$c->getOriginalFileData());
$this->assertEqual(
'8645053452b2cc2f955ef3944ac0831a',
md5($c->getCurrentFileData()));
break;
case '5de5f3dfda1b7db2eb054e57699f05aaf1f4483e':
// "Add a binary image."
// `image.png` is added.
$c = $changes['image.png'];
$this->assertEqual(
ArcanistDiffChangeType::TYPE_ADD,
$c->getType());
$this->assertEqual(
ArcanistDiffChangeType::FILE_BINARY,
$c->getFileType());
$this->assertEqual(
null,
$c->getOriginalFileData());
$this->assertEqual(
'8645053452b2cc2f955ef3944ac0831a',
md5($c->getCurrentFileData()));
break;
case '176a4c2c3fd88b2d598ce41a55d9c3958be9fd2d':
// "Convert \r\n newlines to \n newlines."
case 'a73b28e139296d23ade768f2346038318b331f94':
// "Add text with \r\n newlines."
case '337ccec314075a2bdb4a912ef467d35d04a713e4':
// "Convert \n newlines to \r\n newlines.";
case '6d5e64a4a7a6a036c53b1d087184cb2c70099f2c':
// "Remove tabs."
case '49395994a1a8a06287e40a3b318be4349e8e0288':
// "Add tabs."
case 'a5a53c424f3c2a7e85f6aee35e834c8ec5b3dbe3':
// "Add trailing newline."
case 'd53dc614090c6c7d6d023e170877d7f611f18f5a':
// "Remove trailing newline."
case 'f19fb9fa1385c01b53bdb6d8842dd154e47151ec':
// "Edit a text file."
$this->assertEqual(1, count($changes));
$c = $changes['text'];
$this->assertEqual(
ArcanistDiffChangeType::TYPE_CHANGE,
$c->getType());
$this->assertEqual(
ArcanistDiffChangeType::FILE_TEXT,
$c->getFileType());
break;
case '228d7be4840313ed805c25c15bba0f7b188af3e6':
// "Add a text file."
// This commit is never reached because we skip the 0th commit junk.
$this->assertEqual(true, "This is never reached.");
break;
default:
throw new Exception(
"Commit {$commit} has no change assertions!");
}
}
public function testTrailingContext() { public function testTrailingContext() {
// Diffs need to generate without extra trailing context, or 'patch' will // Diffs need to generate without extra trailing context, or 'patch' will
// choke on them. // choke on them.

View file

@ -0,0 +1,11 @@
diff --git a/text b/text
--- a/text
+++ b/text
@@ -1,3 +1,3 @@
-quack quack
-I am a duck
-quack quack
+quack quack
+I am a duck
+quack quack

View file

@ -0,0 +1,9 @@
diff --git a/text b/text
--- a/text
+++ b/text
@@ -1,2 +1,2 @@
-quack quack
-quack quack
+quack quack
+quack quack

View file

@ -0,0 +1,13 @@
diff --git a/image.png b/image.png
new file mode 100644
index 0000000000000000000000000000000000000000..229920a50597be0644f4b994d213fc7042050289
GIT binary patch
literal 108
zc%17D@N?(olHy`uVBq!ia0vp^EFjFm1SHiab7}%9$r9IylHmNblJdl&R0hYC{G?O`
z&)mfH)S%SFl*+=BsWw1Ge4Z|jAr-fh5)@<^SR@!2Oc)q>5)V!R$}o7k`njxgN@xNA
Dw4xaJ
literal 0
Hc$@<O00001

View file

@ -0,0 +1,9 @@
diff --git a/text b/text
--- a/text
+++ b/text
@@ -1,2 +1,2 @@
-quack quack
-quack quack
+ quack quack
+ quack quack

View file

@ -0,0 +1,13 @@
diff --git a/image.png b/image.png
new file mode 100644
index 0000000000000000000000000000000000000000..229920a50597be0644f4b994d213fc7042050289
GIT binary patch
literal 108
zc%17D@N?(olHy`uVBq!ia0vp^EFjFm1SHiab7}%9$r9IylHmNblJdl&R0hYC{G?O`
z&)mfH)S%SFl*+=BsWw1Ge4Z|jAr-fh5)@<^SR@!2Oc)q>5)V!R$}o7k`njxgN@xNA
Dw4xaJ
literal 0
Hc$@<O00001

View file

@ -0,0 +1,29 @@
diff --git a/image_2.png b/image.png
copy from image_2.png
copy to image.png
index 229920a50597be0644f4b994d213fc7042050289..229920a50597be0644f4b994d213fc7042050289
GIT binary patch
literal 108
zc%17D@N?(olHy`uVBq!ia0vp^EFjFm1SHiab7}%9$r9IylHmNblJdl&R0hYC{G?O`
z&)mfH)S%SFl*+=BsWw1Ge4Z|jAr-fh5)@<^SR@!2Oc)q>5)V!R$}o7k`njxgN@xNA
Dw4xaJ
literal 108
zc%17D@N?(olHy`uVBq!ia0vp^EFjFm1SHiab7}%9$r9IylHmNblJdl&R0hYC{G?O`
z&)mfH)S%SFl*+=BsWw1Ge4Z|jAr-fh5)@<^SR@!2Oc)q>5)V!R$}o7k`njxgN@xNA
Dw4xaJ
diff --git a/image_2.png b/image_2.png
index 229920a50597be0644f4b994d213fc7042050289..69ac76e69fabc49d97339547b91cf748ab703052
GIT binary patch
literal 113
zc%17D@N?(olHy`uVBq!ia0vp^EFjFm1SHiab7}%9$r9IylHmNblJdl&R0hYC{G?O`
z&)mfH)S%SFl*+=BsWw1G!k#XUAr-fh{+w@M@ZoVOWMJZ9Fuush?)u2^8c>?S)78&q
Iol`;+0DguX?EnA(
literal 108
zc%17D@N?(olHy`uVBq!ia0vp^EFjFm1SHiab7}%9$r9IylHmNblJdl&R0hYC{G?O`
z&)mfH)S%SFl*+=BsWw1Ge4Z|jAr-fh5)@<^SR@!2Oc)q>5)V!R$}o7k`njxgN@xNA
Dw4xaJ

View file

@ -0,0 +1,9 @@
diff --git a/text b/text
--- a/text
+++ b/text
@@ -1,2 +1,2 @@
- quack quack
- quack quack
+quack quack
+quack quack

View file

@ -0,0 +1,9 @@
diff --git a/text b/text
--- a/text
+++ b/text
@@ -1,2 +1,2 @@
quack quack
-quack quack
\ No newline at end of file
+quack quack

View file

@ -0,0 +1,8 @@
diff --git a/text b/text
--- a/text
+++ b/text
@@ -1,2 +1,3 @@
quack quack
+I am a duck
quack quack

View file

@ -0,0 +1,16 @@
diff --git a/image.png b/image_2.png
rename from image.png
rename to image_2.png
index 229920a50597be0644f4b994d213fc7042050289..229920a50597be0644f4b994d213fc7042050289
GIT binary patch
literal 108
zc%17D@N?(olHy`uVBq!ia0vp^EFjFm1SHiab7}%9$r9IylHmNblJdl&R0hYC{G?O`
z&)mfH)S%SFl*+=BsWw1Ge4Z|jAr-fh5)@<^SR@!2Oc)q>5)V!R$}o7k`njxgN@xNA
Dw4xaJ
literal 108
zc%17D@N?(olHy`uVBq!ia0vp^EFjFm1SHiab7}%9$r9IylHmNblJdl&R0hYC{G?O`
z&)mfH)S%SFl*+=BsWw1Ge4Z|jAr-fh5)@<^SR@!2Oc)q>5)V!R$}o7k`njxgN@xNA
Dw4xaJ

View file

@ -0,0 +1,9 @@
diff --git a/text b/text
--- a/text
+++ b/text
@@ -1,2 +1,2 @@
quack quack
-quack quack
+quack quack
\ No newline at end of file

View file

@ -0,0 +1,13 @@
diff --git a/image_2.png b/image_2.png
deleted file mode 100644
index 229920a50597be0644f4b994d213fc7042050289..0000000000000000000000000000000000000000
GIT binary patch
literal 0
Hc$@<O00001
literal 108
zc%17D@N?(olHy`uVBq!ia0vp^EFjFm1SHiab7}%9$r9IylHmNblJdl&R0hYC{G?O`
z&)mfH)S%SFl*+=BsWw1Ge4Z|jAr-fh5)@<^SR@!2Oc)q>5)V!R$}o7k`njxgN@xNA
Dw4xaJ

View file

@ -0,0 +1,7 @@
diff --git a/text b/text
--- a/text
+++ b/text
@@ -1 +1,2 @@
quack quack
+quack quack

View file

@ -0,0 +1,31 @@
diff --git a/image.png b/image_2.png
rename from image.png
rename to image_2.png
index 229920a50597be0644f4b994d213fc7042050289..229920a50597be0644f4b994d213fc7042050289
GIT binary patch
literal 108
zc%17D@N?(olHy`uVBq!ia0vp^EFjFm1SHiab7}%9$r9IylHmNblJdl&R0hYC{G?O`
z&)mfH)S%SFl*+=BsWw1Ge4Z|jAr-fh5)@<^SR@!2Oc)q>5)V!R$}o7k`njxgN@xNA
Dw4xaJ
literal 108
zc%17D@N?(olHy`uVBq!ia0vp^EFjFm1SHiab7}%9$r9IylHmNblJdl&R0hYC{G?O`
z&)mfH)S%SFl*+=BsWw1Ge4Z|jAr-fh5)@<^SR@!2Oc)q>5)V!R$}o7k`njxgN@xNA
Dw4xaJ
diff --git a/image.png b/image_3.png
rename from image.png
rename to image_3.png
index 229920a50597be0644f4b994d213fc7042050289..229920a50597be0644f4b994d213fc7042050289
GIT binary patch
literal 108
zc%17D@N?(olHy`uVBq!ia0vp^EFjFm1SHiab7}%9$r9IylHmNblJdl&R0hYC{G?O`
z&)mfH)S%SFl*+=BsWw1Ge4Z|jAr-fh5)@<^SR@!2Oc)q>5)V!R$}o7k`njxgN@xNA
Dw4xaJ
literal 108
zc%17D@N?(olHy`uVBq!ia0vp^EFjFm1SHiab7}%9$r9IylHmNblJdl&R0hYC{G?O`
z&)mfH)S%SFl*+=BsWw1Ge4Z|jAr-fh5)@<^SR@!2Oc)q>5)V!R$}o7k`njxgN@xNA
Dw4xaJ