Fixing bug where files failed to delete that were modified previously

Author: weaverryan

.github/workflows/ci.yaml

@@ -20,6 +20,7 @@ jobs:
  matrix:
  include:
  - php: '7.1'
+ composer: 2.2.x
  - php: '7.2'
  - php: '7.3'
  - php: '7.4'
@@ -33,11 +34,11 @@ jobs:
  uses: actions/checkout@v2.3.3
 
  - name: "Install PHP with extensions"
- uses: shivammathur/setup-php@2.7.0
+ uses: shivammathur/setup-php@2.18.0
  with:
  coverage: "none"
  php-version: ${{ matrix.php }}
- tools: composer:v2
+ tools: composer:${{ matrix.composer }}
 
  - name: "Validate composer.json"
  run: "composer validate --strict --no-check-lock"

src/Update/RecipePatch.php

@@ -15,12 +15,14 @@ class RecipePatch
 {
  private $patch;
  private $blobs;
+ private $deletedFiles;
  private $removedPatches;
 
- public function __construct(string $patch, array $blobs, array $removedPatches = [])
+ public function __construct(string $patch, array $blobs, array $deletedFiles, array $removedPatches = [])
  {
  $this->patch = $patch;
  $this->blobs = $blobs;
+ $this->deletedFiles = $deletedFiles;
  $this->removedPatches = $removedPatches;
  }
 
@@ -34,6 +36,11 @@ public function getBlobs(): array
  return $this->blobs;
  }
 
+ public function getDeletedFiles(): array
+ {
+ return $this->deletedFiles;
+ }
+
  /**
  * Patches for modified files that were removed because the file
  * has been deleted in the user's project.

src/Update/RecipePatcher.php

@@ -38,40 +38,13 @@ public function __construct(string $rootDir, IOInterface $io)
  */
  public function applyPatch(RecipePatch $patch): bool
  {
- if (!$patch->getPatch()) {
- // nothing to do!
- return true;
- }
-
- $addedBlobs = $this->addMissingBlobs($patch->getBlobs());
-
- $patchPath = $this->rootDir.'/_flex_recipe_update.patch';
- file_put_contents($patchPath, $patch->getPatch());
+ $withConflicts = $this->_applyPatchFile($patch);
 
- try {
- $this->execute('git update-index --refresh', $this->rootDir);
-
- $output = '';
- $statusCode = $this->processExecutor->execute('git apply "_flex_recipe_update.patch" -3', $output, $this->rootDir);
-
- if (0 === $statusCode) {
- // successful with no conflicts
- return true;
- }
-
- if (false !== strpos($this->processExecutor->getErrorOutput(), 'with conflicts')) {
- // successful with conflicts
- return false;
- }
-
- throw new \LogicException('Error applying the patch: '.$this->processExecutor->getErrorOutput());
- } finally {
- unlink($patchPath);
- // clean up any temporary blobs
- foreach ($addedBlobs as $filename) {
- unlink($filename);
- }
+ foreach ($patch->getDeletedFiles() as $deletedFile) {
+ $this->execute(sprintf('git rm %s', ProcessExecutor::escape($deletedFile)), $this->rootDir);
  }
+
+ return $withConflicts;
  }
 
  public function generatePatch(array $originalFiles, array $newFiles): RecipePatch
@@ -84,10 +57,13 @@ public function generatePatch(array $originalFiles, array $newFiles): RecipePatc
  return null !== $file;
  });
 
- // find removed files and add them so they will be deleted
+ $deletedFiles = [];
+ // find removed files & record that they are deleted
+ // unset them from originalFiles to avoid unnecessary blobs being added
  foreach ($originalFiles as $file => $contents) {
  if (!isset($newFiles[$file])) {
- $newFiles[$file] = null;
+ $deletedFiles[] = $file;
+ unset($originalFiles[$file]);
  }
  }
 
@@ -130,6 +106,7 @@ public function generatePatch(array $originalFiles, array $newFiles): RecipePatc
  return new RecipePatch(
  $patchString,
  $blobs,
+ $deletedFiles,
  $removedPatches
  );
  } finally {
@@ -223,4 +200,42 @@ private function getBlobPath(string $hash): string
 
  return '.git/objects/'.$hashStart.'/'.$hashEnd;
  }
+
+ private function _applyPatchFile(RecipePatch $patch)
+ {
+ if (!$patch->getPatch()) {
+ // nothing to do!
+ return true;
+ }
+
+ $addedBlobs = $this->addMissingBlobs($patch->getBlobs());
+
+ $patchPath = $this->rootDir.'/_flex_recipe_update.patch';
+ file_put_contents($patchPath, $patch->getPatch());
+
+ try {
+ $this->execute('git update-index --refresh', $this->rootDir);
+
+ $output = '';
+ $statusCode = $this->processExecutor->execute('git apply "_flex_recipe_update.patch" -3', $output, $this->rootDir);
+
+ if (0 === $statusCode) {
+ // successful with no conflicts
+ return true;
+ }
+
+ if (false !== strpos($this->processExecutor->getErrorOutput(), 'with conflicts')) {
+ // successful with conflicts
+ return false;
+ }
+
+ throw new \LogicException('Error applying the patch: '.$this->processExecutor->getErrorOutput());
+ } finally {
+ unlink($patchPath);
+ // clean up any temporary blobs
+ foreach ($addedBlobs as $filename) {
+ unlink($filename);
+ }
+ }
+ }
 }

tests/Update/RecipePatchTest.php

