Let's talk about arc: Phabricator replacement for git-review and more

classic Classic list List threaded Threaded
14 messages Options
Reply | Threaded
Open this post in threaded view
|

Let's talk about arc: Phabricator replacement for git-review and more

Marcin Cieslak-3
I just wrote a very rough and quick walkthrough how to get that tool running:

https://www.mediawiki.org/wiki/Arcanist

My first impression is very good. The UI is very nice (it guides you
when it needs to, it just does the job if all is fine).

The user's guide is unfortunately poor.

I don't know yet how to avoid this warning:

This diff is for the 'E3Experiments' project but the working copy belongs to the '' project.

I see that arc can be also installed as the git pre-receive hook,
but it needs some project configuration for that. Interesting.

Anyway, I managed to download one change:

$ git branch -vv
* arcpatch-D3 be7cfd9 Merge branch 'master' of ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/E3Experiments into munaf/pef2
  master      e40fce0 [origin/master] Rename events.js -> communityClicks.js

//Saper


_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Let's talk about arc: Phabricator replacement for git-review and more

Evan Priestley
I sent out a diff to fix the error message (https://secure.phabricator.com/D3231), the new one reads:

    This patch is for the 'phabricator' project, but the working copy does
    not have an '.arcconfig' file to identify which project it belongs to.
    Still try to apply the patch?

We're trying to catch the case where you're attempting to apply a patch to the wrong project -- I'm guessing revision D3 was made in a working copy with a .gitignored ".arcconfig" file that associates it with "E3Experiments". If you check in the ".arcconfig" file, arc will be able to recognize the working copy's project and will stop complaining.

What could we improve about the user guide?

Evan

On Aug 9, 2012, at 6:15 PM, Marcin Cieslak wrote:

> I just wrote a very rough and quick walkthrough how to get that tool running:
>
> https://www.mediawiki.org/wiki/Arcanist
>
> My first impression is very good. The UI is very nice (it guides you
> when it needs to, it just does the job if all is fine).
>
> The user's guide is unfortunately poor.
>
> I don't know yet how to avoid this warning:
>
> This diff is for the 'E3Experiments' project but the working copy belongs to the '' project.
>
> I see that arc can be also installed as the git pre-receive hook,
> but it needs some project configuration for that. Interesting.
>
> Anyway, I managed to download one change:
>
> $ git branch -vv
> * arcpatch-D3 be7cfd9 Merge branch 'master' of ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/E3Experiments into munaf/pef2
>  master      e40fce0 [origin/master] Rename events.js -> communityClicks.js
>
> //Saper
>
>
> _______________________________________________
> Wikitech-l mailing list
> [hidden email]
> https://lists.wikimedia.org/mailman/listinfo/wikitech-l


_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Let's talk about arc: Phabricator replacement for git-review and more

Daniel Friesen-4
In reply to this post by Marcin Cieslak-3
On Thu, 09 Aug 2012 18:15:45 -0700, Marcin Cieslak <[hidden email]>  
wrote:

> I just wrote a very rough and quick walkthrough how to get that tool  
> running:
>
> https://www.mediawiki.org/wiki/Arcanist
>
> My first impression is very good. The UI is very nice (it guides you
> when it needs to, it just does the job if all is fine).
>
> The user's guide is unfortunately poor.
>
> I don't know yet how to avoid this warning:
>
> This diff is for the 'E3Experiments' project but the working copy  
> belongs to the '' project.
>
> I see that arc can be also installed as the git pre-receive hook,
> but it needs some project configuration for that. Interesting.
>
> Anyway, I managed to download one change:
>
> $ git branch -vv
> * arcpatch-D3 be7cfd9 Merge branch 'master' of  
> ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/E3Experiments into  
> munaf/pef2
>   master      e40fce0 [origin/master] Rename events.js ->  
> communityClicks.js
>
> //Saper

Why is arc written in PHP? That seems to be a horrible software decision  
to me. Core extensions not enabled by default can be hard to install on  
some OS. And imho the packaging setup is not as good. Frankly I gave up  
trying to get mcrypt installed on either version of php installed on my  
Mac.
This kind of tool screams out to me as something perfectly suited for  
python. It's pre-installed a lot of the time. IIRC most of the time you're  
not missing much you need. And python comes with easy_install and better  
yet pip.


But even before seeing this arc is related to the one thing about  
phabricator that concerns me the most.
arc looks as if it works completely with patches on it's own rather than  
doing anything with git.
ie: It looks as if it takes the actual source repo completely out of the  
story for patch submission.
I can't see how phabricator can have commit workflow support any better  
than gerrit when it appears to take the repo completely out of the  
question.

--
~Daniel Friesen (Dantman, Nadir-Seen-Fire) [http://daniel.friesen.name]

_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Let's talk about arc: On Arcanist docs

Marcin Cieslak-3
In reply to this post by Evan Priestley
>> Evan Priestley <[hidden email]> wrote:
> I sent out a diff to fix the error message (https://secure.phabricator.com/D3231), the new one reads:
>
>     This patch is for the 'phabricator' project, but the working copy does
>     not have an '.arcconfig' file to identify which project it belongs to.
>     Still try to apply the patch?
>
> We're trying to catch the case where you're attempting to apply a
> patch to the wrong project -- I'm guessing revision D3 was made in
> a working copy with a .gitignored ".arcconfig" file that associates
> it with "E3Experiments". If you check in the ".arcconfig" file, arc
> will be able to recognize the working copy's project and will stop
> complaining.

I realize my probleme where related to the E3Experiments not having
.arcconfig

I started suspecting E3Experiments is kind of unconfigured when
"arc git-hook-pre-received" hinted me about this:

"Usage Exception: You have installed a git pre-receive hook in a remote without an .arcconfig.")

> What could we improve about the user guide?

First, I found two entry points:

* Arcanist User Guide
  http://www.phabricator.com/docs/phabricator/article/Arcanist_User_Guide.html

Frankly, I don't remember I how I came to this one.

The problem here is that there is no full table of contents of the "User Guide"
(btw. Defined: src/docs/userguide/arcanist.diviner:1 seems useless to the causual
onlooker).

I could find this:

 http://www.phabricator.com/docs/phabricator/article/Arcanist_User_Guide_arc_diff.html

But not much more. While writing this email I discovered

 http://www.phabricator.com/docs/phabricator/article/Arcanist_User_Guide_Configuring_a_New_Project.html

hidden in the arc_diff guide which was like tl;dr to me - I didn't know I needed
to learn about "arc diff" command to find out about .arcconfig and stuff.

Suggestions for improvement:

1) In the "Overview" there should be some links to basic installation and configuration
(the .arcconfig thing).

2) "Arcanist allows you to do things like" should explain more about the commands -
descriptions are too short. There are no links to explanations of particular
commands (certainly "arc diff" has one).

