contributions and applying changes in master branch

[tiagovignatti]

For the changes in master, I think we need to decide something and then write down so eventual developers can read and contribute easily with the project.

developers need:

1. set up a pull request.
2. get review from a OWNER; meaning a LGTM and wait them to merge.

owners need:

1. unless critical build fix nothing should go into master branch without the review from another owner. Does this work?

[kalyankondapally]

LGTM. We can always learn and improve with feedback as people get involved.

[kalyankondapally]

I would also propose this:

While merging changes to fix builds, one should always include this at start of commit message:
Unreviewed Build fix.

This would make it clear for any developer(other than original patch contributor) as to why changes where merged without any review.

[kalyankondapally]

We also need to figure out as to how we should deal with situations similar to recent DesktopRootWindowHost commits.

How about this:
1)Fast implementation which fixes the build/functionality.
2)Set up a proper review request for rest of team (with proper solution or with the same patch).

[tiagovignatti]

@kalyankondapally PTAL in d7db1fd and feel free to close this issue if appropriated. Thanks.

[kalyankondapally]

LGTM. Closing this one.

[kalyankondapally]

Re-Opening.

One issue with pull requests: It could add un-necessary noise to commit log (with un-necessary commits in the branch):

Consider the scenario:
1)A pull request is done.
2)Owner does review and leaves behind comments which need to be fixed.
3)Contributor does the changes and he pushes them to the branch.

This process might continue till the final outcome in achieved(which may mean n no of commits).

I am bir worried that this would make un-necessary noise in commit log. History/Discussion can always be seen from the closed issues.

I am proposing the following after getting a LGTM on current pull request:
in case branch contains more than one commit, one should set up a separate branch (which wouldn't need any review) but the branch can be merged directly(this would also help in case the patch needs re-basing).

Commit message can include:
First Line: Issue/Pull Request no. (Original pull request no where all the discussion happened)
short description as to what it fixes.
Reviewed by: . -> This would help as any other owner could just merge the patch.

[tiagovignatti]

"I am proposing the following after getting a LGTM on current pull request:
in case branch contains more than one commit, one should set up a separate branch (which wouldn't need any review) but the branch can be merged directly(this would also help in case the patch needs re-basing)."

I thought we already been doing this, no?

[kalyankondapally]

currently, we would merge the same branch having " having n commits" but what I am proposing is to ask contributor to create a new branch with the changes after he gets LGTM.

1)Set up a pull request.
2)Go through review process, get a LGTM.
3)If now the branch has more than 1 or 2 commits(lets say around 10 to 15 commits with review feedback), we would merge the branch with all the commits. This could make it hard to follow relevant commits.

What I am proposing is at Step 3 contributor can set up a separate pull request with the final patch and pointing to the pull request(where discussion happened) in his commit log. Any Owner would be able to merge it.
I have been trying to find best practices for GitHub and one useful link I came around was this
http://codeinthehole.com/writing/pull-requests-and-other-good-practices-for-teams-using-github/

[kalyankondapally]

If we do agree on this route but think that it's an un-necessary over burden for the contributor(i.e rebasing and publishing a new branch, closing two pull requests.), than the owner doing the merge can take care of this.(Hopefully we are able to automate it some how.)

[tiagovignatti]

ah, I understood you now -- it's all about cleaning up a patch set after comments were made in there and presenting another set instead just pilling up new quick fixes on top of it.

I just thought that this was some kind of implicit rule that every experienced owner should follow :) Jokes apart, if you feel like, you can propose an expansion to the: https://github.com/otcshare/ozone-wayland/blob/master/README.md#contributing . I was trying to be concise at first in that section and I think that's how we should be on the whole README.md after all.

[kalyankondapally]

cool, sure I will add and close this issue.

[tiagovignatti]

let's do the review before you close and apply. tkx!

[kalyankondapally]

This is something we are going to try out:
1)Push a new branch with changes.
2)Ask Owner to review and comment on the branch.
3)Go through the review process till one gets LGTM.
4)Make a pull request with the final patch.

One thing we still need to figure out is how we track the history (or relevant discussions) for commits as we would need to close branch after the pull.

What I mean is:

I upload a branch and all review happens there.
Now, I get a LGTM and set up a pull request with final patch.
The pull request is merged and commit history will always point to this pull request.
Now, we close the branch(we dont want to be hanging around with "n" branches).

Lets say down the line, we want to go back to a commit and understand the rationale behind the change. Now the history is lost as branch is closed. Is there a way we can preserve the history?
In case of pull request it seems to be always present in closed pull requests.

Thats one reason I proposed earlier (on top) for two pull requests:
1)Have a pull request.
2)After LGTM you should make another pull request if your current pull request(created in step 1) ends up with more than 1 commit. Use 'Fixes #Id of pull request in step1' (Need to try this out) at bottom of the message. Use common sense.

Either of the approaches are fine for me as long as we are able to preserve history of a commit.(Discussion during review process)

[tiagovignatti]

in the following model, the history of each git commit remains intact with one only merge commit in the top of the tree, presented by the developer. So in the section "users and developers" should:
1 ... (same as it is in the current README.md)
2 ...
3 ...
4. address all the review points, until one OWNER gives the LGTM.
5. clean up the branch, separating the solution in several semantically related commits and send a pull request

what do you think?

[kalyankondapally]

I moved this section from README to our wiki page as the section was getting too long. I will update the README file accordingly.

[kalyankondapally]

I was looking for some way to share this information on our IRC channel and wiki fits perfectly. I will add the link to our irc channel once we are public

[kalyankondapally]

Closing this now, feel free to fix anything in wiki.