Skip to content

Commit

Permalink
Merge pull request #67 from kaeluka/master
Browse files Browse the repository at this point in the history 
Add some text about Git usage
  • Loading branch information
@albertnetymk
albertnetymk committed Jan 31, 2015
2 parents 9bb8f28 + def6788 commit 2001e7a
Showing 1 changed file with 7 additions and 0 deletions.
7 changes: 7 additions & 0 deletions CONTRIBUTING.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,13 @@ If you taking the time to mention a problem, even a seemingly minor one, it is
greatly appreciated, and a totally valid contribution to this project. Thank
you!

## Git

Please do **not** push to the `master` branch. Rather than that, clone the
encore repository in your own github account, push there and send a pull
request. This allows for code to be reviewed before it gets into `master`. Also,
when implementing a large feature, please do use a feature branch.

## Guidelines (taken from [here](http://www.booleanknot.com/blog/2013/09/07/pull-requests.html))

### Stay on target
Expand Down

25 comments on commit 2001e7a

@supercooldave
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can someone explain why we not use our own cloned version of the repository rather than just a local copy?
(Or point me to the explanation why.)

@TobiasWrigstad
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Equivalent solutions mostly. But if you clone this into your own account, you can have others working on it there in parallel without having to coordinate via the master repo. As far as I understand.

@supercooldave
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like a complicated solution, especially when a feature branch seems to do the trick.
And for small changes it seems quite likely that we'll have many divergent repos very quickly.

@kikofernandez
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the fork model works well for big changes such as a new feature. For instance, let's say @EliasC is working on Capabilities and his branch is quite behind the master branch. This creates a problem in the long run since he needs to merge with master at some point and could get a whole lot of conflicts. Ideally, he could rebase his branch on top of master.
On the other hand, working with forks means that your work is isolated from the main repo and you are forced to rebase on top of master. The common situation, in this case, is to have two remotes, origin (your fork of the repo) and upstream (the original repo). You should always rebase your work on top of upstream/master to get all the updates from the original repo and fix small merge conflicts as they come, instead of doing the merge when you want to incorporate your changes into master.

For small changes, I told @EliasC that he should commit to master directly.

To sum up, fork works great for external developers. If we think this model is confusing we can use the branch model (with rebasing on top of master to get the latest changes). Nonetheless, I still suggest doing pull requests when there's a new feature added or a change that you would like to be reviewed by someone else. The good thing about the fork model is that I (the team) should not merge a pull request unless the changes on the pull requests have been rebase (this guarantees that merging the pull request succeeds without any merge conflicts).

@supercooldave
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand correctly, a good model is branching for the core developers and forking for external developers, where in both cases rebasing and pull requests should be the norm (except for minor changes).

@kaeluka
Copy link
Contributor

@kaeluka kaeluka commented on 2001e7a Feb 1, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are external and "core" developers not treated the same way?

@albertnetymk
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The difference of "core" developers and external contributors would become more clear once this repo becomes public. After all, we can't give everyone the permission to push to this repo. Then, the only way external contribution could come in is through "fork".

@supercooldave
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition to Albert's comment, I would say that "core" developers are trusted (we trust ourselves to do the right thing), whereas externals may not have the same level of expertise or same vision as us.

@kaeluka
Copy link
Contributor

