Merge branch 'moved-submodules' into 'master'

repository: index submodules by path

See merge request !10798

changelogs/unreleased/moved-submodules.yml

+---
+title: 'Handle renamed submodules in repository browser'
+merge_request: 10798
+author: David Turner

lib/gitlab/git/gitmodules_parser.rb

+module Gitlab
+  module Git
+    class GitmodulesParser
+      def initialize(content)
+        @content = content
+      end
+
+      # Parses the contents of a .gitmodules file and returns a hash of
+      # submodule information, indexed by path.
+      def parse
+        reindex_by_path(get_submodules_by_name)
+      end
+
+      private
+
+      class State
+        def initialize
+          @result = {}
+          @current_submodule = nil
+        end
+
+        def start_section(section)
+          # In some .gitmodules files (e.g. nodegit's), a header
+          # with the same name appears multiple times; we want to
+          # accumulate the configs across these
+          @current_submodule = @result[section] || { 'name' => section }
+          @result[section] = @current_submodule
+        end
+
+        def set_attribute(attr, value)
+          @current_submodule[attr] = value
+        end
+
+        def section_started?
+          !@current_submodule.nil?
+        end
+
+        def submodules_by_name
+          @result
+        end
+      end
+
+      def get_submodules_by_name
+        iterator = State.new
+
+        @content.split("\n").each_with_object(iterator) do |text, iterator|
+          next if text =~ /^\s*#/
+
+          if text =~ /\A\[submodule "(?<name>[^"]+)"\]\z/
+            iterator.start_section($~[:name])
+          else
+            next unless iterator.section_started?
+
+            next unless text =~ /\A\s*(?<key>\w+)\s*=\s*(?<value>.*)\z/
+
+            value = $~[:value].chomp
+            iterator.set_attribute($~[:key], value)
+          end
+        end
+
+        iterator.submodules_by_name
+      end
+
+      def reindex_by_path(submodules_by_name)
+        # Convert from an indexed by name to an array indexed by path
+        # If a submodule doesn't have a path, it is considered bogus
+        # and is ignored
+        submodules_by_name.each_with_object({}) do |(name, data), results|
+          path = data.delete 'path'
+          next unless path
+
+          results[path] = data
+        end
+      end
+    end
+  end
+end

lib/gitlab/git/repository.rb

@@ -617,9 +617,9 @@ module Gitlab
       #
       # Ex.
       #   {
-      #     "rack"  => {
+      #     "current_path/rack"  => {
+      #       "name" => "original_path/rack",
       #       "id" => "c67be4624545b4263184c4a0e8f887efd0a66320",
-      #       "path" => "rack",
       #       "url" => "git://github.com/chneukirchen/rack.git"
       #     },
       #     "encoding" => {
@@ -637,7 +637,8 @@ module Gitlab
           return {}
         end
 
-        parse_gitmodules(commit, content)
+        parser = GitmodulesParser.new(content)
+        fill_submodule_ids(commit, parser.parse)
       end
 
       # Return total commits count accessible from passed ref
@@ -998,42 +999,19 @@ module Gitlab
         end
       end
 
-      # Parses the contents of a .gitmodules file and returns a hash of
-      # submodule information.
-      def parse_gitmodules(commit, content)
-        modules = {}
-
-        name = nil
-        content.each_line do |line|
-          case line.strip
-          when /\A\[submodule "(?<name>[^"]+)"\]\z/ # Submodule header
-            name = $~[:name]
-            modules[name] = {}
-          when /\A(?<key>\w+)\s*=\s*(?<value>.*)\z/ # Key/value pair
-            key = $~[:key]
-            value = $~[:value].chomp
-
-            next unless name && modules[name]
-
-            modules[name][key] = value
-
-            if key == 'path'
-              begin
-                modules[name]['id'] = blob_content(commit, value)
-              rescue InvalidBlobName
-                # The current entry is invalid
-                modules.delete(name)
-                name = nil
-              end
-            end
-          when /\A#/ # Comment
-            next
-          else # Invalid line
-            name = nil
+      # Fill in the 'id' field of a submodule hash from its values
+      # as-of +commit+. Return a Hash consisting only of entries
+      # from the submodule hash for which the 'id' field is filled.
+      def fill_submodule_ids(commit, submodule_data)
+        submodule_data.each do |path, data|
+          id = begin
+            blob_content(commit, path)
+          rescue InvalidBlobName
+            nil
           end
+          data['id'] = id
         end
-
-        modules
+        submodule_data.select { |path, data| data['id'] }
       end
 
       # Returns true if +commit+ introduced changes to +path+, using commit

spec/lib/gitlab/git/gitmodules_parser_spec.rb

+require 'spec_helper'
+
+describe Gitlab::Git::GitmodulesParser do
+  it 'should parse a .gitmodules file correctly' do
+    parser = described_class.new(<<-'GITMODULES'.strip_heredoc)
+      [submodule "vendor/libgit2"]
+         path = vendor/libgit2
+      [submodule "vendor/libgit2"]
+         url = https://github.com/nodegit/libgit2.git
+
+      # a comment
+      [submodule "moved"]
+          path = new/path
+          url = https://example.com/some/project
+      [submodule "bogus"]
+          url = https://example.com/another/project
+    GITMODULES
+
+    modules = parser.parse
+
+    expect(modules).to eq({
+                            'vendor/libgit2' => { 'name' => 'vendor/libgit2',
+                                                  'url' => 'https://github.com/nodegit/libgit2.git' },
+                            'new/path' => { 'name' => 'moved',
+                                            'url' => 'https://example.com/some/project' }
+                          })
+  end
+end

spec/lib/gitlab/git/repository_spec.rb

@@ -358,7 +358,7 @@ describe Gitlab::Git::Repository, seed_helper: true do
         expect(submodule).to eq([
           "six", {
             "id" => "409f37c4f05865e4fb208c771485f211a22c4c2d",
-            "path" => "six",
+            "name" => "six",
             "url" => "git://github.com/randx/six.git"
           }
         ])
@@ -366,14 +366,14 @@ describe Gitlab::Git::Repository, seed_helper: true do
 
       it 'should handle nested submodules correctly' do
         nested = submodules['nested/six']
-        expect(nested['path']).to eq('nested/six')
+        expect(nested['name']).to eq('nested/six')
         expect(nested['url']).to eq('git://github.com/randx/six.git')
         expect(nested['id']).to eq('24fb71c79fcabc63dfd8832b12ee3bf2bf06b196')
       end
 
       it 'should handle deeply nested submodules correctly' do
         nested = submodules['deeper/nested/six']
-        expect(nested['path']).to eq('deeper/nested/six')
+        expect(nested['name']).to eq('deeper/nested/six')
         expect(nested['url']).to eq('git://github.com/randx/six.git')
         expect(nested['id']).to eq('24fb71c79fcabc63dfd8832b12ee3bf2bf06b196')
       end
@@ -393,7 +393,7 @@ describe Gitlab::Git::Repository, seed_helper: true do
         expect(submodules.first).to eq([
           "six", {
             "id" => "409f37c4f05865e4fb208c771485f211a22c4c2d",
-            "path" => "six",
+            "name" => "six",
             "url" => "git://github.com/randx/six.git"
           }
         ])