Coming from gerrit I kind of looked for equivalent of "git fetch ... refs/changes/CD/ABCD/1"
and "git push ... refs/for/master". From the terse description there I could sense
that "arc diff" does something to push the changes for review and "arc patch" fetches
the change from the repo (although "arc export" sound nice, too).
Unfortunately, "arc download/upload" do something different :)

* Arcanist Something - Table of Contents
  http://www.phabricator.com/docs/arcanist/

The good thing is that Phabricator installation has links to this document
at https://phabricator.wmflabs.org/apps/. This is a big plus.

This the Arcanist Something guide is confusing because it contains 95% of
developer API documentation. I hoped to find info about .arcconfig
in "ArcanistConfiguration" or "ArcanistSettings" but both were
disappointing.

Now I see I should go into ArcanistOverview but somehow I missed that.
It links to Arcanist_User_Guide_Configuring_a_New_Project
which I missed so badly yesterday.

1) Probably ArcanistOverview should be *the* front page for the documentation
and the User Guide with full Table of Contents of all docs. Maybe the
TOC should be on all "User Guide" pages.

API pages should be clearly marked. Use different skin if possible
(red instead of blue:) or clearly mark links to API and UserGuide
articles differently (consistent title names? we can't rely on colors or
icons). Javadoc output might be ugly, but at least I know immediately
"uh oh I ended up in the developer documentation".


There is some problem with
http://www.phabricator.com/docs/phabricator/article/Arcanist_User_Guide_Customizing_Lint,_Unit_Tests_and_Workflows.html
it sounds like the gentle introduction into the whole API stuff.
Not sure yet how it fits to the casual user.

And then, there is

http://www.phabricator.com/docs/phabricator/article/Arcanist_User_Guide_Repository_Hooks.html

deals only with SVN. "arc git-hook-pre-receive" sounds promising
but I have no idea where to find out more about it.

Unfortunately, Phabricator docs use "workflow" as a slang description
of some piece of code, so I could not find out "How a typical
workflow with arc looks like" and "How installing a git hook
changes my workflow".


In general: docs seems to aimed either at the advanced person looking
to write "workflows" or "classes" for linting/whatever
or for the user of the already pre-configured repository.
I would review this again in view of the "lonely wolf" developer
that has some (maybe her own repository) and tries to
set up this thing. I didn't look at the rest of the Phabricator
docs yet but I'd be happy to find guides for
"How do I switch to Phabricator with a github/sourceforge/whatever project".


//Saper


_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Let's talk about arc: Phabricator replacement for git-review and more

Marcin Cieslak-3
In reply to this post by Daniel Friesen-4
>> Daniel Friesen <[hidden email]> wrote:
>
> Why is arc written in PHP? That seems to be a horrible software decision  
> to me. Core extensions not enabled by default can be hard to install on  
> some OS. And imho the packaging setup is not as good. Frankly I gave up  
> trying to get mcrypt installed on either version of php installed on my  
> Mac.

It could be improved to check for curl and bcmath (the ones I found
out are needed) on startup, not during some other command
after other succeded (unless of course the extension is needed
only for some specific operation not applicable to general public).

This one I find interesting:


