Page MenuHomePhabricator
Differential D9369 Diff 22592 src/workflow/ArcanistDiffWorkflow.php

Changeset View

Standalone View

View Options

src/workflow/ArcanistDiffWorkflow.php

Show First 20 Lines • Show All 389 Lines • ▼ Show 20 Lines $arguments = array(
), ),
'no-coverage' => array( 'no-coverage' => array(
'help' => 'Always disable coverage information.', 'help' => 'Always disable coverage information.',
'passthru' => array( 'passthru' => array(
'unit' => true, 'unit' => true,
), ),
), ),
'*' => 'paths', '*' => 'paths',
'head' => array(
'param' => 'commit',
'help' => "specify the head commit.\n".
epriestleyUnsubmitted
Not Done
Inline Actions

This should use full sentences and explicitly state that features will be disabled.

epriestley: This should use full sentences and explicitly state that features will be disabled.
"This disables many Arcanist/Phabricator features which depend on ".
"having access to the working copy.",
epriestleyUnsubmitted
Not Done
Inline Actions

Instead of --range, maybe this should be something like --head, --tip, --to, or similar? Particularly:

  • Calls like arc diff X --range Y..Z are confusing/ambiguous.
  • Calls like arc diff --base X --range Y..Z are confusing/ambiguous.
  • a..b is a valid branch name in Mercurial.
  • Generally, there's no way to say "use my base rule normally, just stop at HEAD^" without a lot of extra work. This seems somewhat useful to support.

On the other side of things, it seems easy to run arc diff X --head Y instead of arc diff --range X..Y, so we aren't losing anything by doing this.

epriestley: Instead of `--range`, maybe this should be something like `--head`, `--tip`, `--to`, or similar?
talshiriAuthorUnsubmitted
Not Done
Inline Actions

I mostly wanted to keep it inline with git syntax, since it's well known (if a little confusing) at this point, and it's nice being able to do:

$ git log a..b
(verify output)
$ arc diff --range a..b
and know that you'll send those exact commits in.

I like --head, though, and all of your points are valid. I'll switch it over to --head.
Would it be reasonable to add git-specific syntax when you're in git repos? something like --git-range? It creates ambiguity (--head X --range a..b), but I like the ability to use the same range specifier as with normal git commands.

talshiri: I mostly wanted to keep it inline with git syntax, since it's well known (if a little…
'supports' => array('git'),
'conflicts' => array(
'lintall' => '--head suppresses lint.',
'advice' => '--head suppresses lint.',
),
)
); );
if (phutil_is_windows()) { if (phutil_is_windows()) {
unset($arguments['background']); unset($arguments['background']);
} }
return $arguments; return $arguments;
} }
Show All 22 Lines public function run() {
} }
if ($background) { if ($background) {
$argv = $this->getPassedArguments(); $argv = $this->getPassedArguments();
if (!PhutilConsoleFormatter::getDisableANSI()) { if (!PhutilConsoleFormatter::getDisableANSI()) {
array_unshift($argv, '--ansi'); array_unshift($argv, '--ansi');
} }
if ($this->getRepositoryAPI()->supportsCommitRanges()) { $repo = $this->getRepositoryAPI();
$this->getRepositoryAPI()->getBaseCommit(); $head_commit = $this->getArgument('head');
epriestleyUnsubmitted
Not Done
Inline Actions

(null should be the default-by-default, and can be omitted?)

epriestley: (`null` should be the default-by-default, and can be omitted?)
$range_supported = $repo->supportsCommitRanges();
if ($head_commit) {
if (!$range_supported) {
throw new Exception('Ranged are not supported');
epriestleyUnsubmitted
Not Done
Inline Actions

"ranged" -> "Ranges"

epriestley: "ranged" -> "Ranges"
}
$repo->setHeadCommit($head_commit);
}
if ($range_supported) {
$repo->getBaseCommit();
} }
epriestleyUnsubmitted
Not Done
Inline Actions

(Probably mooted by the above, but this could use some polish if it sticks around. For example, --range quack will explode right now, I think.)

epriestley: (Probably mooted by the above, but this could use some polish if it sticks around. For example…
talshiriAuthorUnsubmitted
Not Done
Inline Actions

Yeah it will explode :)

talshiri: Yeah it will explode :)
$script = phutil_get_library_root('arcanist').'/../scripts/arcanist.php'; $script = phutil_get_library_root('arcanist').'/../scripts/arcanist.php';
if ($argv) { if ($argv) {
$lint_unit = new ExecFuture( $lint_unit = new ExecFuture(
'php %s --recon diff --no-diff %Ls', 'php %s --recon diff --no-diff %Ls',
$script, $script,
$argv); $argv);
} else { } else {
▲ Show 20 Lines • Show All 209 Lines • ▼ Show 20 Lines if ($this->requiresWorkingCopy()) {
} else if ($this->getArgument('uncommitted')) { } else if ($this->getArgument('uncommitted')) {
$this->setCommitMode(self::COMMIT_DISABLE); $this->setCommitMode(self::COMMIT_DISABLE);
} else { } else {
$this->setCommitMode(self::COMMIT_ALLOW); $this->setCommitMode(self::COMMIT_ALLOW);
} }
if ($repository_api instanceof ArcanistSubversionAPI) { if ($repository_api instanceof ArcanistSubversionAPI) {
$repository_api->limitStatusToPaths($this->getArgument('paths')); $repository_api->limitStatusToPaths($this->getArgument('paths'));
} }
if (!$this->getArgument('head')) {
$this->requireCleanWorkingCopy(); $this->requireCleanWorkingCopy();
}
} catch (ArcanistUncommittedChangesException $ex) { } catch (ArcanistUncommittedChangesException $ex) {
if ($repository_api instanceof ArcanistMercurialAPI) { if ($repository_api instanceof ArcanistMercurialAPI) {
$use_dirty_changes = false; $use_dirty_changes = false;
if ($this->getArgument('uncommitted')) { if ($this->getArgument('uncommitted')) {
// OK. // OK.
} else { } else {
$ok = phutil_console_confirm( $ok = phutil_console_confirm(
"You have uncommitted changes in your working copy. You can ". "You have uncommitted changes in your working copy. You can ".
▲ Show 20 Lines • Show All 538 Lines • ▼ Show 20 Lines/* -( Lint and Unit Tests )------------------------------------------------ */
/** /**
* @task lintunit * @task lintunit
*/ */
private function runLint() { private function runLint() {
if ($this->getArgument('nolint') || if ($this->getArgument('nolint') ||
$this->getArgument('only') || $this->getArgument('only') ||
$this->isRawDiffSource()) { $this->isRawDiffSource() ||
$this->getArgument('head')) {
return ArcanistLintWorkflow::RESULT_SKIP; return ArcanistLintWorkflow::RESULT_SKIP;
} }
$repository_api = $this->getRepositoryAPI(); $repository_api = $this->getRepositoryAPI();
$this->console->writeOut("Linting...\n"); $this->console->writeOut("Linting...\n");
try { try {
$argv = $this->getPassthruArgumentsAsArgv('lint'); $argv = $this->getPassthruArgumentsAsArgv('lint');
▲ Show 20 Lines • Show All 64 Lines • ▼ Show 20 Lines/* -( Lint and Unit Tests )------------------------------------------------ */
/** /**
* @task lintunit * @task lintunit
*/ */
private function runUnit() { private function runUnit() {
if ($this->getArgument('nounit') || if ($this->getArgument('nounit') ||
$this->getArgument('only') || $this->getArgument('only') ||
$this->isRawDiffSource()) { $this->isRawDiffSource() ||
$this->getArgument('head')) {
return ArcanistUnitWorkflow::RESULT_SKIP; return ArcanistUnitWorkflow::RESULT_SKIP;
} }
$repository_api = $this->getRepositoryAPI(); $repository_api = $this->getRepositoryAPI();
$this->console->writeOut("Running unit tests...\n"); $this->console->writeOut("Running unit tests...\n");
try { try {
$argv = $this->getPassthruArgumentsAsArgv('unit'); $argv = $this->getPassthruArgumentsAsArgv('unit');
▲ Show 20 Lines • Show All 1,288 Lines • Show Last 20 Lines
Phabricator · Built by Phacility