Page MenuHomePhabricator

In repositories, realign "Track Only", "Autoclose", and "Publish/Notify" toward "Permanent Refs"
Open, NormalPublic

Description

Some time ago, we updated Diffusion to import all refs in Git repositories (tags, branches, and refs like refs/pulls/1 [GitHub] and refs/changes/1 [Gerrit]).

There are some general inconsistencies with our handling of these refs, and with ref handling in general. The major problems are:

Forced Publishing of Non-Branch Refs: At time of writing, in master, commits reachable from non-branch refs are never "Autoclose" (that is, they do not apply Fixes T123 and do not close revisions), but they do always cause publishing effects (email, feed, audits, owners, Herald) unless publishing is completely disabled. This is often (probably "almost always") undesirable, see T11314 for an example. In effectively every case, no one wants refs/pulls/1 to publish, and there's currently no way to prevent this.

Autoclose vs Publish: I believe there is very little reason for the "Autoclose" and "Publish" behaviors to actually be separate. That is, there are very few use cases where users want a commit to trigger only Herald or only feed or only audits. These use cases do exist (see T9210 for an example of "Herald, No Feed") but they're overwhelmingly niche and off-label. In nearly every case, commits are either all-or-nothing and users either want full behavior or no behavior.

Tracked vs Permanent: Currently, installs often use "Track Only" to disable publish behaviors. This also disables importing the commits. This is an undesirable side effect: we generally want these commits to be imported and visible in the web UI (and we want to be able to reference them and do things like create revisions from them in the future), we just don't want to trigger a bunch of behavior after that.


To fix these issues, I'm making these changes:

Merging Autoclose and Publish: All "autoclose" and "publish" behaviors (email, notifications, feed, audits, herald, owners, tasks, revisions) are now under a single umbrella, not governed by separate "Autoclose" and "Publish" configuration. My expectation is that this simplifies configuration at effectively no cost, and that effectively all valid configurations set "Autoclose" and "Publish" to the same value, and that there is essentially no use case for triggering some of these behaviors but not all of them.

