Merge pull request #146 from aik099/use-symfony-process-escaping

Use "symfony/process" for command argument escaping

src/SVNBuddy/Application.php

@@ -57,7 +57,7 @@ public function getLongVersion()
 
 		/** @var Connector $repository_connector */
 		$repository_connector = $this->dic['repository_connector'];
-		$client_version = $repository_connector->getCommand('', '--version --quiet')->run();
+		$client_version = $repository_connector->getCommand('', array('--version', '--quiet'))->run();
 
 		return $version . ' (SVN <comment>v' . trim($client_version) . '</comment>)';
 	}

src/SVNBuddy/Command/CleanupCommand.php

@@ -46,7 +46,7 @@ protected function execute(InputInterface $input, OutputInterface $output)
 		$wc_path = $this->getWorkingCopyPath();
 
 		$this->io->writeln('Cleaning up working copy ... ');
-		$command = $this->repositoryConnector->getCommand('cleanup', '{' . $wc_path . '}');
+		$command = $this->repositoryConnector->getCommand('cleanup', array($wc_path));
 		$command->runLive(array(
 			$wc_path => '.',
 		));

src/SVNBuddy/Command/CommitCommand.php

@@ -175,23 +175,22 @@ protected function execute(InputInterface $input, OutputInterface $output)
 		$tmp_file = tempnam(sys_get_temp_dir(), 'commit_message_');
 		file_put_contents($tmp_file, $edited_commit_message);
 
-		$arguments = array(
-			'-F {' . $tmp_file . '}',
-		);
+		$arguments = array('-F', $tmp_file);
 
 		if ( strlen($changelist) ) {
-			$arguments[] = '--depth empty';
+			$arguments[] = '--depth';
+			$arguments[] = 'empty';
 
 			// Relative path used to make command line shorter.
 			foreach ( array_keys($this->repositoryConnector->getWorkingCopyStatus($wc_path, $changelist)) as $path ) {
-				$arguments[] = '{' . $path . '}';
+				$arguments[] = $path;
 			}
 		}
 		else {
-			$arguments[] = '{' . $wc_path . '}';
+			$arguments[] = $wc_path;
 		}
 
-		$this->repositoryConnector->getCommand('commit', implode(' ', $arguments))->runLive();
+		$this->repositoryConnector->getCommand('commit', $arguments)->runLive();
 		$this->_workingCopyConflictTracker->erase($wc_path);
 		unlink($tmp_file);
 

src/SVNBuddy/Command/MergeCommand.php

@@ -554,11 +554,11 @@ protected function calculateUsableRevisions($source_url, $wc_path)
 	{
 		$command = $this->repositoryConnector->getCommand(
 			'mergeinfo',
-			sprintf(
-				'--show-revs %s {%s} {%s}',
+			array(
+				'--show-revs',
 				$this->isReverseMerge() ? 'merged' : 'eligible',
 				$source_url,
-				$wc_path
+				$wc_path,
 			)
 		);
 
@@ -657,17 +657,6 @@ protected function performMerge($source_url, $wc_path, array $revisions)
 			$revision_count += $used_revision_count;
 		}
 
-		$param_string_beginning = '-c ';
-		$param_string_ending = '{' . $source_url . '} {' . $wc_path . '}';
-
-		if ( $this->isReverseMerge() ) {
-			$param_string_beginning .= '-';
-		}
-
-		if ( $this->io->getOption('record-only') ) {
-			$param_string_ending = '--record-only ' . $param_string_ending;
-		}
-
 		$revision_log = $this->getRevisionLog($source_url);
 		$revisions_data = $revision_log->getRevisionsData('summary', $revisions);
 
@@ -678,11 +667,11 @@ protected function performMerge($source_url, $wc_path, array $revisions)
 			$revision_title_mask .= ' revision';
 		}
 
