Merge branch 'pks-makefile-refactor-git-target' into 'master'

Makefile: Refactor `make git` target

See merge request gitlab-org/gitaly!4804

5 files changed, 29 insertions, 20 deletions

diff --git a/Makefile b/Makefile
index b62d3a494..c61517e61 100644
--- a/Makefile
+++ b/Makefile
@@ -40,7 +40,7 @@ exec_prefix      ?= ${prefix}
 bindir           ?= ${exec_prefix}/bin
 INSTALL_DEST_DIR := ${DESTDIR}${bindir}
 ## The prefix where Git will be installed to.
-GIT_PREFIX       ?= ${GIT_DEFAULT_PREFIX}
+GIT_PREFIX       ?= ${PREFIX}
 
 # Tools
 GIT                         := $(shell command -v git)
@@ -133,9 +133,6 @@ endif
 
 # Git target
 GIT_REPO_URL       ?= https://gitlab.com/gitlab-org/gitlab-git.git
-# The default prefix specifies where Git will be installed to if no GIT_PREFIX
-# was given. This directory will be cleaned up before we install into it.
-GIT_DEFAULT_PREFIX := ${DEPENDENCY_DIR}/git/install
 GIT_QUIET          :=
 ifeq (${Q},@)
     GIT_QUIET = --quiet
@@ -361,9 +358,9 @@ install: install-bundled-git
 
 export GITALY_TESTING_BUNDLED_GIT_PATH ?= ${BUILD_DIR}/bin
 else
-prepare-tests: git
+prepare-tests: ${DEPENDENCY_DIR}/git-distribution/git
 
-export GITALY_TESTING_GIT_BINARY ?= ${GIT_PREFIX}/bin/git
+export GITALY_TESTING_GIT_BINARY ?= ${DEPENDENCY_DIR}/git-distribution/bin-wrappers/git
 endif
 
 .PHONY: prepare-tests
@@ -536,8 +533,13 @@ upgrade-module:
 	${Q}${MAKE} proto
 
 .PHONY: git
-## Build Git.
-git: ${GIT_PREFIX}/bin/git
+# This target is deprecated and will eventually be removed.
+git: install-git
+
+.PHONY: install-git
+## Install Git.
+install-git: ${DEPENDENCY_DIR}/git-distribution/Makefile
+	${Q}env -u PROFILE -u MAKEFLAGS -u GIT_VERSION ${MAKE} -C "${DEPENDENCY_DIR}/git-distribution" -j$(shell nproc) prefix=${GIT_PREFIX} ${GIT_BUILD_OPTIONS} install
 
 .PHONY: libgit2
 ## Build libgit2.
@@ -567,15 +569,9 @@ ${TOOLS_DIR}: | ${BUILD_DIR}
 ${DEPENDENCY_DIR}: | ${BUILD_DIR}
 	${Q}mkdir -p ${DEPENDENCY_DIR}
 
