From fefb6898f072e77803a50542df0581b95b91e869 Mon Sep 17 00:00:00 2001 From: Barney Laurance Date: Thu, 27 Jan 2022 04:48:37 +0000 Subject: [PATCH] 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 | 5 +- src/Logger/GitHub/GitDiffFileProvider.php | 29 ++++++- .../Logger/GitHub/GitDiffFileProviderTest.php | 84 +++++++++++++++++-- 3 files changed, 108 insertions(+), 10 deletions(-) diff --git a/.github/workflows/mt-annotations.yaml b/.github/workflows/mt-annotations.yaml index abcdb1528..6bff8f6e6 100644 --- a/.github/workflows/mt-annotations.yaml +++ b/.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 diff --git a/src/Logger/GitHub/GitDiffFileProvider.php b/src/Logger/GitHub/GitDiffFileProvider.php index 10493d2b1..452f2a8be 100644 --- a/src/Logger/GitHub/GitDiffFileProvider.php +++ b/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; + } } diff --git a/tests/phpunit/Logger/GitHub/GitDiffFileProviderTest.php b/tests/phpunit/Logger/GitHub/GitDiffFileProviderTest.php index 1ae7bd9a4..5e406f65e 100644 --- a/tests/phpunit/Logger/GitHub/GitDiffFileProviderTest.php +++ b/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 ''; + default: + $this->fail("Unexpected shell command: $command"); + } + }); + + $diffProvider = new GitDiffFileProvider($shellCommandLineExecutor); + $filter = $diffProvider->provideWithLines('master'); + + $this->assertSame('', $filter); + } }