Phabricator workflow vs. GitHub

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

Phabricator workflow vs. GitHub

Simon Marlow-7
Here's an interesting blog post relevant to previous discussions about Phabricator / GitHub: https://jg.gg/2018/09/29/stacked-diffs-versus-pull-requests/?fbclid=IwAR3JyQP5uCn6ENiHOTWd41y5D-U0_CCJ55_23nzKeUYTjgLASHu2dq5QCc0

Yes it's a decidedly pro-Phabricator rant, but it does go into a lot of details about why the Phabricator workflow is productive, and might be useful to those who struggle to get to grips with it coming from GitHub.

Cheers
Simon

_______________________________________________
ghc-devs mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs
Reply | Threaded
Open this post in threaded view
|

Re: Phabricator workflow vs. GitHub

Edward Z. Yang
Stacked diffs are so useful that I have literally spent several days
building tooling so that I can write stacked diffs and then ship
them to GitHub (where the project lives).  It's just that good.

Edward

Excerpts from Simon Marlow's message of 2018-10-03 19:32:40 +0100:

> Here's an interesting blog post relevant to previous discussions about
> Phabricator / GitHub:
> https://jg.gg/2018/09/29/stacked-diffs-versus-pull-requests/?fbclid=IwAR3JyQP5uCn6ENiHOTWd41y5D-U0_CCJ55_23nzKeUYTjgLASHu2dq5QCc0
>
> Yes it's a decidedly pro-Phabricator rant, but it does go into a lot of
> details about why the Phabricator workflow is productive, and might be
> useful to those who struggle to get to grips with it coming from GitHub.
>
> Cheers
> Simon
_______________________________________________
ghc-devs mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs
Reply | Threaded
Open this post in threaded view
|

Re: Phabricator workflow vs. GitHub

Niklas Hambüchen
There are some things in these argumentations that I don't get.

When you have a stack of commits on top of master, like:

* C
|
* B
|
* A
|
* master

What do you use as base for `arc diff` for each of them?

If B depends on A (the patch expressed by B doesn't apply if A was applied first),
do you still use master as a base for B, or do you use Phabricator's feature to have diffs depend on other diffs?
_______________________________________________
ghc-devs mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs
Reply | Threaded
Open this post in threaded view
|

Re: Phabricator workflow vs. GitHub

Simon Marlow-7
I think the article is assuming the base for `arc diff` is always the parent revision, i.e. `arc diff HEAD^`, which is how the workflow works best. Strangely I don't think the open source Phabricator is set up to do this by default so you have to actually type `arc diff HEAD^` (there's probably some setting somewhere so that you can make this the default).

On the diff in Phabricator you can enter the dependencies manually. Really the tooling ought to do this for you (and at Facebook our internal tooling does do this) but for now manually specifying the dependencies is not terrible. Then Phabricator shows you the nice dependency tree in the UI, so you can see the state of all of your diffs in the stack.

Cheers
Simon

On Fri, 5 Oct 2018 at 04:30, Niklas Hambüchen <[hidden email]> wrote:
There are some things in these argumentations that I don't get.

When you have a stack of commits on top of master, like:

* C
|
* B
|
* A
|
* master

What do you use as base for `arc diff` for each of them?

