Merge branch 'lib-git-gitaly' into 'master'

Add Gitaly notes to Gitlab::Git, delete diff options

See merge request !12814

lib/gitlab/git/attributes.rb

+# Gitaly note: JV: not sure what to make of this class. Why does it use
+# the full disk path of the repository to look up attributes This is
+# problematic in Gitaly, because Gitaly hides the full disk path to the
+# repository from gitlab-ce.
+
 module Gitlab
   module Git
     # Class for parsing Git attribute files and extracting the attributes for

lib/gitlab/git/blame.rb

+# Gitaly note: JV: needs 1 RPC for #load_blame.
+
 module Gitlab
   module Git
     class Blame
@@ -24,6 +26,7 @@ module Gitlab
 
       private
 
+      # Gitaly migration: https://gitlab.com/gitlab-org/gitaly/issues/376
       def load_blame
         cmd = %W(#{Gitlab.config.git.bin_path} --git-dir=#{@repo.path} blame -p #{@sha} -- #{@path})
         # Read in binary mode to ensure ASCII-8BIT

lib/gitlab/git/blob.rb

+# Gitaly note: JV: seems to be completely migrated (behind feature flags).
+
 module Gitlab
   module Git
     class Blob
@@ -107,6 +109,8 @@ module Gitlab
           detect && detect[:type] == :binary
         end
 
+        private
+
         # Recursive search of blob id by path
         #
         # Ex.

lib/gitlab/git/blob_snippet.rb

+# Gitaly note: JV: no RPC's here.
+
 module Gitlab
   module Git
     class BlobSnippet

lib/gitlab/git/branch.rb

+# Gitaly note: JV: no RPC's here.
+
 module Gitlab
   module Git
     class Branch < Ref

lib/gitlab/git/commit.rb

@@ -48,6 +48,7 @@ module Gitlab
         #
         #   Commit.find(repo, 'master')
         #
+        # Gitaly migration: https://gitlab.com/gitlab-org/gitaly/issues/321
         def find(repo, commit_id = "HEAD")
           return commit_id if commit_id.is_a?(Gitlab::Git::Commit)
           return decorate(commit_id) if commit_id.is_a?(Rugged::Commit)
@@ -124,6 +125,7 @@ module Gitlab
         #        are documented here:
         #        http://www.rubydoc.info/github/libgit2/rugged/Rugged#SORT_NONE-constant)
         #
+        # Gitaly migration: https://gitlab.com/gitlab-org/gitaly/issues/326
         def find_all(repo, options = {})
           actual_options = options.dup
 
@@ -243,6 +245,8 @@ module Gitlab
       # Shows the diff between the commit's parent and the commit.
       #
       # Cuts out the header and stats from #to_patch and returns only the diff.
+      #
+      # Gitaly migration: https://gitlab.com/gitlab-org/gitaly/issues/324
       def to_diff
         diff_from_parent.patch
       end

lib/gitlab/git/commit_stats.rb

+# Gitaly note: JV: 1 RPC, migration in progress.
+
 # Gitlab::Git::CommitStats counts the additions, deletions, and total changes
 # in a commit.
 module Gitlab
@@ -6,6 +8,8 @@ module Gitlab
       attr_reader :id, :additions, :deletions, :total
 
       # Instantiate a CommitStats object
+      #
+      # Gitaly migration: https://gitlab.com/gitlab-org/gitaly/issues/323
       def initialize(commit)
         @id = commit.id
         @additions = 0

lib/gitlab/git/compare.rb

+# Gitaly note: JV: no RPC's here.
+
 module Gitlab
   module Git
     class Compare

lib/gitlab/git/diff.rb

+# Gitaly note: JV: needs RPC for Gitlab::Git::Diff.between.
+
 # Gitlab::Git::Diff is a wrapper around native Rugged::Diff object
 module Gitlab
   module Git
@@ -81,110 +83,16 @@ module Gitlab
         # Return a copy of the +options+ hash containing only keys that can be
         # passed to Rugged.  Allowed options are:
         #
-        #  :max_size ::
-        #    An integer specifying the maximum byte size of a file before a it
-        #    will be treated as binary. The default value is 512MB.
-        #
-        #  :context_lines ::
-        #    The number of unchanged lines that define the boundary of a hunk
-        #    (and to display before and after the actual changes). The default is
-        #    3.
-        #
-        #  :interhunk_lines ::
-        #    The maximum number of unchanged lines between hunk boundaries before
-        #    the hunks will be merged into a one. The default is 0.
-        #
-        #  :old_prefix ::
-        #    The virtual "directory" to prefix to old filenames in hunk headers.
-        #    The default is "a".
-        #
-        #  :new_prefix ::
-        #    The virtual "directory" to prefix to new filenames in hunk headers.
-        #    The default is "b".
-        #
-        #  :reverse ::
-        #    If true, the sides of the diff will be reversed.
-        #
-        #  :force_text ::
-        #    If true, all files will be treated as text, disabling binary
-        #    attributes & detection.
-        #
-        #  :ignore_whitespace ::
-        #    If true, all whitespace will be ignored.
-        #
         #  :ignore_whitespace_change ::
         #    If true, changes in amount of whitespace will be ignored.
         #
-        #  :ignore_whitespace_eol ::
-        #    If true, whitespace at end of line will be ignored.
-        #
-        #  :ignore_submodules ::
-        #    if true, submodules will be excluded from the diff completely.
-        #
-        #  :patience ::
-        #    If true, the "patience diff" algorithm will be used (currenlty
-        #    unimplemented).
-        #
-        #  :include_ignored ::
-        #    If true, ignored files will be included in the diff.
-        #
-        #  :include_untracked ::
-        #   If true, untracked files will be included in the diff.
-        #
-        #  :include_unmodified ::
-        #    If true, unmodified files will be included in the diff.
-        #
-        #  :recurse_untracked_dirs ::
-        #    Even if +:include_untracked+ is true, untracked directories will
-        #    only be marked with a single entry in the diff. If this flag is set
-        #    to true, all files under ignored directories will be included in the
-        #    diff, too.
-        #
         #  :disable_pathspec_match ::
         #    If true, the given +*paths+ will be applied as exact matches,
         #    instead of as fnmatch patterns.
         #
-        #  :deltas_are_icase ::
-        #    If true, filename comparisons will be made with case-insensitivity.
-        #
-        #  :include_untracked_content ::
-        #    if true, untracked content will be contained in the the diff patch
-        #    text.
-        #
-        #  :skip_binary_check ::
-        #    If true, diff deltas will be generated without spending time on
-        #    binary detection. This is useful to improve performance in cases
-        #    where the actual file content difference is not needed.
-        #
-        #  :include_typechange ::
-        #    If true, type changes for files will not be interpreted as deletion
-        #    of the "old file" and addition of the "new file", but will generate
-        #    typechange records.
-        #
-        #  :include_typechange_trees ::
-        #    Even if +:include_typechange+ is true, blob -> tree changes will
-        #    still usually be handled as a deletion of the blob. If this flag is
-        #    set to true, blob -> tree changes will be marked as typechanges.
-        #
-        #  :ignore_filemode ::
-        #    If true, file mode changes will be ignored.
-        #
-        #  :recurse_ignored_dirs ::
-        #    Even if +:include_ignored+ is true, ignored directories will only be
-        #    marked with a single entry in the diff. If this flag is set to true,
-        #    all files under ignored directories will be included in the diff,
-        #    too.
         def filter_diff_options(options, default_options = {})
-          allowed_options = [:max_size, :context_lines, :interhunk_lines,
-                             :old_prefix, :new_prefix, :reverse, :force_text,
-                             :ignore_whitespace, :ignore_whitespace_change,
-                             :ignore_whitespace_eol, :ignore_submodules,
-                             :patience, :include_ignored, :include_untracked,
-                             :include_unmodified, :recurse_untracked_dirs,
-                             :disable_pathspec_match, :deltas_are_icase,
-                             :include_untracked_content, :skip_binary_check,
-                             :include_typechange, :include_typechange_trees,
-                             :ignore_filemode, :recurse_ignored_dirs, :paths,
+          allowed_options = [:ignore_whitespace_change,
+                             :disable_pathspec_match, :paths,
                              :max_files, :max_lines, :limits, :expanded]
 
           if default_options

lib/gitlab/git/diff_collection.rb

+# Gitaly note: JV: no RPC's here.
+
 module Gitlab
   module Git
     class DiffCollection

lib/gitlab/git/env.rb

+# Gitaly note: JV: no RPC's here.
+
 module Gitlab
   module Git
     # Ephemeral (per request) storage for environment variables that some Git

lib/gitlab/git/gitmodules_parser.rb

+# Gitaly note: JV: no RPC's here.
+
 module Gitlab
   module Git
     class GitmodulesParser

lib/gitlab/git/hook.rb

+# Gitaly note: JV: looks like this is only used by GitHooksService in
+# app/services. We shouldn't bother migrating this until we know how
+# GitHooksService will be migrated.
+
 module Gitlab
   module Git
     class Hook

lib/gitlab/git/index.rb

+# Gitaly note: JV: When the time comes I think we will want to copy this
+# class into Gitaly. None of its methods look like they should be RPC's.
+# The RPC's will be at a higher level.
+
 module Gitlab
   module Git
     class Index

lib/gitlab/git/path_helper.rb

+# Gitaly note: JV: no RPC's here.
+
 module Gitlab
   module Git
     class PathHelper

lib/gitlab/git/popen.rb

+# Gitaly note: JV: no RPC's here.
+
 require 'open3'
 
 module Gitlab

lib/gitlab/git/ref.rb

+# Gitaly note: JV: probably no RPC's here (just one interaction with Rugged).
+
 module Gitlab
   module Git
     class Ref
@@ -24,6 +26,7 @@ module Gitlab
         str.gsub(/\Arefs\/heads\//, '')
       end
 
+      # Gitaly: this method will probably be migrated indirectly via its call sites.
       def self.dereference_object(object)
         object = object.target while object.is_a?(Rugged::Tag::Annotation)
 

lib/gitlab/git/repository.rb

@@ -553,6 +553,7 @@ module Gitlab
       #   @repository.submodule_url_for('master', 'rack')
       #   # => git@localhost:rack.git
       #
+      # Gitaly migration: https://gitlab.com/gitlab-org/gitaly/issues/329
       def submodule_url_for(ref, path)
         Gitlab::GitalyClient.migrate(:submodule_url_for) do |is_enabled|
           if is_enabled
@@ -567,6 +568,8 @@ module Gitlab
       end
 
       # Return total commits count accessible from passed ref
+      #
+      # Gitaly migration: https://gitlab.com/gitlab-org/gitaly/issues/330
       def commit_count(ref)
         gitaly_migrate(:commit_count) do |is_enabled|
           if is_enabled
@@ -838,6 +841,7 @@ module Gitlab
       # Ex.
       #   repo.ls_files('master')
       #
+      # Gitaly migration: https://gitlab.com/gitlab-org/gitaly/issues/327
       def ls_files(ref)
         actual_ref = ref || root_ref
 
@@ -864,6 +868,7 @@ module Gitlab
         raw_output.compact
       end
 
+      # Gitaly migration: https://gitlab.com/gitlab-org/gitaly/issues/328
       def copy_gitattributes(ref)
         begin
           commit = lookup(ref)

lib/gitlab/git/rev_list.rb

+# Gitaly note: JV: will probably be migrated indirectly by migrating the call sites.
+
 module Gitlab
   module Git
     class RevList
@@ -15,6 +17,8 @@ module Gitlab
       end
 
       # This methods returns an array of missed references
+      #
+      # Should become obsolete after https://gitlab.com/gitlab-org/gitaly/issues/348.
       def missed_ref
         execute([*base_args, '--max-count=1', oldrev, "^#{newrev}"])
       end

lib/gitlab/git/tag.rb

+# Gitaly note: JV: no RPC's here.
+#
 module Gitlab
   module Git
     class Tag < Ref

lib/gitlab/git/tree.rb

+# Gitaly note: JV: needs 1 RPC, migration is in progress.
+
 module Gitlab
   module Git
     class Tree
@@ -10,6 +12,8 @@ module Gitlab
         # Get list of tree objects
         # for repository based on commit sha and path
         # Uses rugged for raw objects
+        #
+        # Gitaly migration: https://gitlab.com/gitlab-org/gitaly/issues/320
         def where(repository, sha, path = nil)
           path = nil if path == '' || path == '/'
 
@@ -40,6 +44,8 @@ module Gitlab
           end
         end
 
+        private
+
         # Recursive search of tree id for path
         #
         # Ex.

lib/gitlab/git/util.rb

+# Gitaly note: JV: no RPC's here.
+
 module Gitlab
   module Git
     module Util

spec/lib/gitlab/git/diff_spec.rb

@@ -241,7 +241,7 @@ EOT
   end
 
   describe '.filter_diff_options' do
-    let(:options) { { max_size: 100, invalid_opt: true } }
+    let(:options) { { max_files: 100, invalid_opt: true } }
 
     context "without default options" do
       let(:filtered_options) { described_class.filter_diff_options(options) }
@@ -253,7 +253,7 @@ EOT
 
     context "with default options" do
       let(:filtered_options) do
-        default_options = { max_size: 5, bad_opt: 1, ignore_whitespace: true }
+        default_options = { max_files: 5, bad_opt: 1, ignore_whitespace_change: true }
         described_class.filter_diff_options(options, default_options)
       end
 
@@ -263,12 +263,12 @@ EOT
       end
 
       it "should merge with default options" do
-        expect(filtered_options).to have_key(:ignore_whitespace)
+        expect(filtered_options).to have_key(:ignore_whitespace_change)
       end
 
       it "should override default options" do
-        expect(filtered_options).to have_key(:max_size)
-        expect(filtered_options[:max_size]).to eq(100)
+        expect(filtered_options).to have_key(:max_files)
+        expect(filtered_options[:max_files]).to eq(100)
       end
     end
   end