search

microsoft / CodeContracts

Source code for the CodeContracts tools for .NET
Other
883 stars 151 forks source link

Pull requests policies #62

Open SergeyTeplyakov opened 9 years ago

SergeyTeplyakov commented 9 years ago

Hello everyone.

@mike-barnett added me as an owner to this repo and I'm going to start working on pull requests and other stuff.

My team in Microsoft is using Code Contracts library super extensively and I'm going to work on this stuff in near future to get them working with VS14, fix some other bugs, review and merge pull requests etc.

I would like to be as useful as possible and I need some help from you guys. Can you help me to prioritize and review existing pull requests? I would like to see some comments here with a list of most critical pull requests that's are ready to merge and at least one "Reviewed, looks good for me" for each pull requests. (I'll review them myself and for all critical ones I would get Mike's feedback as well).

I'm a big fan of Code Contracts and I personally don't want them to die. So I would do my best to keep the source up-to-date and merge pull requests as soon as I can.

sharwell commented 9 years ago

You should probably make a decision on #61 (for or against) before anything else.

sharwell commented 9 years ago

I'm actively working on updating the Visual Studio extension code to work as a single VSIX seamlessly across Visual Studio 2010 through 2015. It's very similar to one of my other open source extensions so I'm comfortable with the work. I'll have the code finished tomorrow or Wednesday; I'll send a pull request at that point even though the installer will be broken, and we can discuss the best approach to use for that.

SergeyTeplyakov commented 9 years ago

@sharwell for #61: 21K of changes... It seems that this is pull-request fixes #63.

I see that this pull-request already fixes majority of style issues but not all of them. Is it possible to run R# formatter or similar tool to fix spacing issue (use 4 spaces)? Because I would like to have one major fix but not multiple?

SergeyTeplyakov commented 9 years ago

I'm actively working on updating the Visual Studio extension code to work as a single VSIX seamlessly across Visual Studio 2010 through 2015.

Sound awesome!

sharwell commented 9 years ago

@SergeyTeplyakov Aside from the addition of a .gitattributes file, pull request #61 only changes \r\n line endings in the repository to \n. Git always stores text files internally with \n line endings, and then uses settings of the individual developer's system to determine whether they appear as \n or \r\n when they are checked out.

Edit: Ignore me I started it.

SergeyTeplyakov commented 9 years ago

@sharwell My concern that this affect so many files and maybe it would be easier to have only one commit that dramatically affects the code base. What do you think?

P.S. If you'll split the change to separate .gitattributes from \r\n I would merge the first one today.

sharwell commented 9 years ago

@SergeyTeplyakov If the change is split, then Git will not allow users to commit text files without resaving the whole file. It's a major headache.

External merge tools will be unaffected by my change because it changes the way files appear in the database but not the way they appear on disk. GitHub might not automatically merge pull requests made before this date, but you'll be able to merge them on your own machine without problems. When you push a merge commit like that back to GitHub, no one will be able to tell the difference between that, and a commit you merged in the GitHub UI. I can help with the specific commands if it does end up negatively affecting a pull request you want to merge.

Furthermore, my changes to the VSIX packaging are already based on top of the changes in #61 so I would be completely unaffected. :smile:

:memo: If you do a diff of the file system before #61 and after #61, the only files that would actually appear to have changed are files that previously had an inconsistent mix of \r\n and \n line endings. A few such files did exist, but not nearly as many as you'd think from the size of the pull request.

:sleepy: It's late, back for more tomorrow!

SergeyTeplyakov commented 9 years ago

@sharwell Yep, it is late in my timezone as well.

Ok, you convinced me. I'll talk with Mike first, but hope I'll merge this PR tomorrow.

SergeyTeplyakov commented 9 years ago

Merged PR #61.

tom-englert commented 9 years ago

