Allows branches to appear in diffusion with out triggering closing things from the commit message

Summary: Implemented it how it was suggested in ticket comments

Test Plan: create a revision in a branch, push that branch up, verify it's visible in diffusion and also that revision is not closed, then merge and push to master, verify that revision closed

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin, 20after4

Maniphest Tasks: T1210

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

src/applications/conduit/method/repository/ConduitAPI_repository_create_Method.php

@@ -36,24 +36,25 @@ final class ConduitAPI_repository_create_Method
 
   public function defineParamTypes() {
     return array(
-      'name'          => 'required string',
+      'name'                => 'required string',
-      'vcs'           => 'required enum<git, hg, svn>',
+      'vcs'                 => 'required enum<git, hg, svn>',
-      'callsign'      => 'required string',
+      'callsign'            => 'required string',
-      'encoding'      => 'optional string',
+      'encoding'            => 'optional string',
-      'tracking'      => 'optional bool',
+      'tracking'            => 'optional bool',
-      'uri'           => 'optional string',
+      'uri'                 => 'optional string',
-      'sshUser'       => 'optional string',
+      'sshUser'             => 'optional string',
-      'sshKey'        => 'optional string',
+      'sshKey'              => 'optional string',
-      'sshKeyFile'    => 'optional string',
+      'sshKeyFile'          => 'optional string',
-      'httpUser'      => 'optional string',
+      'httpUser'            => 'optional string',
-      'httpPassword'  => 'optional string',
+      'httpPassword'        => 'optional string',
-      'localPath'     => 'optional string',
+      'localPath'           => 'optional string',
-      'svnSubpath'    => 'optional string',
+      'svnSubpath'          => 'optional string',
-      'branchFilter'  => 'optional list<string>',
+      'branchFilter'        => 'optional list<string>',
-      'pullFrequency' => 'optional int',
+      'closeCommitsFilter'  => 'optional list<string>',
-      'defaultBranch' => 'optional string',
+      'pullFrequency'       => 'optional int',
-      'heraldEnabled' => 'optional bool',
+      'defaultBranch'       => 'optional string',
-      'svnUUID'       => 'optional string',
+      'heraldEnabled'       => 'optional bool',
+      'svnUUID'             => 'optional string',
     );
   }
 
@@ -112,6 +113,9 @@ final class ConduitAPI_repository_create_Method
       'branch-filter'     => array_fill_keys(
         $request->getValue('branchFilter', array()),
         true),
+      'close-commits-filter' => array_fill_keys(
+        $request->getValue('closeCommitsFilter', array()),
+        true),
       'pull-frequency'    => $request->getValue('pullFrequency'),
       'default-branch'    => $request->getValue('defaultBranch'),
       'ssh-login'         => $request->getValue('sshUser'),

src/applications/repository/controller/PhabricatorRepositoryEditController.php

@@ -246,6 +246,11 @@ final class PhabricatorRepositoryEditController
         $branch_filter = array_fill_keys($branch_filter, true);
 
         $repository->setDetail('branch-filter', $branch_filter);
+
+        $close_commits_filter = $request->getStrList('close-commits-filter');
+        $close_commits_filter = array_fill_keys($close_commits_filter, true);
+
+        $repository->setDetail('close-commits-filter', $close_commits_filter);
       }
 
       $repository->setDetail(
@@ -611,13 +616,28 @@ final class PhabricatorRepositoryEditController
             'disabled'  => 'Disabled: Ignore Pushed Revisions',
             ))
         ->setCaption(
-          "Automatically close Differential revisions which are pushed to ".
+          "Automatically close Differential revisions when associated commits ".
-          "this repository.")
+          "are pushed to this repository.")
         ->setValue(
           $repository->getDetail('disable-autoclose', false)
           ? 'disabled'
           : 'enabled'));
 
