From e44a2d3ac0bf6d92b482847e79ae09541f07673c Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 3 Apr 2018 10:31:39 -0700 Subject: [PATCH 1/2] Improve argument parsing for "arc patch --revision Dxxx" Summary: See PHI527. Ref T13116. The `--revision` flag currently fails if the argument is in the form `D123` instead of `123`. Normalize monogram arguments. Test Plan: Ran `arc patch --revision Dxxx`, `arc patch --revision xxx`, `arc patch --revision xxx --diff yyy`, `arc patch`; got good behavior on the good ones and sensible error messages on the other ones. Maniphest Tasks: T13116 Differential Revision: https://secure.phabricator.com/D19292 --- src/workflow/ArcanistPatchWorkflow.php | 95 ++++++++++++-------------- 1 file changed, 44 insertions(+), 51 deletions(-) diff --git a/src/workflow/ArcanistPatchWorkflow.php b/src/workflow/ArcanistPatchWorkflow.php index 227b020f..a2b3fe40 100644 --- a/src/workflow/ArcanistPatchWorkflow.php +++ b/src/workflow/ArcanistPatchWorkflow.php @@ -115,66 +115,59 @@ EOTEXT } protected function didParseArguments() { - $source = null; - $requested = 0; - if ($this->getArgument('revision')) { - $source = self::SOURCE_REVISION; - $requested++; - } - if ($this->getArgument('diff')) { - $source = self::SOURCE_DIFF; - $requested++; - } - if ($this->getArgument('arcbundle')) { - $source = self::SOURCE_BUNDLE; - $requested++; - } - if ($this->getArgument('patch')) { - $source = self::SOURCE_PATCH; - $requested++; - } + $arguments = array( + 'revision' => self::SOURCE_REVISION, + 'diff' => self::SOURCE_DIFF, + 'arcbundle' => self::SOURCE_BUNDLE, + 'patch' => self::SOURCE_PATCH, + 'name' => self::SOURCE_REVISION, + ); - $use_revision_id = null; - if ($this->getArgument('name')) { - $namev = $this->getArgument('name'); - if (count($namev) > 1) { - throw new ArcanistUsageException( - pht('Specify at most one revision name.')); + $sources = array(); + foreach ($arguments as $key => $source_type) { + $value = $this->getArgument($key); + if (!$value) { + continue; } - $source = self::SOURCE_REVISION; - $requested++; - $use_revision_id = $this->normalizeRevisionID(head($namev)); + switch ($key) { + case 'revision': + $value = $this->normalizeRevisionID($value); + break; + case 'name': + if (count($value) > 1) { + throw new ArcanistUsageException( + pht('Specify at most one revision name.')); + } + $value = $this->normalizeRevisionID(head($value)); + break; + } + + $sources[] = array( + $source_type, + $value, + ); } - if ($requested === 0) { + if (!$sources) { throw new ArcanistUsageException( pht( - "Specify one of '%s', '%s' (to select the current changes attached ". - "to a Differential revision), '%s' (to select a specific, ". - "out-of-date diff or a diff which is not attached to a revision), ". - "'%s' or '%s' to choose a patch source.", - 'D12345', - '--revision ', - '--diff ', - '--arcbundle ', - '--patch ')); - } else if ($requested > 1) { - throw new ArcanistUsageException( - pht( - "Options '%s', '%s', '%s', '%s' and '%s' are not compatible. ". - "Choose exactly one patch source.", - 'D12345', - '--revision', - '--diff', - '--arcbundle', - '--patch')); + 'You must specify changes to apply to the working copy with '. + '"D12345", "--revision", "--diff", "--arcbundle", or "--patch".')); } - $this->source = $source; - $this->sourceParam = nonempty( - $use_revision_id, - $this->getArgument($source)); + if (count($sources) > 1) { + throw new ArcanistUsageException( + pht( + 'Options "D12345", "--revision", "--diff", "--arcbundle" and '. + '"--patch" are mutually exclusive. Choose exactly one patch '. + 'source.')); + } + + $source = head($sources); + + $this->source = $source[0]; + $this->sourceParam = $source[1]; } public function requiresConduit() { From 73f5afd441109cb712282660c1eb01089b6297fa Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 5 Apr 2018 06:33:47 -0700 Subject: [PATCH 2/2] Remove "very large change" warning from Arcanist Summary: Ref T13110. We now degrade very large changes and I'm not convinced any user ever entered "n" at this prompt. Test Plan: Ran `arc diff` to create this very revision. Maniphest Tasks: T13110 Differential Revision: https://secure.phabricator.com/D19299 --- src/workflow/ArcanistDiffWorkflow.php | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/src/workflow/ArcanistDiffWorkflow.php b/src/workflow/ArcanistDiffWorkflow.php index 14a7ef8f..86296ae5 100644 --- a/src/workflow/ArcanistDiffWorkflow.php +++ b/src/workflow/ArcanistDiffWorkflow.php @@ -966,22 +966,6 @@ EOTEXT throw new Exception(pht('Repository API is not supported.')); } - if (count($changes) > 250) { - $message = pht( - 'This diff has a very large number of changes (%s). Differential '. - 'works best for changes which will receive detailed human review, '. - 'and not as well for large automated changes or bulk checkins. '. - 'See %s for information about reviewing big checkins. Continue anyway?', - phutil_count($changes), - 'https://secure.phabricator.com/book/phabricator/article/'. - 'differential_large_changes/'); - - if (!phutil_console_confirm($message)) { - throw new ArcanistUsageException( - pht('Aborted generation of gigantic diff.')); - } - } - $limit = 1024 * 1024 * 4; foreach ($changes as $change) { $size = 0;