> arc looks as if it works completely with patches on it's own rather than  
> doing anything with git.

> I can't see how phabricator can have commit workflow support any better  
> than gerrit when it appears to take the repo completely out of the  
> question.

Erik also wrote this earlier:

> As I understood it, the big gotchas for Phabricator adoption are that
> Phabricator doesn't manage repositories - it knows how to poll a Git
> repo, but it doesn't have per-repo access controls or even more than a
> shallow awareness of what a repository is; it literally shells out to
> git to perform its operations, e.g. poll for changes - and would still
> need some work to efficiently deal with hundreds of repositories,
> long-lived remote branches, and some of the other fun characteristics
> of Wikimedia's repos. Full repo management is on the roadmap, without
> an exact date, and Evan is very open to making tweaks and changes
> as needed, especially if it serves a potential flagship user like
> Wikimedia.

After Gerrit, I think it might actually be a GoodThing(tm)
to detach the code review tool from managing the repository.

Git at its core is a tool to quickly snapshot directories.
Blobs are its first-class objects, not patches or diffs
(this is I think pretty innovative looking at the traditional
version control systems).

I think there is a reason why Linus settled with email
patch workflow that is even included in the git user
interface.

Keeping patches and commits separately starts making
sense to me - otherwise one ends up in rebase hell.

//Saper


_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Let's talk about arc: Phabricator replacement for git-review and more

Daniel Friesen-4
On Fri, 10 Aug 2012 00:52:33 -0700, Marcin Cieslak <[hidden email]>  
wrote:

>>> Daniel Friesen <[hidden email]> wrote:
>>
>> Why is arc written in PHP? That seems to be a horrible software decision
>> to me. Core extensions not enabled by default can be hard to install on
>> some OS. And imho the packaging setup is not as good. Frankly I gave up
>> trying to get mcrypt installed on either version of php installed on my
>> Mac.
>
> It could be improved to check for curl and bcmath (the ones I found
> out are needed) on startup, not during some other command
> after other succeded (unless of course the extension is needed
> only for some specific operation not applicable to general public).
>
> This one I find interesting:
>
>
>> arc looks as if it works completely with patches on it's own rather than
>> doing anything with git.
>
>> I can't see how phabricator can have commit workflow support any better
>> than gerrit when it appears to take the repo completely out of the
>> question.
>
> Erik also wrote this earlier:
>
>> As I understood it, the big gotchas for Phabricator adoption are that
>> Phabricator doesn't manage repositories - it knows how to poll a Git
>> repo, but it doesn't have per-repo access controls or even more than a
>> shallow awareness of what a repository is; it literally shells out to
>> git to perform its operations, e.g. poll for changes - and would still
>> need some work to efficiently deal with hundreds of repositories,
>> long-lived remote branches, and some of the other fun characteristics
>> of Wikimedia's repos. Full repo management is on the roadmap, without
>> an exact date, and Evan is very open to making tweaks and changes
>> as needed, especially if it serves a potential flagship user like
>> Wikimedia.
>
> After Gerrit, I think it might actually be a GoodThing(tm)
> to detach the code review tool from managing the repository.
>
> Git at its core is a tool to quickly snapshot directories.
> Blobs are its first-class objects, not patches or diffs
> (this is I think pretty innovative looking at the traditional
> version control systems).
>
> I think there is a reason why Linus settled with email
> patch workflow that is even included in the git user
> interface.
>
> Keeping patches and commits separately starts making
> sense to me - otherwise one ends up in rebase hell.
>
> //Saper

Managing repositories and rebasing have absolutely nothing to do with each  
other.
In fact the managing being discussed is talking about access controls and  
perhaps creating repos, etc...

Git's storage of blobs rather than diffs is also irrelevant. There is  
nothing wrong with review being a tree of blobs noted as based off of the  
sha1 of a previous tree of blobs. Heck this is something that would make  
an overview diff of multiple commits inside of a review branch really sane.

And Linus settled with an email patch workflow so he wouldn't have to  
write a new review system in addition to a whole vcs. It has nothing to do  
with whether changes for review in a review system a better as deltas or  
blobs.
Linus is working with a hierarchical pull-request model with multiple  
lieutenant maintainers. Something we explicitly chose not to do. So his  
choices in that area have no bearing on what is the best way to do a  
review system.

Rebasing has absolutely nothing to do with whether changes for review and  
actual commits are stored in the repo or separated from it.
In fact, quite frankly, working with diffs instead of commits and then  
applying the diff to the latest version on commit is almost exactly the  
same as rebasing.

Rebase hell is a screwed up workflow which is 100% by gerrit design. There  
was nothing about the model they chose to write a review system that  
forced them to use a rebase+patchset model. Rebases were their choice when  
they could have used real review heads with multiple commits and normal  
merges. The way a clean pull request model works, but with the automation  
we want.

It is entirely possible to write a review system that integrates with git,  
stores all commits for review inside of the repo, does not use rebases,  
supports multiple commits in a review-head without making buggy commits  
part of the main line of master, support post-commit review as well, and  
doesn't fight git introducing ugly hacks or making non-git things do  
things git is supposed to be doing.

