From 982e48229096ec1788de4859f66f0c71ea1453db Mon Sep 17 00:00:00 2001 From: epriestley Date: Sat, 12 Mar 2011 17:57:35 -0800 Subject: [PATCH] Make some UI things less confusing and reduce the amount of horrific torture that arc apparently inflicts upon users. Summary: Test Plan: . Reviewers: CC: Differential Revision: 223104 --- .../api/base/ArcanistRepositoryAPI.php | 10 +++++- .../api/subversion/ArcanistSubversionAPI.php | 3 ++ src/workflow/base/ArcanistBaseWorkflow.php | 32 ++++++++++++++++--- src/workflow/base/__init__.php | 1 + src/workflow/diff/ArcanistDiffWorkflow.php | 20 +++++++++++- 5 files changed, 59 insertions(+), 7 deletions(-) diff --git a/src/repository/api/base/ArcanistRepositoryAPI.php b/src/repository/api/base/ArcanistRepositoryAPI.php index 549c03de..30df621a 100644 --- a/src/repository/api/base/ArcanistRepositoryAPI.php +++ b/src/repository/api/base/ArcanistRepositoryAPI.php @@ -33,9 +33,13 @@ abstract class ArcanistRepositoryAPI { const FLAG_UNCOMMITTED = 128; const FLAG_EXTERNALS = 256; - // Occurs in SVN when you replace a file with a directory. + // Occurs in SVN when you replace a file with a directory without telling + // SVN about it. const FLAG_OBSTRUCTED = 512; + // Occurs in SVN when an update was interrupted or failed, e.g. you ^C'd it. + const FLAG_INCOMPLETE = 1024; + protected $path; protected $diffLinesOfContext = 0x7FFF; @@ -110,6 +114,10 @@ abstract class ArcanistRepositoryAPI { return $this->getWorkingCopyFilesWithMask(self::FLAG_CONFLICT); } + public function getIncompleteChanges() { + return $this->getWorkingCopyFilesWithMask(self::FLAG_INCOMPLETE); + } + private function getWorkingCopyFilesWithMask($mask) { $match = array(); foreach ($this->getWorkingCopyStatus() as $file => $flags) { diff --git a/src/repository/api/subversion/ArcanistSubversionAPI.php b/src/repository/api/subversion/ArcanistSubversionAPI.php index 5edfbc19..e8dd6bb1 100644 --- a/src/repository/api/subversion/ArcanistSubversionAPI.php +++ b/src/repository/api/subversion/ArcanistSubversionAPI.php @@ -125,6 +125,9 @@ class ArcanistSubversionAPI extends ArcanistRepositoryAPI { case 'conflicted': $mask |= self::FLAG_CONFLICT; break; + case 'incomplete': + $mask |= self::FLAG_INCOMPLETE; + break; default: throw new Exception("Unrecognized item status '{$item}'."); } diff --git a/src/workflow/base/ArcanistBaseWorkflow.php b/src/workflow/base/ArcanistBaseWorkflow.php index 8772422b..c66c300a 100644 --- a/src/workflow/base/ArcanistBaseWorkflow.php +++ b/src/workflow/base/ArcanistBaseWorkflow.php @@ -319,14 +319,36 @@ class ArcanistBaseWorkflow { $untracked = $api->getUntrackedChanges(); if ($this->shouldRequireCleanUntrackedFiles()) { if (!empty($untracked)) { - throw new ArcanistUsageException( - "You have untracked files in this working copy:\n". - " ".implode("\n ", $untracked)."\n\n". - "Add or delete them before proceeding, or include them in your ". - "ignore rules. To bypass this check, use --allow-untracked."); + + echo "You have untracked files in this working copy:\n\n". + " ".implode("\n ", $untracked)."\n\n"; + + if ($api instanceof ArcanistGitAPI) { + echo phutil_console_wrap( + "Since you don't have .gitignore rules for these files and have ". + "not listed them in .git/info/exclude, you may have forgotten ". + "to 'git add' them to your commit."); + } else if ($api instanceof ArcanistSubversionAPI) { + echo phutil_console_wrap( + "Since you don't have svn:ignore rules for these files, you may ". + "have forgotten to 'svn add' them."); + } + + $prompt = "Do you want to continue without adding these files?"; + if (!phutil_console_confirm($prompt, $default_no = false)) { + throw new ArcanistUserAbortException(); + } } } + $incomplete = $api->getIncompleteChanges(); + if ($incomplete) { + throw new ArcanistUsageException( + "You have incompletely checked out directories in this working copy. ". + "Fix them before proceeding: \n\n". + " ".implode("\n ", $incomplete)."\n\n". + "You can fix these paths by running 'svn update' on them."); + } if ($api->getMergeConflicts()) { throw new ArcanistUsageException( diff --git a/src/workflow/base/__init__.php b/src/workflow/base/__init__.php index 5be4f028..b9fb5a2f 100644 --- a/src/workflow/base/__init__.php +++ b/src/workflow/base/__init__.php @@ -9,6 +9,7 @@ phutil_require_module('arcanist', 'differential/revision'); phutil_require_module('arcanist', 'exception'); phutil_require_module('arcanist', 'exception/usage'); +phutil_require_module('arcanist', 'exception/usage/userabort'); phutil_require_module('arcanist', 'parser/bundle'); phutil_require_module('arcanist', 'parser/diff'); phutil_require_module('arcanist', 'parser/diff/change'); diff --git a/src/workflow/diff/ArcanistDiffWorkflow.php b/src/workflow/diff/ArcanistDiffWorkflow.php index e91d1847..fdf27f93 100644 --- a/src/workflow/diff/ArcanistDiffWorkflow.php +++ b/src/workflow/diff/ArcanistDiffWorkflow.php @@ -203,6 +203,14 @@ EOTEXT $paths = $this->generateAffectedPaths(); + // Do this before we start linting or running unit tests so we can detect + // things like a missing test plan or invalid reviewers immediately. + if ($this->shouldOnlyCreateDiff()) { + $commit_message = null; + } else { + $commit_message = $this->getGitCommitMessage(); + } + $lint_result = $this->runLint($paths); $unit_result = $this->runUnit($paths); @@ -322,7 +330,7 @@ EOTEXT " **Diff URI:** __%s__\n\n", $diff_info['uri']); } else { - $message = $this->getGitCommitMessage(); + $message = $commit_message; $revision = array( 'diffid' => $diff_info['diffid'], @@ -353,6 +361,15 @@ EOTEXT } $should_edit = $this->getArgument('edit'); + +/* + + TODO: This is a complicated mess. We need to move to storing a checksum + of the non-auto-sync fields as they existed at original diff time and using + changes from that to detect user edits, not comparison of the client and + server values since they diverge without user edits (because of Herald + and explicit server-side user changes). + if (!$should_edit) { $local_sum = $message->getChecksum(); $remote_sum = $remote_message->getChecksum(); @@ -366,6 +383,7 @@ EOTEXT $default_no = false); } } +*/ $revision['fields'] = $remote_message->getFields();