From 77a9c1814063508802d572366958267533c03cd9 Mon Sep 17 00:00:00 2001 From: Bob Trahan Date: Fri, 11 Apr 2014 10:52:35 -0700 Subject: [PATCH] Arcanist - normalize question about unstaged files across workflows (diff vs land) Summary: Fixes T4163. Re-word the question so its the same as in "arc diff". Draw the line though and don't automagically do anything if the user says "Y", 'cuz amending / adding files last minute in 'arc land' and other workflows is batshit insane. Assuming infinite growth in the future, I think its best to get this language consistent now. I changed the shouldAmend member variable to a shouldAmend() function with a static inside. Previously shouldAmend was getting set as a side effect and it was kind of weird. I thought maybe it was written this way because the calls to the vcs are a little slow or something. As such, I figured caching it in the static was a good idea? Didn't seem awful but maybe a premature optimization with whatever the performance reality turns out to be. Also a modest amount of bonus pht. Test Plan: this very diff. i'm going to arc land it laters too. Reviewers: epriestley Reviewed By: epriestley Subscribers: chad, epriestley, Korvin Maniphest Tasks: T4163 Differential Revision: https://secure.phabricator.com/D8753 --- src/workflow/ArcanistBaseWorkflow.php | 50 +++++++++++++++++---------- 1 file changed, 31 insertions(+), 19 deletions(-) diff --git a/src/workflow/ArcanistBaseWorkflow.php b/src/workflow/ArcanistBaseWorkflow.php index 421b0496..d0fcd929 100644 --- a/src/workflow/ArcanistBaseWorkflow.php +++ b/src/workflow/ArcanistBaseWorkflow.php @@ -44,7 +44,6 @@ abstract class ArcanistBaseWorkflow extends Phobject { const AUTO_COMMIT_TITLE = 'Automatic commit by arc'; private $commitMode = self::COMMIT_DISABLE; - private $shouldAmend; private $conduit; private $conduitURI; @@ -660,15 +659,17 @@ abstract class ArcanistBaseWorkflow extends Phobject { '--'.head($corrected))."\n"); $arg_key = head($corrected); } else { - throw new ArcanistUsageException( - "Unknown argument '{$arg_key}'. Try 'arc help'."); + throw new ArcanistUsageException(pht( + "Unknown argument '%s'. Try 'arc help'.", + $arg_key)); } } } else if (!strncmp($arg, '-', 1)) { $arg_key = substr($arg, 1); if (empty($short_to_long_map[$arg_key])) { - throw new ArcanistUsageException( - "Unknown argument '{$arg_key}'. Try 'arc help'."); + throw new ArcanistUsageException(pht( + "Unknown argument '%s'. Try 'arc help'.", + $arg_key)); } $arg_key = $short_to_long_map[$arg_key]; } else { @@ -681,8 +682,9 @@ abstract class ArcanistBaseWorkflow extends Phobject { $dict[$arg_key] = true; } else { if ($ii == count($args) - 1) { - throw new ArcanistUsageException( - "Option '{$arg}' requires a parameter."); + throw new ArcanistUsageException(pht( + "Option '%s' requires a parameter.", + $arg)); } if (!empty($options['repeat'])) { $dict[$arg_key][] = $args[$ii + 1]; @@ -698,8 +700,9 @@ abstract class ArcanistBaseWorkflow extends Phobject { $dict[$more_key] = $more; } else { $example = reset($more); - throw new ArcanistUsageException( - "Unrecognized argument '{$example}'. Try 'arc help'."); + throw new ArcanistUsageException(pht( + "Unrecognized argument '%s'. Try 'arc help'.", + $example)); } } @@ -779,7 +782,7 @@ abstract class ArcanistBaseWorkflow extends Phobject { if ($this->stashed) { $api = $this->getRepositoryAPI(); $api->unstashChanges(); - echo "Restored stashed changes to the working directory.\n"; + echo pht('Restored stashed changes to the working directory.') . "\n"; } } @@ -820,15 +823,18 @@ abstract class ArcanistBaseWorkflow extends Phobject { $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(); + $prompt = $this->getAskForAddPrompt($untracked); + if (phutil_console_confirm($prompt)) { + throw new ArcanistUsageException(pht( + "Add these files and then run 'arc %s' again.", + $this->getWorkflowName())); } } } } + // NOTE: this is a subversion-only concept. $incomplete = $api->getIncompleteChanges(); if ($incomplete) { throw new ArcanistUsageException( @@ -907,7 +913,7 @@ abstract class ArcanistBaseWorkflow extends Phobject { } if ($must_commit) { - if ($this->shouldAmend) { + if ($this->shouldAmend()) { $commit = head($api->getLocalCommitInformation()); $api->amendCommit($commit['message']); } else if ($api->supportsLocalCommits()) { @@ -921,6 +927,10 @@ abstract class ArcanistBaseWorkflow extends Phobject { } private function shouldAmend() { + return $this->calculateShouldAmend(); + } + + private function calculateShouldAmend() { $api = $this->getRepositoryAPI(); if ($this->isHistoryImmutable() || !$api->supportsAmend()) { @@ -982,13 +992,15 @@ abstract class ArcanistBaseWorkflow extends Phobject { if ($this->commitMode == self::COMMIT_DISABLE) { return false; } - if ($this->shouldAmend === null) { - $this->shouldAmend = $this->shouldAmend(); - } if ($this->commitMode == self::COMMIT_ENABLE) { return true; } - if ($this->shouldAmend) { + $prompt = $this->getAskForAddPrompt($files); + return phutil_console_confirm($prompt); + } + + private function getAskForAddPrompt(array $files) { + if ($this->shouldAmend()) { $prompt = pht( 'Do you want to amend these files to the commit?', count($files)); @@ -997,7 +1009,7 @@ abstract class ArcanistBaseWorkflow extends Phobject { 'Do you want to add these files to the commit?', count($files)); } - return phutil_console_confirm($prompt); + return $prompt; } protected function loadDiffBundleFromConduit(