From 604ceb4fe5ef092f3ba31d7b566b8457482c4abc Mon Sep 17 00:00:00 2001 From: Eitan Adler Date: Sat, 5 Mar 2016 14:29:46 -0800 Subject: [PATCH 1/3] Use python2 instead of python in `arc anoid` shebang` Summary: With the old shebang of `#!/usr/bin/env python` on machines with python 3 as the default python it would fail. Prefer an explicit python2 like PEP 394 suggests. Test Plan: ran ./arc anoid and played a game Reviewers: #blessed_reviewers, epriestley Reviewed By: #blessed_reviewers, epriestley Subscribers: Korvin Differential Revision: https://secure.phabricator.com/D15408 --- scripts/breakout.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/scripts/breakout.py b/scripts/breakout.py index 4ee3af11..c8c8b188 100755 --- a/scripts/breakout.py +++ b/scripts/breakout.py @@ -1,9 +1,9 @@ -#!/usr/bin/env python +#!/usr/bin/env python2 import sys import time import select import curses -import curses.wrapper +from curses import wrapper entities = [] grid = [] @@ -217,4 +217,4 @@ try: print ('You destroyed %s blocks out of %s with %s deaths.' % (Block.killed, Block.total, Ball.killed)) except PowerOverwhelmingException as e: - print e + print (e) From 4a1160e0c3a2fdc6f5e5638c67e6ca8bf65b5da4 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 7 Mar 2016 06:17:43 -0800 Subject: [PATCH 2/3] When pushing changes to staging, also push the base commit Summary: Fixes T10509. Pushing changes to staging can be inefficient. What happens, roughly, is: - Master is at commit "W" -- "W" is the most recent published commit in the main repository. - The local working copy has one change on top of that, "X", so its history is commits "A, B, C, D, E, F, ..., U, V, W, X". - The remote has some other previous changes that I or other users have made, maybe like "A, B, C, ..., S, T, U, Y" and "A, B, C, ..., T, U, V, Z", from previous pushes to staging areas. - "X", "Y" and "Z" will never actually make it to master, because they'll be squash-merged/amended by `arc land`. So the local says "I want to push 'X'", and the remote says "I know about 'Y' and 'Z', are those in the history of 'W'? You only need to send me new stuff if they are". But they aren't, so the local says "nope, so here's the whole history for you". This is slow and sends a ton of data that the remote already has over the network. In theory, Git could use a slightly different algorithm to tell the local about more commits, but this is hard, rarely useful, and not the kind of thing I'd be excited about changing if I was the Git upstream. Instead, when pushing "X", also push "W", to trick Git into telling future clients about it. Now, the remote should say "I know about 'W', 'Y' and 'Z'", and the local will say "oh, great, 'W' is in history, here's just the changes since then". Also, fail `arc diff` if the push to staging fails, and tell users to use `--skip-staging`. This code has been in production for a while and doesn't seem to have any issues, and a failed push to staging prevents builds, lands, etc. Test Plan: - Ran `arc diff`, saw two changes push. - Ran `arc diff --base arc:empty`, saw only one change push. - Ran `arc diff` with an intentionally broken staging area, saw an error. Reviewers: chad Reviewed By: chad Maniphest Tasks: T10509 Differential Revision: https://secure.phabricator.com/D15424 --- src/upload/ArcanistFileUploader.php | 2 +- src/workflow/ArcanistDiffWorkflow.php | 38 ++++++++++++++++++++------- 2 files changed, 30 insertions(+), 10 deletions(-) diff --git a/src/upload/ArcanistFileUploader.php b/src/upload/ArcanistFileUploader.php index d8c65a43..b37b1856 100644 --- a/src/upload/ArcanistFileUploader.php +++ b/src/upload/ArcanistFileUploader.php @@ -26,7 +26,7 @@ final class ArcanistFileUploader extends Phobject { private $conduit; - private $files; + private $files = array(); /* -( Configuring the Uploader )------------------------------------------- */ diff --git a/src/workflow/ArcanistDiffWorkflow.php b/src/workflow/ArcanistDiffWorkflow.php index 8a3fa6f6..40d86faf 100644 --- a/src/workflow/ArcanistDiffWorkflow.php +++ b/src/workflow/ArcanistDiffWorkflow.php @@ -2713,7 +2713,9 @@ EOTEXT $commit = $api->getHeadCommit(); $prefix = idx($staging, 'prefix', 'phabricator'); - $tag = $prefix.'/diff/'.$id; + + $base_tag = $prefix.'/base/'.$id; + $diff_tag = $prefix.'/diff/'.$id; $this->writeOkay( pht('PUSH STAGING'), @@ -2723,23 +2725,41 @@ EOTEXT if (version_compare($api->getGitVersion(), '1.8.2', '>=')) { $push_flags[] = '--no-verify'; } + + $refs = array(); + + // 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 + // already has most of the history. See T10509. + + // In the future, we could avoid this push if the staging area is the same + // as the main repository, or if the staging area is a virtual repository. + // In these cases, the staging area should automatically have up-to-date + // refs. + $base_commit = $api->getSourceControlBaseRevision(); + if ($base_commit !== ArcanistGitAPI::GIT_MAGIC_ROOT_COMMIT) { + $refs[] = "{$base_commit}:refs/tags/{$base_tag}"; + } + + // We're always going to push the change itself. + $refs[] = "{$commit}:refs/tags/{$diff_tag}"; + $err = phutil_passthru( - 'git push %Ls -- %s %s:refs/tags/%s', + 'git push %Ls -- %s %Ls', $push_flags, $staging_uri, - $commit, - $tag); + $refs); if ($err) { $this->writeWarn( pht('STAGING FAILED'), pht('Unable to push changes to the staging area.')); - } else { - $this->writeOkay( - pht('STAGING PUSHED'), + + throw new ArcanistUsageException( pht( - 'Pushed a copy of the changes to tag "%s" in the staging area.', - $tag)); + 'Failed to push changes to staging area. Correct the issue, or '. + 'use --skip-staging to skip this step.')); } } From ccbaee585e1a350d1ff970c30481079dc8471a7c Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 7 Mar 2016 07:13:11 -0800 Subject: [PATCH 3/3] 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; }