Fix an issue where we would try to release an unheld lock

Summary: Fixes T7484. If the lock failed, we'd still try to unlock it, which is incorrect.

Test Plan: Ran two `bin/repository update X` in different windows, got proper LockException instead of indirect symptomatic "not locked by this process" exception.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7484

Differential Revision: https://secure.phabricator.com/D12253

src/applications/repository/management/PhabricatorRepositoryManagementUpdateWorkflow.php

@@ -58,38 +58,42 @@ final class PhabricatorRepositoryManagementUpdateWorkflow
       $lock = PhabricatorGlobalLock::newLock($lock_name);
 
       $lock->lock();
+      try {
+        $no_discovery = $args->getArg('no-discovery');
 
-      $no_discovery = $args->getArg('no-discovery');
+        id(new PhabricatorRepositoryPullEngine())
+          ->setRepository($repository)
+          ->setVerbose($this->getVerbose())
+          ->pullRepository();
 
-      id(new PhabricatorRepositoryPullEngine())
+        if ($no_discovery) {
-        ->setRepository($repository)
+          $lock->unlock();
-        ->setVerbose($this->getVerbose())
+          return;
-        ->pullRepository();
+        }
 
-      if ($no_discovery) {
+        // TODO: It would be nice to discover only if we pulled something, but
+        // this isn't totally trivial. It's slightly more complicated with
+        // hosted repositories, too.
+
+        $repository->writeStatusMessage(
+          PhabricatorRepositoryStatusMessage::TYPE_NEEDS_UPDATE,
+          null);
+
+        $this->discoverRepository($repository);
+
+        $this->checkIfRepositoryIsFullyImported($repository);
+
+        $this->updateRepositoryRefs($repository);
+
+        $this->mirrorRepository($repository);
+
+        $repository->writeStatusMessage(
+          PhabricatorRepositoryStatusMessage::TYPE_FETCH,
+          PhabricatorRepositoryStatusMessage::CODE_OKAY);
+      } catch (Exception $ex) {
         $lock->unlock();
-        return;
+        throw $ex;
       }
-
-      // TODO: It would be nice to discover only if we pulled something, but
-      // this isn't totally trivial. It's slightly more complicated with hosted
-      // repositories, too.
-
-      $repository->writeStatusMessage(
-        PhabricatorRepositoryStatusMessage::TYPE_NEEDS_UPDATE,
-        null);
-
-      $this->discoverRepository($repository);
-
-      $this->checkIfRepositoryIsFullyImported($repository);
-
-      $this->updateRepositoryRefs($repository);
-
-      $this->mirrorRepository($repository);
-
-      $repository->writeStatusMessage(
-        PhabricatorRepositoryStatusMessage::TYPE_FETCH,
-        PhabricatorRepositoryStatusMessage::CODE_OKAY);
     } catch (Exception $ex) {
       $repository->writeStatusMessage(
         PhabricatorRepositoryStatusMessage::TYPE_FETCH,
@@ -98,8 +102,6 @@ final class PhabricatorRepositoryManagementUpdateWorkflow
           'message' => pht(
             'Error updating working copy: %s', $ex->getMessage()),
         ));
-
-      $lock->unlock();
       throw $ex;
     }