And if I had the time (ie: backing) I'd write it.

--
~Daniel Friesen (Dantman, Nadir-Seen-Fire) [http://daniel.friesen.name]

_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Let's talk about arc: Phabricator replacement for git-review and more

Evan Priestley
In reply to this post by Daniel Friesen-4
On Aug 9, 2012, at 9:52 PM, Daniel Friesen wrote:

> Why is arc written in PHP? That seems to be a horrible software decision to me. Core extensions not enabled by default can be hard to install on some OS. And imho the packaging setup is not as good. Frankly I gave up trying to get mcrypt installed on either version of php installed on my Mac.
> This kind of tool screams out to me as something perfectly suited for python. It's pre-installed a lot of the time. IIRC most of the time you're not missing much you need. And python comes with easy_install and better yet pip.

Please let us know when you run into specific problems. As far as I know, this hasn't been a major pain point in practice (some minor pain on Windows, but I think Python would be about as bad there).

arc is written in PHP because Phabricator is written in PHP, and we can reuse more code more easily by having both the client and server in the same language. For example, the code to parse raw diffs lives in Arcanist, but Phabricator uses the same code when it needs to parse raw diffs (e.g., via copy and paste or from the VCS). In turn, Phabricator is written in PHP because it grew on top of a PHP stack at Facebook.

> But even before seeing this arc is related to the one thing about phabricator that concerns me the most.
> arc looks as if it works completely with patches on it's own rather than doing anything with git.
> ie: It looks as if it takes the actual source repo completely out of the story for patch submission.

For pre-push review, the remote repository is completely out of the story, yes. However, in pre-push review the changes aren't available in the remote, so I believe this is the only reasonable architecture.

For post-push review in Phabricator (which does not use arc) the local is completely out of the story.

> I can't see how phabricator can have commit workflow support any better than gerrit when it appears to take the repo completely out of the question.

I'm not sure I understand this concern, can you give me an example of what you mean? (e.g., what features does gerrit have that Phabricator can't?)

Evan
_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Let's talk about arc: Phabricator replacement for git-review and more

Evan Priestley
In reply to this post by Marcin Cieslak-3

On Aug 10, 2012, at 12:52 AM, Marcin Cieslak wrote:

> It could be improved to check for curl and bcmath (the ones I found
> out are needed) on startup, not during some other command
> after other succeded (unless of course the extension is needed
> only for some specific operation not applicable to general public).

We should be checking for curl on startup (and a few other things -- namely JSON and a reasonable PHP version). Was this not your experience? The expectation is that you'll receive this error if curl is not installed:

  $ arc

  PHP CONFIGURATION ERRORS

  You need to install the cURL PHP extension, maybe with 'apt-get install php5-curl' or 'yum install php53-curl' or something similar.

bcmath is used only to encode binaries in base85 for git patches when you generate, apply or export a changeset that includes binaries to a git patch format or working copy, but we're definitely not doing a good job of managing this dependency right now; I filed https://secure.phabricator.com/T1635 to track it. Thanks!

Evan
_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Let's talk about arc: Phabricator replacement for git-review and more

Marcin Cieslak-3
>> Evan Priestley <[hidden email]> wrote:
>
> On Aug 10, 2012, at 12:52 AM, Marcin Cieslak wrote:
>
>> It could be improved to check for curl and bcmath (the ones I found
>> out are needed) on startup, not during some other command
>> after other succeded (unless of course the extension is needed
>> only for some specific operation not applicable to general public).
>
> We should be checking for curl on startup (and a few other things
> -- namely JSON and a reasonable PHP version). Was this not your
> experience?

I have already had php-curl installed and I only noticed I needed
it because the need to configure list of acceptable CAs; so I *knew*
php-curl is needed.

PHP without bcmath failed on me badly with PHP fatal error.

//Marcin


_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Let's talk about arc: On Arcanist docs

Evan Priestley
In reply to this post by Marcin Cieslak-3
Thanks, this is helpful. We plan to eventually move Diviner (the documentation generator) into Phabricator itself and I want to do that before making significant layout/style/organizational changes so I'm not going to hit all these points immediately, but some of the things you point out are just dumb on our part and easy to fix. I sent out a couple of diffs that should improve things:

https://secure.phabricator.com/D3235
https://secure.phabricator.com/D3236

> (btw. Defined: src/docs/userguide/arcanist.diviner:1 seems useless to the causual
> onlooker).

For slightly-less casual onlookers who spot a typo, it's a good way for them to know how to fix it. :)

Evan

On Aug 10, 2012, at 12:26 AM, Marcin Cieslak wrote:

