Page MenuHomePhabricator

[Draft] Make pushes to magical branches create diffs
Needs RevisionPublic

Authored by epriestley on Jun 17 2014, 2:38 PM.



Ref T5000. Works fine, but totally unusuable in present form.

Test Plan


Diff Detail

Event Timeline

epriestley updated this revision to Diff 22995.Jun 17 2014, 2:38 PM
epriestley retitled this revision from to [Draft] Make pushes to magical branches create diffs.
epriestley updated this object.
epriestley edited the test plan for this revision. (Show Details)
epriestley updated this object.Jun 17 2014, 2:39 PM
$ git push origin HEAD:review
Counting objects: 22, done.
Delta compression using up to 8 threads.
Compressing objects: 100% (3/3), done.
Writing objects: 100% (3/3), 388 bytes | 0 bytes/s, done.
Total 3 (delta 2), reused 0 (delta 0)
remote: Success! Created a new diff fom your changes:
remote: Since this was a push for review, your changes will not be
remote: applied to the remote. This push will now be rejected.
To ssh://dweller@localhost/diffusion/POEMS/
 ! [remote rejected] HEAD -> review (pre-receive hook declined)
error: failed to push some refs to 'ssh://dweller@localhost/diffusion/POEMS/'
Pas added a subscriber: Pas.Dec 16 2014, 5:01 PM
dagar added a subscriber: dagar.Apr 22 2015, 6:33 PM
eadler added a subscriber: eadler.Apr 30 2015, 4:12 AM
smcv added a subscriber: smcv.Jun 24 2015, 11:34 AM
revi added a subscriber: revi.Jun 24 2015, 2:25 PM
Luke081515.2 accepted this revision.Oct 26 2015, 10:52 AM
Luke081515.2 added a reviewer: Luke081515.2.
This revision is now accepted and ready to land.Oct 26 2015, 10:52 AM

After I apply this patch on the latest codebase, it raises an error.

$ git push origin HEAD:review
Counting objects: 3, done.
Delta compression using up to 4 threads.
Compressing objects: 100% (3/3), done.
Writing objects: 100% (3/3), 301 bytes | 0 bytes/s, done.
Total 3 (delta 2), reused 0 (delta 0)
remote: [2016-01-10 05:55:15] EXCEPTION: (TypeError) Argument 1 passed to DifferentialDiff::newFromRawChanges() must be an instance of PhabricatorUser, array given, called in /Users/Leedy/Developer/github/phacility/phabricator/src/applications/diffusion/engine/DiffusionCommitHookEngine.php on line 747 at [<phabricator>/src/applications/differential/storage/DifferentialDiff.php:140]
remote: arcanist(head=master, ref.master=aeb374b33348), phabricator(head=master, ref.master=efba69a44022, custom=2), phutil(head=master, ref.master=adb8a9c074ba)
remote:   #0 DifferentialDiff::newFromRawChanges(array) called at [<phabricator>/src/applications/diffusion/engine/DiffusionCommitHookEngine.php:747]
remote:   #1 DiffusionCommitHookEngine::generateGitDiffForReview(PhabricatorRepositoryPushLog) called at [<phabricator>/src/applications/diffusion/engine/DiffusionCommitHookEngine.php:327]
remote:   #2 DiffusionCommitHookEngine::generateDiffForReview(array) called at [<phabricator>/src/applications/diffusion/engine/DiffusionCommitHookEngine.php:131]
remote:   #3 DiffusionCommitHookEngine::execute() called at [<phabricator>/scripts/repository/commit_hook.php:133]
 ! [remote rejected] HEAD -> review (pre-receive hook declined)
error: failed to push some refs to ''
chad requested changes to this revision.Jan 10 2016, 6:13 AM
chad added a reviewer: chad.
chad added a subscriber: chad.

This patch is 18 months old.

This revision now requires changes to proceed.Jan 10 2016, 6:13 AM
Treri added a subscriber: Treri.Jan 12 2016, 4:38 AM
Treri added a comment.EditedJan 12 2016, 5:05 AM

Like this feature. Any more process on this?

Or can phabricator server-side auto-merge like gerrit?

So I can just

git push origin HEAD:review

then if the Diff is accepted, it will auto-merged into repo. I have no need to merge and push it myself

chad added a comment.Jan 12 2016, 5:11 AM

Land from Differential is T182 and works today as a v0 type product. Code review from plain git:branch is T5000.

techdragon added a subscriber: techdragon.
ariel added a subscriber: ariel.Mar 9 2016, 9:17 AM
20after4 added inline comments.

This applies cleanly to a recent version of stable, however, there is the bug mentioned above by @liudangyi. But that one is easy enough to resolve:

return DifferentialDiff::newFromRawChanges($this->getViewer(), $changes);

I wonder if there is a way to maintain this as some sort of extension if the upstream is not interested in merging it?

You could implement it as a Git pre-receive hook, I think, but that might be a fair amount of work.

There aren't really any reasonable extension points on this pathway.

We're inching closer to proxying the Git protocol and being able to bring this upstream, but it's still probably not going to happen for a while. (And this has way too many rough edges for me to be comfortable bringing it upstream without a lot more work.)

ariel removed a subscriber: ariel.Mar 11 2016, 8:28 AM
ariel added a subscriber: ariel.
tsy added a subscriber: tsy.Apr 19 2016, 3:32 PM
J5lx added a subscriber: J5lx.Feb 15 2017, 8:38 PM
EddieGP added a subscriber: EddieGP.Apr 1 2017, 6:38 PM
EddieGP added inline comments.

"fom": typo

mves added a subscriber: mves.Oct 11 2017, 9:27 AM