Do not publish/notify about commits which are not reachable from any "Autoclose" ref
ClosedPublic
Actions

Authored by epriestley on Apr 14 2019, 6:39 PM.

Details

Reviewers

Maniphest Tasks

T11314: Observe Mode + GitHub has weird mail interactions due to pull request refs
T13277: In repositories, realign "Track Only", "Autoclose", and "Publish/Notify" toward "Permanent Refs"

Commits

rP1cda1402c7c2: Do not publish/notify about commits which are not reachable from any…

Summary

Depends on D20418. Ref T13277. Fixes T11314.

Currently, when you push commits to some arbitrary ref or tag (like refs/pull/123 on GitHub, refs/tags/phabricator/diff/123 on Phabricator, or refs/changes/whatever on Gerrit), we do not "autoclose" related objects. This means that we don't process Ref T123 to create links to tasks, and don't process Differential Revision: xyz to close revisions.

However, we do still publish these commits. "Publish" means: trigger audits, publish feed stories, and run Herald rules.

Stop publishing these commits.
In the UI, show these commits as "Not Permanent" with a note that they are "Not [on any permanent branch]."

These commits will publish and autoclose if they ever become reachable from an "autoclose" ref (most commonly, if they are later merged to "master").

Test Plan

Pushed a commit to refs/tags/quack.
Before: got a feed story.
After: no feed story, UI shows commit as "Not Permanent".

Diff Detail

Repository

rP Phabricator

Branch

autoclose3

Lint

Lint Passed

Unit

Tests Passed

Build Status

Buildable 22591
Build 30947: Run Core Tests
Build 30946: arc lint + arc unit

Event Timeline

epriestley created this revision.Apr 14 2019, 6:39 PM

Harbormaster completed remote builds in B22590: Diff 48720.Apr 14 2019, 6:40 PM

epriestley requested review of this revision.Apr 14 2019, 6:40 PM

epriestley mentioned this in T11314: Observe Mode + GitHub has weird mail interactions due to pull request refs.Apr 14 2019, 6:46 PM

Slightly smaller/more accurate change: this does not affect "Push" rules, which still act on every commit.

Harbormaster completed remote builds in B22591: Diff 48721.Apr 14 2019, 6:50 PM

epriestley added a child revision: D20420: Accept pushes with arbitrary Git refs.Apr 14 2019, 6:57 PM

amckinley accepted this revision.Apr 17 2019, 6:38 PM

amckinley added inline comments.

src/applications/diffusion/controller/DiffusionCommitController.php
252	"Permanent" feels like a weird way to describe this, but I don't have any obviously-better ideas. "Non-virtual"?
src/applications/repository/storage/PhabricatorRepository.php
1059–1061	Maybe expand `PhabricatorRepository->shouldPublish()` to add an "Enable notifications, feed, and Herald, even for virtual branches" option to the "publish/notify" setting? It's probably not what users want to have a commit that `Fixes T4444` actually close the task until their commit lands someone non-virtual, but I could imagine someone wanting to run Herald rules on it or something.

This revision is now accepted and ready to land.Apr 17 2019, 6:38 PM

I'm likely going to do another pass here and change the user-facing description of these commits to "Unpublished" rather than "Not Permanent".

None of these terms feel great to me, but I haven't managed to come up with anything better. "Virtual" isn't awful but I worry it risks confusion with the emerging concept of "virtual refs", where "virtual" seems like a clearer term.

src/applications/repository/storage/PhabricatorRepository.php
1059–1061	If anyone does want this, I'm planning to add `tag(...)` / `ref(...)` rules to "Permanent Refs". Then you could get this behavior ("always publish absolutely everything, no matter what") with `ref-regexp(.*)` or similar, you'd just have to opt into it and explicitly make more stuff permanent than is defualt. (But AFAIK there are no outstanding requests for this other than PHI1148, which is still a little murky.)