If or where use cases do exist (like T9210, or perhaps some stuff we'll discover like "run builds when I push to some weirdly named ref") I suspect we can find better ways to support the workflows.

This also fixes the inconsistent behavior of non-branch refs by disabling both "autoclose" and "publish" behaviors for them.

Renaming "Autoclose" to "Permanent Refs": The existing "Autoclose" setting is now "Permanent Refs". This generally aligns better with the most common use case, which is this:

I have a repository with some "permanent" branches like "master" and "stable", but I also push random branches like "tmp-epriestley-save-123-bugfix" to save my work, share code, etc.

In this case, users generally listed "Autoclose Only" as "master, stable" to disable autoclose on "tmp-epriestley", or "Track Only" as "master, stable" to disable autoclose and publishing.

Broadly, the new behavior is:

  • By default, commits are considered non-permanent and don't trigger ANY autoclose/publishing behavior.
  • Once a commit becomes an ancestor of a permanent ref, it becomes permanent and triggers ALL the behavior.
  • By default, all branches are permanent refs and all other refs are not.

Fetch Only: A small set of cases are best handled by instructing Phabricator to fetch only certain refs from a remote.

Deprecating "Track Only": Now that "Permanent Refs" covers all triggered behavior (and "Fetch Only" exists), there is essentially no reason to use "Track Only", since "Track Only" was primarily (perhaps almost exclusively) used to stop publish behaviors, not to stop import into the web UI. "Track Only" rules can be moved to "Permanent Refs" and/or "Fetch Only" in effectively all cases.


This older variation of the writeup has some bad assumptions, don't trust it! It's all lies!

See PHI1195. See PHI1148. See T11314. Maybe see PHI1067.

See PHI395.

Some time ago, we updated Diffusion to import all refs in Git repositories. This includes tags and arbitrary non-branch/tag refs, most commonly refs/pull/* created by GitHub pull requests and refs/changes/* created by Gerrit. When a repository is used as its own staging area, this also includes refs/tags/phabricator/diff/*.

The "Track Only" and "Autoclose Only" rule specifications were not been updated, and can only apply to branches: refs and tags are always tracked and always autoclose.

This primarily creates problems when these various "staging" refs trigger revision closure, and there's no real workaround for this because "Autoclose Only" can't exclude them.

A minimal approach might be this rule:

  • Only branches can be "Autoclose". [Edit: This was already the behavior. There was confusion over "Autoclose" and "Publish" being separate behaviors.]
  • [Edit: Actual approach is "Only branches can autoclose or publish."]

This probably fixes all the actual issues with no configuration changes. The only thing it might break is intentional use of release tags with no corresponding branches, but I believe this is rare or nonexistent.

A slightly less-minimal approach would be:

  • Change the "Autoclose Only" rule list to allow specification of refs in the general case. The current ruleset master becomes branch(master) or similar, and so on.

This isn't terribly bad, but somewhat complex on our side and requires a somewhat-complex migration, and then installs with hundreds of repositories maybe need to go script an API call to update things or something, and we probably want to change the default behavior to "only branches can autoclose" anyway so this is just as breaking as the first change.

I'd generally like to move toward a world where "Track Only" is removed. The only "legitimate" use case I know of for "Track Only" (in a world where "Autoclose Only" works properly) is limiting what is fetched from an observed remote with tens of thousands of branches, primarily for performance reasons. This would be better implemented as "Fetch Only" with an explicit ref listing, versus "Track Only" with a complex pattern list. However, removing "Track Only" will require some configuration changes for most installs.

Entangled here to some degree are T8093 (virtualizing Git repository refs), arc save, and the many open requests to make Staging refs actually work properly (see T13278). Hypothetically, it would be nice to do most of these changes in a single release so we can say "a bunch of ref stuff has changed, here's the complete new behavior so you can update your stuff exactly once" rather than stringing it out over multiple releases.


Possibly see also T8936.

Revisions and Commits

rP Phabricator
Closed
D20493
D20467
D20466
D20465
D20464
D20434
D20433
D20432
D20428
D20427
D20426
D20425
D20424
D20423
D20422
D20421
D20419
D20420

Related Objects

Event Timeline

epriestley triaged this task as Normal priority.Apr 6 2019, 4:52 PM
epriestley created this task.
This comment was removed by epriestley.

Only branches can be "Autoclose".

This is already supposedly the behavior since D16129:

public function shouldAutocloseRef(DiffusionRepositoryRef $ref) {
  if (!$ref->isBranch()) {
    return false;
  }

...

...so either that isn't working or a bunch of users have been making up a lot of bug reports for a long time now.

Examining "master" (branch) at "02f94cd7d2885de502d03934af3a0c24453e8e58" (autoclose: Y).
Skipping, HEAD is known.
Examining "stable" (branch) at "d3df0a49f92a8be2f5b8a030af2f8fa5471cc45d" (autoclose: Y).
Skipping, HEAD is known.
Examining "refs/pull/846/merge" (ref) at "d816dc011f935fdda9468c905e4af4c7fc73b322" (autoclose: N).
Skipping, HEAD is known.
Examining "refs/pull/839/head" (ref) at "d3df0a49f92a8be2f5b8a030af2f8fa5471cc45d" (autoclose: N).
Skipping, HEAD is known.
Examining "refs/pull/846/head" (ref) at "1263f335abfc5bcf7b5e2c60ae0632b4660815d8" (autoclose: N).
Skipping, HEAD is known.
< ... >
< ... many many non-branch refs properly identified as non-autoclosing ... >
< ... >
Examining "refs/pull/4/head" (ref) at "e282a7707da11e3e2bca2b3a41925aeefd4f22c9" (autoclose: N).
Skipping, HEAD is known.

Okay. I'm not crazy, I'm just really bad at reading.

T11314 is only complaining about publishing, not autoclosing.

PHI1195 is complaining about pushing to "temporary branches", i.e. a case already handled by "Autoclose Only".

PHI1148 wants "Autoclose" on tags, even if those commits are not reachable from any branch head.

I think the actual fix here is this:

  • Commits that do not autoclose also not publish.

And that's probably like one line of code.

epriestley renamed this task from Update Diffusion "Track Only" for all ref types to In repositories, realign "Track Only", "Autoclose" and "Publish/Notify" toward "Permanent Refs".Apr 16 2019, 3:03 PM
epriestley updated the task description. (Show Details)
epriestley renamed this task from In repositories, realign "Track Only", "Autoclose" and "Publish/Notify" toward "Permanent Refs" to In repositories, realign "Track Only", "Autoclose", and "Publish/Notify" toward "Permanent Refs".Apr 16 2019, 3:06 PM

I believe there is very little reason for the "Autoclose" and "Publish" behaviors to actually be separate. That is, there are very few use cases where users want a commit to trigger only Herald or only feed or only audits. These use cases do exist (see T9210 for an example of "Herald, No Feed") but they're overwhelmingly niche and off-label.

FWIW, I actually want this behaviour. Specifically, I want to be able to trigger Herald rules on pushes to non-tracked branches, but not generate notifications or trigger autoclose behaviour. The reason for this is to be able to trigger a Harbormaster build on these non-tracked branches.