If B depends on A (the patch expressed by B doesn't apply if A was applied first),
do you still use master as a base for B, or do you use Phabricator's feature to have diffs depend on other diffs?

_______________________________________________
ghc-devs mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs
Reply | Threaded
Open this post in threaded view
|

Re: Phabricator workflow vs. GitHub

Niklas Hambüchen
> I think the article is assuming the base for `arc diff` is always the parent revision, i.e. `arc diff HEAD^`, which is how the workflow works best. Strangely I don't think the open source Phabricator is set up to do this by default so you have to actually type `arc diff HEAD^`

Perhaps that is exactly to address the problem in my example:
If you submit a patch B that depends on A, by default this patch will fail to apply against master on the Phabricator side unless you manually set up dependencies? I suppose this is why it defaults to submitting the whole master-A-B history instead?

> for now manually specifying the dependencies is not terrible.

I have found it pretty terrible:
Setting up dependencies between commits by hand is time consuming, and you can do it wrong, which easily leads to confusion.

If I do 4 refactor commits and on top a new feature that needs them, why should I have to manually click together the dependencies between those commits? The whole point of git is that it tracks that already for me in its DAG.

It gets worse if I have to react to review feedback:

Say Ben tells me in review that I should really squash commits 2 and 3 because they don't work independent of each other. Easily done with `git rebase -i` as suggested, but now I have to go and reflect what I just did in version control by manual clicking in an external tool again (and I better kick out the right Diff).

Similarly, if want to rename all occurrences of my_var to myVar across my 5 commits using rebase -i, I have to manually invoke the right arc invocation after each commit.

So I've found it a big pain to maintain a series of dependent commits with this workflow.

I can imagine this to be only painless if you have access to the tooling you said you have at facebook, that automates these things for you.

In my ideal world, it should work like this:

* Locally, a series of dependent patches goes into a git branch.
* Branches that are dependent on each other are based on each other.
* You have a tool that, if you amend a commit in a branch, can rebase all the dependent branches accordingly.
* You can tell `arc` to submit a whole branch, and it will automatically upload all dependent branches and set up the Phabricator dependency relationships for you.
* When you react to review feedback, you change your history locally, and run an `arc upload-changes`, that automatically updates all Diffs accordingly.

Niklas
_______________________________________________
ghc-devs mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs
Reply | Threaded
Open this post in threaded view
|

Re: Phabricator workflow vs. GitHub

Simon Marlow-7
On Fri, 5 Oct 2018 at 15:22, Niklas Hambüchen <[hidden email]> wrote:
> I think the article is assuming the base for `arc diff` is always the parent revision, i.e. `arc diff HEAD^`, which is how the workflow works best. Strangely I don't think the open source Phabricator is set up to do this by default so you have to actually type `arc diff HEAD^`

Perhaps that is exactly to address the problem in my example:
If you submit a patch B that depends on A, by default this patch will fail to apply against master on the Phabricator side unless you manually set up dependencies? I suppose this is why it defaults to submitting the whole master-A-B history instead?

> for now manually specifying the dependencies is not terrible.

I have found it pretty terrible:
Setting up dependencies between commits by hand is time consuming, and you can do it wrong, which easily leads to confusion.

If I do 4 refactor commits and on top a new feature that needs them, why should I have to manually click together the dependencies between those commits? The whole point of git is that it tracks that already for me in its DAG.

It gets worse if I have to react to review feedback:

Say Ben tells me in review that I should really squash commits 2 and 3 because they don't work independent of each other. Easily done with `git rebase -i` as suggested, but now I have to go and reflect what I just did in version control by manual clicking in an external tool again (and I better kick out the right Diff).

Similarly, if want to rename all occurrences of my_var to myVar across my 5 commits using rebase -i, I have to manually invoke the right arc invocation after each commit.

So I've found it a big pain to maintain a series of dependent commits with this workflow.

I can imagine this to be only painless if you have access to the tooling you said you have at facebook, that automates these things for you.

In fact we did it manually for a long time, the tool support is a recent development. Tool support can always improve things, but I'll take the inconvenience of having to specify dependencies manually in exchange for the other benefits of stacked diffs. You can put the dependencies in the commit log using "Depends on: D1234", as an alternative to the UI.
 
'git rebase -i' with 'x arc diff HEAD^ -m rebase' is a nice trick for rebasing your stack.

Cheers
Simon


In my ideal world, it should work like this:

* Locally, a series of dependent patches goes into a git branch.
* Branches that are dependent on each other are based on each other.
* You have a tool that, if you amend a commit in a branch, can rebase all the dependent branches accordingly.
* You can tell `arc` to submit a whole branch, and it will automatically upload all dependent branches and set up the Phabricator dependency relationships for you.
* When you react to review feedback, you change your history locally, and run an `arc upload-changes`, that automatically updates all Diffs accordingly.

Niklas

_______________________________________________
ghc-devs mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs
Reply | Threaded
Open this post in threaded view
|

Re: Phabricator workflow vs. GitHub

Ben Gamari-2
In reply to this post by Niklas Hambüchen
Niklas Hambüchen <[hidden email]> writes:

..[snip].

>
> So I've found it a big pain to maintain a series of dependent commits with this workflow.
>
> I can imagine this to be only painless if you have access to the tooling you said you have at facebook, that automates these things for you.
>
> In my ideal world, it should work like this:
>
> * Locally, a series of dependent patches goes into a git branch.
> * Branches that are dependent on each other are based on each other.
> * You have a tool that, if you amend a commit in a branch, can rebase all the dependent branches accordingly.
> * You can tell `arc` to submit a whole branch, and it will automatically upload all dependent branches and set up the Phabricator dependency relationships for you.
> * When you react to review feedback, you change your history locally, and run an `arc upload-changes`, that automatically updates all Diffs accordingly.
>
Yes, I agree that this would be ideal. I have spent quite some time
manually updating related differentials in this way.

On the other hand, I still think this manual process is in many ways
better than the typical GitHub model, where lack of any sort of PR
dependency structure to make reviewing larger changes extremely painful.

Cheers,

- Ben

_______________________________________________
ghc-devs mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs

signature.asc (497 bytes) Download Attachment