@@ -20,12 +20,14 @@ public function testBasicFunctioning()
  {
  $thePatch = 'the patch';
  $blobs = ['blob1', 'blob2', 'beware of the blob'];
+ $deletedFiles = ['old_file.txt'];
  $removedPatches = ['foo' => 'some diff'];
 
- $patch = new RecipePatch($thePatch, $blobs, $removedPatches);
+ $patch = new RecipePatch($thePatch, $blobs, $deletedFiles, $removedPatches);
 
  $this->assertSame($thePatch, $patch->getPatch());
  $this->assertSame($blobs, $patch->getBlobs());
+ $this->assertSame($deletedFiles, $patch->getDeletedFiles());
  $this->assertSame($removedPatches, $patch->getRemovedPatches());
  }
 }

tests/Update/RecipePatcherTest.php

@@ -31,7 +31,7 @@ protected function setUp(): void
  /**
  * @dataProvider getGeneratePatchTests
  */
- public function testGeneratePatch(array $originalFiles, array $newFiles, string $expectedPatch)
+ public function testGeneratePatch(array $originalFiles, array $newFiles, string $expectedPatch, array $expectedDeletedFiles = [])
  {
  $this->getFilesystem()->remove(FLEX_TEST_DIR);
  $this->getFilesystem()->mkdir(FLEX_TEST_DIR);
@@ -44,6 +44,7 @@ public function testGeneratePatch(array $originalFiles, array $newFiles, string
 
  $patch = $patcher->generatePatch($originalFiles, $newFiles);
  $this->assertSame($expectedPatch, rtrim($patch->getPatch(), "\n"));
+ $this->assertSame($expectedDeletedFiles, $patch->getDeletedFiles());
 
  // find all "index 7d30dc7.." in patch
  $matches = [];
@@ -121,31 +122,15 @@ public function getGeneratePatchTests(): iterable
  yield 'file_deleted_in_update_because_missing' => [
  ['file1.txt' => 'New file'],
  [],
- <<<EOF
-diff --git a/file1.txt b/file1.txt
-deleted file mode 100644
-index b78ca63..0000000
---- a/file1.txt
-+++ /dev/null
-@@ -1 +0,0 @@
--New file
-\ No newline at end of file
-EOF
+ '',
+ ['file1.txt'],
  ];
 
  yield 'file_deleted_in_update_because_null' => [
  ['file1.txt' => 'New file'],
  ['file1.txt' => null],
- <<<EOF
-diff --git a/file1.txt b/file1.txt
-deleted file mode 100644
-index b78ca63..0000000
---- a/file1.txt
-+++ /dev/null
-@@ -1 +0,0 @@
--New file
-\ No newline at end of file
-EOF
+ '',
+ ['file1.txt'],
  ];
 
  yield 'mixture_of_added_updated_removed' => [
@@ -169,15 +154,9 @@ public function getGeneratePatchTests(): iterable
 @@ -0,0 +1 @@
 +file to create
 \ No newline at end of file
-diff --git a/will_be_deleted.txt b/will_be_deleted.txt
-deleted file mode 100644
-index 98ff166..0000000
---- a/will_be_deleted.txt
-+++ /dev/null
-@@ -1 +0,0 @@
--file to delete
-\ No newline at end of file
 EOF
+ ,
+ ['will_be_deleted.txt'],
  ];
  }
 
@@ -243,7 +222,8 @@ public function getApplyPatchTests(): iterable
  ['.env' => $dotEnvClean['in_app']],
  new RecipePatch(
  $dotEnvClean['patch'],
- [$dotEnvClean['hash'] => $dotEnvClean['blob']]
+ [$dotEnvClean['hash'] => $dotEnvClean['blob']],
+ []
  ),
  ['.env' => $dotEnvClean['expected']],
  false,
@@ -253,7 +233,8 @@ public function getApplyPatchTests(): iterable
  ['package.json' => $packageJsonConflict['in_app']],
  new RecipePatch(
  $packageJsonConflict['patch'],
- [$packageJsonConflict['hash'] => $packageJsonConflict['blob']]
+ [$packageJsonConflict['hash'] => $packageJsonConflict['blob']],
+ []
  ),
  ['package.json' => $packageJsonConflict['expected']],
  true,
@@ -265,6 +246,7 @@ public function getApplyPatchTests(): iterable
  new RecipePatch(
  $webpackEncoreAdded['patch'],
  // no blobs needed for a new file
+ [],
  []
  ),
  ['config/packages/webpack_encore.yaml' => $webpackEncoreAdded['expected']],
@@ -274,8 +256,9 @@ public function getApplyPatchTests(): iterable
  yield 'removed_one_file' => [
  ['config/packages/security.yaml' => $securityRemoved['in_app']],
  new RecipePatch(
- $securityRemoved['patch'],
- [$securityRemoved['hash'] => $securityRemoved['blob']]
+ '',
+ [$securityRemoved['hash'] => $securityRemoved['blob']],
+ ['config/packages/security.yaml']
  ),
  // expected to be deleted
  ['config/packages/security.yaml' => null],
@@ -290,12 +273,15 @@ public function getApplyPatchTests(): iterable
  'config/packages/security.yaml' => $securityRemoved['in_app'],
  ],
  new RecipePatch(
- $dotEnvClean['patch']."\n".$packageJsonConflict['patch']."\n".$webpackEncoreAdded['patch']."\n".$securityRemoved['patch'],
+ $dotEnvClean['patch']."\n".$packageJsonConflict['patch']."\n".$webpackEncoreAdded['patch'],
  [
  $dotEnvClean['hash'] => $dotEnvClean['blob'],
  $packageJsonConflict['hash'] => $packageJsonConflict['blob'],
  $webpackEncoreAdded['hash'] => $webpackEncoreAdded['blob'],
  $securityRemoved['hash'] => $securityRemoved['blob'],
+ ],
+ [
+ 'config/packages/security.yaml',
  ]
  ),
  [