Working off of Caliper’s flay report for Redmine, I was able to refactor another code duplication.
The Refactoring
Redmine supports connecting to seven different source control systems to import their changes (e.g. git, subversion, mercurial). When Redmine runs the import, it adds each Changeset (a commit) into the database and also information about each Change (e.g. file modification, file added, etc) using the fetch_changesets
method.
The fetch_changesets
method for the darcs, mercurial, and subversion repositories all used the same code to add a new Change to the database. Literally, a copy an paste of one another.
Before
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 |
# app/models/changeset.rb class Changeset < ActiveRecord::Base # ... end # app/models/repository/darcs.rb class Repository::Darcs changeset, :action => change[:action], :path => change[:path], :from_path => change[:from_path], :from_revision => change[:from_revision]) end # ... end # app/models/repository/mercurial.rb class Repository::Mercurial changeset, :action => change[:action], :path => change[:path], :from_path => change[:from_path], :from_revision => change[:from_revision]) end # ... end # app/models/repository/subversion.rb class Repository::Subversion changeset, :action => change[:action], :path => change[:path], :from_path => change[:from_path], :from_revision => change[:from_revision]) end unless changeset.new_record? # ... end |
After
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 |
# app/models/changeset.rb class Changeset self, :action => change[:action], :path => change[:path], :from_path => change[:from_path], :from_revision => change[:from_revision]) end end # app/models/repository/darcs.rb class Repository::Darcs < ActiveRecord::Base # Deep inside the fetch_changesets method revision.paths.each do |change| changeset.create_change(change) end # ... end # app/models/repository/mercurial.rb class Repository::Mercurial < ActiveRecord::Base # Deep inside the fetch_changesets method revision.paths.each do |change| changeset.create_change(change) end # ... end # app/models/repository/subversion.rb class Repository::Subversion < ActiveRecord::Base # Deep inside the fetch_changesets method revision.paths.each do |change| changeset.create_change(change) end unless changeset.new_record? # ... end |
Review
Even though this duplication looks like it was caused by a copy and paste, notice that the Subversion version has an extra guard to make sure the Changeset was saved. That tells me that at some point in it’s history, there was a bug that needed the guard in Subversion. Sure enough git blame
shows that exact conclusion in commit 7bd4590cd.
So why wasn’t it ported to the other versions? Might they have similar bugs?