Accept pushes with arbitrary Git refs
ClosedPublic
Actions

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

Details

Reviewers

Maniphest Tasks

T12300: git push --mirror --force does not work on Phacility with repo that has refs/pull
T9383: Support `git notes`
T8936: Unable to identify the reftype of 'refs/remotes/origin/master'. Rejecting push.
T13277: In repositories, realign "Track Only", "Autoclose", and "Publish/Notify" toward "Permanent Refs"

Commits

rP23b86bae6c61: Accept pushes with arbitrary Git refs

Summary

Depends on D20419. Ref T13277. Fixes T8936. Fixes T9383. Fixes T12300. When you push arbitrary refs to Phabricator, the push log currently complains if those refs are not tags or branches.

Upstream Git now features "notes", and there's no reason to prevent writes to arbitrary refs, particularly beause we plan to start using them soon (see T13278).

Allow these writes as affecting raw refs.

Test Plan

Pushed an arbitrary ref.
Pushed some Git notes.
Wrote a Herald ref rule, saw "ref" in the dropdown.

Diff Detail

Repository

rP Phabricator

Branch

autoclose4

Lint

Lint Passed

Unit

Tests Passed

Build Status

Buildable 22592
Build 30949: Run Core Tests
Build 30948: arc lint + arc unit

Event Timeline

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

Harbormaster completed remote builds in B22592: Diff 48722.Apr 14 2019, 6:58 PM

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

epriestley added a child revision: D20421: When observing a repository in Git, just "fetch <url>" without worrying about the "origin" remote.Apr 14 2019, 7:24 PM

epriestley edited the summary of this revision. (Show Details)Apr 15 2019, 4:10 PM

epriestley added a task: T9383: Support `git notes`.

epriestley mentioned this in T9383: Support `git notes`.Apr 15 2019, 5:14 PM

epriestley mentioned this in T8936: Unable to identify the reftype of 'refs/remotes/origin/master'. Rejecting push..

epriestley mentioned this in T12300: git push --mirror --force does not work on Phacility with repo that has refs/pull.Apr 15 2019, 7:55 PM

epriestley edited the summary of this revision. (Show Details)

epriestley added a task: T12300: git push --mirror --force does not work on Phacility with repo that has refs/pull.

epriestley edited the test plan for this revision. (Show Details)

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

amckinley added inline comments.

src/applications/diffusion/engine/DiffusionCommitHookEngine.php
462	So `[branch, tag, ref]` is exhaustive? And we can't regex on it?

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

epriestley added inline comments.Apr 17 2019, 7:01 PM

src/applications/diffusion/engine/DiffusionCommitHookEngine.php
462	Everything is a ref, named `refs/something/something`. I'm pretty sure you can't push anywhere else -- if you try, Git assumes you're pushing to a branch: $ git push origin HEAD:not-refs/blah/blah ... $ git ls-remote origin ... eae726bc814e71b61c3f58c891866647ad6a649f refs/heads/not-refs/blah/blah ... Refs beginning with `refs/heads/` are branches. Refs beginning with `refs/tags/` are tags. This is not exhaustive. Git supports notes today (in `refs/notes/`) and various other systems define things that we could arguably say aren't "just" refs, like `refs/pull`. All of these are refs, they just may also be some other kind of more convenient/common object that users are more familiar with. The REFTYPE stuff is just for convenience -- it only affects these things: How the ref is shown in the push logs. The Herald "Ref Type" rule. We could call everything a "ref", this would just be a little less nice since users would need to interact with "refs/heads/master" instead of "master". We could also go crazy and label every kind of magic ref we can find in the wild (and it might be reasonable to do this at some point). I'm not sure if they're all `refs/<x>/` or not. We may being introducing `refs/virtual/staging/...` soon, and in some future world could label this as a "Staging Base Commit" or something, I guess?