Page MenuHomePhabricator
Differential D9599

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

Authored by epriestley on Jun 17 2014, 2:38 PM.
Tags
None
Referenced Files
F14397760: D9599.diff
Sun, Dec 22, 12:02 PM
Unknown Object (File)
Sun, Dec 15, 4:47 PM
Unknown Object (File)
Sat, Dec 14, 7:28 AM
Unknown Object (File)
Fri, Dec 13, 6:24 PM
Unknown Object (File)
Wed, Dec 11, 12:45 PM
Unknown Object (File)
Sun, Dec 8, 11:37 AM
Unknown Object (File)
Sat, Nov 23, 5:16 AM
Unknown Object (File)
Sat, Nov 23, 5:05 AM
View All Files
Subscribers
02JanDal
20after4
ariel
benstiglitz
bgamari
chad
dagar
View All 32 Subscribers
Tokens
"Mountain of Wealth" token, awarded by techdragon.

Details

Reviewers
chad
Maniphest Tasks
T5000: Using Differential with plain Git, without requiring Arc
Summary

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

Test Plan

N/A

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)
Herald added a subscriber: epriestley. · View Herald TranscriptJun 17 2014, 2:38 PM
epriestley updated this object.Jun 17 2014, 2:39 PM
epriestley added a task: T5000: Using Differential with plain Git, without requiring Arc.
$ 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/'
Harbormaster completed remote builds in B1173: Diff 22995.Jun 17 2014, 2:39 PM
YenaHong added a subscriber: YenaHong.Jun 17 2014, 2:45 PM
sophiebits added a subscriber: sophiebits.Jun 17 2014, 7:02 PM
joshuaspence added a subscriber: joshuaspence.Jul 7 2014, 11:52 AM
techdragon mentioned this in T5000: Using Differential with plain Git, without requiring Arc.Sep 11 2014, 6:55 AM
sergey.vfx added a subscriber: sergey.vfx.Sep 18 2014, 11:36 AM
lfaraone added a subscriber: lfaraone.Nov 25 2014, 9:39 AM
Pas added a subscriber: Pas.Dec 16 2014, 5:01 PM
02JanDal added a subscriber: 02JanDal.Jan 13 2015, 10:54 PM
jpoehls added a subscriber: jpoehls.Apr 15 2015, 1:41 AM
dagar added a subscriber: dagar.Apr 22 2015, 6:33 PM
• rjmackay added a subscriber: • rjmackay.Apr 29 2015, 9:49 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
Luke081515.2 mentioned this in Z1336: General Chat.Oct 29 2015, 6:45 PM
benstiglitz added a subscriber: benstiglitz.Dec 15 2015, 10:29 PM
pritambaral added a subscriber: pritambaral.Dec 26 2015, 5:26 PM
• dozniak added a subscriber: • dozniak.Jan 5 2016, 11:21 AM
liudangyi added a subscriber: liudangyi.Jan 10 2016, 5:58 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
chad removed a reviewer: Luke081515.2.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 awarded a token.Jan 24 2016, 1:24 PM
techdragon added a subscriber: techdragon.
greggrossmeier added a subscriber: greggrossmeier.Mar 9 2016, 1:16 AM
• paladox added a subscriber: • paladox.Mar 9 2016, 8:42 AM
ariel added a subscriber: ariel.Mar 9 2016, 9:17 AM
20after4 added a subscriber: 20after4.Mar 11 2016, 12:54 AM
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?

epriestley added a comment.Mar 11 2016, 1:47 AM

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
bgamari added a subscriber: bgamari.May 12 2016, 5:37 PM
J5lx added a subscriber: J5lx.Feb 15 2017, 8:38 PM
• pasik added a subscriber: • pasik.Feb 17 2017, 12:51 PM
EddieGP added a subscriber: EddieGP.Apr 1 2017, 6:38 PM
EddieGP added inline comments.
src/applications/diffusion/engine/DiffusionCommitHookEngine.php
147

"fom": typo

mves added a subscriber: mves.Oct 11 2017, 9:27 AM
sascha-egerer added a subscriber: sascha-egerer.Sep 24 2018, 11:17 AM

Revision Contents
Changeset List

PathSize
src/
applications/
diffusion/
engine/
142 lines

Diff 22995

View Options

src/applications/diffusion/engine/DiffusionCommitHookEngine.php

Loading...
Phabricator · Built by Phacility