>>> Evan Priestley <[hidden email]> wrote:
>> I sent out a diff to fix the error message (https://secure.phabricator.com/D3231), the new one reads:
>>
>>    This patch is for the 'phabricator' project, but the working copy does
>>    not have an '.arcconfig' file to identify which project it belongs to.
>>    Still try to apply the patch?
>>
>> We're trying to catch the case where you're attempting to apply a
>> patch to the wrong project -- I'm guessing revision D3 was made in
>> a working copy with a .gitignored ".arcconfig" file that associates
>> it with "E3Experiments". If you check in the ".arcconfig" file, arc
>> will be able to recognize the working copy's project and will stop
>> complaining.
>
> I realize my probleme where related to the E3Experiments not having
> .arcconfig
>
> I started suspecting E3Experiments is kind of unconfigured when
> "arc git-hook-pre-received" hinted me about this:
>
> "Usage Exception: You have installed a git pre-receive hook in a remote without an .arcconfig.")
>
>> What could we improve about the user guide?
>
> First, I found two entry points:
>
> * Arcanist User Guide
>  http://www.phabricator.com/docs/phabricator/article/Arcanist_User_Guide.html
>
> Frankly, I don't remember I how I came to this one.
>
> The problem here is that there is no full table of contents of the "User Guide"
> (btw. Defined: src/docs/userguide/arcanist.diviner:1 seems useless to the causual
> onlooker).
>
> I could find this:
>
> http://www.phabricator.com/docs/phabricator/article/Arcanist_User_Guide_arc_diff.html
>
> But not much more. While writing this email I discovered
>
> http://www.phabricator.com/docs/phabricator/article/Arcanist_User_Guide_Configuring_a_New_Project.html
>
> hidden in the arc_diff guide which was like tl;dr to me - I didn't know I needed
> to learn about "arc diff" command to find out about .arcconfig and stuff.
>
> Suggestions for improvement:
>
> 1) In the "Overview" there should be some links to basic installation and configuration
> (the .arcconfig thing).
>
> 2) "Arcanist allows you to do things like" should explain more about the commands -
> descriptions are too short. There are no links to explanations of particular
> commands (certainly "arc diff" has one).
>
> Coming from gerrit I kind of looked for equivalent of "git fetch ... refs/changes/CD/ABCD/1"
> and "git push ... refs/for/master". From the terse description there I could sense
> that "arc diff" does something to push the changes for review and "arc patch" fetches
> the change from the repo (although "arc export" sound nice, too).
> Unfortunately, "arc download/upload" do something different :)
>
> * Arcanist Something - Table of Contents
>  http://www.phabricator.com/docs/arcanist/
>
> The good thing is that Phabricator installation has links to this document
> at https://phabricator.wmflabs.org/apps/. This is a big plus.
>
> This the Arcanist Something guide is confusing because it contains 95% of
> developer API documentation. I hoped to find info about .arcconfig
> in "ArcanistConfiguration" or "ArcanistSettings" but both were
> disappointing.
>
> Now I see I should go into ArcanistOverview but somehow I missed that.
> It links to Arcanist_User_Guide_Configuring_a_New_Project
> which I missed so badly yesterday.
>
> 1) Probably ArcanistOverview should be *the* front page for the documentation
> and the User Guide with full Table of Contents of all docs. Maybe the
> TOC should be on all "User Guide" pages.
>
> API pages should be clearly marked. Use different skin if possible
> (red instead of blue:) or clearly mark links to API and UserGuide
> articles differently (consistent title names? we can't rely on colors or
> icons). Javadoc output might be ugly, but at least I know immediately
> "uh oh I ended up in the developer documentation".
>
>
> There is some problem with
> http://www.phabricator.com/docs/phabricator/article/Arcanist_User_Guide_Customizing_Lint,_Unit_Tests_and_Workflows.html
> it sounds like the gentle introduction into the whole API stuff.
> Not sure yet how it fits to the casual user.
>
> And then, there is
>
> http://www.phabricator.com/docs/phabricator/article/Arcanist_User_Guide_Repository_Hooks.html
>
> deals only with SVN. "arc git-hook-pre-receive" sounds promising
> but I have no idea where to find out more about it.
>
> Unfortunately, Phabricator docs use "workflow" as a slang description
> of some piece of code, so I could not find out "How a typical
> workflow with arc looks like" and "How installing a git hook
> changes my workflow".
>
>
> In general: docs seems to aimed either at the advanced person looking
> to write "workflows" or "classes" for linting/whatever
> or for the user of the already pre-configured repository.
> I would review this again in view of the "lonely wolf" developer
> that has some (maybe her own repository) and tries to
> set up this thing. I didn't look at the rest of the Phabricator
> docs yet but I'd be happy to find guides for
> "How do I switch to Phabricator with a github/sourceforge/whatever project".
>
>
> //Saper
>
>
> _______________________________________________
> Wikitech-l mailing list
> [hidden email]
> https://lists.wikimedia.org/mailman/listinfo/wikitech-l


_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Let's talk about arc: Phabricator replacement for git-review and more

Daniel Friesen-4
In reply to this post by Evan Priestley
On Fri, 10 Aug 2012 05:52:16 -0700, Evan Priestley  
<[hidden email]> wrote:

