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
# app/models/changeset.rb
class Changeset < ActiveRecord::Base
# ...
end
# app/models/repository/darcs.rb
class Repository::Darcs < ActiveRecord::Base
# Deep inside the fetch_changesets method
revision.paths.each do |change|
Change.create(:changeset => 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 < ActiveRecord::Base
# Deep inside the fetch_changesets method
revision.paths.each do |change|
Change.create(:changeset => 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 < ActiveRecord::Base
# Deep inside the fetch_changesets method
revision.paths.each do |change|
Change.create(:changeset => changeset,
:action => change[:action],
:path => change[:path],
:from_path => change[:from_path],
:from_revision => change[:from_revision])
end unless changeset.new_record?
# ...
end
After
# app/models/changeset.rb
class Changeset < ActiveRecord::Base
# Creates a new Change from it's common parameters
def create_change(change)
Change.create(: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?
