Page MenuHomePhabricator

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

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

Details

Summary

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

Test Plan

N/A

Diff Detail

Event Timeline

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)
$ 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: 
remote: Success! Created a new diff fom your changes:
remote: 
remote:     http://local.aphront.com:8080/differential/diff/588/
remote: 
remote: Since this was a push for review, your changes will not be
remote: applied to the remote. This push will now be rejected.
remote: 
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/'
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]
To http://127.0.0.1:3000/diffusion/FUN/funtudy.git
 ! [remote rejected] HEAD -> review (pre-receive hook declined)
error: failed to push some refs to 'http://127.0.0.1:3000/diffusion/FUN/funtudy.git'
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

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

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

20after4 added inline comments.
src/applications/diffusion/engine/DiffusionCommitHookEngine.php
722

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 added a subscriber: ariel.
EddieGP added inline comments.
src/applications/diffusion/engine/DiffusionCommitHookEngine.php
147

"fom": typo