> On Aug 9, 2012, at 9:52 PM, Daniel Friesen wrote:
>
>> Why is arc written in PHP? That seems to be a horrible software  
>> decision to me. Core extensions not enabled by default can be hard to  
>> install on some OS. And imho the packaging setup is not as good.  
>> Frankly I gave up trying to get mcrypt installed on either version of  
>> php installed on my Mac.
>> This kind of tool screams out to me as something perfectly suited for  
>> python. It's pre-installed a lot of the time. IIRC most of the time  
>> you're not missing much you need. And python comes with easy_install  
>> and better yet pip.
>
> Please let us know when you run into specific problems. As far as I  
> know, this hasn't been a major pain point in practice (some minor pain  
> on Windows, but I think Python would be about as bad there).
>
> arc is written in PHP because Phabricator is written in PHP, and we can  
> reuse more code more easily by having both the client and server in the  
> same language. For example, the code to parse raw diffs lives in  
> Arcanist, but Phabricator uses the same code when it needs to parse raw  
> diffs (e.g., via copy and paste or from the VCS). In turn, Phabricator  
> is written in PHP because it grew on top of a PHP stack at Facebook.
>
>> But even before seeing this arc is related to the one thing about  
>> phabricator that concerns me the most.
>> arc looks as if it works completely with patches on it's own rather  
>> than doing anything with git.
>> ie: It looks as if it takes the actual source repo completely out of  
>> the story for patch submission.
>
> For pre-push review, the remote repository is completely out of the  
> story, yes. However, in pre-push review the changes aren't available in  
> the remote, so I believe this is the only reasonable architecture.

This holds true for centralized version control systems where the only  
actual
commits are ones that have made it into the centralized repo.
But IMHO it does not hold true for dvcs' like Git.
Git handles commits and refs really nicely. It is entirely possible to have
the change available inside the remote even when the change is not merge  
into
the actual repo.

In fact this is hugely advantageous. You have practically none of the  
downsides that making the actual commit would have. But you have almost  
every single advantage you could possibly have by having it in the repo:
- You get all the tools you need for free. There is no need to  
re-implement anything that already exists inside the vcs.
- Dependencies, deltas, merges, handling conflicts, etc... everything is  
already handled for you.
- Because the commit already exists you can already start making new  
commits based on it before the commit even gets merged into the repo. The  
review system doesn't interfere with the development process. And this is  
possible without the use of any specialized tools.
- Because the commit is in the repo you can freely pull the commit  
anywhere you want, cherry-pick it, merge it into another branch, write  
code based off of it, etc... natively using all the standard tools without  
any unnecessary custom tool.

