Use merge-base (three dot diff) instead of direct diff (#1653) (#1654) (

#1655)

* Use merge-base (three dot diff) instead of direct diff for git-diff-ase (#1653) (#1654)

* Use merge-base (three dot diff) instead of direct diff for git-diff-base (#1653)

* Add command line dumping for debugging

* Fetch full history to allow finding common ancestor, not just last commit

* Remove var_dumps

* Debug with git depth 1

* Fall back to direct diff if merge base commit is not avialable

* Restore deep git fetch

* Kill UnwrapTrim mutant

* Fix php parse error

* Add test_it_falls_back_to_direct_diff_if_merge_base_is_not_availabe

* Skip test on Windows

.github/workflows/mt-annotations.yaml

@@ -19,7 +19,8 @@ jobs:
         steps:
             - name: Checkout code
               uses: actions/checkout@v2
-
+              with:
+                  fetch-depth: 0
             - name: Setup PHP
               uses: shivammathur/setup-php@v2
               with:
@@ -47,5 +48,5 @@ jobs:
 
             - name: Run Infection for added files only
               run: |
-                  git fetch --depth=1 origin $GITHUB_BASE_REF
+                  git fetch origin $GITHUB_BASE_REF
                   php bin/infection -j2 --git-diff-lines --git-diff-base=origin/$GITHUB_BASE_REF --logger-github --ignore-msi-with-no-mutations --only-covered

src/Logger/GitHub/GitDiffFileProvider.php

@@ -38,6 +38,8 @@
 use function escapeshellarg;
 use Infection\Process\ShellCommandLineExecutor;
 use function Safe\sprintf;
+use Symfony\Component\Process\Exception\ProcessFailedException;
+use function trim;
 
 /**
  * @final
@@ -57,9 +59,11 @@ public function __construct(ShellCommandLineExecutor $shellCommandLineExecutor)
 
     public function provide(string $gitDiffFilter, string $gitDiffBase): string
     {
+        $referenceCommit = $this->findReferenceCommit($gitDiffBase);
+
         $filter = $this->shellCommandLineExecutor->execute(sprintf(
             'git diff %s --diff-filter=%s --name-only | grep src/ | paste -s -d "," -',
-            escapeshellarg($gitDiffBase),
+            escapeshellarg($referenceCommit),
             escapeshellarg($gitDiffFilter)
         ));
 
@@ -72,9 +76,30 @@ public function provide(string $gitDiffFilter, string $gitDiffBase): string
 
     public function provideWithLines(string $gitDiffBase): string
     {
+        $referenceCommit = $this->findReferenceCommit($gitDiffBase);
+
         return $this->shellCommandLineExecutor->execute(sprintf(
             "git diff %s --unified=0 --diff-filter=AM | grep -v -e '^[+-]' -e '^index'",
-            escapeshellarg($gitDiffBase)
+            escapeshellarg($referenceCommit)
         ));
     }
+
+    private function findReferenceCommit(string $gitDiffBase): string
+    {
+        try {
+            $comparisonCommit = trim($this->shellCommandLineExecutor->execute(sprintf(
+                    'git merge-base %s HEAD',
+                    escapeshellarg($gitDiffBase))
+                )
+            );
+        } catch (ProcessFailedException $_e) {
+            /**
+             * there is no common ancestor commit, or we are in a shallow checkout and do have a copy of it.
+             * Fall back to direct diff
+             */
+            $comparisonCommit = $gitDiffBase;
+        }
+
+        return $comparisonCommit;
+    }
 }

tests/phpunit/Logger/GitHub/GitDiffFileProviderTest.php

@@ -40,13 +40,14 @@
 use Infection\Process\ShellCommandLineExecutor;
 use const PHP_OS_FAMILY;
 use PHPUnit\Framework\TestCase;
