Phabricator for patches and code review

Tue Jun 24 17:17:36 UTC 2014

Personally I don't particularly mind the fact Phabricator squashes
things and adds onto the URL. For one it means we can always refer to
the differential revision solely by the commit itself, as opposed to
digging up some history. But also, Phab generally asks you to put some
useful information in there. I've found myself to be much more
explicit and verbose when filling out Phab commits than most other
tools, and meaty, verbose commit messages are very useful IME.

As for squashing, there's an argument to be made, I think, that this
just requires a little practice to get comfortable with. :) For
example, I have a review called D4:

https://phabricator.haskell.org/D4

This really adds two separate things: it adds a set of flags to the
frontend, and a set of optimizations to the backend (based on the
flags). Really, this *should* be two reviews instead of one! The fact
that Phabricator wants to squash them into a single commit when
landing is a symptom of the problem, not the root problem, I think. So
while I may have to fiddle to separate the two later (not really that
hard,) I think this was more a case of me putting two unrelated things
in one review.

In fact it's very regular for GHC developers to squash commits
themselves, and I know many of us do exactly that. So I think the main
change is just having the tool embody this for us, as opposed to
always doing it manually.

On Tue, Jun 24, 2014 at 9:27 AM, Johan Tibell <johan.tibell at gmail.com> wrote:
> I find the automatic squashing to rather harmful to the commit history. So
> if you have several nice commits that you want to send for review, don't use
> arc land to commit them, as it will ruin the history. Instead git push them
> as per normal and use `arc close` (IIRC) to close the code review. I also
> find it useful to remove lots of the arc gunk that was added to the commit
> message, to not pollute our revision history with data from some specific
> tool.
>
>
> On Tue, Jun 24, 2014 at 4:09 PM, Richard Eisenberg <eir at cis.upenn.edu>
> wrote:
>>
>> Thanks so much for writing this! I have some questions:
>>
>> 1) I'm just setting things up on my machine. It says to `arc
>> install-certificate` in my GHC directory. Is it important precisely which
>> clone of GHC my directory is set up against? For example, my "pull" origin
>> is git://git.haskell.org/ghc.git and my "push" origin is
>> ssh://git@git.haskell.org/ghc.git. Is this the right set-up? If this bit
>> matters, could you add it to the page? Or, if not, could you comment on what
>> `arc` is pulling from the ghc directory?
>>
>> 2) I'm confused about what, precisely, `arc diff` does. You describe that
>> it updates the review available online. Does that reference some git
>> commits? Do I need to push by `wip` branch before `arc diff`ing? Do I
>> *never* need to push my branch, because `arc diff` pushes it for me? Do I
>> *never* need to push my branch, because Phab uses other ways of moving the
>> code around? For better or worse, I tend to keep my branches local until I'm
>> ready to merge with master, and I want to know if this needs to change.
>>
>> 3) You say "A change cannot be merged until at least one reviewer has
>> signed off." Does this mean "merged with ghc-7.8" (or whatever the current
>> stable branch is)? Or does it mean "merged with master"? The former is the
>> status quo, but with a new route, so to speak. The latter is something new,
>> as several of us push directly to `master` without a review. I'm not against
>> such a change, per se, just trying to understand it.
>>
>> 4) Is it now compulsory to use Phab when contributing? This page makes it
>> sound that way. Again, no complaints -- just trying to understand it.
>>
>> 5) The page says to add `austin` as a reviewer. I would expect
>> `thoughtpolice`. What's up with Phab usernames? Do other people I know and
>> love have Phab usernames distinct from Trac usernames?
>>
>> 6) Who is the reviewer `#ghc`? Is this related to the IRC channel? What
>> does the # signify?
>>
>> 7) I'm baffled by "Landing reviews". The `arc land` bit is clearer, but
>> I'm still unsure of what my local state and the git upstream state must be
>> beforehand for this to work. Will this ask me for a commit message? Will it
>> use the one I specified to Phab during `arc diff`? In general, I'm confused
>> about how much info `arc` pulls from various places to do its work. I know I
>> could learn by doing, but I'm afraid of mashing on the Phab and/or git
>> history as I do so.
>>
>> 8) Some of the same questions surround `arc patch`: What does my git state
>> need to be for this to work?
>>
>> 9) How do I contribute to others' revisions? Or, will this be obvious what
>> it comes to pass?
>>
>> I realize I've asked a lot here, and it might be too much to expect all of
>> these answers to be on the page. If that's the case, could you perhaps link
>> to relevant manuals or places to learn more? The way `arc` works, in
>> particular, seems like magic; magic is very powerful, but it can be equally
>> dangerous, and so I'd like to learn more.
>>
>> Thanks so much for doing all this!
>> Richard
>>
>> On Jun 23, 2014, at 10:44 AM, Austin Seipp <austin at well-typed.com> wrote:
>>
>> > Hi all,
>> >
>> > I went ahead and took some time to write some stuff down about
>> > Phabricator, including some basic tips on the workflows and
>> > applications here:
>> >
>> > https://ghc.haskell.org/trac/ghc/wiki/Phabricator
>> >
>> > It's definitely going to need more expanding. Do let me know if
>> > anything is confusing.
>> >
>> > On Wed, Jun 18, 2014 at 2:38 AM, Jan Stolarek <jan.stolarek at p.lodz.pl>
>> > wrote:
>> >> Duh, ignore what I wrote. I just realized I'm working on a non-rebased
>> >> branch :-)
>> >>
>> >> Janek
>> >>
>> >> Dnia środa, 18 czerwca 2014, Jan Stolarek napisał:
>> >>> I read the friendly Arcanist manual and I wonder if we intend to have
>> >>> a
>> >>> default .arcconfig file in the GHC repo? From the docs it seems like a
>> >>> good
>> >>> idea.
>> >>>
>> >>> Janek
>> >>>
>> >>> Dnia wtorek, 17 czerwca 2014, Simon Marlow napisał:
>> >>>> On 13/06/14 10:47, Jan Stolarek wrote:
>> >>>>> It seems that most people are in favour of using Phabricator for
>> >>>>> code
>> >>>>> review. So what are the next steps? Can we just start using the
>> >>>>> existing phabricator instance? I'm working on some code right now
>> >>>>> that
>> >>>>> definitely needs reviewing.
>> >>>>
>> >>>> You can use it, and a few of us have already been doing so.  There
>> >>>> isn't
>> >>>> any Trac integration yet, but it works nicely for patch review.
>> >>>>
>> >>>> There's a short intro doc here:
>> >>>>
>> >>>> https://secure.phabricator.com/book/phabricator/article/differential/,
>> >>>> but it's not hard to figure out the basics, and you'll learn by
>> >>>> watching
>> >>>> how other people use it.  If you go to the Herald tool you have
>> >>>> yourself
>> >>>> automatically subscribed to diffs that touch areas of the code that
>> >>>> you're interested in.
>> >>>>
>> >>>> Pro tip: the keyboard shortcuts are really useful, especially "z".
>> >>>> Hit
>> >>>> "?" to see all the shortcuts.
>> >>>>
>> >>>> Cheers,
>> >>>> Simon
>> >>>>
>> >>>>> Janek
>> >>>>>
>> >>>>> Dnia niedziela, 8 czerwca 2014, Simon Marlow napisał:
>> >>>>>> On 07/06/2014 07:21, Manuel M T Chakravarty wrote:
>> >>>>>>> So, why not put everything on GutHub and use pull requests and so
>> >>>>>>> on?
>> >>>>>>
>> >>>>>> github just isn't great for doing code reviews. No side-by-side
>> >>>>>> diffs,
>> >>>>>> and it sends you a separate email for every single comment, there's
>> >>>>>> no
>> >>>>>> concept of a "review" consisting of multiple inline comments
>> >>>>>> (unless
>> >>>>>> I've missed something). I'm afraid if we were using this for
>> >>>>>> regular
>> >>>>>> reviews I would have to disable the email notifications, which
>> >>>>>> makes
>> >>>>>> it significantly less useful.
>> >>>>>>
>> >>>>>>> SimonM writes that Phabricator is better than GitHub. I’m happy to
>> >>>>>>> believe that, but he also writes that using it requires installing
>> >>>>>>> local software and quite a bit of work. Moreover, I like to add
>> >>>>>>> that
>> >>>>>>> lots of people already know how to use GitHub and probably few
>> >>>>>>> know
>> >>>>>>> Phabricator.
>> >>>>>>>
>> >>>>>>> So, we are talking about having a somewhat better tool in return
>> >>>>>>> for
>> >>>>>>> three very significant disadvantages: (1) local installation, (2)
>> >>>>>>> work to set up and maintain Phabricator, and (3) effort by many
>> >>>>>>> people to learn to use it.
>> >>>>>>
>> >>>>>> Well, you've tipped the balance with "somewhat" and "significant"
>> >>>>>> here, I'd say Phabricator is "significantly" better than github for
>> >>>>>> code reviews, while installing arc is "somewhat" annoying :-)
>> >>>>>>
>> >>>>>> I have to admit it's not a no-brainer, but I do worry that github
>> >>>>>> just
>> >>>>>> wouldn't cut it for doing a lot of code reviewing, whereas I spend
>> >>>>>> my
>> >>>>>> life inside Phabricator so I know it works really well.
>> >>>>>>
>> >>>>>> What's more, github doesn't let you put animated gifs in code
>> >>>>>> reviews.
>> >>>>>> Need I say more?
>> >>>>>>
>> >>>>>> Cheers,
>> >>>>>> Simon
>> >>>>>>
>> >>>>>>> We also have a constant lack of sufficient men power. So, why
>> >>>>>>> spend
>> >>>>>>> effort on building our own infrastructure, which will only
>> >>>>>>> increase
>> >>>>>>> the hurdle for contributors (as they have to deal with an unknown
>> >>>>>>> system)? Let’s outsource the effort to GitHub.
>> >>>>>>>
>> >>>>>>> Manuel
>> >>>>>>>
>> >>>>>>> Simon Peyton Jones <simonpj at microsoft.com>:
>> >>>>>>>> At the moment GHC's main sources aren't on github, which means
>> >>>>>>>> that
>> >>>>>>>> that (in my highly imperfect understanding) people can't submit
>> >>>>>>>> pull
>> >>>>>>>> requests or use their code review mechanisms.  Moreover, most
>> >>>>>>>> people
>> >>>>>>>> don't have commit rights on the main GHC server, so if someone
>> >>>>>>>> wants
>> >>>>>>>> to offer a patch they can really only do so in textual form
>> >>>>>>>> attached
>> >>>>>>>> to Trac. People with commit rights can make a branch, but there's
>> >>>>>>>> a
>> >>>>>>>> danger that over a decade we'll accumulate zillions of dead
>> >>>>>>>> branches
>> >>>>>>>> which people forgot to delete.  I think on github the branch is
>> >>>>>>>> in a
>> >>>>>>>> different repo, belonging to the patch author.
>> >>>>>>>>
>> >>>>>>>> So we really don't have a good work flow for creating, reviewing,
>> >>>>>>>> modifying, and finally apply patches.  I am no expert on these
>> >>>>>>>> matters. If Phabricator would help with that I'm all for it.  But
>> >>>>>>>> perhaps there are other alternatives?  Or is Phab the lead thing.
>> >>>>>>>> Will it stay around?
>> >>>>>>>>
>> >>>>>>>> Also before going too far I'd really like someone to document the
>> >>>>>>>> workflow carefully, and make sure it works from Windows equally
>> >>>>>>>> well.
>> >>>>>>>>
>> >>>>>>>> I'm not too stressed out about losing the review trail of a
>> >>>>>>>> patch.
>> >>>>>>>> Much of it will be commenting on stuff that no longer appears in
>> >>>>>>>> the
>> >>>>>>>> final patch.  Anything that's important should appear in a Note
>> >>>>>>>> in
>> >>>>>>>> the source code; even the commit messages are invisible until you
>> >>>>>>>> really start digging.
>> >>>>>>>>
>> >>>>>>>> Simon
>> >>>>>>>>
>> >>>>>>>> | -----Original Message-----
>> >>>>>>>> | From: ghc-devs [mailto:ghc-devs-bounces at haskell.org] On Behalf
>> >>>>>>>> Of
>> >>>>>>>> | Austin Seipp
>> >>>>>>>> | Sent: 06 June 2014 05:06
>> >>>>>>>> | To: ghc-devs at haskell.org
>> >>>>>>>> | Subject: RFC: Phabricator for patches and code review
>> >>>>>>>> |
>> >>>>>>>> | Hello all,
>> >>>>>>>> |
>> >>>>>>>> | Recently, while doing server maintenance, several of the
>> >>>>>>>> | administrators for Haskell.org set up an instance of
>> >>>>>>>> | Phabricator[1], located at https://phabricator.haskell.org
>> >>>>>>>> |
>> >>>>>>>> | For those who aren't aware, Phabricator (or "Phab") is a suite
>> >>>>>>>> of
>> >>>>>>>> | tools for software development. Think of it like a polished,
>> >>>>>>>> | semi-private GitHub with a lot of applications and tools for
>> >>>>>>>> all
>> >>>>>>>> | kinds of needs. We've been using it to do issue tracking for
>> >>>>>>>> | Haskell.org maintenance and like it a lot so far.
>> >>>>>>>> |
>> >>>>>>>> | One very nice aspect of Phabricator though is it has a very
>> >>>>>>>> nice
>> >>>>>>>> | code review tool, called 'Differential', that is very useful.
>> >>>>>>>> For
>> >>>>>>>> | people who have used a tool like Review Board, it's similar.
>> >>>>>>>> | Furthermore, it has a very convenient userland tool called
>> >>>>>>>> | 'Arcanist' which makes it easy for newcomers to post a review
>> >>>>>>>> and
>> >>>>>>>> | get it merged when it's ready all from the command line.
>> >>>>>>>> |
>> >>>>>>>> | I'd like to see if people are interested in using Phab
>> >>>>>>>> _strictly_
>> >>>>>>>> | for code review of GHC patches. It is a dedicated tool
>> >>>>>>>> | specifically for this, and I think it works much better than
>> >>>>>>>> Trac
>> >>>>>>>> | or inline GitHub comments.
>> >>>>>>>> |
>> >>>>>>>> | Also, Phab can also support post-commit reviews. So if I touch
>> >>>>>>>> | something in the runtime system and just push, perhaps Simon or
>> >>>>>>>> | Edward would like to look, and they can be alerted right when I
>> >>>>>>>> do
>> >>>>>>>> | this, and then yell if I did something stupid.
>> >>>>>>>> |
>> >>>>>>>> | Before I go much further, I'd like to ask: is there *any*
>> >>>>>>>> interest
>> >>>>>>>> | in this? Or are people satisifed with Trac? The primary
>> >>>>>>>> | motivations are roughly, in no particular order:
>> >>>>>>>> |
>> >>>>>>>> |  1) Code review is good for everyone, a good way for people to
>> >>>>>>>> | learn the code and ask questions, and useful to give feedback
>> >>>>>>>> to
>> >>>>>>>> | newcomers. And even experienced GHC hackers can learn things
>> >>>>>>>> from
>> >>>>>>>> | reading code, as we all do regularly, or find things that need
>> >>>>>>>> | cleanup.
>> >>>>>>>> |
>> >>>>>>>> |  2) Phabricator in particular makes it very easy to submit
>> >>>>>>>> patches
>> >>>>>>>> | for review. To submit a patch, I just run the command 'arc
>> >>>>>>>> diff'
>> >>>>>>>> | and it Does The Right Thing. It also makes it easy to ensure
>> >>>>>>>> | people are *alerted* when a patch might be relevant to them.
>> >>>>>>>> |
>> >>>>>>>> |  3) They can be uploaded and created from the command line, and
>> >>>>>>>> | merged easily afterwords the same way. This is particularly
>> >>>>>>>> useful
>> >>>>>>>> | for newcomers, and for me. :)
>> >>>>>>>> |
>> >>>>>>>> |  4) Differential is dedicated to code review, and much better
>> >>>>>>>> at
>> >>>>>>>> | it than just reading patches on Trac IMO.
>> >>>>>>>> |
>> >>>>>>>> |  5) It supports both post-commit code review, as well as
>> >>>>>>>> | pre-commit review. Post commit would be especially useful for
>> >>>>>>>> us
>> >>>>>>>> | too, I think.
>> >>>>>>>> |
>> >>>>>>>> | Point #2 and #3 are mostly relevant for me, because I mostly
>> >>>>>>>> | handle incoming patches. But I think in general it would be
>> >>>>>>>> nice,
>> >>>>>>>> | and make it a lot easier for newcomers to submit patches, and
>> >>>>>>>> us
>> >>>>>>>> | to look over them.
>> >>>>>>>> |
>> >>>>>>>> | Here's an example of a Differential code review:
>> >>>>>>>> |
>> >>>>>>>> | https://phabricator.haskell.org/D4
>> >>>>>>>> |
>> >>>>>>>> | This is a demo using my 'wip/ermsb' patch. You'll need to
>> >>>>>>>> create
>> >>>>>>>> | an account to login, but it shouldn't be much trouble, you can
>> >>>>>>>> | login several ways. I'll fix the login requirement soon. Feel
>> >>>>>>>> free
>> >>>>>>>> | to read the code, comment on it, and play around. It's more of
>> >>>>>>>> a
>> >>>>>>>> | demonstration, but real code review would be welcome too. :)
>> >>>>>>>> |
>> >>>>>>>> | If people are interested in doing this, I can add notes to the
>> >>>>>>>> | wiki pages for newcomers, and I'll send another email about
>> >>>>>>>> Phab
>> >>>>>>>> | so people can understand it a little better. But I want to ask
>> >>>>>>>> | first.
>> >>>>>>>> |
>> >>>>>>>> | There is an argument that our team is so small, code review has
>> >>>>>>>> | unnecessary burdens. But I think Phab could help a lot with
>> >>>>>>>> | tracking outside patches and getting good reviews for incoming
>> >>>>>>>> | patches, and it'll make it easier for newcomers. And
>> >>>>>>>> experienced
>> >>>>>>>> | pros can probably learn a thing as well.
>> >>>>>>>> |
>> >>>>>>>> | Again, to be clear, I don't propose we migrate anything to
>> >>>>>>>> | Phabricator from, say, Trac. There's no real pressure to do so
>> >>>>>>>> and
>> >>>>>>>> | it would be tons of work. I only propose we use it for code
>> >>>>>>>> | review, which is perfectly fine, and how other projects like
>> >>>>>>>> LLVM
>> >>>>>>>> | do code review (they use Bugzilla).
>> >>>>>>>> |
>> >>>>>>>> | I also don't think the usage of Phabricator should be mandatory
>> >>>>>>>> | (unless we decide that later because we like it), but I would
>> >>>>>>>> like
>> >>>>>>>> | to see people use it if possible.
>> >>>>>>>> |
>> >>>>>>>> | [1] http://phabricator.org
>> >>>>>>>> |
>> >>>>>>>> | --
>> >>>>>>>> | Regards,
>> >>>>>>>> |
>> >>>>>>>> | Austin Seipp, Haskell Consultant
>> >>>>>>>> | Well-Typed LLP, http://www.well-typed.com/
>> >>>>>>>> | _______________________________________________
>> >>>>>>>> | ghc-devs mailing list
>> >>>>>>>> | ghc-devs at haskell.org
>> >>>>>>>> | http://www.haskell.org/mailman/listinfo/ghc-devs
>> >>>>>>>>
>> >>>>>>>> _______________________________________________
>> >>>>>>>> ghc-devs mailing list
>> >>>>>>>> ghc-devs at haskell.org
>> >>>>>>>> http://www.haskell.org/mailman/listinfo/ghc-devs
>> >>>>>>>
>> >>>>>>> _______________________________________________
>> >>>>>>> ghc-devs mailing list
>> >>>>>>> ghc-devs at haskell.org
>> >>>>>>> http://www.haskell.org/mailman/listinfo/ghc-devs
>> >>>>>>
>> >>>>>> _______________________________________________
>> >>>>>> ghc-devs mailing list
>> >>>>>> ghc-devs at haskell.org
>> >>>>>> http://www.haskell.org/mailman/listinfo/ghc-devs
>> >>>>>
>> >>>>> _______________________________________________
>> >>>>> ghc-devs mailing list
>> >>>>> ghc-devs at haskell.org
>> >>>>> http://www.haskell.org/mailman/listinfo/ghc-devs
>> >>>
>> >>> _______________________________________________
>> >>> ghc-devs mailing list
>> >>> ghc-devs at haskell.org
>> >>> http://www.haskell.org/mailman/listinfo/ghc-devs
>> >>
>> >>
>> >
>> >
>> >
>> > --
>> > Regards,
>> >
>> > Austin Seipp, Haskell Consultant
>> > Well-Typed LLP, http://www.well-typed.com/
>> > _______________________________________________
>> > ghc-devs mailing list
>> > ghc-devs at haskell.org
>> > http://www.haskell.org/mailman/listinfo/ghc-devs
>> >
>>
>> _______________________________________________
>> ghc-devs mailing list
>> ghc-devs at haskell.org
>> http://www.haskell.org/mailman/listinfo/ghc-devs
>
>

-- 
Regards,

Austin Seipp, Haskell Consultant
Well-Typed LLP, http://www.well-typed.com/