> For post-push review in Phabricator (which does not use arc) the local  
> is completely out of the story.
>
>> I can't see how phabricator can have commit workflow support any better  
>> than gerrit when it appears to take the repo completely out of the  
>> question.
>
> I'm not sure I understand this concern, can you give me an example of  
> what you mean? (e.g., what features does gerrit have that Phabricator  
> can't?)

I wouldn't really say Gerrit does this right. Rather I believe that Gerrit  
does this wrong, that's the reason we're trying to ditch Gerrit, but  
Phabricator looks as if it might not do it any better so I can't see why  
it would be an improvement.

Perhaps this would be best served by you explaining how things would work  
in Phabricator for two situations:

First situation:
- Someone makes a change to core
- Then they submit it for review
- A reviewer notes that there is something wrong with the code that needs  
to be fixed
- Someone fixes the code
- Then they update the review (*This is usually the important point where  
review systems differ)
- The change is accepted and merge into the repo

In the gerrit situation:
- The change to core is committed as a local commit with a Change-ID
- Submission for review is done with `git review -R`
- [A reviewer notes that there is something wrong with the code that needs  
to be fixed]
- An --amend is done to edit the commit.
- `git review -R` updates the review adding a new patchset to replace the  
one in gerrit.
- Gerrit merges one commit into the repo

The ideal situation IMHO:
- The change is committed as a local commit using normal git tools
- *I won't go into the ideal submission process.
- [A reviewer notes that there is something wrong with the code that needs  
to be fixed]
- A local commit is made on top of the previous commit
- The same submission process is used leading to the review head  
containing two new commits
- The review system does a --no-ff merge keeping the full history of  
changes without any rebase or amending

Second situation:
- Someone makes a change to core
- It is submitted for review
- Then they make another change that is based of the changes made in the  
other commit
- The second commit is also submitted for review

How does Phabricator handle a commit based off another commit when it  
seems like no commit exists for the unreviewed part?

Gerrit handles dependencies like this fine. However I do admit that the  
insistent amending and rebasing as well as the UI and suggested workflow  
make a chilling effect that discourages people from merging other peoples  
changes into their code before review and lots of people just wait for it  
to make it into core and hold off on doing things based on other people's  
unreviewed commits.

((I had a perfect example of this. Platonides broke maintenance/dev/ and  
it got in my way of testing one of my branches. I fixed maintenance/dev/  
and submitted it for review. Because of how uncomfortable Gerrit made  
working with multiple commits I went and waited a day for my commit to be  
reviewed and make it into core before I made use of the fix to do a test  
in another branch. ie: Because of Gerrit's harsh workflow I was dissuaded  
 from using one of my own commits until review.))

> Evan

--
~Daniel Friesen (Dantman, Nadir-Seen-Fire) [http://daniel.friesen.name]

_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Let's talk about arc: Phabricator replacement for git-review and more

Evan Priestley
> First situation (updates)

You can update a Differential Revision with any new change you want -- including a change from another project or repository, or a raw diff you typed by hand, or whatever else. You do not need to structure the new change in any special way or relate it to the previous change whatsoever. This isn't terribly common (usually new changes are amended versions of old changes, or old changes plus new commits), but handles situations like:

  - User A makes a change to fix a bug in the Y extension.
  - User B says "this is good, but we should fix the root cause in the X library, not the symptom here in the Y extension".
  - User A updates the same revision with a change to the X library instead.

In Differential, Revisions are about goals and ideas ("fix this bug"), not specific commits. You do not need to amend commits to send changes for review, and are free to represent them locally in whatever form you prefer.

Depending on configuration, some workflows will amend commit messages for you ("arc diff", "arc amend") or perform --squash merges instead of --no-ff merges ("arc land"). You can customize these behaviors with configuration. Some discussion here:

http://www.phabricator.com/docs/phabricator/article/Arcanist_User_Guide_Configuring_a_New_Project.html#history-mutability

That said, there is also a (purely advisory) document here extolling the virtues of amending (if not during development, at least before pushing changes to the remote):

http://www.phabricator.com/docs/phabricator/article/Recommendations_on_Revision_Control.html

However, we fully support immutable history workflows if you prefer them: you do not ever need to amend or rebase anything.

> Second situation (changes based on unreviewed code)

Phabricator treats changes based on unreviewed code no differently from changes based on reviewed code -- at a basic level, you can paste in any arbitrary diff file if you want, and it works without additional context. If additional context is available, it just stores and presents more metadata and improves workflows.

Making changes that are based on unreviewed code is very common in Phabricator workflows. The intent is for code review to block only when it actually needs to (e.g., you need feedback from peers to proceed because you aren't sure about something). Making it easy to continue work without review also encourages smaller, more reviewable diffs.



On Aug 10, 2012, at 9:47 AM, Daniel Friesen wrote:

> On Fri, 10 Aug 2012 05:52:16 -0700, Evan Priestley <[hidden email]> wrote:
>
>> On Aug 9, 2012, at 9:52 PM, Daniel Friesen wrote:
>>
>>> Why is arc written in PHP? That seems to be a horrible software decision to me. Core extensions not enabled by default can be hard to install on some OS. And imho the packaging setup is not as good. Frankly I gave up trying to get mcrypt installed on either version of php installed on my Mac.
>>> This kind of tool screams out to me as something perfectly suited for python. It's pre-installed a lot of the time. IIRC most of the time you're not missing much you need. And python comes with easy_install and better yet pip.
>>
>> Please let us know when you run into specific problems. As far as I know, this hasn't been a major pain point in practice (some minor pain on Windows, but I think Python would be about as bad there).
>>
>> arc is written in PHP because Phabricator is written in PHP, and we can reuse more code more easily by having both the client and server in the same language. For example, the code to parse raw diffs lives in Arcanist, but Phabricator uses the same code when it needs to parse raw diffs (e.g., via copy and paste or from the VCS). In turn, Phabricator is written in PHP because it grew on top of a PHP stack at Facebook.
>>
>>> But even before seeing this arc is related to the one thing about phabricator that concerns me the most.
>>> arc looks as if it works completely with patches on it's own rather than doing anything with git.
>>> ie: It looks as if it takes the actual source repo completely out of the story for patch submission.
>>
>> For pre-push review, the remote repository is completely out of the story, yes. However, in pre-push review the changes aren't available in the remote, so I believe this is the only reasonable architecture.
>
> This holds true for centralized version control systems where the only actual
> commits are ones that have made it into the centralized repo.
> But IMHO it does not hold true for dvcs' like Git.
> Git handles commits and refs really nicely. It is entirely possible to have
> the change available inside the remote even when the change is not merge into
> the actual repo.
>
> In fact this is hugely advantageous. You have practically none of the downsides that making the actual commit would have. But you have almost every single advantage you could possibly have by having it in the repo:
> - You get all the tools you need for free. There is no need to re-implement anything that already exists inside the vcs.
> - Dependencies, deltas, merges, handling conflicts, etc... everything is already handled for you.
> - Because the commit already exists you can already start making new commits based on it before the commit even gets merged into the repo. The review system doesn't interfere with the development process. And this is possible without the use of any specialized tools.
> - Because the commit is in the repo you can freely pull the commit anywhere you want, cherry-pick it, merge it into another branch, write code based off of it, etc... natively using all the standard tools without any unnecessary custom tool.
>
>> For post-push review in Phabricator (which does not use arc) the local is completely out of the story.
>>
>>> I can't see how phabricator can have commit workflow support any better than gerrit when it appears to take the repo completely out of the question.
>>
>> I'm not sure I understand this concern, can you give me an example of what you mean? (e.g., what features does gerrit have that Phabricator can't?)
>
> I wouldn't really say Gerrit does this right. Rather I believe that Gerrit does this wrong, that's the reason we're trying to ditch Gerrit, but Phabricator looks as if it might not do it any better so I can't see why it would be an improvement.
>
> Perhaps this would be best served by you explaining how things would work in Phabricator for two situations:
>
> First situation:
> - Someone makes a change to core
> - Then they submit it for review
> - A reviewer notes that there is something wrong with the code that needs to be fixed
> - Someone fixes the code
> - Then they update the review (*This is usually the important point where review systems differ)
> - The change is accepted and merge into the repo
>
> In the gerrit situation:
> - The change to core is committed as a local commit with a Change-ID
> - Submission for review is done with `git review -R`
> - [A reviewer notes that there is something wrong with the code that needs to be fixed]
> - An --amend is done to edit the commit.
> - `git review -R` updates the review adding a new patchset to replace the one in gerrit.
> - Gerrit merges one commit into the repo
>
> The ideal situation IMHO:
> - The change is committed as a local commit using normal git tools
> - *I won't go into the ideal submission process.
> - [A reviewer notes that there is something wrong with the code that needs to be fixed]
> - A local commit is made on top of the previous commit
> - The same submission process is used leading to the review head containing two new commits
> - The review system does a --no-ff merge keeping the full history of changes without any rebase or amending
>
> Second situation:
> - Someone makes a change to core
> - It is submitted for review
> - Then they make another change that is based of the changes made in the other commit
> - The second commit is also submitted for review
>
> How does Phabricator handle a commit based off another commit when it seems like no commit exists for the unreviewed part?
>
> Gerrit handles dependencies like this fine. However I do admit that the insistent amending and rebasing as well as the UI and suggested workflow make a chilling effect that discourages people from merging other peoples changes into their code before review and lots of people just wait for it to make it into core and hold off on doing things based on other people's unreviewed commits.
>
> ((I had a perfect example of this. Platonides broke maintenance/dev/ and it got in my way of testing one of my branches. I fixed maintenance/dev/ and submitted it for review. Because of how uncomfortable Gerrit made working with multiple commits I went and waited a day for my commit to be reviewed and make it into core before I made use of the fix to do a test in another branch. ie: Because of Gerrit's harsh workflow I was dissuaded from using one of my own commits until review.))
>
>> Evan
>
> --
> ~Daniel Friesen (Dantman, Nadir-Seen-Fire) [http://daniel.friesen.name]
>
> _______________________________________________
> Wikitech-l mailing list
> [hidden email]
> https://lists.wikimedia.org/mailman/listinfo/wikitech-l


_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Let's talk about arc: Phabricator replacement for git-review and more

Daniel Friesen-4
On Fri, 10 Aug 2012 17:06:12 -0700, Evan Priestley  
<[hidden email]> wrote:

> [...]
> That said, there is also a (purely advisory) document here extolling the  
> virtues of amending (if not during development, at least before pushing  
> changes to the remote):
>
> http://www.phabricator.com/docs/phabricator/article/Recommendations_on_Revision_Control.html

Looks to me like the same mistakes I see all the time. ie: Thinking about  
commit history as a single linear tree. Almost none of the arguments in  
the "Why" section are valid when you --no-ff instead of squashing.

--
~Daniel Friesen (Dantman, Nadir-Seen-Fire) [http://daniel.friesen.name]

_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Let's talk about arc: Phabricator replacement for git-review and more

Evan Priestley
Definitely -- it's less clear than it could be because I don't call out "--no-ff" explicitly (I'm hedging my bets against Mercurial adding something similar some day), but for all practical purposes I mean "--no-ff" when I say "having a strict policy where your master/trunk contains only merge commits" and "or create an abstraction layer (merge commits)".

On Aug 10, 2012, at 8:29 PM, Daniel Friesen wrote:

> On Fri, 10 Aug 2012 17:06:12 -0700, Evan Priestley <[hidden email]> wrote:
>
>> [...]
>> That said, there is also a (purely advisory) document here extolling the virtues of amending (if not during development, at least before pushing changes to the remote):
>>
>> http://www.phabricator.com/docs/phabricator/article/Recommendations_on_Revision_Control.html
>
> Looks to me like the same mistakes I see all the time. ie: Thinking about commit history as a single linear tree. Almost none of the arguments in the "Why" section are valid when you --no-ff instead of squashing.
>
> --
> ~Daniel Friesen (Dantman, Nadir-Seen-Fire) [http://daniel.friesen.name]
>
> _______________________________________________
> Wikitech-l mailing list
> [hidden email]
> https://lists.wikimedia.org/mailman/listinfo/wikitech-l


_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l