On Fri, 10 Aug 2012 05:52:16 -0700, Evan Priestley
<epriestley(a)phacility.com> 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]