On Wed, 18 Jul 2012 22:37:56 -0700, Daniel Friesen
<[email protected]> wrote:
On Wed, 18 Jul 2012 21:35:00 -0700, Roan Kattouw
<[email protected]> wrote:
On Wed, Jul 18, 2012 at 9:30 PM, Subramanya Sastry
<[email protected]> 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)
--
~Daniel Friesen (Dantman, Nadir-Seen-Fire) [http://daniel.friesen.name]
_______________________________________________
Wikitech-l mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
