From 22d8e7467b24f47168f0b563c0c58bff6bc4d6ee Mon Sep 17 00:00:00 2001 From: vrana Date: Thu, 15 Nov 2012 12:33:36 -0800 Subject: [PATCH] Allow running `arc diff` without `git commit` Summary: There's quite some logic in here: - It automatically decides whether to create a new commit or amend. - It partially respects 'default-relative-commit'. - However if it points to a closed revision then it creates a new commit. Resolves T2025. Test Plan: `arc diff` on: - Clean committed repository. - Dirty repository without commit since 'default-relative-commit'. - Dirty repository with non-revision commit since 'default-relative-commit'. - Dirty repository with revision commit since 'default-relative-commit'. - `arc diff HEAD^` on dirty repository on top of closed revision. Reviewers: epriestley, btrahan Reviewed By: epriestley CC: aran, Korvin Maniphest Tasks: T2025 Differential Revision: https://secure.phabricator.com/D3967 --- src/repository/api/ArcanistGitAPI.php | 14 ++ src/repository/api/ArcanistMercurialAPI.php | 14 ++ src/repository/api/ArcanistRepositoryAPI.php | 8 + src/repository/api/ArcanistSubversionAPI.php | 6 + src/workflow/ArcanistBaseWorkflow.php | 172 +++++++++++++++---- src/workflow/ArcanistDiffWorkflow.php | 14 +- 6 files changed, 198 insertions(+), 30 deletions(-) diff --git a/src/repository/api/ArcanistGitAPI.php b/src/repository/api/ArcanistGitAPI.php index 1158f896..9e51c0a0 100644 --- a/src/repository/api/ArcanistGitAPI.php +++ b/src/repository/api/ArcanistGitAPI.php @@ -472,6 +472,20 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI { return $this->status; } + public function addToCommit(array $paths) { + $this->execxLocal( + 'add -- %Ls', + $paths); + } + + public function doCommit($message) { + $tmp_file = new TempFile(); + Filesystem::writeFile($tmp_file, $message); + $this->execxLocal( + 'commit --allow-empty-message -F %s', + $tmp_file); + } + public function amendCommit($message) { $tmp_file = new TempFile(); Filesystem::writeFile($tmp_file, $message); diff --git a/src/repository/api/ArcanistMercurialAPI.php b/src/repository/api/ArcanistMercurialAPI.php index 8ddd0cd7..2f9aa167 100644 --- a/src/repository/api/ArcanistMercurialAPI.php +++ b/src/repository/api/ArcanistMercurialAPI.php @@ -594,6 +594,20 @@ final class ArcanistMercurialAPI extends ArcanistRepositoryAPI { $this->execxLocal('up'); } + public function addToCommit(array $paths) { + $this->execxLocal( + 'add -- %Ls', + $paths); + } + + public function doCommit($message) { + $tmp_file = new TempFile(); + Filesystem::writeFile($tmp_file, $message); + $this->execxLocal( + 'commit -l %s', + $tmp_file); + } + public function amendCommit($message) { $tmp_file = new TempFile(); Filesystem::writeFile($tmp_file, $message); diff --git a/src/repository/api/ArcanistRepositoryAPI.php b/src/repository/api/ArcanistRepositoryAPI.php index df4e899c..cc5b27f5 100644 --- a/src/repository/api/ArcanistRepositoryAPI.php +++ b/src/repository/api/ArcanistRepositoryAPI.php @@ -192,6 +192,14 @@ abstract class ArcanistRepositoryAPI { throw new ArcanistCapabilityNotSupportedException($this); } + public function addToCommit(array $paths) { + throw new ArcanistCapabilityNotSupportedException($this); + } + + public function doCommit($message) { + throw new ArcanistCapabilityNotSupportedException($this); + } + public function amendCommit($message) { throw new ArcanistCapabilityNotSupportedException($this); } diff --git a/src/repository/api/ArcanistSubversionAPI.php b/src/repository/api/ArcanistSubversionAPI.php index b813fe41..16460c5a 100644 --- a/src/repository/api/ArcanistSubversionAPI.php +++ b/src/repository/api/ArcanistSubversionAPI.php @@ -171,6 +171,12 @@ final class ArcanistSubversionAPI extends ArcanistRepositoryAPI { } } + public function addToCommit(array $paths) { + $this->execxLocal( + 'add -- %Ls', + $paths); + } + public function getSVNProperty($path, $property) { list($stdout) = execx( 'svn propget %s %s@', diff --git a/src/workflow/ArcanistBaseWorkflow.php b/src/workflow/ArcanistBaseWorkflow.php index 547f1b1f..4f091d8c 100644 --- a/src/workflow/ArcanistBaseWorkflow.php +++ b/src/workflow/ArcanistBaseWorkflow.php @@ -36,6 +36,13 @@ */ abstract class ArcanistBaseWorkflow { + const COMMIT_DISABLE = 0; + const COMMIT_ALLOW = 1; + const COMMIT_ENABLE = 2; + + private $commitMode = self::COMMIT_DISABLE; + private $shouldAmend; + private $conduit; private $conduitURI; private $conduitCredentials; @@ -51,7 +58,7 @@ abstract class ArcanistBaseWorkflow { private $passedArguments; private $command; - private $repositoryEncoding; + private $projectInfo; private $arcanistConfiguration; private $parentWorkflow; @@ -709,9 +716,16 @@ abstract class ArcanistBaseWorkflow { return empty($this->arguments['allow-untracked']); } + public function setCommitMode($mode) { + $this->commitMode = $mode; + return $this; + } + public function requireCleanWorkingCopy() { $api = $this->getRepositoryAPI(); + $must_commit = array(); + $working_copy_desc = phutil_console_format( " Working copy: __%s__\n\n", $api->getPath()); @@ -740,10 +754,16 @@ abstract class ArcanistBaseWorkflow { "may have forgotten to 'hg add' them to your commit."); } - $prompt = "Do you want to continue without adding these files?"; - if (!phutil_console_confirm($prompt, $default_no = false)) { - throw new ArcanistUserAbortException(); + if ($this->askForAdd()) { + $api->addToCommit($untracked); + $must_commit += array_flip($untracked); + } else if ($this->commitMode == self::COMMIT_DISABLE) { + $prompt = "Do you want to continue without adding these files?"; + if (!phutil_console_confirm($prompt, $default_no = false)) { + throw new ArcanistUserAbortException(); + } } + } } @@ -770,25 +790,120 @@ abstract class ArcanistBaseWorkflow { $unstaged = $api->getUnstagedChanges(); if ($unstaged) { - throw new ArcanistUsageException( - "You have unstaged changes in this working copy. Stage and commit (or ". - "revert) them before proceeding.\n\n". + echo "You have unstaged changes in this working copy.\n\n". $working_copy_desc. " Unstaged changes in working copy:\n". - " ".implode("\n ", $unstaged)."\n"); + " ".implode("\n ", $unstaged)."\n\n"; + if ($this->askForAdd()) { + $api->addToCommit($unstaged); + $must_commit += array_flip($unstaged); + } else { + throw new ArcanistUsageException( + "Stage and commit (or revert) them before proceeding."); + } } $uncommitted = $api->getUncommittedChanges(); + foreach ($uncommitted as $key => $path) { + if (array_key_exists($path, $must_commit)) { + unset($uncommitted[$key]); + } + } if ($uncommitted) { - throw new ArcanistUncommittedChangesException( - "You have uncommitted changes in this working copy. Commit (or ". - "revert) them before proceeding.\n\n". + echo "You have uncommitted changes in this working copy.\n\n". $working_copy_desc. - " Uncommitted changes in working copy\n". - " ".implode("\n ", $uncommitted)."\n"); + " Uncommitted changes in working copy:\n". + " ".implode("\n ", $uncommitted)."\n\n"; + if ($this->askForAdd()) { + $must_commit += array_flip($uncommitted); + } else { + throw new ArcanistUncommittedChangesException( + "Commit (or revert) them before proceeding."); + } + } + + if ($must_commit) { + if ($this->shouldAmend) { + $commit = head($api->getLocalCommitInformation()); + $api->amendCommit($commit['message']); + } else if ($api->supportsRelativeLocalCommits()) { + $api->doCommit(''); + } } } + private function shouldAmend() { + $api = $this->getRepositoryAPI(); + + if ($this->isHistoryImmutable() || !$api->supportsAmend()) { + return false; + } + + $commits = $api->getLocalCommitInformation(); + if (!$commits) { + return false; + } + $commit = reset($commits); + + $message = ArcanistDifferentialCommitMessage::newFromRawCorpus( + $commit['message']); + if ($message->getGitSVNBaseRevision()) { + return false; + } + + // TODO: Check last commit's author. If not me then return false. + // TODO: Check commits since tracking branch. If empty then return false. + + $repository_phid = idx($this->getProjectInfo(), 'repositoryPHID'); + if ($repository_phid) { + $repositories = $this->getConduit()->callMethodSynchronous( + 'repository.query', + array()); + $callsigns = ipull($repositories, 'callsign', 'phid'); + $callsign = idx($callsigns, $repository_phid); + if ($callsign) { + $known_commits = $this->getConduit()->callMethodSynchronous( + 'diffusion.getcommits', + array('commits' => array('r'.$callsign.$commit['commit']))); + if ($known_commits) { + return false; + } + } + } + + if (!$message->getRevisionID()) { + return true; + } + + $in_working_copy = $api->loadWorkingCopyDifferentialRevisions( + $this->getConduit(), + array( + 'authors' => array($this->getUserPHID()), + 'status' => 'status-open', + )); + if ($in_working_copy) { + return true; + } + + return false; + } + + private function askForAdd() { + if ($this->commitMode == self::COMMIT_DISABLE) { + return false; + } else if ($this->commitMode == self::COMMIT_ENABLE) { + return true; + } + if ($this->shouldAmend === null) { + $this->shouldAmend = $this->shouldAmend(); + } + if ($this->shouldAmend) { + $prompt = "Do you want to amend these files to the commit?"; + } else { + $prompt = "Do you want to add these files to the commit?"; + } + return phutil_console_confirm($prompt); + } protected function loadDiffBundleFromConduit( ConduitClient $conduit, @@ -1276,26 +1391,25 @@ abstract class ArcanistBaseWorkflow { } protected function getRepositoryEncoding() { - if ($this->repositoryEncoding) { - return $this->repositoryEncoding; - } - $default = 'UTF-8'; + return nonempty(idx($this->getProjectInfo(), 'encoding'), $default); + } - $project_id = $this->getWorkingCopy()->getProjectID(); - if (!$project_id) { - return $default; + protected function getProjectInfo() { + if ($this->projectInfo === null) { + $project_id = $this->getWorkingCopy()->getProjectID(); + if (!$project_id) { + $this->projectInfo = array(); + } else { + $this->projectInfo = $this->getConduit()->callMethodSynchronous( + 'arcanist.projectinfo', + array( + 'name' => $project_id, + )); + } } - $project_info = $this->getConduit()->callMethodSynchronous( - 'arcanist.projectinfo', - array( - 'name' => $project_id, - )); - - $this->repositoryEncoding = nonempty($project_info['encoding'], $default); - - return $this->repositoryEncoding; + return $this->projectInfo; } protected function newInteractiveEditor($text) { diff --git a/src/workflow/ArcanistDiffWorkflow.php b/src/workflow/ArcanistDiffWorkflow.php index 43c049e5..881843da 100644 --- a/src/workflow/ArcanistDiffWorkflow.php +++ b/src/workflow/ArcanistDiffWorkflow.php @@ -267,12 +267,17 @@ EOTEXT 'lint' => true, ), ), + 'add-all' => array( + 'help' => + 'Automatically add all untracked, unstaged and uncommitted files to '. + 'the commit.', + ), 'json' => array( 'help' => 'Emit machine-readable JSON. EXPERIMENTAL! Probably does not work!', ), 'no-amend' => array( - 'help' => 'Never amend commits in the working copy.', + 'help' => 'Never amend commits in the working copy with lint patches.', ), 'uncommitted' => array( 'help' => 'Suppress warning about uncommitted changes.', @@ -582,6 +587,13 @@ EOTEXT if ($this->requiresWorkingCopy()) { try { + if ($this->getArgument('add-all')) { + $this->setCommitMode(self::COMMIT_ENABLE); + } else if ($this->getArgument('uncommitted')) { + $this->setCommitMode(self::COMMIT_DISABLE); + } else { + $this->setCommitMode(self::COMMIT_ALLOW); + } $this->requireCleanWorkingCopy(); } catch (ArcanistUncommittedChangesException $ex) { $repository_api = $this->getRepositoryAPI();