Proposal for moving WebKit development to Git

Benefits of moving to Git

• A large number of WebKit devs are already using git-svn.
• All major organizations involved with WebKit development have significant sub-sets of devs already using git-svn.
• Would make code review easier:

Discussion

Bugzilla could be modified to point to a private branch for code review, making code review easier and more thorough.

I can't see how a private branch makes reviewing easier or more thorough. From the reviewer's point of view the process would be almost identical.

WebKit common practice right now is to use two different types of system for code review - pastebin and attaching a patch to bugzilla. Pastebin has the advantage that it is typically fast and allows instant gratification since the code review is done in realtime over IRC, but a major disadvantage is that review comments and discussion are lossy. Attaching a bug to the bugzilla solves this since code review comments can be captured for posterity in the actual bug report. However, it is not as convenient and fast as pastebin.

Using Git private branches for code review has several advantages over both of our current common code review practices. First, it can be just as fast and convenient as pastebin since a developer need only push his private branch and post the url to his IRC client. Second, using GitHub or Gitorious the private branch can record review comments for posterity. You can click on a line number and record your comment inline with the patch and if/when the developer modifies the branch later your comments still persist and you can easily check if the developer has fixed according to your comments. What's more, using Git private branches offers several benefits above and beyond the current bugzilla approach. With Git private branches you are not just looking at the patch. You can also review the patch in the context of the entire branch. You can look at the patch, view the file, look at its history, the previous changesets and the entire development history of the branch and generally do any research you want because you have the full branch right there.

Finally, there is one other thing that Git makes much easier than our current form of code review: you can easily checkout/merge the branch itself and actually test the changes very cheaply. While in theory we can do this with our current code review process the cost involved (downloading the patch file, cleaning up your own repo, applying the patch, manually fixing any merge problems) makes it much rarer than need be.

For more see here: ​Code Review Should Be Free

As a patch author, what would I do if the reviewer asks for a change in a patch which I already have committed to a branch, with more patches on top of it?

We would want to work out a full strategy for handling patch branches. The easiest way to handle this is to treat patch branches as temporary. The author can rewrite an earlier patch in a series using "git rebase -i", and then notify the reviewers that the branch has changed. The reviewers would then re-download the branch. Unfortunately they couldn't simply pull to their existing copy of the branch since the history has changed - this is a drawback. (However, if they were just looking at the code on the web instead of downloading and building their own copy, this wouldn't affect them, and reviewing directly through gitweb would be pretty common.) Another drawback is that this would overwrite the history of changes to the patch and leave only the final result.

Another option is to copy your existing branch (which is very cheap) and then make the changes to the new branch, again with git-rebase, then publish the new branch. For this we would probably want to define a standard naming scheme for patch review branches, such as including an iteration number or the date in the branch name, to avoid a confusing mess of names.

evmar's doc describing ​Chrome's git/reitveld workflow discusses some of these issues in more depth

• Would allow us to do away with manually editing/resolving ChangeLog files:

Discussion

I believe the current practice of saving commit logs on disk to ChangeLog file(s) originated to ensure that commit logs received review too. Git renders this unnecessary since pushing to a private branch will show all commit messages and thus they can be reviewed before the changesets are pushed to the primary repository. And if a commit message was found wanting it is easy to adjust with 'git commit --amend'

• Distributed nature of git allows to commit even when server goes down.
• Those who are working far away / a slow connection can have local git mirrors rather then going through the one svn server (A real issue today).
• Branching is cheap with git which would encourage the practice of developing unstable features in smaller chunks. This would also help to reduce the number of build breakages, as branches that require changes in the platform layers can be done and tested outside of trunk. When all platforms build the changes can be merged to trunk.
• We have a significant amount of git knowledge amongst various WebKit devs for training the remaining devs.
• WebKitTools/Scripts already contains lots of git coverage.
• Using GitHub or setting up our own gitorious server for WebKit would allow others to more easily see the different projects that are going on.
• Please fill in here...

Costs of moving to Git

• X number of WebKit developers do not know git and would have to learn a new tool.
• Git itself can be hard to learn and hard to use.
• Please fill in here...

Problems to be solved

• Bugzilla integration
• A common suggested workflow would need to be worked out.
• An option for contributing for those who are unwilling/unable to learn git? (​Easy Git?)
• Please fill in here...

TODO

• Unsure of the current status of Git on Windows and how many Windows developers we have.
• Find out just how many git users we have who actively contribute to WebKit. (Educated guess puts us at 20% git users?)
• Find out how many svn users we have and their feelings on moving to git.
• Please fill in here...