On Wed, 18 Jul 2012 22:37:56 -0700, Daniel Friesen lists@nadir-seen-fire.com wrote:
On Wed, 18 Jul 2012 21:35:00 -0700, Roan Kattouw roan.kattouw@gmail.com wrote:
On Wed, Jul 18, 2012 at 9:30 PM, Subramanya Sastry ssastry@wikimedia.org wrote:
(b) Commit amends hide evolution of an idea and the tradeoffs and considerations that went into development of something -- the reviews are all separate from git commit history. All that is seen is the final amended commit -- the discussion and the arguments even that inform the commit are lost. Instead, if as in (a), a topic branch was explicitly (or automatically created when a reviewer rejects a commit), and fixes are additional commits, then, review documentation could be part of the commit history of git and shows the considerations that went into developing something and the evolution of an idea.
There was an email recently on wikitext-l where Mark Bergsma was asked to squash his commits (http://osdir.com/ml/general/2012-07/msg20847.html) -- I personally think it is a bad idea that is a result of the gerrit review model. In a different review model, a suggestion like this would not be made.
Although squashing and amending has downsides, there is also an advantage: now that Jenkins is set up properly for mediawiki/core.git , we will never put commits in master that don't pass the tests. With the fixup commit model, intermediate commits often won't pass the tests or even lint, which leads to false positives in git bisect and makes things like rolling back deployments harder.
Roan
I don't see this as a case against not-squashing commits. The fact that previous commits are broken is not a bug. It's a fact of development and history that MUST be accepted rather than covered up. Making sure that every single commit inside the repo passes test does NOT matter. The only thing that matters is that final commits that we merge into the master branch pass tests. In other words only the actual commits (ie: merge commits) directly committed to the master branch should be required to pass tests, not the potentially broken commits they are based on. If you are bisecting and trying to track down an issue you should NOT be randomly going down every topic branch and testing it. If you test the commit from which the topic branch was merged into master and it comes back OK then you should ignore the topic branch since those were never directly part of master and are not the source of your bug. You should continue down master until you hit the topic/merge commit that introduced the bug directly into master.
Frankly while it may look odd my recommendation is to make use of --no-ff to ensure that the commit that actually goes into master is not the HEAD of the topic branch but a merge commit. Even if there is no rebasing or merging needed. This helps keep a clean line where you can trust that everything inside master was explicitly committed there. And instead of having random cases where either a commit is directly put into master or you get some commit saying something like "merged from" just because of some conflicts. You instead end up with your merge commits becoming a proofread and edited version that's gone through the final review, testing, and been approved by everyone.
One of the ideas I had for Gareth was to let a commit message be web-editable on the review page. The commit message would be based on the original commit's commit message. Afterwards anyone could contribute to the commit message in the web view and make tweaks to it. Instead of forcing someone to amend a commit to change the commit message you would just make a tweak to the commit message displayed on the review page. You could easily re-word it a bit, add some items not included in the original (eg: you added a new feature in a follow up commit), and fix any typos the original committer made. When the review/topic branch is okayed and accepted into master Gareth would use --no-ff to force Git to create a brand new commit for the merge and it would use the commit message that everyone made edits to as the commit message for that merge commit instead of some "merged from" message. Then master would become a clean vetted linear history. And when you wanted to learn the history of an individual addition to the codebase you would follow the parents of that commit into the review branch that was merged into master.
Here's an example of the flow I mean: https://www.mediawiki.org/wiki/User:Dantman/Git/FastForward-vs-no-ff
On the left is the normal project flow. You make changes to the topic branch and at the end merge it into the master branch. When you do so because there are no conflicts all the commits in the topic branch become part of master. Not necessarily what you want.
On the right is the flow I consider ideal for large projects like MediaWiki. The pattern follows the exact same pattern as the left. You make changes into the topic branch and then at the end merge it into master. But this time when you do the merge (or rather the review system does) you use --no-ff and add a customized message with an overview of all the changes in the topic branch that got merged into master (This essentially becomes your proofread and edited final commit that everyone sees). As a result of --no-ff: - None of the individual changes which weren't guaranteed to have passed tests made their way into master. - You have a very clean overview of the branch as the actual commit in master leading to a tidy history that everyone can understand. - The entire history of the project is contained, including the small tweaks that everyone has made. And no-one ends up overriding a commiter's name with their own because they fixed a typo. - Anyone trying to bisect can simply ignore the individual irrelevant commits inside of a topic branch and just test master itself. (In fact at this level you can probably find some way to use a test-suite to auto-bisect a codebase using a simple test case and running it over the history of the repo)