Page MenuHomePhabricator

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

Authored by epriestley on Apr 14 2019, 6:39 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Mar 16, 11:04 AM
Unknown Object (File)
Sun, Mar 10, 10:28 AM
Unknown Object (File)
Sun, Mar 10, 3:23 AM
Unknown Object (File)
Fri, Mar 1, 5:05 AM
Unknown Object (File)
Fri, Mar 1, 5:04 AM
Unknown Object (File)
Fri, Mar 1, 5:04 AM
Unknown Object (File)
Fri, Mar 1, 5:04 AM
Unknown Object (File)
Feb 21 2024, 1:16 PM
Subscribers
None

Details

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
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

  • Slightly smaller/more accurate change: this does not affect "Push" rules, which still act on every commit.
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.)

This revision was automatically updated to reflect the committed changes.