From ccbaee585e1a350d1ff970c30481079dc8471a7c Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 7 Mar 2016 07:13:11 -0800 Subject: [PATCH] When `arc` pushes to the staging area, tell Phabricator what we did Summary: Ref T10093. Right now, Phabricator kind of guesses that `arc` probably pushed stuff to the staging area. This can cause confusing/misleading errors later, if it didn't actually push. Instead, tell Phabricator that we pushed, so we can raise more tailored messages in the web UI (e.g., make "Land Revision" say "this wasn't pushed to the staging area" instead of "whoops, error!!~"). Test Plan: Ran `arc diff` a few times, then looked in the database for properties. {F1161655} Reviewers: chad Reviewed By: chad Maniphest Tasks: T10093 Differential Revision: https://secure.phabricator.com/D15426 --- src/workflow/ArcanistDiffWorkflow.php | 80 ++++++++++++++++++++++----- 1 file changed, 67 insertions(+), 13 deletions(-) diff --git a/src/workflow/ArcanistDiffWorkflow.php b/src/workflow/ArcanistDiffWorkflow.php index 40d86faf..9ffdb049 100644 --- a/src/workflow/ArcanistDiffWorkflow.php +++ b/src/workflow/ArcanistDiffWorkflow.php @@ -22,6 +22,15 @@ final class ArcanistDiffWorkflow extends ArcanistWorkflow { private $commitMessageFromRevision; private $hitAutotargets; + const STAGING_PUSHED = 'pushed'; + const STAGING_USER_SKIP = 'user.skip'; + const STAGING_DIFF_RAW = 'diff.raw'; + const STAGING_REPOSITORY_UNKNOWN = 'repository.unknown'; + const STAGING_REPOSITORY_UNAVAILABLE = 'repository.unavailable'; + const STAGING_REPOSITORY_UNSUPPORTED = 'repository.unsupported'; + const STAGING_REPOSITORY_UNCONFIGURED = 'repository.unconfigured'; + const STAGING_CLIENT_UNSUPPORTED = 'client.unsupported'; + public function getWorkflowName() { return 'diff'; } @@ -519,7 +528,7 @@ EOTEXT 'unitResult' => $unit_result, )); - $this->pushChangesToStagingArea($this->diffID); + $this->submitChangesToStagingArea($this->diffID); $phid = idx($diff_info, 'phid'); if ($phid) { @@ -2657,26 +2666,50 @@ EOTEXT return $this->getArgument('browse'); } + private function submitChangesToStagingArea($id) { + $result = $this->pushChangesToStagingArea($id); + + // We'll either get a failure constant on error, or a list of pushed + // refs on success. + $ok = is_array($result); + + if ($ok) { + $staging = array( + 'status' => self::STAGING_PUSHED, + 'refs' => $result, + ); + } else { + $staging = array( + 'status' => $result, + 'refs' => array(), + ); + } + + $this->updateDiffProperty( + 'arc.staging', + phutil_json_encode($staging)); + } + private function pushChangesToStagingArea($id) { if ($this->getArgument('skip-staging')) { $this->writeInfo( pht('SKIP STAGING'), pht('Flag --skip-staging was specified.')); - return; + return self::STAGING_USER_SKIP; } if ($this->isRawDiffSource()) { $this->writeInfo( pht('SKIP STAGING'), pht('Raw changes can not be pushed to a staging area.')); - return; + return self::STAGING_DIFF_RAW; } if (!$this->getRepositoryPHID()) { $this->writeInfo( pht('SKIP STAGING'), pht('Unable to determine repository for this change.')); - return; + return self::STAGING_REPOSITORY_UNKNOWN; } $staging = $this->getRepositoryStagingConfiguration(); @@ -2684,7 +2717,7 @@ EOTEXT $this->writeInfo( pht('SKIP STAGING'), pht('The server does not support staging areas.')); - return; + return self::STAGING_REPOSITORY_UNAVAILABLE; } $supported = idx($staging, 'supported'); @@ -2692,7 +2725,7 @@ EOTEXT $this->writeInfo( pht('SKIP STAGING'), pht('Phabricator does not support staging areas for this repository.')); - return; + return self::STAGING_REPOSITORY_UNSUPPORTED; } $staging_uri = idx($staging, 'uri'); @@ -2700,7 +2733,7 @@ EOTEXT $this->writeInfo( pht('SKIP STAGING'), pht('No staging area is configured for this repository.')); - return; + return self::STAGING_REPOSITORY_UNCONFIGURED; } $api = $this->getRepositoryAPI(); @@ -2708,14 +2741,14 @@ EOTEXT $this->writeInfo( pht('SKIP STAGING'), pht('This client version does not support staging this repository.')); - return; + return self::STAGING_CLIENT_UNSUPPORTED; } $commit = $api->getHeadCommit(); $prefix = idx($staging, 'prefix', 'phabricator'); - $base_tag = $prefix.'/base/'.$id; - $diff_tag = $prefix.'/diff/'.$id; + $base_tag = "refs/tags/{$prefix}/base/{$id}"; + $diff_tag = "refs/tags/{$prefix}/diff/{$id}"; $this->writeOkay( pht('PUSH STAGING'), @@ -2728,6 +2761,10 @@ EOTEXT $refs = array(); + $remote = array( + 'uri' => $staging_uri, + ); + // If the base commit is a real commit, we're going to push it. We don't // use this, but pushing it to a ref reduces the amount of redundant work // that Git does on later pushes by helping it figure out that the remote @@ -2739,17 +2776,32 @@ EOTEXT // refs. $base_commit = $api->getSourceControlBaseRevision(); if ($base_commit !== ArcanistGitAPI::GIT_MAGIC_ROOT_COMMIT) { - $refs[] = "{$base_commit}:refs/tags/{$base_tag}"; + $refs[] = array( + 'ref' => $base_tag, + 'type' => 'base', + 'commit' => $base_commit, + 'remote' => $remote, + ); } // We're always going to push the change itself. - $refs[] = "{$commit}:refs/tags/{$diff_tag}"; + $refs[] = array( + 'ref' => $diff_tag, + 'type' => 'diff', + 'commit' => $commit, + 'remote' => $remote, + ); + + $ref_list = array(); + foreach ($refs as $ref) { + $ref_list[] = $ref['commit'].':'.$ref['ref']; + } $err = phutil_passthru( 'git push %Ls -- %s %Ls', $push_flags, $staging_uri, - $refs); + $ref_list); if ($err) { $this->writeWarn( @@ -2761,6 +2813,8 @@ EOTEXT 'Failed to push changes to staging area. Correct the issue, or '. 'use --skip-staging to skip this step.')); } + + return $refs; }