Is there a way to prevent AutoClose of Revisions

#1

The recent publish change removed the ability to not have a commit not AutoClose a review

If you run a system which allows post commit reviews, the revision now automatically closes it before its been manually accepted.

If we turn off publishing, then I believe herald rules wouldn’t be run on commit.

Is there a way to turn off AutoClose?

#2

If we turn off publishing, then I believe herald rules wouldn’t be run on commit.

Correct. There is no way to prevent commits from closing associated revisions without also preventing Herald from running.

#3

The expectation is that this option is rarely useful

I understand the change, but In my view i disagree with the assessment that this was rarely useful, its is extremely useful to be able to have herald run on commit but the review not closed.

This hit my organization within hours of them arriving at work after the weekend upgrade.

The team I represent uses an asynchronous code review approach, because our teams are spread around the world its often you will add a reviewer who is currently asleep and may be a sleep almost your entire working day. We don’t want to allow that delay of waiting for that reviewer to wake to block forward progress by an individual.

To accommodate this we allow people to create the review and if they feel confident, not wait for the review process to complete before committing. This follows the model of continuous integration, lets the developer move on with the next stage. The developer know if the reviewer rejects or requests a change they may have to go back in and make a subsequent change, we accept the extra commit as being the price of allowing the developer to continue to be productive and not blocked. We don’t want to allow users to make any changes without reviews so even small trivial changes (fixing tests,fixing cross platform builds) go through this same review process.

These commits cause information to appear in the activity feed, where its seen by other developers who maybe not on the original reviewer list, this lets them chime in on the review process.

Of course each commit, uses herald to connect to the CI system have have builds and unit tests run, static_analysis etc… which in themselves could add comments to the review associated with the commit when that stage completes.

The “Code review conversation” doesn’t stop just because someone has committed. Just because you get one accept and commit it doesn’t means its the end of the process (especially if its your buddy! giving you the LGTM)

Hence why we always ran with AutoClose always turned off… you close the review when your done, and done is not necessarily commit.

At it now stands as soon as they commit, it will close the review, thus causing it to disappear from the reviewers list. This not only breaks our desire to have everything reviewed either pre or post commit it doesn’t let us see that its not been reviewed. (I don’t think its creating a audit when the review isn’t accepted at commit time, which is what would happen if someone created a commit without a review)

Sorry for the long reply, I just wanted to highlight the use case for AutoClose being allowed to be turned off, it may be we’ll be the only one this impacts.

Its definitely not ideal to no longer have this as an option, but in case others suffer the same, this was the smallest change I could make in my local instance to turn off the auto-closing of reviews

As I said, far from ideal, but I had to make a change in production as I had dozens of developers contacting me asking what happened to their reviews…

diff --git a/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php b/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php
index c7f00df..4631311 100644
--- a/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php
+++ b/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php
@@ -164,7 +164,8 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker
     $publisher = $repository->newPublisher();
     if ($publisher->shouldPublishCommit($commit)) {
       $actor = PhabricatorUser::getOmnipotentUser();
-      $this->closeRevisions($actor, $ref, $commit, $data);
+      // remove for now
+      // $this->closeRevisions($actor, $ref, $commit, $data);
       $this->closeTasks($actor, $ref, $commit, $data);
     } else {
       $hold_reasons = $publisher->getCommitHoldReasons($commit);