Pull request workflow

[Marcus Brubaker]

Hey guys,

Two thoughts on the pull request workflow now that it looks like
master is back in working order.

First, I was thinking that we should have some guidelines on when we
should do pull requests. It seems like overkill to do it for really
small and obvious changes/bug fixes. Here is a proposal, comments
welcome:

Pull requests are not strictly needed if then changes:
- are sufficiently small and/or obvious
- consist of things like typo fixes, simple bug fixes, code
reformatting, documentation, etc.

Pull requests should be used if the changes:
- touch a subsystem you don't usually play in (e.g., if I started
futzing around in the parser)
- may change exposed/documented behaviour
- alter the available function signatures/distributions
- ... or generally if you're not sure

The other thing is how pull requests should work. E.G., if you submit
a pull request, and someone else gives it a thumbs up who should merge
it. Or, if no one says anything, what's the default behaviour? I
would suggest that we leave the final merge up to whoever submitted
the pull request for regular contributors with commit access. If no
one comments within a reasonable window, I would take it as a green
light. For pull requests from contributors without commit access,
whoever comments should be responsible for doing the merge unless they
explicitly say otherwise (e.g., "This looks good to me, but it touches
the parser so Bob you should look it over and do the merge").

What we want to avoid is pull requests becoming a major drag on
development. We should also make sure that external pull requests get
quick feedback. It's bad form to leave other peoples contributions
languishing for too long. In fact, looking now, we already have one
pull request which hasn't been commented on by anyone for over a week.

Thoughts?

Cheers,
Marcus

[Ben Goodrich]

On Tuesday, May 14, 2013 5:11:09 PM UTC-4, Marcus Brubaker wrote:

First, I was thinking that we should have some guidelines on when we
should do pull requests.  It seems like overkill to do it for really
small and obvious changes/bug fixes.  Here is a proposal, comments
welcome:

I agree. I think there are at least three kinds of changes

-- Those that don't need any additional action, such as changes to TO-DO, fixing typos in the manual, etc.
-- Those that the pusher should verify pass test-unit but doesn't need someone else to review, such as fixing a build failure, adding more tests, etc.
-- Those that need human review in which case anyone should have the opportunity to raise objections before they go into master

But pretty much anybody should be able to push pretty much anything into a branch that is not master. The big problem is that our workflow has largely been to commit directly to the master branch and push to origin/master as if we were using SVN. Git and GitHub support lots of different workflows such as

gitflow: https://github.com/nvie/gitflow
NoSY:  http://root.cern.ch/drupal/content/suggested-work-flow-distributed-projects-nosy
gerrit: which was recently compared to pull requests from GitHub at http://julien.danjou.info/blog/2013/rant-about-github-pull-request-workflow-implementation

and a bunch of others.

Furthermore, an approved pull request is not much good if it includes individual commits that break the build or are otherwise not good. These should be squashed before pushing or pulling anywhere.

Ben

 

 

[Bob Carpenter]

That'll give me a place to start. And thanks for your
patience --- this must be extremely frustrating
from your point of view.

I've read that nvie one several times and I'm still
confused about what you think's going wrong, but I'll
give it another go. It seemed to be about branching
and merging, not about code review. Just making people
branch and merge isn't going to solve the problem with
broken builds and premature commits.

Is there a way to set things up so some pushes (like
doc and tests) are allowed, but not other ones? Part of
my confusion is that I don't understand git or GitHub
very well.

- Bob

> --
> You received this message because you are subscribed to the Google Groups "stan development mailing list" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to stan-dev+u...@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
>
>

[Michael Betancourt]

> -- Those that don't need any additional action, such as changes to TO-DO, fixing typos in the manual, etc.
> -- Those that the pusher should verify pass test-unit but doesn't need someone else to review, such as fixing a build failure, adding more tests, etc.
> -- Those that need human review in which case anyone should have the opportunity to raise objections before they go into master

I don't agree. If master is in a stable state then the first two options shouldn't happen. The problem is that we never let master get sufficiently stable so we're always playing catch up with quick commits.

If we stick to the pull request paradigm for a bit then master should settle down and we'll only need the last option. Refactoring or new features don't get added until they're fully unit and compile tested (with the relevant parts of the to-do list and manual updated as well).

[Marcus Brubaker]

I took a quick look over gitflow and I think from the perspective of
that workflow model
(http://nvie.com/posts/a-successful-git-branching-model/), we've never
really had a master branch and we've been treating our master branch
as the develop branch. If we followed the gitflow model, while the
develop branch was churning with new features, we could have still
branched master for hot fixes and point releases.

I'll look over the other docs when I get some more time, but so far
using something like gitflow seems very reasonable to me. Master
would remain stable at all times, features would be developed in their
own branches and, when done, merged into develop which would churn
and, when settled, would periodically get merged back into master at
points (maybe not always triggering a release, but rather just marking
a point of stability in the development cycle).

Cheers,
Marcus

[Ben Goodrich]

I agree that we have to get master stable enough to release v1.3.1 or v1.4.0 or whatever and then make the workflow better. But I think things like 1) and 2) are inevitably going to happen sometime. What about commits like "I added a new example under src/models/misc ?" To me, if the committer has verified that the example runs, it doesn't need someone else to re-verify that. If in fact the example doesn't run on Windows or wasn't that good, then it will get noticed at some point. But it is not like adding the example is going to break some other part of Stan, so is that really worth getting an approval for a pull request in expectation?

Ben