Merge branch 'handle-invalid-submodule' into 'master'

Handle invalid submodule

Add better error handling to the submodule processing in `Gitlab::Git::Repository`:

* Return an empty hash from `#submodules` when the specified commit doesn't have a `.gitmodules` file.

* Omit entries from the return value of `#submodules` when the entry in `.gitmodules` has an invalid path.

Fixes #7, gitlab-org/gitlab-ce#714, and gitlab-org/gitlab-ce#763.  Also fixes one of the recent [comments](https://gitlab.com/gitlab-org/gitlab-ce/issues/333#note_367106) from gitlab-org/gitlab-ce#333.

/cc @jacobvosmaer @dzaporozhets

See merge request !14

Author: dzaporozhets

lib/gitlab_git/repository.rb

@@ -11,6 +11,7 @@ class Repository
       SEARCH_CONTEXT_LINES = 3
 
       class NoRepository < StandardError; end
+      class InvalidBlobName < StandardError; end
 
       # Default branch in the repository
       attr_accessor :root_ref
@@ -416,7 +417,12 @@ def lookup(oid_or_ref_name)
       def submodules(ref)
         commit = rugged.rev_parse(ref)
 
-        content = blob_content(commit, ".gitmodules")
+        begin
+          content = blob_content(commit, ".gitmodules")
+        rescue InvalidBlobName
+          return {}
+        end
+
         parse_gitmodules(commit, content)
       end
 
@@ -724,6 +730,10 @@ def rev_parse_target(revspec)
       def blob_content(commit, blob_name)
         blob_entry = tree_entry(commit, blob_name)
 
+        unless blob_entry
+          raise InvalidBlobName.new("Invalid blob name: #{blob_name}")
+        end
+
         if blob_entry[:type] == :commit
           blob_entry[:oid]
         else
@@ -742,11 +752,16 @@ def parse_gitmodules(commit, content)
             current = txt.match(/(?<=").*(?=")/)[0]
             results[current] = {}
           else
+            next unless results[current]
             match_data = txt.match(/(\w+)\s*=\s*(.*)/)
             results[current][match_data[1]] = match_data[2]
 
             if match_data[1] == "path"
-              results[current]["id"] = blob_content(commit, match_data[2])
+              begin
+                results[current]["id"] = blob_content(commit, match_data[2])
+              rescue InvalidBlobName
+                results.delete(current)
+              end
             end
           end
         end

spec/repository_spec.rb

@@ -160,14 +160,14 @@
 
   context :submodules do
     let(:repository) { Gitlab::Git::Repository.new(TEST_REPO_PATH) }
-    let(:submodules) { repository.submodules('master') }
 
-    it { submodules.should be_kind_of Hash }
-    it { submodules.empty?.should be_false }
-
-    describe :submodule do
+    context 'where repo has submodules' do
+      let(:submodules) { repository.submodules('master') }
       let(:submodule) { submodules.first }
 
+      it { submodules.should be_kind_of Hash }
+      it { submodules.empty?.should be_false }
+
       it 'should have valid data' do
         submodule.should == [
           "six", {
@@ -191,6 +191,17 @@
         expect(nested['url']).to eq('git://github.com/randx/six.git')
         expect(nested['id']).to eq('24fb71c79fcabc63dfd8832b12ee3bf2bf06b196')
       end
+
+      it 'should not have an entry for an invalid submodule' do
+        expect(submodules).not_to have_key('invalid/path')
+      end
+    end
+
+    context 'where repo doesn\'t have submodules' do
+      let(:submodules) { repository.submodules('6d39438') }
+      it 'should return an empty hash' do
+        expect(submodules).to be_empty
+      end
     end
   end