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_Con…
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_Revi…
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
<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]
_______________________________________________
Wikitech-l mailing list
Wikitech-l(a)lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/wikitech-l