Fix two problems with local git diffs

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.

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'     => (

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
 		);
 

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 <a href="https://github.com/automattic/vip-go-ci/">vip-go-ci</a>\'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,
 			)
 		);
 	}

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

tests/unit/GitRepoDiffsCleanExtraWhitespaceTest.php

@@ -0,0 +1,61 @@
+<?php
+/**
+ * Test vipgoci_gitrepo_diffs_clean_extra_whitespace() function.
+ *
+ * @package Automattic/vip-go-ci
+ */
+
+declare(strict_types=1);
+
+namespace Vipgoci\Tests\Unit;
+
+use PHPUnit\Framework\TestCase;
+
+/**
+ * Class that implements the testing.
+ *
+ * @runTestsInSeparateProcesses
+ * @preserveGlobalState disabled
+ */
+final class GitRepoDiffsCleanExtraWhitespaceTest extends TestCase {
+	/**
+	 * Setup function. Require files, etc.
+	 *
+	 * @return void
+	 */
+	protected function setUp() :void {
+		require_once __DIR__ . '/../../git-repo.php';
+	}
+
+	/**
+	 * Test common usage of the function.
+	 *
+	 * @covers ::vipgoci_gitrepo_diffs_clean_extra_whitespace
+	 *
+	 * @return void
+	 */
+	public function testCleanExtraWhitespace1(): void {
+		$this->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" )
+			)
+		);
+	}
+}