@SergeyTeplyakov: I have created a pull request (https://github.com/Microsoft/CodeContracts/pull/21) that contains many fixes for the contract reference libraries. I already use these with the current version of CC and would like to see them in the next release.

However after merging a fix from @hubuk changes from other people have slipped in. Also my changes have already been merged to other peoples forks. I'm not the GIT expert, so maybe s.o. can figure out how to resolve such cross-fork merges.

Also maybe we should think of some basic rules or do's and don'ts how to use git forks and branches properly.

sharwell commented 9 years ago

@tom-englert I can help with restoring #21 to a clean state. We probably don't need to document the branching model as long as we stay close to one of the most common ones on GitHub, which we currently are. The basic idea is just this:

  1. Fork the repository to your profile
  2. Create a new branch which will contain your changes
  3. Commit your proposed changes to the branch
  4. Push the branch to your fork
  5. Send a pull request

The problem with #21 is you used master for step 2/3. The branch is in your fork, so you were able to send a pull request, but failing to use a unique branch for just the contract changes meant future commits to tom-englert/master also showed up in the pull request. It's a problem that appears from time-to-time, but once you get the pattern down you'll be able to work on most GitHub projects. If we were to make some nice documentation for brand-new users, it would probably make sense to do so at a very high level, such as a document written (or aggregated) by the .NET Foundation.

SergeyTeplyakov commented 9 years ago

@sharwell Agree about branching model. We had a problems because no one merged PR's. Merging them on time will eliminated majority of issues.

As soon as you're ready, ping me, I'll merge updated PR as well.

tom-englert commented 9 years ago

@sharwell I doubt using a different branch for 2/3 would have made a difference. It was intended for all my changes to go into this pull request, since all are fixes for reference contracts. The problem arose when I had to merge a fix from hubuk, which indirectly merged all changes from others that had been merged into his branch before. Restoring a "clean" state would mean removing all changes that were not done by me, except for one single commit by hubuk. How would that work?

sharwell commented 9 years ago

@tom-englert If I were working on a feature like you describe, I would have done the following locally:

  1. Create a branch fix-reference-contracts (other people use other names)
  2. Commit the changes for reference contracts to the fix-reference-contracts branch
  3. Push the fix-reference-contracts branch to my fork on GitHub, and submit a pull request
  4. Create a branch tmp-working which will include multiple pull requests that have not yet been merged (after they are merged into master I won't need the temporary working branch)
  5. Merge fix-reference-contracts into tmp-working
  6. Merge hubuk's branch into tmp-working
  7. Merge any other PRs I need into tmp-working

In the above set of steps, fix-reference-contracts always reflects the content of the pull request to update reference contracts, and tmp-working allows me to keep making progress even if that branch (or others) hasn't been merged yet.

Restoring a "clean" state would mean removing all changes that were not done by me, except for one single commit by hubuk. How would that work?

Let me clone your fork and take a look at its current state. I'll get back to you.

tom-englert commented 9 years ago

Let me layout the problem like this:

It would not have helped merging hubuks change into tmp-working, as I needed it in my fix-reference-contracts branch, because my commit 5 based on that. The clean solution would have been to merge only the one and only commit from hubuk that contained his fix - would GIT allow this? I didn't find any command that would do that.

sharwell commented 9 years ago

@tom-englert In your scenario, there are two things to look into:

Don't stress too much about these issues. I'm still trying to get some of the basic build infrastructure back in shape but I will definitely help you pull out the contract changes so you can create a new PR with them.

sharwell commented 9 years ago

@tom-englert Not sure how you're using Git, but I'll give you my Number 1 Git Pro-tip.

Use Git Extensions¹

When you use the GUI, you'll always be able to see which branch you are working in along with its relation to all other branches. It makes it much more difficult to make commits where you aren't expecting them.

I avoid the command line to the maximum extent possible, which in practice means the only time I use the command line is when the code review tools used by a project were not designed to work well with Git. I don't think I've ever needed the command line for working on a GitHub project.

¹ GitHub for Windows is not a viable alternative at this time, because it will not provide you with the same consistent high-level visual overview alongside access to all the standard Git commands.

tom-englert commented 9 years ago

@sharwell But my intent was to include all 1-5 in the push, and not only have it in my private tmp-working

tom-englert commented 9 years ago

@sharwell I've fixed it the brute-force way - rollback until the merge and then restored my own two commits. I think now it's ready to be pulled.

tom-englert commented 9 years ago

@SergeyTeplyakov https://github.com/Microsoft/CodeContracts/pull/21 is hanging around now for a very long time, with already most of it reviewed by others. I think this should be merged soon before more conflicts arise.

SergeyTeplyakov commented 9 years ago

@tom-englert Due to recent changes (maybe that was me today), I can't merge this PR automatically.

If you could rebase this PR with recent changes I'll merge it today.

tom-englert commented 9 years ago

@SergeyTeplyakov as @sharwell has stated in his comment in #21 he will take care, and I'm fine with this.