Have -enforce-clean cause a failure when "before" location is unlcean (…

…#86)

Properly handle changes to .gitignore files.

cli/flags.go

@@ -3,7 +3,6 @@ package cli
 import (
 	"flag"
 	"fmt"
-	"log"
 	"os"
 	"path/filepath"
 	"strings"
@@ -187,6 +186,7 @@ func ResolveCommonConfig(commonFlags *CommonFlags, beforeRevStr string) (*Common
 		AnalysisCacheClearStrategy:             *commonFlags.AnalysisCacheClearStrategy,
 		CompareQueriesAroundAnalysisCacheClear: commonFlags.CompareQueriesAroundAnalysisCacheClear,
 		FilterIncompatibleTargets:              commonFlags.FilterIncompatibleTargets,
+		EnforceCleanRepo:                       commonFlags.EnforceCleanRepo == EnforceClean,
 	}
 
 	// Non-context attributes
@@ -201,22 +201,6 @@ func ResolveCommonConfig(commonFlags *CommonFlags, beforeRevStr string) (*Common
 		return nil, fmt.Errorf("failed to parse targets: %w", err)
 	}
 
-	// Additional checks
-
-	uncleanFileStatuses, err := pkg.GitStatusFiltered(workingDirectory, *commonFlags.IgnoredFiles)
-	if err != nil {
-		return nil, fmt.Errorf("failed to check whether the repository is clean: %w", err)
-	}
-
-	if len(uncleanFileStatuses) > 0 && commonFlags.EnforceCleanRepo == EnforceClean {
-		log.Printf("Current working tree has %v non-ignored untracked files:\n",
-			len(uncleanFileStatuses))
-		for _, status := range uncleanFileStatuses {
-			log.Printf("%s\n", status)
-		}
-		return nil, fmt.Errorf("current repository is not clean and --enforce-clean option is set to '%v'. Exiting.", EnforceClean.String())
-	}
-
 	return &CommonConfig{
 		Context:        context,
 		RevisionBefore: beforeRev,

pkg/target_determinator.go

@@ -135,6 +135,8 @@ type Context struct {
 	CompareQueriesAroundAnalysisCacheClear bool
 	// FilterIncompatibleTargets controls whether we filter out incompatible targets from the candidate set of affected targets.
 	FilterIncompatibleTargets bool
+	// EnforceCleanRepo controls whether we should fail if the repository is unclean.
+	EnforceCleanRepo bool
 }
 
 // FullyProcess returns the before and after metadata maps, with fully filled caches.
@@ -212,6 +214,7 @@ func LoadIncompleteMetadata(context *Context, rev LabelledGitRev, targets Target
 		AnalysisCacheClearStrategy:             context.AnalysisCacheClearStrategy,
 		CompareQueriesAroundAnalysisCacheClear: context.CompareQueriesAroundAnalysisCacheClear,
 		FilterIncompatibleTargets:              context.FilterIncompatibleTargets,
+		EnforceCleanRepo:                       context.EnforceCleanRepo,
 	}
 	cleanupFunc := func() {}
 
@@ -374,6 +377,10 @@ func gitSafeCheckout(context *Context, rev LabelledGitRev, ignoredFiles []common
 		return "", fmt.Errorf("failed to check whether the repository is clean: %w", err)
 	}
 	if !isPreCheckoutClean {
+		if context.EnforceCleanRepo {
+			return "", fmt.Errorf("repository was not clean before checking out %v", rev)
+		}
+
 		log.Printf("Workspace is unclean, using git worktree. This will be slower the first time. " +
 			"You can avoid this by committing local changes and ignoring untracked files.")
 		useGitWorktree = true
@@ -387,6 +394,10 @@ func gitSafeCheckout(context *Context, rev LabelledGitRev, ignoredFiles []common
 			return "", fmt.Errorf("failed to check whether the repository is clean: %w", err)
 		}
 		if !isPostCheckoutClean {
+			if context.EnforceCleanRepo {
+				return "", fmt.Errorf("repository was not clean after checking out %v", rev)
+			}
+
 			log.Printf("Detected unclean repository after checkout (likely due to submodule or " +
 				".gitignore changes). Using git worktree to leave original repository pristine.")
 			useGitWorktree = true

tests/integration/java/com/github/bazel_contrib/target_determinator/integration/TargetDeterminatorSpecificFlagsTest.java

@@ -126,18 +126,34 @@ public void failForUncleanRepositoryWithEnforceClean() throws Exception {
   }
 
   @Test
-  public void succeedForUncleanIgnoredFilesWithEnforceClean() throws Exception {
+  public void ignoresIgnoredFile() throws Exception {
     TestdataRepo.gitCheckout(testDir, Commits.TWO_TESTS_WITH_GITIGNORE);
 
     Path ignoredFile = testDir.resolve("ignored-file");
     Files.createFile(ignoredFile);
 
-    Set<Label> targets = getTargets(Commits.ONE_TEST, "//...", true, true);
+    Set<Label> targets = getTargets(Commits.ONE_TEST_WITH_GITIGNORE, "//...", true, true);
     Util.assertTargetsMatch(targets, Set.of("//java/example:OtherExampleTest"), Set.of(), false);
 
     assertThat("expected ignored file to still be present after invocation", ignoredFile.toFile().exists());
   }
 
+  @Test
+  public void failsIfChangingCommitsCausesAnIgnoredFileToBecomeUntracked() throws Exception {
+    TestdataRepo.gitCheckout(testDir, Commits.TWO_TESTS_WITH_GITIGNORE);
+
+    Path ignoredFile = testDir.resolve("ignored-file");
+    Files.createFile(ignoredFile);
+
+    try {
+      getTargets(Commits.ONE_TEST, "//...", true, true);
+      fail("Expected target-determinator command to fail but it succeeded");
+    } catch (TargetComputationErrorException e) {
+      assertThat(e.getStdout(), CoreMatchers.equalTo("Target Determinator invocation Error\n"));
+      assertThat(e.getStderr(), containsString("repository was not clean after checking out revision 'before'"));
+    }
+  }
+
   @Test
   public void failForUncleanSubmoduleWithEnforceClean() throws Exception {
     TestdataRepo.gitCheckout(testDir, Commits.SUBMODULE_CHANGE_DIRECTORY);

tests/integration/java/com/github/bazel_contrib/target_determinator/integration/Tests.java

@@ -673,6 +673,7 @@ class Commits {
       "c0ef0f9805e65817299eb7a794ed66655c0dd5aa";
   public static final String REDUCE_DEPENDENCY_VISIBILITY =
       "396dae111684b893ec6e04b2f6e86ed603a01082";
+  public static final String ONE_TEST_WITH_GITIGNORE = "51bf9b729dddf35694019c19b5dbffb36bf83af4";
   public static final String TWO_TESTS_WITH_GITIGNORE = "55845a3a08525f2aa66c3d7a2115dad684c46995";
   public static final String SUBMODULE_ADD_TRIVIAL_SUBMODULE =
       "b88ddcfe3da63c8308ce6d3274dd424d2c7b211a";