+		$merge_command_arguments = $this->getMergeCommandArguments($source_url, $wc_path);
+
 		foreach ( $revisions as $index => $revision ) {
-			$command = $this->repositoryConnector->getCommand(
-				'merge',
-				$param_string_beginning . $revision . ' ' . $param_string_ending
-			);
+			$command_arguments = str_replace('{revision}', $revision, $merge_command_arguments);
+			$command = $this->repositoryConnector->getCommand('merge', $command_arguments);
 
 			$progress_bar = $this->createMergeProgressBar($used_revision_count + $index + 1, $revision_count);
 
@@ -710,6 +699,31 @@ protected function performMerge($source_url, $wc_path, array $revisions)
 		$this->performCommit();
 	}
 
+	/**
+	 * Returns merge command arguments.
+	 *
+	 * @param string $source_url Merge source: url.
+	 * @param string $wc_path    Merge target: working copy path.
+	 *
+	 * @return array
+	 */
+	protected function getMergeCommandArguments($source_url, $wc_path)
+	{
+		$ret = array(
+			'-c',
+			$this->isReverseMerge() ? '-{revision}' : '{revision}',
+		);
+
+		if ( $this->io->getOption('record-only') ) {
+			$ret[] = '--record-only';
+		}
+
+		$ret[] = $source_url;
+		$ret[] = $wc_path;
+
+		return $ret;
+	}
+
 	/**
 	 * Create merge progress bar.
 	 *

src/SVNBuddy/Command/RevertCommand.php

@@ -73,7 +73,7 @@ protected function execute(InputInterface $input, OutputInterface $output)
 		$this->io->writeln('Reverting local changes in working copy ... ');
 		$command = $this->repositoryConnector->getCommand(
 			'revert',
-			'--depth infinity {' . $wc_path . '}'
+			array('--depth', 'infinity', $wc_path)
 		);
 		$command->runLive(array(
 			$wc_path => '.',

src/SVNBuddy/Command/UpdateCommand.php

@@ -87,17 +87,18 @@ protected function execute(InputInterface $input, OutputInterface $output)
 			'Updating working copy to <info>' . $show_revision . '</info> revision ' . $show_externals . ' ... '
 		);
 
-		$param_string = '{' . $wc_path . '}';
+		$arguments = array($wc_path);
 
 		if ( $revision ) {
-			$param_string .= ' --revision ' . $revision;
+			$arguments[] = '--revision';
+			$arguments[] = $revision;
 		}
 
 		if ( $ignore_externals ) {
-			$param_string .= ' --ignore-externals';
+			$arguments[] = '--ignore-externals';
 		}
 
-		$command = $this->repositoryConnector->getCommand('update', $param_string);
+		$command = $this->repositoryConnector->getCommand('update', $arguments);
 		$command->runLive(array(
 			$wc_path => '.',
 		));

src/SVNBuddy/Process/IProcessFactory.php

@@ -19,15 +19,12 @@ interface IProcessFactory
 	/**
 	 * Creates new Symfony process with given arguments.
 	 *
-	 * @param string       $commandline  The command line to run.
+	 * @param array        $command_line The command line to run.
 	 * @param integer|null $idle_timeout Idle timeout.
 	 *
 	 * @return Process
 	 */
-	public function createProcess(
-		$commandline,
-		$idle_timeout = null
-	);
+	public function createProcess(array $command_line, $idle_timeout = null);
 
 	/**
 	 * Creates new Symfony PHP process with given arguments.

src/SVNBuddy/Process/ProcessFactory.php

@@ -13,24 +13,21 @@
 
 use Symfony\Component\Process\PhpExecutableFinder;
 use Symfony\Component\Process\Process;
-use Symfony\Component\Process\ProcessBuilder;
 
 class ProcessFactory implements IProcessFactory
 {
 
 	/**
 	 * Creates new Symfony process with given arguments.
 	 *
-	 * @param string       $commandline  The command line to run.
+	 * @param array        $command_line The command line to run.
 	 * @param integer|null $idle_timeout Idle timeout.
 	 *
 	 * @return Process
 	 */
