Daily Refactor #7: Extract method in fetch_changesets

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?

Reference commit