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.