@kaeluka kaeluka commented on 2001e7a Feb 1, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What the model with forks allows is that two developers that may implement a feature together can collaborate with each other without causing much noise on slack (by pulling from each others' clones, rather than upstream). It'd be great if whatever we replace the above text with would at least allow such a workflow.

@supercooldave
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want to work with that variant of the model when collaborating on a feature, then that's fine, though probably unnecessary.
The noise-on-slack problem is unrelated to how we run the git -- that's a slack configuration issue.

@kaeluka
Copy link
Contributor

@kaeluka kaeluka commented on 2001e7a Feb 2, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ noise on git: it is related -- slack monitors parapluu/encore. So if there's a push to alice/encore and then a pull from there to bob/encore, this avoids generating any noise and avoids knowingly pushing not-done-yet code to upstream.

For more details, see here http://nvie.com/posts/a-successful-git-branching-model/#decentralized-but-centralized

@supercooldave
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's such a problem, reconfigure the slack settings so that you don't get notified.
Or maybe move the checkin notifications out of general into another slack channel, and then turn off notifications for you.

Using slack notifications to motivate a particular git convention is unconvincing.

@kikofernandez
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the best way to move forward is to test different approaches.

Since we have already been doing something similar to what @supercooldave proposed, I would strongly suggest using forks and pull requests. We test this for a period of 2 weeks. 2 weeks it's enough to suffer and/or see the benefits of this approach. If after two weeks we consider that it was more of an inconvenience rather than a benefit, we discuss it and add some changes.

If we want pull requests (code review) to work, we need to take it seriously. The reviewer needs to check the code and not just click on the merge button. Possible cause for not doing a merge are in the CONTRIBUTING.md

I believe that, as soon as we consider the merge with the new PonyRT a success, we should start using the fork and pull request model.

@EliasC
Copy link
Contributor

@EliasC EliasC commented on 2001e7a Feb 6, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kikofernandez What is the benefit of using forks over branches for us in the "core-team"?

@kikofernandez
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there will be no noise in the master repo (by noise I mean branches dangling there, currently we have 20 branches and I don't think that all are alive).
The other reason is that we force ourselves to do code review whenever you think that something is ready to be merged.

As I mentioned, I believe the best way to test if something works is to test it! If after 2 weeks we think that this does not work, then we try another workflow (core-team uses only branches, mixing up a little bit of git-flow, using git-flow, removing pull requests for core-team, etc).

For me, the main point is to dare to try new things. If some people in the team would like to test a new workflow that might make sense, we should test it and check if it works. Of course, if only one person wants to change something then tough luck :)

@kaeluka
Copy link
Contributor

@kaeluka kaeluka commented on 2001e7a Feb 6, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be on board with trying it for a certain time. In general, I think that there's little reason to treat the core team any different than students, we all make mistakes and we all write code that should be reviewed. This request (#74) is an example: the change is not "broken", but the request gets discussed before it's in the feature branch. In this case it got merged anyway, but people now have some idea of how to change it in the future and that there is a reason for things being as they are.

Besides that, having my own fork has worked well for me, with little overhead and it has made me think harder about what goes into a commit and what doesn't (anticipating code review). Not allowing myself to push to a feature branch to upstream/some-branch (== encore/some-branch) directly is useful because it keeps the commits I'm writing more focused on the task at hand. Your mileage may, of course, vary.

Also, it's an excellent way to learn git, so even if we don't continue this after two weeks, the git practice may be useful.

Edit:
Some clarifications:

  • I should add that the overhead I'm experiencing is mostly that I sometimes have to ask @kikofernandez to double check what I'm doing and to clarify what's happening. So I will probably stay with the model even if the team does not (if the coding standard permits).
  • A big advantage of the fork model is that it allows me to pull half-baked changes from @EliasC (or someone else) and we work on them together. Only when we've reached a state where everything works, we push to upstream/feature-branch. This way, we can get the invariant that it's always safe to pull from upstream/feature-branch -- which I consider very important.
  • TL;DR: Forking costs little, it subjectively increases quality, it allows for useful workflows and avoids common pitfalls (push broken stuff to upstream/feature-branch just so @EliasC can continue working on it. Better make it a habit now, than suffer the consequences later.

@supercooldave
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With all due respect, I think there are very strong reasons for treating the core team differently from students. The main ones are that we have vastly more experience and we are working together with a common vision. Students may "fix things" that they shouldn't touch, add things that we may not approve of, or simply break things while going about their work. Not to say that we won't do those things, but we have a better idea of the bigger picture than students do (no offence students who may read this). I'd rather allow a few small mistakes to creep in than to add further hindrances to my workflow.

Feel free to work with whatever model you see fit.

@kikofernandez
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I vote for a dictatorship model for 2 weeks (we all follow the same model)! We give it a try and act according to our experience. We might (or not) suffer for two weeks but, we will get a better understanding about git and how to use it in the future.

No pain, no gain! :)

@supercooldave
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the dictatorship model?

@EliasC
Copy link
Contributor

@EliasC EliasC commented on 2001e7a Feb 6, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm all for pull requests and code reviews, but a bit more sceptical about forks over branches. I could give it a try for two weeks, but I suggest waiting until after Brussels with this to not "waste" precious time learning git (and rather waste unprecious time later).

@kikofernandez
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dictatorship model, we all follow the same model ( no exceptions) and check if it's worth it, vs the hippy (chaotic) model, were everyone does what he/she wants. It's a way to "try things together" as a team

@TobiasWrigstad
Copy link
Contributor

@TobiasWrigstad TobiasWrigstad commented on 2001e7a Feb 6, 2015 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@supercooldave
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Veto.

@supercooldave
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry. Just being a dicktator.

@kikofernandez
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dictatorship model and we can all have an objective opinion about forks vs branches

Please sign in to comment.