-# This target builds a full Git distribution and installs it into GIT_PREFIX.
-${GIT_PREFIX}/bin/git: ${DEPENDENCY_DIR}/git-distribution/Makefile
-	@ # Remove the Git installation first in case GIT_PREFIX is the default
-	@ # prefix which always points into our build directory. This is done so
-	@ # we never end up with mixed Git installations on developer machines.
-	@ # We cannot ever remove GIT_PREFIX though in case they're different
-	@ # because it may point to a user-controlled directory.
-	${Q}if [ "x${GIT_DEFAULT_PREFIX}" = "x${GIT_PREFIX}" ]; then rm -rf "${GIT_DEFAULT_PREFIX}"; fi
-	${Q}env -u PROFILE -u MAKEFLAGS -u GIT_VERSION ${MAKE} -C "$(<D)" -j$(shell nproc) prefix=${GIT_PREFIX} ${GIT_BUILD_OPTIONS} install
+# This target builds a full Git distribution.
+${DEPENDENCY_DIR}/git-distribution/git: ${DEPENDENCY_DIR}/git-distribution/Makefile
+	${Q}env -u PROFILE -u MAKEFLAGS -u GIT_VERSION ${MAKE} -C "$(<D)" -j$(shell nproc) prefix=${GIT_PREFIX} ${GIT_BUILD_OPTIONS}
 	${Q}touch $@
 
 ${BUILD_DIR}/bin/gitaly-%-v2.35.1.gl1: override GIT_PATCHES := $(sort $(wildcard ${SOURCE_DIR}/_support/git-patches/v2.35.1.gl1/*))
diff --git a/_support/test-boot b/_support/test-boot
index c92c254ac..095c61eb0 100755
--- a/_support/test-boot
+++ b/_support/test-boot
@@ -16,7 +16,7 @@ def main(params)
     if params[:"bundled-git"]
       ['', true]
     else
-      [File.join(build_dir, 'deps', 'git', 'install', 'bin', 'git'), false]
+      [File.join(build_dir, 'deps', 'git-distribution', 'bin-wrappers', 'git'), false]
     end
 
   version = IO.popen("#{File.join(bin_dir, 'gitaly')} -version").read.delete_prefix('Gitaly, version ').strip
diff --git a/internal/git/command_factory_test.go b/internal/git/command_factory_test.go
index 7414f3bbf..d4847028a 100644
--- a/internal/git/command_factory_test.go
+++ b/internal/git/command_factory_test.go
@@ -210,6 +210,7 @@ func TestCommandFactory_ExecutionEnvironment(t *testing.T) {
 		}, git.ExecutionEnvironment{
 			BinaryPath: "/path/to/env_git",
 			EnvironmentVariables: []string{
+				"NO_SET_GIT_TEMPLATE_DIR=YesPlease",
 				"LANG=en_US.UTF-8",
 				"GIT_TERMINAL_PROMPT=0",
 				"GIT_CONFIG_GLOBAL=/dev/null",
diff --git a/internal/git/execution_environment.go b/internal/git/execution_environment.go
index 99aa795c3..9de1a0b94 100644
--- a/internal/git/execution_environment.go
+++ b/internal/git/execution_environment.go
@@ -87,8 +87,17 @@ type DistributedGitEnvironmentConstructor struct{}
 // `GITALY_TESTING_GIT_BINARY` environment variable is set.
 func (c DistributedGitEnvironmentConstructor) Construct(cfg config.Cfg) (ExecutionEnvironment, error) {
 	binaryPath := cfg.Git.BinPath
+	var environmentVariables []string
 	if override := os.Getenv("GITALY_TESTING_GIT_BINARY"); binaryPath == "" && override != "" {
 		binaryPath = override
+		environmentVariables = []string{
+			// When using Git's bin-wrappers as testing binary then the wrapper will
+			// automatically set up the location of the Git templates and export the
+			// environment variable. This would override our own defaults though and
+			// thus leads to diverging behaviour. To fix this we simply ask the bin
+			// wrappers not to do this.
+			"NO_SET_GIT_TEMPLATE_DIR=YesPlease",
+		}
 	}
 
 	if binaryPath == "" {
@@ -96,7 +105,8 @@ func (c DistributedGitEnvironmentConstructor) Construct(cfg config.Cfg) (Executi
 	}
 
 	return ExecutionEnvironment{
-		BinaryPath: binaryPath,
+		BinaryPath:           binaryPath,
+		EnvironmentVariables: environmentVariables,
 	}, nil
 }
 
diff --git a/internal/git/execution_environment_test.go b/internal/git/execution_environment_test.go
index 8432b8724..5340110a5 100644
--- a/internal/git/execution_environment_test.go
+++ b/internal/git/execution_environment_test.go
@@ -46,7 +46,9 @@ func TestDistributedGitEnvironmentConstructor(t *testing.T) {
 		defer execEnv.Cleanup()
 
 		require.Equal(t, "/foo/bar", execEnv.BinaryPath)
-		require.Equal(t, []string(nil), execEnv.EnvironmentVariables)
+		require.Equal(t, []string{
+			"NO_SET_GIT_TEMPLATE_DIR=YesPlease",
+		}, execEnv.EnvironmentVariables)
 	})
 
 	t.Run("configuration overrides environment variable", func(t *testing.T) {