diff --git a/git-repo.php b/git-repo.php index c225fc080..309c019a5 100644 --- a/git-repo.php +++ b/git-repo.php @@ -1109,7 +1109,7 @@ function vipgoci_gitrepo_diffs_fetch_unfiltered( * as that is what GitHub uses: https://docs.github.com/en/github/collaborating-with-issues-and-pull-requests/about-comparing-branches-in-pull-requests#three-dot-and-two-dot-git-diff-comparisons */ $git_diff_cmd = sprintf( - '%s -C %s diff %s', + '%s --no-pager -C %s diff --no-color %s', escapeshellcmd( 'git' ), escapeshellarg( $local_git_repo ), escapeshellarg( $commit_id_a . '...' . $commit_id_b ) @@ -1256,10 +1256,20 @@ function vipgoci_gitrepo_diffs_fetch_unfiltered( $cur_file_status = null; - $cur_file_minus = $git_result_item_arr[2]; + if ( 4 === count( $git_result_item_arr ) ) { + $cur_file_minus = $git_result_item_arr[2]; + } else { + $cur_file_minus = null; + } + $cur_file_minus_path_cleaned = false; - $cur_file_plus = $git_result_item_arr[3]; + if ( 4 === count( $git_result_item_arr ) ) { + $cur_file_plus = $git_result_item_arr[3]; + } else { + $cur_file_plus = null; + } + $cur_file_plus_path_cleaned = false; $cur_file_previous_filename = null; @@ -1280,11 +1290,21 @@ function vipgoci_gitrepo_diffs_fetch_unfiltered( ) { $cur_file_previous_filename = $git_result_item_arr[2]; } elseif ( '---' === $git_result_item_arr[0] ) { - $cur_file_minus = $git_result_item_arr[1]; + $cur_file_minus = vipgoci_gitrepo_get_file_path_from_diff( + vipgoci_gitrepo_diffs_clean_extra_whitespace( + $git_result_item_arr + ), + 1 + ); $cur_file_minus_path_cleaned = false; } elseif ( '+++' === $git_result_item_arr[0] ) { - $cur_file_plus = $git_result_item_arr[1]; + $cur_file_plus = vipgoci_gitrepo_get_file_path_from_diff( + vipgoci_gitrepo_diffs_clean_extra_whitespace( + $git_result_item_arr + ), + 1 + ); $cur_file_plus_path_cleaned = false; } elseif ( @@ -1362,11 +1382,18 @@ function vipgoci_gitrepo_diffs_fetch_unfiltered( } else { /* * No match and no mention of /dev/null, - * so the file must have been renamed. + * so the file must have been renamed + * (with exceptions, see below). */ + $cur_file = $cur_file_plus; - $cur_file = $cur_file_plus; - $cur_file_status = 'renamed'; + /* + * Avoid resetting status if determined + * that content has been modified elsewhere. + */ + if ( 'modified' !== $cur_file_status ) { + $cur_file_status = 'renamed'; + } } } } @@ -1391,7 +1418,10 @@ function vipgoci_gitrepo_diffs_fetch_unfiltered( ); } - if ( null !== $cur_file_status ) { + if ( + ( null !== $cur_file ) && + ( null !== $cur_file_status ) + ) { /* * Update file-status each time we loop * as the calculated status might change. @@ -1400,7 +1430,10 @@ function vipgoci_gitrepo_diffs_fetch_unfiltered( $cur_file_status; } - if ( null !== $cur_file_previous_filename ) { + if ( + ( null !== $cur_file ) && + ( null !== $cur_file_previous_filename ) + ) { $diff_results['files'][ $cur_file ]['previous_filename'] = $cur_file_previous_filename; } @@ -1452,6 +1485,20 @@ function vipgoci_gitrepo_diffs_fetch_unfiltered( $diff_results['statistics'][ VIPGOCI_GIT_DIFF_CALC_CHANGES[ $git_result_item[0] ] ]++; $diff_results['statistics']['changes']++; + + /* + * Avoid incorrect status for file when file has been + * renamed and modified. See similar logic in + * vipgoci_github_diffs_fetch_unfiltered(). + */ + if ( + ( $diff_results['files'][ $cur_file ]['changes'] > 0 ) && + ( 'renamed' === $cur_file_status ) + ) { + $cur_file_status = 'modified'; + + $diff_results['files'][ $cur_file ]['status'] = $cur_file_status; + } } /* @@ -1505,6 +1552,74 @@ function vipgoci_gitrepo_diffs_fetch_unfiltered( return $diff_results; } +/** + * Join path array into string and return. Remove first array + * items if specified. + * + * @param array $path_arr Path as array. + * @param int $index_to_start From what element in the array to start the join. + * + * @return string File path. + */ +function vipgoci_gitrepo_get_file_path_from_diff( + array $path_arr, + int $index_to_start = 0, +) :string { + $removed_items = false; + + $path_arr_length = count( $path_arr ); + + for ( + $i = 0; + $i < $index_to_start && $index_to_start < $path_arr_length; + $i++ + ) { + unset( $path_arr[ $i ] ); + + $removed_items = true; + } + + if ( true === $removed_items ) { + $path_arr = array_values( $path_arr ); + } + + $path = join( ' ', $path_arr ); + + return $path; +} + +/** + * Git and/or a pager may leave a "\t" at the end of a file path, + * even though we request no such pager, but only when spaces are in + * file names. The source of this issue is unknown despite + * experimentation and research. + * + * Here we remove the extra "\t". + * + * @param array $path_arr Path to file to clean as array. + * + * @return array Cleaned path. + */ +function vipgoci_gitrepo_diffs_clean_extra_whitespace( + array $path_arr +) :array { + $path_arr_length = count( $path_arr ); + + if ( $path_arr_length <= 1 ) { + return $path_arr; + } + + $last_string = $path_arr[ $path_arr_length - 1 ]; + + if ( true === str_ends_with( $last_string, "\t" ) ) { + $last_string = rtrim( $last_string, "\t" ); + + $path_arr[ $path_arr_length - 1 ] = $last_string; + } + + return $path_arr; +} + /** * Fetch diffs between two commits, * filter the results if requested. diff --git a/github-api.php b/github-api.php index c89916d54..7cfa81d1a 100644 --- a/github-api.php +++ b/github-api.php @@ -122,6 +122,20 @@ function vipgoci_github_diffs_fetch_unfiltered( ); foreach ( array_values( $resp_raw['files'] ) as $file_item ) { + /* + * If there are content modifications to the file, + * and GitHub indicates file status is 'renamed', + * override status as 'modified'. This is in line + * with expectations elsewhere in the code. See + * similar logic in vipgoci_gitrepo_diffs_fetch_unfiltered(). + */ + if ( + ( $file_item['changes'] > 0 ) && + ( 'renamed' === $file_item['status'] ) + ) { + $file_item['status'] = 'modified'; + } + $diff_results['files'][ $file_item['filename'] ] = array( 'filename' => $file_item['filename'], 'patch' => ( diff --git a/results.php b/results.php index 79a398e46..c3f315538 100644 --- a/results.php +++ b/results.php @@ -794,6 +794,8 @@ function vipgoci_results_sort_by_severity( * this is used to understand if the specific * comment has already been submitted earlier. * + * Handles special case as well. + * * @param string $file_issue_path Path to file. * @param int $file_issue_line Line number in file. * @param string $file_issue_comment Comment to look for. @@ -838,10 +840,14 @@ function vipgoci_results_comment_match( /* * The comment might contain formatting, such * as "Warning: ..." -- remove all of that. + * + * Handle special case when "/**" is included in comments. + * Ensure to preserve comments with this pattern, as otherwise + * strings that include them will be re-posted during re-runs. */ $comment_made_body = str_replace( - array( '**', 'Warning', 'Error', 'Info', ':no_entry_sign:', ':warning:', ':information_source:' ), - array( '', '', '', '', '' ), + array( '/**', '**', 'Warning', 'Error', 'Info', ':no_entry_sign:', ':warning:', ':information_source:', '/\*\*' ), + array( '/\*\*', '', '', '', '', '', '', '', '/**' ), $comment_made->body ); diff --git a/tests/integration/GitDiffsFetchUnfilteredTrait.php b/tests/integration/GitDiffsFetchUnfilteredTrait.php index c0212ace1..af5b2946c 100644 --- a/tests/integration/GitDiffsFetchUnfilteredTrait.php +++ b/tests/integration/GitDiffsFetchUnfilteredTrait.php @@ -145,6 +145,19 @@ private function _dataGitDiffsAssert5() { private function _dataGitDiffsAssert6() { return array( 'files' => array( + /* File renamed, content added */ + 'README file.md' => array( + 'filename' => 'README file.md', + 'patch' => "@@ -1,2 +1,5 @@\n" . + ' # vip-go-ci-testing' . PHP_EOL . + ' Pull-Requests, commits and data to test vip-go-ci\'s functionality. Please do not remove or alter unless you\'ve contacted the VIP Team first. ' . PHP_EOL . '+' . PHP_EOL . '+' . PHP_EOL . '+', + 'status' => 'modified', + 'additions' => 3, + 'deletions' => 0, + 'changes' => 3, + 'previous_filename' => 'README.md', + ), + 'a/new-file.txt' => array( /* File added, starting with name 'a/' */ 'filename' => 'a/new-file.txt', @@ -155,6 +168,16 @@ private function _dataGitDiffsAssert6() { 'changes' => 2, ), + 'b/new file 2.txt' => array( + /* File added, starting with name 'b/', with spaces in file name */ + 'filename' => 'b/new file 2.txt', + 'patch' => '@@ -0,0 +1 @@' . PHP_EOL . '+This is a new file.', + 'status' => 'added', + 'additions' => 1, + 'deletions' => 0, + 'changes' => 1, + ), + 'b/new-file.txt' => array( /* File added, starting with name 'b/' */ 'filename' => 'b/new-file.txt', @@ -165,6 +188,15 @@ private function _dataGitDiffsAssert6() { 'changes' => 3, ), + 'new file 2.txt' => array( + 'filename' => 'new file 2.txt', + 'patch' => '@@ -0,0 +1 @@' . PHP_EOL . '+This is a new test file.', + 'status' => 'added', + 'additions' => 1, + 'deletions' => 0, + 'changes' => 1, + ), + 'new-file-permissions-changed.txt' => array( /* New file, with permissions changed */ 'filename' => 'new-file-permissions-changed.txt', @@ -238,9 +270,9 @@ private function _dataGitDiffsAssert6() { ), 'statistics' => array( - 'additions' => 15, - 'deletions' => 6, - 'changes' => 21, + 'additions' => 20, + 'deletions' => 6, + 'changes' => 26, ) ); } diff --git a/tests/integration/GitHubDiffsFetchUnfilteredTest.php b/tests/integration/GitHubDiffsFetchUnfilteredTest.php index ac2072f57..a32f7d5f2 100644 --- a/tests/integration/GitHubDiffsFetchUnfilteredTest.php +++ b/tests/integration/GitHubDiffsFetchUnfilteredTest.php @@ -284,10 +284,12 @@ public function testGitHubDiffsFetch6() { $this->_dataGitDiffsAssert6(), $diff ); - + /* * As an additional check, verify that caching is OK. */ + vipgoci_unittests_output_suppress(); + $diff_same = vipgoci_github_diffs_fetch_unfiltered( $this->options['repo-owner'], $this->options['repo-name'], @@ -296,6 +298,8 @@ public function testGitHubDiffsFetch6() { $this->options['commit-test-repo-pr-diffs-2-b'] ); + vipgoci_unittests_output_unsuppress(); + $this->assertSame( $diff, $diff_same diff --git a/tests/unit/GitRepoDiffsCleanExtraWhitespaceTest.php b/tests/unit/GitRepoDiffsCleanExtraWhitespaceTest.php new file mode 100644 index 000000000..019d27697 --- /dev/null +++ b/tests/unit/GitRepoDiffsCleanExtraWhitespaceTest.php @@ -0,0 +1,61 @@ +assertSame( + array( "a\t" ), + vipgoci_gitrepo_diffs_clean_extra_whitespace( + array( "a\t" ) + ) + ); + } + + /** + * Test common usage of the function. + * + * @covers ::vipgoci_gitrepo_diffs_clean_extra_whitespace + * + * @return void + */ + public function testCleanExtraWhitespace2(): void { + $this->assertSame( + array( "a\t", "b\t", 'c.php' ), + vipgoci_gitrepo_diffs_clean_extra_whitespace( + array( "a\t", "b\t", "c.php\t" ) + ) + ); + } +} diff --git a/tests/unit/GitRepoGetFilePathFromDiffTest.php b/tests/unit/GitRepoGetFilePathFromDiffTest.php new file mode 100644 index 000000000..df2d47d8d --- /dev/null +++ b/tests/unit/GitRepoGetFilePathFromDiffTest.php @@ -0,0 +1,46 @@ +assertSame( + 'b/file 30 test b.php', + vipgoci_gitrepo_get_file_path_from_diff( + array( '+++', 'b/file', '30', '', 'test', '', '', 'b.php' ), + 1 + ) + ); + } +} diff --git a/tests/unit/ResultsCommentMatchTest.php b/tests/unit/ResultsCommentMatchTest.php index 88b79c6ea..06b3899b8 100644 --- a/tests/unit/ResultsCommentMatchTest.php +++ b/tests/unit/ResultsCommentMatchTest.php @@ -1,63 +1,86 @@ array( - json_decode( - '{"url":"https:\/\/api.github.com\/repos\/gudmdharalds-a8c\/testing123\/pulls\/comments\/249878202","pull_request_review_id":195129115,"id":249878202,"node_id":"MDI0Ol123","diff_hunk":"@@ -0,0 +1,3 @@\n+ array( + (object) array( + 'body' => ':no_entry_sign: **Error**: All output should be ..., found \'mysql_query\'.', + ), + (object) array( + 'body' => ':no_entry_sign: **Error**: Extension \'mysql_\' is deprecated since PHP 5.5 and removed since PHP 7.0; Use mysqli instead', ), - json_decode( - '{"url":"https:\/\/api.github.com\/repos\/gudmdharalds-a8c\/testing123\/pulls\/comments\/255465011","pull_request_review_id":202053661,"id":255465011,"node_id":"MDI0O01245","diff_hunk":"@@ -0,0 +1,3 @@\n+ ':no_entry_sign: **Error**: Any HTML passed to `innerHTML` gets executed. Consider using `.textContent` or make sure that used variables are properly escaped (*WordPressVIPMinimum.JS.InnerHTML.innerHTML*).', ), - json_decode( - '{"url":"https:\/\/api.github.com\/repos\/gudmdharalds-a8c\/testing123\/pulls\/comments\/255465011","pull_request_review_id":202053661,"id":255465011,"node_id":"MDI0O01245","diff_hunk":"@@ -0,0 +1,3 @@\n+ array( - json_decode( - '{"url":"https:\/\/api.github.com\/repos\/gudmdharalds-a8c\/testing123\/pulls\/comments\/249878202","pull_request_review_id":195129115,"id":249878202,"node_id":"MDI0Ol123","diff_hunk":"@@ -0,0 +1,3 @@\n+ array( + (object) array( + 'body' => ':no_entry_sign: **Error**: Extension \'mysql_\' is deprecated since PHP 5.5 and removed since PHP 7.0; Use mysqli instead', ), ), - 'bla-9.php:90' => array( - json_decode( - '{"url":"https:\/\/api.github.com\/repos\/gudmdharalds-a8c\/testing123\/pulls\/comments\/255465011","pull_request_review_id":202053661,"id":255465011,"node_id":"MDI0O01245","diff_hunk":"@@ -0,0 +1,3 @@\n+ array( + (object) array( + 'body' => ':no_entry_sign: **Error( severity 11 )**: Extension \'mysql_\' is deprecated since PHP 5.5 and removed since PHP 7.0; Use mysqli instead', ), ), - 'bla-10.php:3' => array( - json_decode( - '{"url":"https:\/\/api.github.com\/repos\/gudmdharalds-a8c\/testing123\/pulls\/comments\/255465011","pull_request_review_id":202053661,"id":255465011,"node_id":"MDI0O01245","diff_hunk":"@@ -0,0 +1,3 @@\n+ array( + (object) array( + 'body' => ':no_entry_sign: **Error( severity 11 )**: /** cannot be used', ), ), - 'bla-11.php:5' => array( - json_decode( - '{"url":"https:\/\/api.github.com\/repos\/gudmdharalds-a8c\/testing123\/pulls\/comments\/255465011","pull_request_review_id":202053661,"id":255465011,"node_id":"MDI0O01245","diff_hunk":"@@ -0,0 +1,3 @@\n+ array( + (object) array( + 'body' => ':no_entry_sign: **Error**: All output should be run ..., found \'mysql_query\'.', ), ), - ); + 'file-9.php:90' => array( + (object) array( + 'body' => ':no_entry_sign: **Error**: Extension \'mysql_\' is deprecated since PHP 5.5 and removed since PHP 7.0; Use mysqli instead', + ), + ), + ); $this->assertTrue( vipgoci_results_comment_match( - 'bla-8.php', + 'file-8.php', 3, 'Extension \'mysql_\' is deprecated since PHP 5.5 and removed since PHP 7.0; Use mysqli instead', $prs_comments @@ -66,7 +89,7 @@ public function testCommentMatch1() { $this->assertTrue( vipgoci_results_comment_match( - 'bla-8.php', + 'file-8.php', 3, 'Any HTML passed to `innerHTML` gets executed. Consider using `.textContent` or make sure that used variables are properly escaped', $prs_comments @@ -75,63 +98,77 @@ public function testCommentMatch1() { $this->assertFalse( vipgoci_results_comment_match( - 'bla-8.php', + 'file-8.php', 3, 'The extension \'mysql_\' is deprecated since PHP 5.5 and removed since PHP 7.0; Use mysqli instead', - $prs_comments + $prs_comments ) ); $this->assertFalse( vipgoci_results_comment_match( - 'bla-8.php', + 'file-8.php', 4, 'Extension \'mysql_\' is deprecated since PHP 5.5 and removed since PHP 7.0; Use mysqli instead', - $prs_comments + $prs_comments ) ); $this->assertFalse( vipgoci_results_comment_match( - 'bla-8.php', + 'file-8.php', 4, 'Any HTML passed to `innerHTML` gets executed. Consider using `.textContent` or make sure that used variables are properly escaped', $prs_comments ) ); - $this->assertFalse( vipgoci_results_comment_match( - 'bla-9.php', + 'file-9.php', 3, 'Extension \'mysql_\' is deprecated since PHP 5.5 and removed since PHP 7.0; Use mysqli instead', - $prs_comments + $prs_comments ) ); - /* * Test with severity level */ $this->assertTrue( vipgoci_results_comment_match( - 'bla-11.php', + 'file-11.php', 5, 'Extension \'mysql_\' is deprecated since PHP 5.5 and removed since PHP 7.0; Use mysqli instead', - $prs_comments + $prs_comments ) ); $this->assertFalse( vipgoci_results_comment_match( - 'bla-11.php', + 'file-11.php', 5, 'Extension \'mysql_\' is deprecated since PHP 300 and removed since PHP 700; Use mysqli instead', - $prs_comments + $prs_comments ) ); + $this->assertTrue( + vipgoci_results_comment_match( + 'file-12.php', + 70, + '/** cannot be used', + $prs_comments + ) + ); + $this->assertFalse( + vipgoci_results_comment_match( + 'file-12.php', + 70, + '/*** cannot be used', + $prs_comments + ) + ); } } diff --git a/unittests.ini.dist b/unittests.ini.dist index bcb34915f..f7b31fabc 100644 --- a/unittests.ini.dist +++ b/unittests.ini.dist @@ -86,7 +86,7 @@ commit-test-repo-pr-diffs-1-e=26227c626b265a5256bedfb7a0edb068dcfc8014 commit-test-repo-pr-diffs-1-f=12fed31bf1758113cecb01e309c03286bbe2bcf0 commit-test-repo-pr-diffs-1-g=6148a7b4193dfa42bcc9d3cce0d4a09221fb8a6b commit-test-repo-pr-diffs-2-a=1efcdedefa1dbdbb067e7aca56f1458da0026a65 -commit-test-repo-pr-diffs-2-b=3b27c4267c23846c20030d91531bfe2ec71e95d2 +commit-test-repo-pr-diffs-2-b=f3545e389e8ef569ca0eb28061b293d4d4b8ef75 commit-test-repo-pr-diffs-3-a=ac10d1f29e64504d7741cd8ca22981c426c26e9a ; The following commit is from outside of the repository,