Merge branch 'rs-git-bin-path' into 'master'

Replace all usages of `git` command with configurable binary path

Closes #3311

See merge request !1742

Author: Douwe Maan

app/models/repository.rb

@@ -89,7 +89,7 @@ def commits_between(from, to)
 
   def find_commits_by_message(query)
     # Limited to 1000 commits for now, could be parameterized?
-    args = %W(git log --pretty=%H --max-count 1000 --grep=#{query})
+    args = %W(#{Gitlab.config.git.bin_path} log --pretty=%H --max-count 1000 --grep=#{query})
 
     git_log_results = Gitlab::Popen.popen(args, path_to_repo).first.lines.map(&:chomp)
     commits = git_log_results.map { |c| commit(c) }
@@ -296,7 +296,7 @@ def submodule_url_for(ref, path)
   end
 
   def last_commit_for_path(sha, path)
-    args = %W(git rev-list --max-count=1 #{sha} -- #{path})
+    args = %W(#{Gitlab.config.git.bin_path} rev-list --max-count=1 #{sha} -- #{path})
     sha = Gitlab::Popen.popen(args, path_to_repo).first.strip
     commit(sha)
   end
@@ -347,7 +347,7 @@ def prev_blob_for_diff(commit, diff)
   end
 
   def branch_names_contains(sha)
-    args = %W(git branch --contains #{sha})
+    args = %W(#{Gitlab.config.git.bin_path} branch --contains #{sha})
     names = Gitlab::Popen.popen(args, path_to_repo).first
 
     if names.respond_to?(:split)
@@ -364,7 +364,7 @@ def branch_names_contains(sha)
   end
 
   def tag_names_contains(sha)
-    args = %W(git tag --contains #{sha})
+    args = %W(#{Gitlab.config.git.bin_path} tag --contains #{sha})
     names = Gitlab::Popen.popen(args, path_to_repo).first
 
     if names.respond_to?(:split)
@@ -505,7 +505,7 @@ def merge_base(first_commit_id, second_commit_id)
 
   def search_files(query, ref)
     offset = 2
-    args = %W(git grep -i -n --before-context #{offset} --after-context #{offset} -e #{query} #{ref || root_ref})
+    args = %W(#{Gitlab.config.git.bin_path} grep -i -n --before-context #{offset} --after-context #{offset} -e #{query} #{ref || root_ref})
     Gitlab::Popen.popen(args, path_to_repo).first.scrub.split(/^--$/)
   end
 
@@ -537,7 +537,7 @@ def parse_search_result(result)
   end
 
   def fetch_ref(source_path, source_ref, target_ref)
-    args = %W(git fetch -f #{source_path} #{source_ref}:#{target_ref})
+    args = %W(#{Gitlab.config.git.bin_path} fetch -f #{source_path} #{source_ref}:#{target_ref})
     Gitlab::Popen.popen(args, path_to_repo)
   end
 

config/initializers/2_app.rb

@@ -1,8 +1,8 @@
 module Gitlab
-  VERSION = File.read(Rails.root.join("VERSION")).strip
-  REVISION = Gitlab::Popen.popen(%W(git log --pretty=format:%h -n 1)).first.chomp
-
   def self.config
     Settings
   end
+
+  VERSION = File.read(Rails.root.join("VERSION")).strip
+  REVISION = Gitlab::Popen.popen(%W(#{config.git.bin_path} log --pretty=format:%h -n 1)).first.chomp
 end

doc/development/shell_commands.md

@@ -35,6 +35,16 @@ Gitlab::Popen.popen(%W(find /some/path -not -path /some/path -mmin +120 -delete)
 
 This coding style could have prevented CVE-2013-4490.
 
+## Always use the configurable git binary path for git commands
+
+```ruby
+# Wrong
+system(*%W(git branch -d -- #{branch_name}))
+
+# Correct
+system(*%W(#{Gitlab.config.git.bin_path} branch -d -- #{branch_name}))
+```
+
 ## Bypass the shell by splitting commands into separate tokens
 
 When we pass shell commands as a single string to Ruby, Ruby will let `/bin/sh` evaluate the entire string. Essentially, we are asking the shell to evaluate a one-line script. This creates a risk for shell injection attacks. It is better to split the shell command into tokens ourselves. Sometimes we use the scripting capabilities of the shell to change the working directory or set environment variables. All of this can also be achieved securely straight from Ruby
@@ -81,9 +91,9 @@ In the GitLab codebase, we avoid the option/argument ambiguity by _always_ using
 
 ```ruby
 # Wrong
-system(*%W(git branch -d #{branch_name}))
+system(*%W(#{Gitlab.config.git.bin_path} branch -d #{branch_name}))
 # Correct
-system(*%W(git branch -d -- #{branch_name}))
+system(*%W(#{Gitlab.config.git.bin_path} branch -d -- #{branch_name}))
 ```
 
 This coding style could have prevented CVE-2013-4582.
@@ -94,9 +104,9 @@ Capturing the output of shell commands with backticks reads nicely, but you are
 
 ```ruby
 # Wrong
-logs = `cd #{repo_dir} && git log`
+logs = `cd #{repo_dir} && #{Gitlab.config.git.bin_path} log`
 # Correct
-logs, exit_status = Gitlab::Popen.popen(%W(git log), repo_dir)
+logs, exit_status = Gitlab::Popen.popen(%W(#{Gitlab.config.git.bin_path} log), repo_dir)
 
 # Wrong
 user = `whoami`
@@ -108,7 +118,7 @@ In other repositories, such as gitlab-shell you can also use `IO.popen`.
 
 ```ruby
 # Safe IO.popen example
-logs = IO.popen(%W(git log), chdir: repo_dir) { |p| p.read }
+logs = IO.popen(%W(#{Gitlab.config.git.bin_path} log), chdir: repo_dir) { |p| p.read }
 ```
 
 Note that unlike `Gitlab::Popen.popen`, `IO.popen` does not capture standard error.

lib/backup/repository.rb

@@ -35,7 +35,7 @@ def dump
           if wiki.repository.empty?
             $progress.puts " [SKIPPED]".cyan
           else
-            cmd = %W(git --git-dir=#{path_to_repo(wiki)} bundle create #{path_to_bundle(wiki)} --all)
+            cmd = %W(#{Gitlab.config.git.bin_path} --git-dir=#{path_to_repo(wiki)} bundle create #{path_to_bundle(wiki)} --all)
             output, status = Gitlab::Popen.popen(cmd)
             if status.zero?
               $progress.puts " [DONE]".green
@@ -67,7 +67,7 @@ def restore
           FileUtils.mkdir_p(path_to_repo(project))
           cmd = %W(tar -xf #{path_to_bundle(project)} -C #{path_to_repo(project)})
         else
-          cmd = %W(git init --bare #{path_to_repo(project)})
+          cmd = %W(#{Gitlab.config.git.bin_path} init --bare #{path_to_repo(project)})
         end
 
         if system(*cmd, silent)
@@ -87,7 +87,7 @@ def restore
           # that was initialized with ProjectWiki.new() and then
           # try to restore with 'git clone --bare'.
           FileUtils.rm_rf(path_to_repo(wiki))
-          cmd = %W(git clone --bare #{path_to_bundle(wiki)} #{path_to_repo(wiki)})
+          cmd = %W(#{Gitlab.config.git.bin_path} clone --bare #{path_to_bundle(wiki)} #{path_to_repo(wiki)})
 
           if system(*cmd, silent)
             $progress.puts " [DONE]".green

lib/gitlab/force_push_check.rb

@@ -7,7 +7,7 @@ def self.force_push?(project, oldrev, newrev)
       if Gitlab::Git.blank_ref?(oldrev) || Gitlab::Git.blank_ref?(newrev)
         false
       else
-        missed_refs, _ = Gitlab::Popen.popen(%W(git --git-dir=#{project.repository.path_to_repo} rev-list #{oldrev} ^#{newrev}))
+        missed_refs, _ = Gitlab::Popen.popen(%W(#{Gitlab.config.git.bin_path} --git-dir=#{project.repository.path_to_repo} rev-list #{oldrev} ^#{newrev}))
         missed_refs.split("\n").size > 0
       end
     end

lib/gitlab/git_ref_validator.rb

@@ -6,7 +6,7 @@ module GitRefValidator
     # Returns true for a valid reference name, false otherwise
     def validate(ref_name)
       Gitlab::Utils.system_silent(
-        %W(git check-ref-format refs/#{ref_name}))
+        %W(#{Gitlab.config.git.bin_path} check-ref-format refs/#{ref_name}))
     end
   end
 end

lib/gitlab/upgrader.rb

@@ -50,15 +50,15 @@ def latest_version_raw
     end
 
     def fetch_git_tags
-      remote_tags, _ = Gitlab::Popen.popen(%W(git ls-remote --tags https://gitlab.com/gitlab-org/gitlab-ce.git))
+      remote_tags, _ = Gitlab::Popen.popen(%W(#{Gitlab.config.git.bin_path} ls-remote --tags https://gitlab.com/gitlab-org/gitlab-ce.git))
       remote_tags.split("\n").grep(/tags\/v#{current_version.major}/)
     end
 
     def update_commands
       {
-        "Stash changed files" => %W(git stash),
-        "Get latest code" => %W(git fetch),
-        "Switch to new version" => %W(git checkout v#{latest_version}),
+        "Stash changed files" => %W(#{Gitlab.config.git.bin_path} stash),
+        "Get latest code" => %W(#{Gitlab.config.git.bin_path} fetch),
+        "Switch to new version" => %W(#{Gitlab.config.git.bin_path} checkout v#{latest_version}),
         "Install gems" => %W(bundle),
         "Migrate DB" => %W(bundle exec rake db:migrate),
         "Recompile assets" => %W(bundle exec rake assets:clean assets:precompile),

lib/tasks/gitlab/check.rake

@@ -824,7 +824,7 @@ namespace :gitlab do
         repo_dirs = Dir.glob(File.join(namespace_dir, '*'))
         repo_dirs.each do |dir|
           puts "\nChecking repo at #{dir}"
-          system(*%w(git fsck), chdir: dir)
+          system(*%W(#{Gitlab.config.git.bin_path} fsck), chdir: dir)
         end
       end
     end

lib/tasks/gitlab/shell.rake

@@ -17,7 +17,7 @@ namespace :gitlab do
 
       # Clone if needed
       unless File.directory?(target_dir)
-        system(*%W(git clone -- #{args.repo} #{target_dir}))
+        system(*%W(#{Gitlab.config.git.bin_path} clone -- #{args.repo} #{target_dir}))
       end
 
       # Make sure we're on the right tag
@@ -27,7 +27,7 @@ namespace :gitlab do
         reseted = reset_to_commit(args)
 
         unless reseted
-          system(*%W(git fetch origin))
+          system(*%W(#{Gitlab.config.git.bin_path} fetch origin))
           reset_to_commit(args)
         end
 
@@ -128,14 +128,14 @@ namespace :gitlab do
   end
 
   def reset_to_commit(args)
-    tag, status = Gitlab::Popen.popen(%W(git describe -- #{args.tag}))
+    tag, status = Gitlab::Popen.popen(%W(#{Gitlab.config.git.bin_path} describe -- #{args.tag}))
 
     unless status.zero?
-      tag, status = Gitlab::Popen.popen(%W(git describe -- origin/#{args.tag}))
+      tag, status = Gitlab::Popen.popen(%W(#{Gitlab.config.git.bin_path} describe -- origin/#{args.tag}))
     end
 
     tag = tag.strip
-    system(*%W(git reset --hard #{tag}))
+    system(*%W(#{Gitlab.config.git.bin_path} reset --hard #{tag}))
   end
 end
 

spec/models/project_wiki_spec.rb

@@ -223,7 +223,7 @@
 
   def create_temp_repo(path)
     FileUtils.mkdir_p path
-    system(*%W(git init --quiet --bare -- #{path}))
+    system(*%W(#{Gitlab.config.git.bin_path} init --quiet --bare -- #{path}))
   end
 
   def remove_temp_repo(path)

spec/requests/api/repositories_spec.rb

@@ -36,8 +36,8 @@
       it 'should create a new annotated tag' do
         # Identity must be set in .gitconfig to create annotated tag.
         repo_path = project.repository.path_to_repo
-        system(*%W(git --git-dir=#{repo_path} config user.name #{user.name}))
-        system(*%W(git --git-dir=#{repo_path} config user.email #{user.email}))
+        system(*%W(#{Gitlab.config.git.bin_path} --git-dir=#{repo_path} config user.name #{user.name}))
+        system(*%W(#{Gitlab.config.git.bin_path} --git-dir=#{repo_path} config user.email #{user.email}))
 
         post api("/projects/#{project.id}/repository/tags", user),
              tag_name: 'v7.1.0',

spec/support/test_env.rb

@@ -96,15 +96,15 @@ def setup_repo(repo_path, repo_path_bare, repo_name, branch_sha)
     clone_url = "https://gitlab.com/gitlab-org/#{repo_name}.git"
 
     unless File.directory?(repo_path)
-      system(*%W(git clone -q #{clone_url} #{repo_path}))
+      system(*%W(#{Gitlab.config.git.bin_path} clone -q #{clone_url} #{repo_path}))
     end
 
     Dir.chdir(repo_path) do
       branch_sha.each do |branch, sha|
         # Try to reset without fetching to avoid using the network.
-        reset = %W(git update-ref refs/heads/#{branch} #{sha})
+        reset = %W(#{Gitlab.config.git.bin_path} update-ref refs/heads/#{branch} #{sha})
         unless system(*reset)
-          if system(*%w(git fetch origin))
+          if system(*%W(#{Gitlab.config.git.bin_path} fetch origin))
             unless system(*reset)
               raise 'The fetched test seed '\
               'does not contain the required revision.'
@@ -117,7 +117,7 @@ def setup_repo(repo_path, repo_path_bare, repo_name, branch_sha)
     end
 
     # We must copy bare repositories because we will push to them.
-    system(git_env, *%W(git clone -q --bare #{repo_path} #{repo_path_bare}))
+    system(git_env, *%W(#{Gitlab.config.git.bin_path} clone -q --bare #{repo_path} #{repo_path_bare}))
   end
 
   def copy_repo(project)