On Wed, 18 Jul 2012 21:35:00 -0700, Roan Kattouw <roan.kattouw(a)gmail.com>
wrote:
On Wed, Jul 18, 2012 at 9:30 PM, Subramanya Sastry
<ssastry(a)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.
--
~Daniel Friesen (Dantman, Nadir-Seen-Fire) [
http://daniel.friesen.name]