-	public function createProcess(
-		$commandline,
-		$idle_timeout = null
-	) {
-		$process = new Process($commandline);
+	public function createProcess(array $command_line, $idle_timeout = null)
+	{
+		$process = new Process($command_line);
 		$process->setTimeout(null);
 		$process->setIdleTimeout($idle_timeout);
 
@@ -51,13 +48,15 @@ public function createCommandProcess($command, array $arguments = array())
 		$php_executable_finder = new PhpExecutableFinder();
 		$php_executable = $php_executable_finder->find();
 
+		// @codeCoverageIgnoreStart
 		if ( !$php_executable ) {
 			throw new \RuntimeException('The PHP executable cannot be found.');
 		}
+		// @codeCoverageIgnoreEnd
 
 		array_unshift($arguments, $php_executable, $_SERVER['argv'][0], $command);
 
-		return ProcessBuilder::create($arguments)->getProcess();
+		return new Process($arguments);
 	}
 
 }

src/SVNBuddy/Repository/Connector/Command.php

@@ -32,7 +32,7 @@ class Command
 	/**
 	 * Command line.
 	 *
-	 * @var string
+	 * @var array
 	 */
 	private $_commandLine;
 
@@ -74,13 +74,13 @@ class Command
 	/**
 	 * Creates a command instance.
 	 *
-	 * @param string          $command_line    Command line.
+	 * @param array           $command_line    Command line.
 	 * @param ConsoleIO       $io              Console IO.
 	 * @param CacheManager    $cache_manager   Cache manager.
 	 * @param IProcessFactory $process_factory Process factory.
 	 */
 	public function __construct(
-		$command_line,
+		array $command_line,
 		ConsoleIO $io,
 		CacheManager $cache_manager,
 		IProcessFactory $process_factory
@@ -166,7 +166,7 @@ public function run($callback = null)
 			}
 		}
 
-		if ( strpos($this->_commandLine, '--xml') !== false ) {
+		if ( in_array('--xml', $this->_commandLine) ) {
 			return simplexml_load_string($output);
 		}
 
@@ -181,11 +181,13 @@ public function run($callback = null)
 	private function _getCacheKey()
 	{
 		if ( $this->_cacheInvalidator || $this->_cacheDuration ) {
-			if ( preg_match(Connector::URL_REGEXP, $this->_commandLine, $regs) ) {
-				return $regs[2] . $regs[3] . $regs[4] . '/command:' . $this->_commandLine;
+			$command_string = (string)$this;
+
+			if ( preg_match(Connector::URL_REGEXP, $command_string, $regs) ) {
+				return $regs[2] . $regs[3] . $regs[4] . '/command:' . $command_string;
 			}
 
-			return 'misc/command:' . $this->_commandLine;
+			return 'misc/command:' . $command_string;
 		}
 
 		return '';
@@ -202,6 +204,7 @@ private function _getCacheKey()
 	private function _doRun($callback = null)
 	{
 		$process = $this->_processFactory->createProcess($this->_commandLine, 1200);
+		$command_string = (string)$this;
 
 		try {
 			$start = microtime(true);
@@ -210,7 +213,7 @@ private function _doRun($callback = null)
 			if ( $this->_io->isVerbose() ) {
 				$runtime = sprintf('%01.2f', microtime(true) - $start);
 				$this->_io->writeln(
-					array('', '<debug>[svn, ' . round($runtime, 2) . 's]: ' . $this->_commandLine . '</debug>')
+					array('', '<debug>[svn, ' . round($runtime, 2) . 's]: ' . $command_string . '</debug>')
 				);
 			}
 
@@ -224,7 +227,7 @@ private function _doRun($callback = null)
 		}
 		catch ( ProcessFailedException $e ) {
 			throw new RepositoryCommandException(
-				$this->_commandLine,
+				$command_string,
 				$process->getErrorOutput()
 			);
 		}
@@ -276,4 +279,14 @@ private function _createLiveOutputCallback(array $replacements = array())
 		};
 	}
 
+	/**
+	 * Returns a string representation of a command.
+	 *
+	 * @return string
+	 */
+	public function __toString()
+	{
+		return implode(' ', $this->_commandLine);
+	}
+
 }