+    if ($has_branch_filter) {
+      $close_commits_filter_str = implode(
+          ', ',
+          array_keys($repository->getDetail('close-commits-filter', array())));
+      $inset
+        ->appendChild(
+          id(new AphrontFormTextControl())
+            ->setName('close-commits-filter')
+            ->setLabel('Autoclose Branches')
+            ->setValue($close_commits_filter_str)
+            ->setCaption(
+              'Optional list of branches which can trigger autoclose. '.
+              'If left empty, all branches trigger autoclose.'));
+    }
+
     $inset
       ->appendChild(
         id(new AphrontFormTextControl())

src/applications/repository/daemon/PhabricatorRepositoryPullLocalDaemon.php

@@ -198,8 +198,6 @@ final class PhabricatorRepositoryPullLocalDaemon
     if (!Filesystem::pathExists($local_path)) {
       $dirname = dirname($local_path);
       if (!Filesystem::pathExists($dirname)) {
-        echo "Creating new directory '{$dirname}' ".
-             "for repository '{$callsign}'.\n";
         Filesystem::createDirectory($dirname, 0755, $recursive = true);
       }
 
@@ -257,6 +255,36 @@ final class PhabricatorRepositoryPullLocalDaemon
     return true;
   }
 
+  private static function isKnownCommitOnBranch(
+    PhabricatorRepository $repository,
+    $target,
+    $branch,
+    $any_autoclose_branch = false) {
+
+    $commit = id(new PhabricatorRepositoryCommit())->loadOneWhere(
+      'repositoryID = %s AND commitIdentifier = %s',
+      $repository->getID(),
+      $target);
+
+    $data = $commit->loadCommitData();
+    if (!$data) {
+      return false;
+    }
+
+    if ($any_autoclose_branch) {
+      if ($repository->shouldAutocloseCommit($commit, $data)) {
+        return true;
+      }
+    }
+
+    $branches = $data->getCommitDetail('seenOnBranches', array());
+    if (in_array($branch, $branches)) {
+      return true;
+    }
+
+    return false;
+  }
+
   private static function recordCommit(
     PhabricatorRepository $repository,
     $commit_identifier,
@@ -303,9 +331,40 @@ final class PhabricatorRepositoryPullLocalDaemon
     }
   }
 
+  private static function updateCommit(
+    PhabricatorRepository $repository,
+    $commit_identifier,
+    $branch) {
+
+    $commit = id(new PhabricatorRepositoryCommit())->loadOneWhere(
+      'repositoryID = %s AND commitIdentifier = %s',
+      $repository->getID(),
+      $commit_identifier);
+
+    $data = id(new PhabricatorRepositoryCommitData())->loadOneWhere(
+      'commitID = %d',
+      $commit->getID());
+    if (!$data) {
+      $data = new PhabricatorRepositoryCommitData();
+      $data->setCommitID($commit->getID());
+    }
+    $branches = $data->getCommitDetail('seenOnBranches', array());
+    $branches[] = $branch;
+    $data->setCommitDetail('seenOnBranches', $branches);
+    $data->save();
+
+    self::insertTask(
+      $repository,
+      $commit,
+      array(
+        'only' => true
+      ));
+  }
+
   private static function insertTask(
     PhabricatorRepository $repository,
-    PhabricatorRepositoryCommit $commit) {
+    PhabricatorRepositoryCommit $commit,
+    $data = array()) {
 
     $vcs = $repository->getVersionControlSystem();
     switch ($vcs) {
@@ -324,10 +383,9 @@ final class PhabricatorRepositoryPullLocalDaemon
 
     $task = new PhabricatorWorkerTask();
     $task->setTaskClass($class);
-    $task->setData(
+    $data['commitID'] = $commit->getID();
-      array(
+
-        'commitID' => $commit->getID(),
+    $task->setData($data);
-      ));
     $task->save();
   }
 
@@ -467,6 +525,7 @@ final class PhabricatorRepositoryPullLocalDaemon
       $only_this_remote = DiffusionBranchInformation::DEFAULT_GIT_REMOTE);
 
     $tracked_something = false;
+    $found_something = false;
     foreach ($branches as $name => $commit) {
       if (!$repository->shouldTrackBranch($name)) {
         continue;
@@ -488,6 +547,22 @@ final class PhabricatorRepositoryPullLocalDaemon
         "Repository r{$repo_callsign} '{$repo_name}' has no tracked branches! ".
         "Verify that your branch filtering settings are correct.");
     }
