Move GitHooksService into Gitlab::Git

What does this MR do?

• move GitHooksService in to Gitlab::Git
• decouple GitHooksService from User and Project
• make gl_repository an attribute of Gitlab::Git::Repository

Are there points in the code the reviewer needs to double check?

Why was this MR needed?

We want to move the code that creates commits and updates refs in repositories into Gitlab::Git.

Screenshots (if relevant)

Does this MR meet the acceptance criteria?

• Changelog entry added, if necessary
• Documentation created/updated
• API support added
• Tests added for this feature/bug
• Review
  • Has been reviewed by UX
  • Has been reviewed by Frontend
  • Has been reviewed by Backend
  • Has been reviewed by Database
• Conform by the merge request performance guides
• Conform by the style guides
• Squashed related commits together

What are the relevant issue numbers?

changed title from WIP Git operation user to WIP Move GitHooksService into Gitlab::Git

changed the description

changed the description

added 3 commits

• 67a7a242 - Move GitHooksService tests
• 6daa837b - Rubocop whitespace
• 743e04db - Fix Hook.new call sites in tests

Compare with previous version

unmarked as a Work In Progress

assigned to @smcgivern

This MR doesn't make that much sense on its own, but I find myself making so many changes that it seems useful to submit them as I go.

changed milestone to %10.0

added 2 commits

• 56cb8c1b - Rubocop whitespace
• a27b14d7 - Fix Hook.new call sites in tests

Compare with previous version

Failed because of a rubocop violation in a file I didn't touch. Rebased on master.

added 101 commits

• a27b14d7...0f74ba96 - 95 commits from branch master
• 9b930932 - Decouple GitOperationService from User
• 65f83941 - Make gl_repository a G::G::Repository attribute
• dc7c6bed - Move GitHooksService to Gitlab::Git
• c47b947a - Move GitHooksService tests
• da769135 - Rubocop whitespace
• bc97931e - Fix Hook.new call sites in tests

Compare with previous version

@jacobvosmaer-gitlab thanks! Couple of questions.

assigned to @jacobvosmaer-gitlab

assigned to @smcgivern

assigned to @jacobvosmaer-gitlab

assigned to @smcgivern

added 1 commit

• 961da7d0 - Add tests for Committer#==

Compare with previous version

mentioned in merge request !13773 (merged)

Waiting for a1a91499 to show up in the diff.

Assigning to self until diffs are up to date.

assigned to @jacobvosmaer-gitlab

added 1 commit

• a1a91499 - Avoid committer = lines

Compare with previous version

added 1 commit

• a1a91499 - Avoid committer = lines

Compare with previous version

assigned to @smcgivern

resolved all discussions

Thanks @jacobvosmaer-gitlab, LGTM!

approved this merge request

merged

mentioned in commit e46a3d2f

mentioned in issue gitaly#491 (closed)

This appears to have introduced a failure on master:

  1) Gitlab::Git::Repository#branch_count with local and remote branches returns the count of local branches
     Failure/Error:
             def initialize(storage, relative_path, gl_repository)
               @storage = storage
               @relative_path = relative_path
               @gl_repository = gl_repository

               storage_path = Gitlab.config.repositories.storages[@storage]['path']
               @path = File.join(storage_path, @relative_path)
               @name = @relative_path.split("/").last
               @attributes = Gitlab::Git::Attributes.new(path)
             end

     ArgumentError:
       wrong number of arguments (given 2, expected 3)
     # ./lib/gitlab/git/repository.rb:56:in `initialize'
     # ./spec/lib/gitlab/git/repository_spec.rb:983:in `new'
     # ./spec/lib/gitlab/git/repository_spec.rb:983:in `block (4 levels) in <top (required)>'
     # ./spec/lib/gitlab/git/repository_spec.rb:987:in `block (4 levels) in <top (required)>'
     # -e:1:in `<main>'


  2) Gitlab::Git::Repository#branch_count with local and remote branches with Gitaly disabled returns the count of local branches
     Failure/Error:
             def initialize(storage, relative_path, gl_repository)
               @storage = storage
               @relative_path = relative_path
               @gl_repository = gl_repository

               storage_path = Gitlab.config.repositories.storages[@storage]['path']
               @path = File.join(storage_path, @relative_path)
               @name = @relative_path.split("/").last
               @attributes = Gitlab::Git::Attributes.new(path)
             end

     ArgumentError:
       wrong number of arguments (given 2, expected 3)
     # ./lib/gitlab/git/repository.rb:56:in `initialize'
     # ./spec/lib/gitlab/git/repository_spec.rb:983:in `new'
     # ./spec/lib/gitlab/git/repository_spec.rb:983:in `block (4 levels) in <top (required)>'
     # ./spec/lib/gitlab/git/repository_spec.rb:987:in `block (4 levels) in <top (required)>'
     # -e:1:in `<main>'

Finished in 6.38 seconds (files took 0.83515 seconds to load)
173 examples, 2 failures

Failed examples:

rspec ./spec/lib/gitlab/git/repository_spec.rb:996 # Gitlab::Git::Repository#branch_count with local and remote branches returns the count of local branches
rspec ./spec/lib/gitlab/git/repository_spec.rb:1005 # Gitlab::Git::Repository#branch_count with local and remote branches with Gitaly disabled returns the count of local branches

Traced back to this MR via git bisect.

Fixed via 51ceacb1

This requires an EE version: https://gitlab.com/gitlab-org/gitlab-ee/commit/9df78bc430d536119a9dde3c6378aea54ad1dd25

I'm curious why, if the third argument to Gitlab::Git::Repository#initialize isn't actually required and we can get around this change by just passing '', why didn't we give it that as a default value and make it optional?

@jacobvosmaer-gitlab I'm curious why, if the third argument to Gitlab::Git::Repository#initialize isn't actually required and we can get around this change by just passing '', why didn't we give it that as a default value and make it optional?

@rspeicher this argument is used for hooks. We wouldn't want to make it optional, because that would break hooks, but specs never run hooks