+use Symfony\Component\Process\Exception\ProcessFailedException;
 
 final class GitDiffFileProviderTest extends TestCase
 {
     public function test_it_throws_no_code_to_mutate_exception_when_diff_is_empty(): void
     {
         $shellCommandLineExecutor = $this->createMock(ShellCommandLineExecutor::class);
-        $shellCommandLineExecutor->expects($this->once())
+        $shellCommandLineExecutor
             ->method('execute')
             ->willReturn('');
 
@@ -58,21 +59,92 @@ public function test_it_throws_no_code_to_mutate_exception_when_diff_is_empty():
 
     public function test_it_executes_diff_and_returns_filter_as_a_string(): void
     {
-        $expectedCommandLine = 'git diff \'master\' --diff-filter=\'AM\' --name-only | grep src/ | paste -s -d "," -';
+        $expectedMergeBaseCommandLine = 'git merge-base \'master\' HEAD';
+        $expectedDiffCommandLine = 'git diff \'0ABCMERGE_BASE_342\' --diff-filter=\'AM\' --name-only | grep src/ | paste -s -d "," -';
 
         if (PHP_OS_FAMILY === 'Windows') {
-            $expectedCommandLine = 'git diff "master" --diff-filter="AM" --name-only | grep src/ | paste -s -d "," -';
+            $expectedMergeBaseCommandLine = 'git merge-base "master" HEAD';
+            $expectedDiffCommandLine = 'git diff "0ABCMERGE_BASE_342" --diff-filter="AM" --name-only | grep src/ | paste -s -d "," -';
         }
 
         $shellCommandLineExecutor = $this->createMock(ShellCommandLineExecutor::class);
-        $shellCommandLineExecutor->expects($this->once())
+
+        $shellCommandLineExecutor->expects($this->any())
+            ->method('execute')
+            ->willReturnCallback(function (string $command) use ($expectedDiffCommandLine, $expectedMergeBaseCommandLine): string {
+                switch ($command) {
+                    case $expectedMergeBaseCommandLine:
+                        return "0ABCMERGE_BASE_342\n";
+                    case $expectedDiffCommandLine:
+                        return 'src/A.php,src/B.php';
+                    default:
+                        $this->fail("Unexpected shell command: $command");
+                }
+            });
+
+        $diffProvider = new GitDiffFileProvider($shellCommandLineExecutor);
+        $filter = $diffProvider->provide('AM', 'master');
+
+        $this->assertSame('src/A.php,src/B.php', $filter);
+    }
+
+    public function test_it_falls_back_to_direct_diff_if_merge_base_is_not_availabe(): void
+    {
+        if (PHP_OS_FAMILY === 'Windows') {
+            $this->markTestSkipped('Not testing this on Windows');
+        }
+
+        $expectedMergeBaseCommandLine = 'git merge-base \'master\' HEAD';
+        $shellCommandLineExecutor = $this->createMock(ShellCommandLineExecutor::class);
+        $expectedDiffCommandLine = 'git diff \'master\' --diff-filter=\'AM\' --name-only | grep src/ | paste -s -d "," -';
+
+        $shellCommandLineExecutor->expects($this->any())
             ->method('execute')
-            ->with($expectedCommandLine)
-            ->willReturn('src/A.php,src/B.php');
+            ->willReturnCallback(function (string $command) use ($expectedDiffCommandLine, $expectedMergeBaseCommandLine): string {
+                switch ($command) {
+                    case $expectedMergeBaseCommandLine:
+                        throw $this->createStub(ProcessFailedException::class);
+                    case $expectedDiffCommandLine:
+                        return 'src/A.php,src/B.php';
+                    default:
+                        $this->fail("Unexpected shell command: $command");
+                }
+            });
 
         $diffProvider = new GitDiffFileProvider($shellCommandLineExecutor);
         $filter = $diffProvider->provide('AM', 'master');
 
         $this->assertSame('src/A.php,src/B.php', $filter);
     }
+
+    public function test_it_provides_lines_filter_as_a_string(): void
+    {
+        $expectedMergeBaseCommandLine = 'git merge-base \'master\' HEAD';
+        $expectedDiffCommandLine = 'git diff \'0ABCMERGE_BASE_342\' --unified=0 --diff-filter=AM | grep -v -e \'^[+-]\' -e \'^index\'';
+
+        if (PHP_OS_FAMILY === 'Windows') {
+            $expectedMergeBaseCommandLine = 'git merge-base "master" HEAD';
+            $expectedDiffCommandLine = 'git diff "0ABCMERGE_BASE_342" --unified=0 --diff-filter=AM | grep -v -e \'^[+-]\' -e \'^index\'';
+        }
+
+        $shellCommandLineExecutor = $this->createMock(ShellCommandLineExecutor::class);
+
+        $shellCommandLineExecutor->expects($this->any())
+            ->method('execute')
+            ->willReturnCallback(function (string $command) use ($expectedDiffCommandLine, $expectedMergeBaseCommandLine): string {
+                switch ($command) {
+                    case $expectedMergeBaseCommandLine:
+                        return '0ABCMERGE_BASE_342';
+                    case $expectedDiffCommandLine:
+                        return '<LINE BY LINE GIT DIFF>';
+                    default:
+                        $this->fail("Unexpected shell command: $command");
+                }
+            });
+
+        $diffProvider = new GitDiffFileProvider($shellCommandLineExecutor);
+        $filter = $diffProvider->provideWithLines('master');
+
+        $this->assertSame('<LINE BY LINE GIT DIFF>', $filter);
+    }
 }