Merge branch 'do-clean-after-checkout' into 'master'

Improve support for `git clean`

Closes #4109

See merge request gitlab-org/gitlab-runner!1281

common/build.go

@@ -28,6 +28,11 @@ const (
 	GitNone
 )
 
+const (
+	gitCleanFlagsDefault = "-ffdx"
+	gitCleanFlagsNone    = "none"
+)
+
 type SubmoduleStrategy int
 
 const (
@@ -64,7 +69,8 @@ const (
 )
 
 const (
-	FFDockerHelperImageV2 string = "FF_DOCKER_HELPER_IMAGE_V2"
+	FFDockerHelperImageV2       string = "FF_DOCKER_HELPER_IMAGE_V2"
+	FFUseLegacyGitCleanStrategy string = "FF_USE_LEGACY_GIT_CLEAN_STRATEGY"
 )
 
 type Build struct {
@@ -536,6 +542,7 @@ func (b *Build) GetDefaultVariables() JobVariables {
 func (b *Build) GetDefaultFeatureFlagsVariables() JobVariables {
 	return JobVariables{
 		{Key: "FF_K8S_USE_ENTRYPOINT_OVER_COMMAND", Value: "true", Public: true, Internal: true, File: false}, // TODO: Remove in 12.0
+		{Key: FFUseLegacyGitCleanStrategy, Value: "false", Public: true, Internal: true, File: false},         // TODO: Remove in 12.0
 	}
 }
 
@@ -698,6 +705,19 @@ func (b *Build) GetSubmoduleStrategy() SubmoduleStrategy {
 	}
 }
 
+func (b *Build) GetGitCleanFlags() []string {
+	flags := b.GetAllVariables().Get("GIT_CLEAN_FLAGS")
+	if flags == "" {
+		flags = gitCleanFlagsDefault
+	}
+
+	if flags == gitCleanFlagsNone {
+		return []string{}
+	}
+
+	return strings.Fields(flags)
+}
+
 func (b *Build) IsDebugTraceEnabled() bool {
 	trace, err := strconv.ParseBool(b.GetAllVariables().Get("CI_DEBUG_TRACE"))
 	if err != nil {

common/build_test.go

@@ -1069,3 +1069,43 @@ func TestBuild_IsLFSSmudgeDisabled(t *testing.T) {
 		})
 	}
 }
+
+func TestGitCleanFlags(t *testing.T) {
+	tests := map[string]struct {
+		value          string
+		expectedResult []string
+	}{
+		"empty clean flags": {
+			value:          "",
+			expectedResult: []string{"-ffdx"},
+		},
+		"use custom flags": {
+			value:          "custom-flags",
+			expectedResult: []string{"custom-flags"},
+		},
+		"use custom flags with multiple arguments": {
+			value:          "-ffdx -e cache/",
+			expectedResult: []string{"-ffdx", "-e", "cache/"},
+		},
+		"disabled": {
+			value:          "none",
+			expectedResult: []string{},
+		},
+	}
+
+	for name, test := range tests {
+		t.Run(name, func(t *testing.T) {
+			build := &Build{
+				Runner: &RunnerConfig{},
+				JobResponse: JobResponse{
+					Variables: JobVariables{
+						{Key: "GIT_CLEAN_FLAGS", Value: test.value},
+					},
+				},
+			}
+
+			result := build.GetGitCleanFlags()
+			assert.Equal(t, test.expectedResult, result)
+		})
+	}
+}

docs/configuration/feature-flags.md

@@ -31,6 +31,7 @@ change hidden behind the feature flag disabled a corresponding environment varia
 | `FF_K8S_USE_ENTRYPOINT_OVER_COMMAND` | `true`        | ✓          | 12.0               | Enables [the fix][mr-1010] for entrypoint configuration when `kubernetes` executor is used. |
 | `FF_DOCKER_HELPER_IMAGE_V2`          | `false`       | ✓          | 12.0               | Enable the helper image to use the new commands when [helper_image](https://docs.gitlab.com/runner/configuration/advanced-configuration.html#the-runnersdocker-section) is specified. This will start using the new API that will be used in 12.0 and stop showing the warning message in the build log. |
 | `FF_CMD_DISABLE_DELAYED_ERROR_LEVEL_EXPANSION` | `false`       | ✓          | TBA                | Disables [EnableDelayedExpansion](https://ss64.com/nt/delayedexpansion.html) for error checking for when using [Window Batch](https://docs.gitlab.com/runner/shells/#windows-batch) shell.                                                                                                               |
+| `FF_USE_LEGACY_GIT_CLEAN_STRATEGY` | `false`       | ✓          | 12.0                | Enables the new strategy for `git clean` that moves the clean operation after checkout and enables support for `GIT_CLEAN_FLAGS` |
 
 
 [mr-1010]: https://gitlab.com/gitlab-org/gitlab-runner/merge_requests/1010

executors/shell/executor_shell_test.go

@@ -217,9 +217,9 @@ func TestBuildWithShallowLock(t *testing.T) {
 		build, cleanup := newBuild(t, successfulBuild, shell)
 		defer cleanup()
 
-		build.Variables = append(build.Variables, []common.JobVariable{
+		build.Variables = append(build.Variables,
 			common.JobVariable{Key: "GIT_DEPTH", Value: "1"},
-			common.JobVariable{Key: "GIT_STRATEGY", Value: "fetch"}}...)
+			common.JobVariable{Key: "GIT_STRATEGY", Value: "fetch"})
 
 		err = runBuild(t, build)
 		assert.NoError(t, err)
@@ -975,6 +975,41 @@ func TestInteractiveTerminal(t *testing.T) {
 	}
 }
 
+func TestBuildWithGitCleanFlags(t *testing.T) {
+	shellstest.OnEachShell(t, func(t *testing.T, shell string) {
+		jobResponse, err := common.GetSuccessfulBuild()
+		assert.NoError(t, err)
+
+		build, cleanup := newBuild(t, jobResponse, shell)
+		defer cleanup()
+
+		build.Variables = append(build.Variables,
+			common.JobVariable{Key: "GIT_STRATEGY", Value: "fetch"},
+			common.JobVariable{Key: "GIT_CLEAN_FLAGS", Value: "-ffdx cleanup_file"})
+
+		// Run build and save file
+		err = runBuild(t, build)
+		require.NoError(t, err)
+
+		excludedFilePath := filepath.Join(build.BuildDir, "excluded_file")
+		cleanUpFilePath := filepath.Join(build.BuildDir, "cleanup_file")
+
+		err = ioutil.WriteFile(excludedFilePath, []byte{}, os.ModePerm)
+		require.NoError(t, err)
+		err = ioutil.WriteFile(cleanUpFilePath, []byte{}, os.ModePerm)
+		require.NoError(t, err)
+
+		// Re-run build and ensure that file still exists
+		err = runBuild(t, build)
+		require.NoError(t, err)
+
+		_, err = os.Stat(excludedFilePath)
+		assert.NoError(t, err, "excluded_file does exist")
+		_, err = os.Stat(cleanUpFilePath)
+		assert.Error(t, err, "cleanup_file does not exist")
+	})
+}
+
 // TODO: Remove in 12.0
 func TestBuildNoRefspecs(t *testing.T) {
 	strategies := []string{"clone", "fetch", "none"}

shells/abstract.go

@@ -86,7 +86,7 @@ func (b *AbstractShell) writeCloneCmd(w ShellWriter, build *common.Build, projec
 	w.Cd(projectDir)
 }
 
-func (b *AbstractShell) writeGitCleanup(w ShellWriter) {
+func (b *AbstractShell) writeGitCleanup(w ShellWriter, build *common.Build) {
 	// Remove .git/{index,shallow,HEAD}.lock files from .git, which can fail the fetch command
 	// The file can be left if previous build was terminated during git operation
 	w.RmFile(".git/index.lock")
@@ -95,13 +95,16 @@ func (b *AbstractShell) writeGitCleanup(w ShellWriter) {
 
 	w.RmFile(".git/hooks/post-checkout")
 
-	w.Command("git", "clean", "-ffdx")
-	w.IfCmd("git", "diff", "--no-ext-diff", "--quiet", "--exit-code")
-	// git 1.7 cannot reset before a checkout, if no diffs we can avoid git reset
-	w.Print("Clean repository")
-	w.Else()
-	w.Command("git", "reset", "--hard")
-	w.EndIf()
+	// TODO: Remove in 12.0
+	if build.IsFeatureFlagOn(common.FFUseLegacyGitCleanStrategy) {
+		w.Command("git", "clean", "-ffdx")
+		w.IfCmd("git", "diff", "--no-ext-diff", "--quiet", "--exit-code")
+		// git 1.7 cannot reset before a checkout, if no diffs we can avoid git reset
+		w.Print("Clean repository")
+		w.Else()
+		w.Command("git", "reset", "--hard")
+		w.EndIf()
+	}
 }
 
 // TODO: Remove in 12.0
@@ -121,7 +124,7 @@ func (b *AbstractShell) writeFetchCmd(w ShellWriter, build *common.Build, projec
 		b.writeGitSSLConfig(w, build, nil)
 	}
 
-	b.writeGitCleanup(w)
+	b.writeGitCleanup(w, build)
 
 	w.Command("git", "remote", "set-url", "origin", build.GetRemoteURL())
 	if depth != "" {
@@ -154,7 +157,7 @@ func (b *AbstractShell) writeRefspecFetchCmd(w ShellWriter, build *common.Build,
 
 	w.Command("git", "init", projectDir, "--template", templateDir)
 	w.Cd(projectDir)
-	b.writeGitCleanup(w)
+	b.writeGitCleanup(w, build)
 
 	// fetching
 	if depth > 0 {
@@ -182,6 +185,14 @@ func (b *AbstractShell) writeRefspecFetchCmd(w ShellWriter, build *common.Build,
 func (b *AbstractShell) writeCheckoutCmd(w ShellWriter, build *common.Build) {
 	w.Notice("Checking out %s as %s...", build.GitInfo.Sha[0:8], build.GitInfo.Ref)
 	w.Command("git", "checkout", "-f", "-q", build.GitInfo.Sha)
+
+	if !build.IsFeatureFlagOn(common.FFUseLegacyGitCleanStrategy) {
+		cleanFlags := build.GetGitCleanFlags()
+		if len(cleanFlags) > 0 {
+			cleanArgs := append([]string{"clean"}, cleanFlags...)
+			w.Command("git", cleanArgs...)
+		}
+	}
 }
 
 func (b *AbstractShell) writeSubmoduleUpdateCmd(w ShellWriter, build *common.Build, recursive bool) {

shells/abstract_test.go

@@ -193,3 +193,76 @@ func TestWriteWritingArtifactsOnFailure(t *testing.T) {
 	err := shell.writeScript(mockWriter, common.BuildStageUploadOnFailureArtifacts, info)
 	require.NoError(t, err)
 }
+
+func TestGitCleanFlags(t *testing.T) {
+	tests := map[string]struct {
+		value               string
+		legacyCleanStrategy string
+
+		expectedGitClean      bool
+		expectedGitCleanFlags []interface{}
+	}{
+		"empty clean flags": {
+			value:                 "",
+			legacyCleanStrategy:   "false",
+			expectedGitClean:      true,
+			expectedGitCleanFlags: []interface{}{"-ffdx"},
+		},
+		"use custom flags": {
+			value:                 "custom-flags",
+			legacyCleanStrategy:   "false",
+			expectedGitClean:      true,
+			expectedGitCleanFlags: []interface{}{"custom-flags"},
+		},
+		"use custom flags with multiple arguments": {
+			value:                 "-ffdx -e cache/",
+			legacyCleanStrategy:   "false",
+			expectedGitClean:      true,
+			expectedGitCleanFlags: []interface{}{"-ffdx", "-e", "cache/"},
+		},
+		"uses legacy strategy": {
+			value:               "custom-flags",
+			legacyCleanStrategy: "true",
+			expectedGitClean:    false,
+		},
+		"disabled": {
+			value:               "none",
+			legacyCleanStrategy: "false",
+			expectedGitClean:    false,
+		},
+	}
+
+	for name, test := range tests {
+		t.Run(name, func(t *testing.T) {
+			shell := AbstractShell{}
+
+			const dummySha = "01234567abcdef"
+			const dummyRef = "master"
+
+			build := &common.Build{
+				Runner: &common.RunnerConfig{},
+				JobResponse: common.JobResponse{
+					GitInfo: common.GitInfo{Sha: dummySha, Ref: dummyRef},
+					Variables: common.JobVariables{
+						{Key: "GIT_CLEAN_FLAGS", Value: test.value},
+						{Key: common.FFUseLegacyGitCleanStrategy, Value: test.legacyCleanStrategy},
+					},
+				},
+			}
+
+			mockWriter := new(MockShellWriter)
+			defer mockWriter.AssertExpectations(t)
+
+			mockWriter.On("Notice", "Checking out %s as %s...", dummySha[0:8], dummyRef).Once()
+			mockWriter.On("Command", "git", "checkout", "-f", "-q", dummySha).Once()
+
+			if test.expectedGitClean {
+				command := []interface{}{"git", "clean"}
+				command = append(command, test.expectedGitCleanFlags...)
+				mockWriter.On("Command", command...).Once()
+			}
+
+			shell.writeCheckoutCmd(mockWriter, build)
+		})
+	}
+}