+
+    foreach ($branches as $name => $commit) {
+      if (!$repository->shouldTrackBranch($name)) {
+        continue;
+      }
+
+      if (!$repository->shouldAutocloseBranch($name)) {
+        continue;
+      }
+
+      if (self::isKnownCommitOnBranch($repository, $commit, $name, true)) {
+        continue;
+      }
+
+      self::executeGitDiscoverCommit($repository, $commit, $name);
+    }
   }
 
 
@@ -496,7 +571,8 @@ final class PhabricatorRepositoryPullLocalDaemon
    */
   private static function executeGitDiscoverCommit(
     PhabricatorRepository $repository,
-    $commit) {
+    $commit,
+    $branch = null) {
 
     $discover = array($commit);
     $insert = array($commit);
@@ -518,7 +594,12 @@ final class PhabricatorRepositoryPullLocalDaemon
           continue;
         }
         $seen_parent[$parent] = true;
-        if (!self::isKnownCommit($repository, $parent)) {
+        if ($branch !== null) {
+          $known = self::isKnownCommitOnBranch($repository, $parent, $branch);
+        } else {
+          $known = self::isKnownCommit($repository, $parent);
+        }
+        if (!$known) {
           $discover[] = $parent;
           $insert[] = $parent;
         }
@@ -535,7 +616,11 @@ final class PhabricatorRepositoryPullLocalDaemon
         $target);
       $epoch = trim($epoch);
 
-      self::recordCommit($repository, $target, $epoch);
+      if ($branch !== null) {
+        self::updateCommit($repository, $target, $branch);
+      } else {
+        self::recordCommit($repository, $target, $epoch);
+      }
 
       if (empty($insert)) {
         break;

src/applications/repository/storage/PhabricatorRepository.php

@@ -395,7 +395,7 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO {
     return idx($default_branches, $this->getVersionControlSystem());
   }
 
-  public function shouldTrackBranch($branch) {
+  private function isBranchInFilter($branch, $filter_key) {
     $vcs = $this->getVersionControlSystem();
 
     $is_git = ($vcs == PhabricatorRepositoryType::REPOSITORY_TYPE_GIT);
@@ -403,16 +403,42 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO {
     $use_filter = ($is_git);
 
     if ($use_filter) {
-      $filter = $this->getDetail('branch-filter', array());
+      $filter = $this->getDetail($filter_key, array());
-      if ($filter && !isset($filter[$branch])) {
+      if ($filter && empty($filter[$branch])) {
         return false;
       }
     }
 
-    // By default, track all branches.
+    // By default, all branches pass.
     return true;
   }
 
+  public function shouldTrackBranch($branch) {
+    return $this->isBranchInFilter($branch, 'branch-filter');
+  }
+
+  public function shouldAutocloseBranch($branch) {
+    return $this->isBranchInFilter($branch, 'close-commits-filter');
+  }
+
+  public function shouldAutocloseCommit(
+    PhabricatorRepositoryCommit $commit,
+    PhabricatorRepositoryCommitData $data) {
+
+    if ($this->getDetail('disable-autoclose', false)) {
+      return false;
+    }
+
+    $branches = $data->getCommitDetail('seenOnBranches', array());
+    foreach ($branches as $branch) {
+      if ($this->shouldAutocloseBranch($branch)) {
+        return true;
+      }
+    }
+
+    return false;
+  }
+
   public function formatCommitName($commit_identifier) {
     $vcs = $this->getVersionControlSystem();
 

src/applications/repository/storage/PhabricatorRepositoryCommitData.php

@@ -21,8 +21,8 @@ final class PhabricatorRepositoryCommitData extends PhabricatorRepositoryDAO {
   const SUMMARY_MAX_LENGTH = 100;
 
   protected $commitID;
-  protected $authorName;
+  protected $authorName    = '';
-  protected $commitMessage;
+  protected $commitMessage = '';
   protected $commitDetails = array();
 
   public function getConfiguration() {

src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php

@@ -116,7 +116,7 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker
 
         $status_closed = ArcanistDifferentialRevisionStatus::CLOSED;
         $should_close = ($revision->getStatus() != $status_closed) &&
-                        (!$repository->getDetail('disable-autoclose', false));
+                        $repository->shouldAutocloseCommit($commit, $data);
 
         if ($should_close) {
           $revision->setDateCommitted($commit->getEpoch());